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

Implement Network Configurators (nmc, nmstate, nmconnections) #819

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

anmazzotti
Copy link
Contributor

No description provided.

Copy link
Member

@fgiudici fgiudici left a comment

Choose a reason for hiding this comment

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

Patch looks good... anyway, why not support both nmc and nmstate?
The library behind it is the same, ti changes "just" the binary and the syntax, changes to the code could be minimal.
This would open to wider usage in community images where nmstate could be already be available or packaged for some base OS images, while supporting also the nmc binary where nmstate is not available (or nmc is preferred).
The idea could be to have a kind of "autodetection" just probing for the nmc binary first.
How would that look?

@@ -31,7 +31,9 @@ import (
)

const (
nmstateTempPath = "/tmp/elemental-nmstate.yaml"
nmcDesiredStatesDir = "/tmp/desired-states"
nmcNewtorkConfigDir = "/tmp/network-config"
Copy link
Member

Choose a reason for hiding this comment

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

nit: would consider grouping nmc dirs under a common /tmp/something dir (e.g., "/tmp/nmc/desired-states", "/tmp/nmc/network-config")

@anmazzotti
Copy link
Contributor Author

Patch looks good... anyway, why not support both nmc and nmstate? The library behind it is the same, ti changes "just" the binary and the syntax, changes to the code could be minimal. This would open to wider usage in community images where nmstate could be already be available or packaged for some base OS images, while supporting also the nmc binary where nmstate is not available (or nmc is preferred). The idea could be to have a kind of "autodetection" just probing for the nmc binary first. How would that look?

Could be an idea, but I would avoid any autodetection for simplicity.
We could have a configurator.type field that defaults to nmc but allows nmstate and at this point I can reintroduce the nmconnections one, since the implementation is also very similar.

@anmazzotti anmazzotti force-pushed the nm_configurator_support branch from 0ab1172 to 4ec49a0 Compare August 12, 2024 10:52
@anmazzotti
Copy link
Contributor Author

anmazzotti commented Aug 12, 2024

@fgiudici I added network.configurator that can accept nmc (default), nmstate, nmconnections values.

I'd also like to install both nmc and nmstate in the dev image, so that we can locally test with both and for example have three flavors of sample registrations.

Documentation should also be updated accordingly (so most likely one subpage for each configurator, with a few examples)

I'll work on that if you are ok with this solution

Copy link
Member

@fgiudici fgiudici left a comment

Choose a reason for hiding this comment

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

Having the three options looks quite nice!
Thanks for going the extra mile and adding the network configuration option 💚
LGTM!

@kkaempf kkaempf added this to the Micro6.1 milestone Aug 13, 2024
@anmazzotti anmazzotti changed the title Refactor to use nm-configurator Implement Network Configurators (nmc, nmstate, nmconnections) Aug 13, 2024
Signed-off-by: Andrea Mazzotti <[email protected]>
Signed-off-by: Andrea Mazzotti <[email protected]>
Signed-off-by: Andrea Mazzotti <[email protected]>
@anmazzotti anmazzotti force-pushed the nm_configurator_support branch from 5295343 to 8dda728 Compare August 13, 2024 12:43
@anmazzotti anmazzotti merged commit d857bda into rancher:main Aug 13, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/operator operator related changes area/tests test related changes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants