Skip to content

migrate net/prefix types to oxnet #266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jun 25, 2024
Merged

migrate net/prefix types to oxnet #266

merged 14 commits into from
Jun 25, 2024

Conversation

ahl
Copy link
Contributor

@ahl ahl commented Jun 5, 2024

I've hit at least two interesting problem (as distinct from the countless uninteresting problems I've encountered).

  1. This v1 stuff? It seems that we've baked in an old ddm somewhere to ensure compatibility... compatibility which I've broken by changing the json schema for IpNet/Prefix. I guess this means we're heading to v3, but I don't understand this mechanism. Update addressed by disable the ddm v1 tests #276 and opened test for ddm compatibility with the most recent release #275.

  2. Dependencies, how do they work? mgd depends transitively on libnet which ends up dumping in a bunch of dependencies on illumos-only shared libraries. It builds on Linux... somehow? It's surprising (baffling) to me how that works. Or worked. So I put in some proper feature flags to help make sure this actually works as intended.

@ahl ahl marked this pull request as ready for review June 24, 2024 21:50
@ahl ahl requested a review from rcgoodfellow June 24, 2024 21:51
@rcgoodfellow
Copy link
Collaborator

Dependencies, how do they work? mgd depends transitively on libnet which ends up dumping in a bunch of dependencies on illumos-only shared libraries. It builds on Linux... somehow? It's surprising (baffling) to me how that works. Or worked. So I put in some proper feature flags to help make sure this actually works as intended.

libnet should not be pulling in much in the way of shared library dependencies. It interacts with illumos using ioctl and doors interfaces. I think the only illumos library that has snuck in there is the kstat, and I'm not sure if any of the kstat-based stuff is used in maghemite.

@ahl
Copy link
Contributor Author

ahl commented Jun 24, 2024

@rcgoodfellow this failure led me to the feature: https://buildomat.eng.oxide.computer/wg/0/details/01HZN7DPHGCA4NTWA88AY1GYTG/K6UXLFfvHdWloZLAjDs8BKluNUnVgGx6uh4sGUn9CiTCsI2z/01HZN7EFMT4RNCBZX20PAZGAS6

In particular if you scroll to the bottom of this Linux build:

  = note: /usr/bin/ld: cannot find -lkstat: No such file or directory
          /usr/bin/ld: cannot find -lnvpair: No such file or directory
          /usr/bin/ld: cannot find -lzfs: No such file or directory
          /usr/bin/ld: cannot find -ldlpi: No such file or directory
          /usr/bin/ld: cannot find -ldoor: No such file or directory
          collect2: error: ld returned 1 exit status

I think there was some dead-code analysis that cause these not to be included that somehow I disrupted.

Copy link
Collaborator

@rcgoodfellow rcgoodfellow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Adam! LGTM just a few small questions.

@@ -1,3 +0,0 @@
fn main() {
println!("cargo:rerun-if-changed=../openapi/ddm-admin.json");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is sensitivity to changes in the OpenAPI spec now built into progenitor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my knowledge it has been for quite a while. Did you find this not to be the case? oxidecomputer/progenitor#16

});
}
client.advertise_prefixes(&prefixes).await?;
client.advertise_prefixes(&ac.prefixes).await?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! So glad to see this getting rid of the manual type conversions into equivalent types!

@@ -32,3 +32,4 @@ smf.workspace = true

[features]
default = ["mg-lower"]
mg-lower = ["dep:mg-lower"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just to pick up the post rust 1.60 explicit dependency syntax - or is there something more to what's going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was the (new) right way to specify the inclusion of optional dependencies.

@rcgoodfellow
Copy link
Collaborator

Ah right - beyond kstat, those other deps are being picked up by other crates like rusty-doors and dlpi-sys. The whole compiling on Linux and Mac thing has been in service of supporting control plane build and test environments that require compiling and running somewhat vacuous maghemite daemon builds as placeholders that can answer API requests but don't really do much.

@ahl ahl merged commit 77749e3 into main Jun 25, 2024
10 checks passed
@ahl ahl deleted the oxnet branch June 25, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants