MGMT-20499: Always generate iSCSI NIC reapply manifest for day2 host support #8705
Conversation
…support A machine config manifest was introduced to work around OCPBUGS-26580, where DNS is not properly set up on first boot when booting from iSCSI storage. However, this manifest was only added during day1 installation when iSCSI disks were detected. This caused day2 nodes with iSCSI disks to fail joining clusters that had no iSCSI disks on day1, because the workaround manifest was not present in the cluster. This PR changes the behavior to always add the workaround manifest. The script installed by the manifest is a no-op if no iSCSI disks are detected, making this change safe. Related: https://issues.redhat.com/browse/OCPBUGS-26580
|
@yoavsc0302: This pull request references MGMT-20499 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. |
openshift-ci-robot
added
the
jira/valid-reference
WalkthroughChanges modify NIC reapply manifest generation from a conditional approach based on ISCSI boot detection to an unconditional strategy that always generates manifests across all roles. Implementation logic is updated alongside test cases to reflect this new behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)internal/network/manifests_generator_test.go (2)
🔇 Additional comments (3)
Comment |
openshift-ci
bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/network/manifests_generator.go (1)
627-627: Fix typo in error log message.The error message contains "reqpply" which should be "reapply".
🔎 Proposed fix
- log.WithError(err).Error("Failed to create nic reqpply manifest") + log.WithError(err).Error("Failed to create nic reapply manifest")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
internal/network/manifests_generator.gointernal/network/manifests_generator_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
internal/network/manifests_generator.gointernal/network/manifests_generator_test.go
🧬 Code graph analysis (1)
internal/network/manifests_generator_test.go (2)
internal/operators/manager.go (1)
Manifest(44-49)models/manifest.go (1)
ManifestFolderOpenshift(71-71)
🔇 Additional comments (3)
internal/network/manifests_generator_test.go (3)
1207-1233: LGTM!The test correctly validates the new unconditional manifest generation behavior. The updated description and expectations (2 manifest creation calls) align with the implementation changes, ensuring manifests are created even for Multipath FC drives.
1234-1241: LGTM!This test case directly validates the core requirement from the PR objectives: manifests are now created unconditionally to support day2 hosts with iSCSI disks, even when day1 hosts don't have iSCSI drives. The test expectations are correct (2 calls for master and worker roles).
1242-1248: LGTM!The failure test correctly validates error propagation and message formatting. The test ensures that manifest creation failures are properly handled and reported with clear context (filename).
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yoavsc0302 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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8705 +/- ##
==========================================
- Coverage 43.50% 43.50% -0.01%
==========================================
Files 411 411
Lines 71264 71254 -10
==========================================
- Hits 31004 30999 -5
+ Misses 37500 37497 -3
+ Partials 2760 2758 -2
🚀 New features to boost your workflow:
|
|
/retest |
|
@yoavsc0302: 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. |
2 participants
A machine config manifest was introduced to work around OCPBUGS-26580, where DNS is not properly set up on first boot when booting from iSCSI storage.
However, this manifest was only added during day1 installation when iSCSI disks were detected. This caused day2 nodes with iSCSI disks to fail joining clusters that had no iSCSI disks on day1, because the workaround manifest was not present in the cluster.
This PR changes the behavior to always add the workaround manifest. The script installed by the manifest is a no-op if no iSCSI disks are detected, making this change safe.
Related: https://issues.redhat.com/browse/OCPBUGS-26580
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