MGMT-22849: Remove the nmconnection that the openshift-installer writes for the bootstrap.#10086
Conversation
…es for the bootstrap. The openshift-installer includes an nmconnection in the bootstrap.ign in order to configure static networking, but we have our own way to do it, so this file is always empty. If static networking is configured for the bootstrap, and that static networking actually just requests dhcp, then this empty nmconnection makes the bootstrap fail to reboot.
openshift-ci-robot
added
the
jira/valid-reference
|
@giladravid16: This pull request references MGMT-22849 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis PR adds a helper function to detect NetworkManager connection files and updates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giladravid16 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/ignition/installmanifests_test.go (1)
176-190: Assert the path directly instead of reusingisNMConnection.The test currently shares the same predicate as
updateBootstrap, so a bad helper change can make both the filter and the assertion fail in the same way and still pass. Check the literal path here so the regression is actually pinned.♻️ Suggested test hardening
var file *config_32_types.File foundNMConfig := false - foundNMConncetion := false + foundNMConnection := false for i := range config.Storage.Files { if isBMHFile(&config.Storage.Files[i]) { file = &config.Storage.Files[i] } if config.Storage.Files[i].Node.Path == "/etc/NetworkManager/conf.d/99-kni.conf" { foundNMConfig = true } - if isNMConnection(&config.Storage.Files[i]) { - foundNMConncetion = true + if config.Storage.Files[i].Node.Path == "/etc/NetworkManager/system-connections/nmconnection" { + foundNMConnection = true } } bmh, _ = fileToBMH(file) Expect(foundNMConfig).To(BeTrue(), "file /etc/NetworkManager/conf.d/99-kni.conf not present in bootstrap.ign") - Expect(foundNMConncetion).To(BeFalse()) + Expect(foundNMConnection).To(BeFalse(), "file /etc/NetworkManager/system-connections/nmconnection should be removed from bootstrap.ign")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ignition/installmanifests_test.go` around lines 176 - 190, Replace the test's use of the helper isNMConnection when checking for network connection files: instead of setting foundNMConncetion via isNMConnection(&config.Storage.Files[i]), explicitly compare config.Storage.Files[i].Node.Path to the concrete NM connection path used by updateBootstrap (locate the expected path in the updateBootstrap implementation) and set foundNMConncetion based on that literal string comparison; then assert Expect(foundNMConncetion).To(BeFalse()). This pins the test to the actual path rather than the same predicate the production code uses (isNMConnection).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/ignition/installmanifests_test.go`:
- Around line 176-190: Replace the test's use of the helper isNMConnection when
checking for network connection files: instead of setting foundNMConncetion via
isNMConnection(&config.Storage.Files[i]), explicitly compare
config.Storage.Files[i].Node.Path to the concrete NM connection path used by
updateBootstrap (locate the expected path in the updateBootstrap implementation)
and set foundNMConncetion based on that literal string comparison; then assert
Expect(foundNMConncetion).To(BeFalse()). This pins the test to the actual path
rather than the same predicate the production code uses (isNMConnection).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7692acb-6c4f-43ac-bdd9-a344dd0bf994
📒 Files selected for processing (2)
internal/ignition/installmanifests.gointernal/ignition/installmanifests_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10086 +/- ##
==========================================
+ Coverage 44.17% 45.95% +1.77%
==========================================
Files 416 416
Lines 72404 77812 +5408
==========================================
+ Hits 31985 35757 +3772
- Misses 37522 38983 +1461
- Partials 2897 3072 +175
🚀 New features to boost your workflow:
|
|
/retest-required |
|
/lgtm |
|
/retest-required |
1 similar comment
|
/retest-required |
|
@giladravid16: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
openshift-merge-bot
Bot
merged commit 3281224
into
openshift:master
3 participants
The openshift-installer includes an nmconnection in the bootstrap.ign in order to configure static networking, but we have our own way to do it, so this file is always empty.
If static networking is configured for the bootstrap, and that static networking actually just requests dhcp, then this empty nmconnection makes the bootstrap fail to reboot.
I tested this on an environment that used ACM with siteconfig to deploy a 3+2 spoke cluster.
The ClusterInstance had node network configuration for each node, and they all requested dhcp.
Without this fix all nodes were installed successfully except for the bootstrap which was stuck after the reboot because it didn't have networking, manually deleting the empty nmconnection and rebooting fixed the issue.
With this fix the spoke cluster finishes the installation successfully without manual intervention.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist