OCPBUGS-85498: BareMetal skew e2e fails patching provisioning CR after CBO webhook fix#6031
OCPBUGS-85498: BareMetal skew e2e fails patching provisioning CR after CBO webhook fix#6031djoshy wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@djoshy: This pull request references Jira Issue OCPBUGS-85498, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 updates a single test case in the BareMetal skew enforcement test suite. The legacy qcow2 simulation now reads provisioning network fields from the CR, constructs a JSON patch that includes those fields alongside a legacy download URL, and restores them during cleanup. ChangesBareMetal Legacy Provisioning Patch Test
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy 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
|
/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-metal-ipi-ovn-ipv6-mco-disruptive-1of2 |
|
@djoshy: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/601c4090-4e3a-11f1-925e-03564194e031-0 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@djoshy: This pull request references Jira Issue OCPBUGS-85498, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@djoshy: This pull request references Jira Issue OCPBUGS-85498, which is valid. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended-priv/mco_bootimages_skew.go`:
- Around line 276-277: The code immediately indexes parts[0..3] after parts :=
strings.SplitN(fields, "\n", 4) which can panic if the split produced fewer than
4 elements; add an explicit length assertion before indexing (e.g., use
o.Expect(parts).To(o.HaveLen(4)) with a clear message) so the test fails with a
helpful diagnostic instead of an index-out-of-range panic; place this check
right after the SplitN call in the same function where originalNetwork,
originalIP, originalCIDR, originalDHCP are assigned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c8bc30a4-8bd1-4d36-9192-4f378bc5ba98
📒 Files selected for processing (1)
test/extended-priv/mco_bootimages_skew.go
| parts := strings.SplitN(fields, "\n", 4) | ||
| originalNetwork, originalIP, originalCIDR, originalDHCP := parts[0], parts[1], parts[2], parts[3] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify parsing/indexing pattern around provisioning field extraction.
rg -n -C3 'SplitN\(|parts\[[0-9]+\]' test/extended-priv/mco_bootimages_skew.goRepository: openshift/machine-config-operator
Length of output: 2091
🏁 Script executed:
cat test/extended-priv/mco_bootimages_skew.go | head -300 | tail -100Repository: openshift/machine-config-operator
Length of output: 5837
🏁 Script executed:
grep -n "strings.SplitN\|parts\[" test/extended-priv/mco_bootimages_skew.goRepository: openshift/machine-config-operator
Length of output: 228
🏁 Script executed:
grep -n "o.Expect.*HaveLen\|o.Expect.*len(" test/extended-priv/mco_bootimages_skew.go | head -20Repository: openshift/machine-config-operator
Length of output: 59
🏁 Script executed:
sed -n '260,290p' test/extended-priv/mco_bootimages_skew.goRepository: openshift/machine-config-operator
Length of output: 2029
🏁 Script executed:
grep -B5 -A5 "strings.Split" test/extended-priv/mco_bootimages_skew.goRepository: openshift/machine-config-operator
Length of output: 714
🏁 Script executed:
wc -l test/extended-priv/mco_bootimages_skew.goRepository: openshift/machine-config-operator
Length of output: 123
🏁 Script executed:
rg "strings\.SplitN.*\n.*parts\[" test/extended-priv/ -A3Repository: openshift/machine-config-operator
Length of output: 269
🏁 Script executed:
rg "o\.Expect.*HaveLen|Expect.*len\(" test/extended-priv/ --type go | head -10Repository: openshift/machine-config-operator
Length of output: 1238
🏁 Script executed:
sed -n '270,282p' test/extended-priv/mco_bootimages_skew.goRepository: openshift/machine-config-operator
Length of output: 814
Add explicit length validation before indexing split result
Line 277 directly indexes parts[0..3] immediately after strings.SplitN(fields, "\n", 4) without checking the resulting slice length. While the jsonpath format is structured to produce 4 fields, missing or empty provisioning fields could cause a panic (index out of range) with no meaningful diagnostic context. This violates the assertion quality guideline.
Add a length check using o.Expect(parts).To(o.HaveLen(4)) to validate the output format and provide clear error messages if the provisioning CR structure is unexpected.
Suggested fix
- parts := strings.SplitN(fields, "\n", 4)
- originalNetwork, originalIP, originalCIDR, originalDHCP := parts[0], parts[1], parts[2], parts[3]
+ parts := strings.SplitN(strings.TrimSpace(fields), "\n", 4)
+ o.Expect(parts).To(o.HaveLen(4), "Expected 4 provisioning fields from CR, got %d in: %q", len(parts), fields)
+ originalNetwork := strings.TrimSpace(parts[0])
+ originalIP := strings.TrimSpace(parts[1])
+ originalCIDR := strings.TrimSpace(parts[2])
+ originalDHCP := strings.TrimSpace(parts[3])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parts := strings.SplitN(fields, "\n", 4) | |
| originalNetwork, originalIP, originalCIDR, originalDHCP := parts[0], parts[1], parts[2], parts[3] | |
| parts := strings.SplitN(strings.TrimSpace(fields), "\n", 4) | |
| o.Expect(parts).To(o.HaveLen(4), "Expected 4 provisioning fields from CR, got %d in: %q", len(parts), fields) | |
| originalNetwork := strings.TrimSpace(parts[0]) | |
| originalIP := strings.TrimSpace(parts[1]) | |
| originalCIDR := strings.TrimSpace(parts[2]) | |
| originalDHCP := strings.TrimSpace(parts[3]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended-priv/mco_bootimages_skew.go` around lines 276 - 277, The code
immediately indexes parts[0..3] after parts := strings.SplitN(fields, "\n", 4)
which can panic if the split produced fewer than 4 elements; add an explicit
length assertion before indexing (e.g., use o.Expect(parts).To(o.HaveLen(4))
with a clear message) so the test fails with a helpful diagnostic instead of an
index-out-of-range panic; place this check right after the SplitN call in the
same function where originalNetwork, originalIP, originalCIDR, originalDHCP are
assigned.
2 participants
- What I did
The
Verify BareMetal defaults to None mode and flips to Manual on legacy provisioning pathtest began failing after cluster-baremetal-operator PR #587 (merged May 4, 2026) fixed thevprovisioning.kb.iovalidating webhook server. Prior to that fix, a missingWithValidator(r)call meant no webhook handlers were ever registered; combined withfailurePolicy: Ignore, all patch requests to the provisioning CR silently passed. The test was added in 9971d2f under those conditions.After the webhook fix, patching provisioningOSDownloadURL alone is rejected on machine-os-images clusters (4.10+). We need to our patch request so it passes the webhook’s validation, which requires certain values to be read back and set.
- How to verify it
The baremetal e2es should now begin to pass, no additional QE is necessary.
Summary by CodeRabbit