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

customize: Add --network-nmstate option #864

Merged
merged 1 commit into from Jul 26, 2022

Conversation

qinqon
Copy link
Contributor

@qinqon qinqon commented May 26, 2022

It's possible to generate NetworkManager keyfiles from a Nmstate
configuration. This change add a new option to use Nmstate format
instead of multiple NetworkManager keyfiles to specify network
configuration.

The output from nmstate gen_conf is a HashMap of backend name to nmconnection vectors with the following format

NetworkManager:
  - "[connection]\nautoconnect=true\nautoconnect-slaves=-1\nid=dummyfoo\ninterface-name=dummyfoo\ntype=dummy\nuuid=157d8614-c3bd-4a10-9330-310fcf692468\n\n[ipv4]\nmethod=disabled\n\n[ipv6]\nmethod=disabled\n"

One call to gen_conf can dump multiple keyfiles from the "NetworkManager" backend.

@qinqon qinqon force-pushed the customize-network-nmstate branch 2 times, most recently from 903ec82 to 221d142 Compare May 26, 2022 13:49
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.

Cool, thanks! Assuming getting the deps right in Fedora isn't too much work, this SGTM. I think even if we end up doing coreos/fedora-coreos-tracker#1175, this still has value because it allows configuring the network in the initramfs of the live system too using NMState files.

src/cmdline/mod.rs Outdated Show resolved Hide resolved
src/cmdline/mod.rs Outdated Show resolved Hide resolved
src/live/customize.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
fixtures/customize/installer-test.json Outdated Show resolved Hide resolved
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

This is great, thank you!

tests/images/customize.sh Outdated Show resolved Hide resolved
src/cmdline/mod.rs Outdated Show resolved Hide resolved
src/cmdline/mod.rs Outdated Show resolved Hide resolved
src/cmdline/mod.rs Outdated Show resolved Hide resolved
src/live/customize.rs Outdated Show resolved Hide resolved
src/live/customize.rs Outdated Show resolved Hide resolved
src/live/customize.rs Outdated Show resolved Hide resolved
src/live/customize.rs Outdated Show resolved Hide resolved
src/live/customize.rs Outdated Show resolved Hide resolved
@qinqon qinqon force-pushed the customize-network-nmstate branch 5 times, most recently from b1bd86a to 6dee4ce Compare May 27, 2022 12:39
@qinqon qinqon requested review from bgilbert and jlebon May 27, 2022 12:40
@qinqon
Copy link
Contributor Author

qinqon commented May 27, 2022

Testing part is not fixed yet, but code is reviewable now.

@qinqon qinqon force-pushed the customize-network-nmstate branch 2 times, most recently from 41dac55 to c441795 Compare May 27, 2022 15:16
src/cmdline/mod.rs Outdated Show resolved Hide resolved
src/cmdline/mod.rs Show resolved Hide resolved
src/live/customize.rs Outdated Show resolved Hide resolved
src/live/customize.rs Outdated Show resolved Hide resolved
tests/images/customize.sh Outdated Show resolved Hide resolved
tests/images/customize.sh Outdated Show resolved Hide resolved
src/live/customize.rs Outdated Show resolved Hide resolved
@qinqon qinqon force-pushed the customize-network-nmstate branch 7 times, most recently from 6654626 to 6e39520 Compare May 31, 2022 08:14
@qinqon
Copy link
Contributor Author

qinqon commented May 31, 2022

Cargo.toml has now a pin to nmstate/nmstate#1915, test should pass, but this cannot be merged until the PR is merge and nmstate published to cargo.io.

@qinqon qinqon force-pushed the customize-network-nmstate branch 2 times, most recently from 56d7001 to 65f74e0 Compare May 31, 2022 09:45
@qinqon qinqon marked this pull request as ready for review May 31, 2022 11:15
@qinqon qinqon requested review from cgwalters and bgilbert May 31, 2022 11:16
@qinqon qinqon changed the title customize: WIP --network-nmstate customize: --network-nmstate Jun 1, 2022
@qinqon qinqon changed the title customize: --network-nmstate customize: Add --network-nmstate option Jun 1, 2022
@qinqon
Copy link
Contributor Author

qinqon commented Jun 10, 2022

