-
Notifications
You must be signed in to change notification settings - Fork 106
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
providers/vmware: Process guestinfo.metadata netplan configuration #888
Conversation
@bgilbert @lucab Any preference? While I think that doing all processing in Afterburn would be nice, the more practical approach would be to write the netplan config file out and let the distro handle it through the regular |
Thanks for the PR. Afterburn already has a mechanism for this on VMware, #404, but it isn't very user-friendly (initrd I see an opportunity here to revisit our network configuration design, and reuse existing abstractions rather than maintaining our own. Rather than having the DigitalOcean and Packet providers populate networkd-format structs, they (and the VMware provider) could populate a WDYT? If you have an immediate need for this, it could be reasonable to add some stub Netplan support for VMware but defer migration of the other providers to a later PR. |
Thanks for the feedback, ok, I'm actually happy that we generate networkd units directly in afterburn but it's good to know that you are looking for a solution that also covers NetworkManager-only systems. Flatcar doesn't ship netplan (and ok course also doesn't ship nmstate). We could add it to the VMware Flatcar images and limit the use to it at first, and later see if that's something we could use for other providers, too. I'll drop the unit generation then and only parse the file once and emit it again to have a way where other providers could expose a netplan struct as you said. |
ea7f1e2
to
761b850
Compare
Done but keeping it it draft mode until I test it on a real system |
0eeec2d
to
bce3c6a
Compare
Tested with a Flatcar VM as follows, the VM didn't have the guestinfo.metadata set on provisioning:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor change requested, and then it's good to go!
CI failures are a failed lint, missing release notes in I won't be able to do the review here, unfortunately, but hopefully one of the other folks on the team can take a look. Thanks for your work on this PR! |
Would flatcar ship code to translate netplan to systemd-networkd? I am not totally sure we want to own code doing this for NetworkManager. (Not to architecture astronaut this too much but maybe what could be tractable is translating some networkd configs to NetworkManager...) |
The plan is like @bgilbert mentioned above "The distro would be responsible for configuring Netplan to translate to networkd or NetworkManager as desired.". There are also plans to write a rust implementation of netplan from what I heard. |
bce3c6a
to
a8239bf
Compare
I've addressed the partly review, it's ready for a full review.
Netplan supports translation to either networkd or NetworkManager. For Flatcar we would add Netplan to the VMware OEM image and invoke it for translation to ephemeral networkd units under |
a8239bf
to
2a33e02
Compare
src/providers/vmware/amd64.rs
Outdated
// Also check the version to make it easier for the OS to say that | ||
// this mechanism is supported or not. | ||
if netplan_config.network.version != 2 { | ||
bail!("only netplan version 2 supported"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much about Netplan though that seems out of scope for Afterburn. Why not be version agnostic and let the OS deal with version compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OS would run afterburn with this flag and then netplan with a particular version. If any netplan data can come in I thought it's better to narrow the supported values and to error out early - also, the netplan_types crate may not support newer versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From previous comments:
The distro would be responsible for configuring Netplan to translate to networkd or NetworkManager as desired.
I don't think we should parse the netplan config in Afterburn. We should leave that to another unit in the initrd or the root to do the translation for the desired backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the OS wants to error out before getting all the way to Netplan generation, integrators can ship a systemd service that runs right After
Afterburn to check the config version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And actually, it seems like the only requirement on netplan-types
? So if we drop this function, we should be able to get rid of the feature gating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is one field that it would make sense to parse out of guestinfo.metadata
and that's local-hostname
. But we could do that without the netplan-types
dependency, by carrying our own simple struct definition.
i would suggest doing this as a follow-up, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we also had the idea that netplan-types
could be used in the providers for a NetworkManager-capable alternative to the --network-units=
flag that Flatcar uses. If that needs arises we can still add it and it's not really needed at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found one problem with dropping the parsing: Netplan seems to want YAML but cloud-init is also fine with JSON which the current PR here would convert to YAML. I guess we would have to drop JSON support then. I'll stop for now and leave the feature gate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local-hostname
is another top-level entry and not part of netplan-types
because it's a cloud-init thing. Supporting and parsing that would be done with a separate struct.
f5a6276
to
d162a8c
Compare
Have changed it now to use blocks.
Have changed it to be behind a feature gate, and the CI will enable all features. |
You probably should link to the cloud-init documentation in the docs if we don't have anything more platform generic. |
src/providers/vmware/amd64.rs
Outdated
// Also check the version to make it easier for the OS to say that | ||
// this mechanism is supported or not. | ||
if netplan_config.network.version != 2 { | ||
bail!("only netplan version 2 supported"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the OS wants to error out before getting all the way to Netplan generation, integrators can ship a systemd service that runs right After
Afterburn to check the config version.
src/providers/vmware/amd64.rs
Outdated
// Also check the version to make it easier for the OS to say that | ||
// this mechanism is supported or not. | ||
if netplan_config.network.version != 2 { | ||
bail!("only netplan version 2 supported"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And actually, it seems like the only requirement on netplan-types
? So if we drop this function, we should be able to get rid of the feature gating.
3ebadb5
to
cd23788
Compare
I looked into removing the |
Sorry, can you explain the issue in more details? Is the issue that we're not retaining the original format? Can't we note down which format it was in at deserialization time and make sure to reserialize it in the same format? |
The metadata can be either JSON or YAML but the output for Netplan must be YAML, thus it gets converted from JSON to YAML with the |
I thought YAML is a superset of JSON so valid JSON is automatically also valid YAML. |
cd23788
to
99aea9c
Compare
Updated it to remove the netplan-types dependency.
Yeah, maybe I delete the match statement. |
99aea9c
to
dde31e7
Compare
Removed the match statement because it works to only rely on YAML parsing. |
dde31e7
to
f8ceb3e
Compare
5c30613
to
afa2c33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment, but LGTM overall. Nice work!
The network environment can be dynamic and thus needs to be provided as VM metadata. Since the format should not depend on whether the VM runs uses Ignition and Afterburn or Cloud-Init, the idea is to also support the guestinfo.metadata variable as used by Cloud-Init which contains Netplan YAML/JSON network configuration. Add a new command to write out Netplan configs to a given directory, similar as we do with networkd units. (While this is currently just used for VMware, other providers could also construct the netplan data type from the netplan-types crate to provide Netplan configurations if the OS rather wants to use NetworkManager than systemd-networkd. For backwards compatibility and to not need Netplan it would be nice to keep the systemd-networkd support as long as its used.) References: https://cloudinit.readthedocs.io/en/latest/reference/datasources/vmware.html#walkthrough-of-guestinfo-keys-transport https://cloudinit.readthedocs.io/en/latest/reference/network-config-format-v2.html https://netplan.io/reference/ https://linux-on-z.blogspot.com/p/using-netplan-on-ibm-z.html
afa2c33
to
7e71b88
Compare
The network environment can be dynamic and thus needs to be provided as
VM metadata. Since the format should not depend on whether the VM runs
uses Ignition and Afterburn or Cloud-Init, the idea is to also support
the guestinfo.metadata variable as used by Cloud-Init which contains
Netplan YAML network configuration.
Add a new command to write out netplan configs to a given directory,
similar as we do with networkd units. While this is currently just used
for VMware, other providers could also construct the netplan data type
to provide netplan configurations if the OS rather wants to use
NetworkManager than systemd-networkd. For backwards compatibility and
to not need netplan it would be nice to keep the systemd-networkd
support as long as its used.
What do you think, rather only write out to
/etc/netplan/afterburn.yaml
and rely on the netplan implementation, or do the processing YAML directly in afterburn?References: