Skip to content
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

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

pothos
Copy link
Contributor

@pothos pothos commented Mar 10, 2023

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:

@pothos
Copy link
Contributor Author

pothos commented Mar 13, 2023

@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 netplan implementation.
Edit: Also an option, don't do it in afterburn but call vmtoolsd --cmd 'info-get guestinfo.metadata' directly to write out this file for netplan.

@bgilbert
Copy link
Contributor

Thanks for the PR. Afterburn already has a mechanism for this on VMware, #404, but it isn't very user-friendly (initrd ip= syntax is not great). Afterburn's network configuration support hasn't gotten a lot of attention in recent years, due to the longstanding problem that it's designed for networkd and doesn't map well onto NetworkManager. I wasn't aware of Netplan and wish I had known about it sooner; it looks like a good fit for Afterburn's needs.

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 netplan-types struct, and then Afterburn could write that out via a new --netplan-conf <directory> option. The distro would be responsible for configuring Netplan to translate to networkd or NetworkManager as desired.

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.

@pothos
Copy link
Contributor Author

pothos commented Mar 20, 2023

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.

@pothos pothos force-pushed the guestinfo-metadata-netplan branch 3 times, most recently from ea7f1e2 to 761b850 Compare March 30, 2023 04:36
@pothos
Copy link
Contributor Author

pothos commented Mar 30, 2023

Done but keeping it it draft mode until I test it on a real system

@pothos pothos force-pushed the guestinfo-metadata-netplan branch 3 times, most recently from 0eeec2d to bce3c6a Compare May 4, 2023 04:52
src/providers/vmware/amd64.rs Outdated Show resolved Hide resolved
@pothos
Copy link
Contributor Author

pothos commented May 4, 2023

Tested with a Flatcar VM as follows, the VM didn't have the guestinfo.metadata set on provisioning:

cargo build
scp -C target/debug/afterburn core@ESXIVM:
ssh core@ESXIVM
# now in the VM:
./afterburn multi --netplan-configs $PWD/ --provider vmware
# no afterburn.yaml will be written to PWD
/usr/share/oem/bin/vmtoolsd --cmd "info-set guestinfo.metadata bmV0d29yazoKICB2ZXJzaW9uOiAyCiAgZXRoZXJuZXRzOgogICAgbmljczoKICAgICAgbWF0Y2g6CiAgICAgICAgbmFtZTogZW5zKgogICAgICBkaGNwNDogdHJ1ZQo="
/usr/share/oem/bin/vmtoolsd --cmd "info-set guestinfo.metadata.encoding base64"
./afterburn multi --netplan-configs $PWD/ --provider vmware
# the afterburn.yaml gets written to PWD
cat afterburn.yaml
# prints this:
# network:
#  version: 2
#  ethernets:
#    nics:
#      match:
#        name: ens*
#      dhcp4: true

@pothos pothos marked this pull request as ready for review May 4, 2023 05:00
Copy link

@akutz akutz left a 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!

src/providers/vmware/amd64.rs Show resolved Hide resolved
src/providers/vmware/amd64.rs Outdated Show resolved Hide resolved
@bgilbert
Copy link
Contributor

bgilbert commented Aug 1, 2023

CI failures are a failed lint, missing release notes in docs/release-notes.md, and the fact that netplan-types isn't packaged in Fedora. We have a rule that new dependencies must be packaged in Fedora first, so we don't have an unknown amount of packaging to do (including transitive dependencies) at release time. You're not necessarily responsible for handling that yourself (unless you want to!) but it'll need to be addressed.

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!

Cargo.toml Outdated Show resolved Hide resolved
@cgwalters
Copy link
Member

The distro would be responsible for configuring Netplan to translate to networkd or NetworkManager as desired.

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...)

@jepio
Copy link

jepio commented Sep 27, 2023

The distro would be responsible for configuring Netplan to translate to networkd or NetworkManager as desired.

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.

@pothos pothos force-pushed the guestinfo-metadata-netplan branch from bce3c6a to a8239bf Compare September 28, 2023 10:30
@pothos
Copy link
Contributor Author

pothos commented Sep 28, 2023

I've addressed the partly review, it's ready for a full review.
Can someone do that?
Is there someone I could ask about Rust Fedora packaging?

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...)

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 /run (probably from the initrd to have networking for Ignition, and this would also be carried over to the final system).

@pothos pothos force-pushed the guestinfo-metadata-netplan branch from a8239bf to 2a33e02 Compare September 28, 2023 10:43
docs/release-notes.md Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/providers/vmware/amd64.rs Show resolved Hide resolved
src/providers/vmware/amd64.rs Outdated Show resolved Hide resolved
Comment on lines 77 to 86
// 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");
}
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link

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?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me! 👍