@bgilbert fixed clippy issues.

@qinqon qinqon force-pushed the customize-network-nmstate branch from dc3cdc3 to 334f5b3 Compare July 11, 2022 14:23
@qinqon
Copy link
Contributor Author

qinqon commented Jul 11, 2022

Hey @qinqon Looks like fedora 36 update is ready at https://bodhi.fedoraproject.org/updates/FEDORA-2022-b78be96657
But the coreos-install build RPM need to add requirements (crate(nmstate/default) >= 2.1.2 with crate(nmstate/default) < 3.0)

@bgilbert
Copy link
Contributor

Rust package requirements are automatically added by rust-srpm-macros.

@qinqon qinqon force-pushed the customize-network-nmstate branch from 334f5b3 to 3336237 Compare July 19, 2022 06:52
@qinqon
Copy link
Contributor Author

qinqon commented Jul 19, 2022

@bgilbert Do you know what happening with CI ?

Cargo.lock.orig Outdated Show resolved Hide resolved
@bgilbert
Copy link
Contributor

Do you know what happening with CI ?

kola basic tests are failing with ENOSPC. I'm checking whether it's a broader issue.

@bgilbert
Copy link
Contributor

The MSRV tests should be fixed by #926. The RPM test is awaiting FEDORA-2022-b78be96657 going stable.

@bgilbert
Copy link
Contributor

I'm not seeing that ENOSPC error in other PRs.

@bgilbert
Copy link
Contributor

Could you try rebasing? That should fix the GitHub Actions checks, and we can see whether it fixes the kola failure too. If it doesn't, there may be an actual problem with the PR.

@qinqon qinqon force-pushed the customize-network-nmstate branch from 3336237 to 473b49e Compare July 20, 2022 05:25
@bgilbert bgilbert marked this pull request as ready for review July 20, 2022 05:26
@bgilbert bgilbert marked this pull request as draft July 20, 2022 05:26
@qinqon qinqon requested a review from bgilbert July 20, 2022 11:27
@jlebon
Copy link
Member

jlebon commented Jul 20, 2022

I think @bgilbert and I came to the same realization in parallel. :) I suspect CI is hitting ENOSPC because rdcore includes debuginfo and we're not using LTO in debug mode so it's including nmstate. But even without this change, I think we should probably strip the binary when installing it in debug mode like CI does. Did that in #929.

@bgilbert
Copy link
Contributor

We're testing with 1 GB of RAM, and this PR increases the rdcore size from 132 to 178 MiB.

(Actually we're not using LTO at all anymore; it was too buggy.)

@bgilbert bgilbert force-pushed the customize-network-nmstate branch 3 times, most recently from 7c2c286 to 473b49e Compare July 21, 2022 01:37
@bgilbert
Copy link
Contributor

Confirmed that #929 fixes the ENOSPC.

@qinqon
Copy link
Contributor Author

qinqon commented Jul 21, 2022

We're testing with 1 GB of RAM, and this PR increases the rdcore size from 132 to 178 MiB.

(Actually we're not using LTO at all anymore; it was too buggy.)

Do we have to do something at nmstate to reduce rdcore size ? or 178 is ok ?

@bgilbert
Copy link
Contributor

No, the rdcore binary is only about 2 MiB with all symbols stripped, or 6 MiB with debug symbols stripped. That's what we actually ship in the OS, so this is only a CI problem. We'll just land #929 and then you can rebase on top of it.

@bgilbert
Copy link
Contributor

Okay, if you rebase this on main it should fix CI.

It's possible to generate NetworkManager keyfiles from a Nmstate
configuration. This change add a new option to use Nmstate format
instead of multiple NetworkManager keyfiles to specify network
configuration.
@qinqon qinqon force-pushed the customize-network-nmstate branch from 473b49e to e896a5e Compare July 26, 2022 06:36
@bgilbert bgilbert marked this pull request as ready for review July 26, 2022 06:40
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

Thanks for all your work on this! 🎉 🎈 🎆

@bgilbert bgilbert enabled auto-merge July 26, 2022 07:21
@bgilbert bgilbert merged commit 03d0c9a into coreos:main Jul 26, 2022
12 checks passed
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.

None yet

4 participants