-
Notifications
You must be signed in to change notification settings - Fork 815
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
Set the highest autoconnect priority for network-scripts #1212
Set the highest autoconnect priority for network-scripts #1212
Conversation
…ded by NetworkManager ifcfg-rh plugin. Note that keyfile is the only and default existing plugin on RHEL9, by setting the highest autoconnect priority for network-scripts, NetworkManager will activate network-scripts but keyfile. Network-scripts path: /etc/sysconfig/network-scripts/* keyfile path: /etc/NetworkManager/system-connections/*
This was also tested by our QA for the related BZ: |
Can I get a little more context as to what's driving this change? Where are the other connections coming from that are getting ranked higher? What if people have other connections they want ranked higher than cloud-init? How can we be sure this won't cause a regression? @otubo , I don't have access to that bug, so I can't see what's happening there. |
@TheRealFalcon This issue reproduces on RHEL9 (Currently at BETA stage) image only. Cloud-init writes network configuration to network-scripts file (ex: /etc/sysconfig/network-scripts/ifcfg-ens192) for RHEL, but the active profile is keyfile (ex: /etc/NetworkManager/system-connections/ens192.nmconnection) on RHEL9, so whatever network configured by cloud-init, it won't take effect. The 'AUTOCONNECT_PRIORITY' key's value is from -999 to 999, here this change sets it to the highest value 999. By default, keyfile profile doesn't contain this 'AUTOCONNECT_PRIORITY' key, so it defaults to 0. If people explicitly set this 'AUTOCONNECT_PRIORITY' to 999 in keyfile which means they want to keep keyfile active, then still network-scripts profile will not be activated after booting. @otubo told me that there is another PR #1180 related to this issue. |
@TheRealFalcon I requested the reporter to set the BZ to public we all can have access. Bare in mind it's Chinese New Years and our QA (and the reporter) are all away. They'll be back next week though. |
Please also note #1224 |
@otubo @PengpengSun It might be worth it to prioritize #1224 instead of doing more weird things to cloud-init's ifcfg generator. |
(NetworkManager developer at Red Hat here) I think this shouldn't be applied. When the user has explicit configuration for an interface that's also configured by cloud-init, it makes little sense to prioritize the cloud-init configuration. This should be just not creating the unwanted configuration in the first place instead of beating it with higher priority. Moreover, this makes even less sense considering cloud-init's sysconfig backend explicitly tells NetworkManager to stay away with NM_CONTROLLED=no. Red Hat historically patches this away downstream. My hope is that once #1224 is in, we can align this with upstream. |
Thanks @tyll @Conan-Kudo @lkundrak, The idea of this PR jumped to my head when I saw 'autoconnect-priority=-999' in key file /etc/NetworkManager/system-connections/ens192.nmconnection on a RHEL9 VM installed from ISO, the setting looks like a backward compatible to me, since once network-scripts file created by user, it will have default 'autoconnect-priority' 0, then network-scripts will be activated since 0>-999. I also noticed NM_CONTROLLED=no is dropped in Red Hat cloud-init repo, then NetworkManager can load both keyfile and network-scripts profiles. I agree PR #1224 is better choice for RHEL9 which prefer keyfile by default, let me close this PR. |
@PengpengSun @lkundrak do you guys think it make sense to apply this as a temporary measure and add a patch to revert it on PR#1224? We have a slight time pressure on this BZ https://bugzilla.redhat.com/show_bug.cgi?id=2036060 |
@Conan-Kudo because I understand it's a work in progress that still needs some fixing. And it's not merged. |
Proposed Commit Message
Additional Context
Test Steps
Checklist: