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

fix(rhel): Fix network ordering in upstream cloud-init #5089

Merged
merged 2 commits into from
Mar 26, 2024

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented Mar 22, 2024

Proposed Commit Message

fix(rhel): Fix network ordering in upstream cloud-init

1. NM_CONTROLLED=true should be set to allow cloud-init to wait until network
   devices are online.
2. RHEL derivatives should be using the NetworkManager renderer, so set that
   in the default config.

Fixes GH-3781

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

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

@holmanb
Copy link
Member Author

holmanb commented Mar 22, 2024

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.

@blackboxsw
Copy link
Collaborator

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 NM_CONTROLLED=False logic and one PR for the cloud.cfg.tmpl.

@@ -317,7 +317,6 @@ class Renderer(renderer.Renderer):
"rhel": {
"ONBOOT": True,
"USERCTL": False,
"NM_CONTROLLED": False,
Copy link
Contributor

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']
Copy link
Contributor

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.

Copy link
Collaborator

@blackboxsw blackboxsw left a 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.
@holmanb holmanb merged commit d53623e into canonical:main Mar 26, 2024
28 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.

sysconfig: NM_CONTROLLED=False should not be set on RHEL8
3 participants