Copy link
Contributor Author

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.

Copy link
Contributor Author

@pothos pothos Oct 5, 2023

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.

Copy link
Contributor Author

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.

@pothos pothos force-pushed the guestinfo-metadata-netplan branch 2 times, most recently from f5a6276 to d162a8c Compare October 2, 2023 12:30
@pothos
Copy link
Contributor Author

pothos commented Oct 2, 2023

But blocks have the advantage that the compiler guarantees that the dropping is implemented correctly rather than having to verify the manual implementation. A comment on the first instance should suffice to explain to the reader what's going on.

Have changed it now to use blocks.

One way to avoid having to package this in Fedora is to gate it behind a feature for now. If we do that though, we should also make sure that CI here is still exercising that path.

Have changed it to be behind a feature gate, and the CI will enable all features.

@travier
Copy link
Member

travier commented Oct 2, 2023

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 Show resolved Hide resolved
Comment on lines 77 to 86
// 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");
}
Copy link
Member

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.

Comment on lines 77 to 86
// 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");
}
Copy link
Member

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.

src/providers/vmware/amd64.rs Show resolved Hide resolved
docs/usage/vmware-netplan-guestinfo-metadata.md Outdated Show resolved Hide resolved
docs/usage/vmware-netplan-guestinfo-metadata.md Outdated Show resolved Hide resolved
docs/usage/vmware-netplan-guestinfo-metadata.md Outdated Show resolved Hide resolved
@pothos pothos force-pushed the guestinfo-metadata-netplan branch from 3ebadb5 to cd23788 Compare October 5, 2023 17:12
@pothos
Copy link
Contributor Author

pothos commented Oct 5, 2023

I looked into removing the netplan-types crate but that means dropping JSON support. Cloud-init handles both YAML and JSON, and since @akutz asked for JSON in #888 (comment) I don't want to remove it if the feature gate is ok.

@jlebon
Copy link
Member

jlebon commented Nov 9, 2023

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?

@pothos
Copy link
Contributor Author

pothos commented Nov 10, 2023

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 netplan-types crate.
I thought that when we drop the crate we drop JSON support but maybe I can also do a generic JSON to YAML conversion with serde but we need to filter unknown keys from the YAML output.

@jepio
Copy link

jepio commented Nov 10, 2023

The metadata can be either JSON or YAML but the output for Netplan must be YAML

I thought YAML is a superset of JSON so valid JSON is automatically also valid YAML.

@pothos pothos force-pushed the guestinfo-metadata-netplan branch from cd23788 to 99aea9c Compare November 10, 2023 17:46
@pothos
Copy link
Contributor Author

pothos commented Nov 10, 2023

Updated it to remove the netplan-types dependency.

I thought YAML is a superset of JSON so valid JSON is automatically also valid YAML.

Yeah, maybe I delete the match statement.

@pothos pothos force-pushed the guestinfo-metadata-netplan branch from 99aea9c to dde31e7 Compare November 10, 2023 17:54
@pothos
Copy link
Contributor Author

pothos commented Nov 10, 2023

Removed the match statement because it works to only rely on YAML parsing.

@pothos pothos force-pushed the guestinfo-metadata-netplan branch from dde31e7 to f8ceb3e Compare November 10, 2023 17:59
src/cli/multi.rs Outdated Show resolved Hide resolved
src/cli/multi.rs Outdated Show resolved Hide resolved
src/cli/multi.rs Outdated Show resolved Hide resolved
src/providers/vmware/amd64.rs Outdated Show resolved Hide resolved
src/providers/vmware/amd64.rs Outdated Show resolved Hide resolved
src/providers/vmware/amd64.rs Outdated Show resolved Hide resolved
src/providers/vmware/amd64.rs Outdated Show resolved Hide resolved
src/providers/vmware/amd64.rs Show resolved Hide resolved
src/providers/vmware/amd64.rs Outdated Show resolved Hide resolved
@pothos pothos force-pushed the guestinfo-metadata-netplan branch from 5c30613 to afa2c33 Compare November 15, 2023 16:32
Copy link
Member

@jlebon jlebon left a 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!

src/providers/vmware/amd64.rs Outdated Show resolved Hide resolved
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
@pothos pothos force-pushed the guestinfo-metadata-netplan branch from afa2c33 to 7e71b88 Compare November 17, 2023 10:25
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.

7 participants