-
Notifications
You must be signed in to change notification settings - Fork 908
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
fix(rhel): Fix network ordering in upstream cloud-init #5089
Conversation
Requesting review from @ani-sinha. I assume that this will conflict with some RHEL patches, but I think this should align upstream and downstream by both 1) selecting NetworkManager over sysconfig in the default config and 2) fix sysconfig, if it is in fact still used by any RHEL derivatives. |
While both changes seem scoped at just Redhat deriviates. Some of these changes could be "risky" to existing downstreams in that it represents a change in behavior which render our cloud.cfg.tmpl directly during package build vs a runtime changeset in the configs we write. Please separate this PR into two different PRs for discussions of each changeset so we can evaluate those separate changes more specifically to make sure we aren't missing something. One PR for dropping the |
@@ -317,7 +317,6 @@ class Renderer(renderer.Renderer): | |||
"rhel": { | |||
"ONBOOT": True, | |||
"USERCTL": False, | |||
"NM_CONTROLLED": False, |
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.
Thanks for this change. We have been maintaining a similar patch downstream. With this change, we can remove the downstream patch. See this patch in c9s: https://gitlab.com/redhat/centos-stream/src/cloud-init/-/commit/5129908caa1867c7f584ec8d38607cf56b20521a
@@ -308,7 +308,7 @@ system_info: | |||
activators: ['netplan', 'eni', 'network-manager', 'networkd'] | |||
{% elif is_rhel %} | |||
network: | |||
renderers: ['sysconfig', 'eni', 'netplan', 'network-manager', 'networkd'] | |||
renderers: ['eni', 'netplan', 'network-manager', 'sysconfig', 'networkd'] |
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.
Yes we intend to make network-manager renderer default in c10s. So this is also good.
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.
Thank you for confiming @ani-sinha this as desired downstream for Redhat derivatives. We've agreed we can get behind this in upstream provided that these commits also declare the "BREAKING CHANGE" footer and maybe even include a !
in the commit summary to highlight the change in behavior for any redhat derivative downstream using config/cloud.cfg.tmpl
during their package build process
NM_CONTROLLED=true allows cloud-init to wait until network devices are online.
673bd97
to
9a7674a
Compare
Proposed Commit Message
Additional Context
Issues were discovered with testing upstream-built RPMs. This should fix any remaining rhel-derivative sysconfig users (are there any?) as well as correctly prioritize NetworkManager when available.
Fixes #3781
Merge type