OCPBUGS-87962: Add OCP-89593 test for boot image skew reset on reconcile-skip#6331
OCPBUGS-87962: Add OCP-89593 test for boot image skew reset on reconcile-skip#6331ptalgulk01 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
|
@ptalgulk01: This pull request references Jira Issue OCPBUGS-87962, 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. |
WalkthroughAdds an extended-privileged Ginkgo test that validates ChangesBoot Image Skew Validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ptalgulk01 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.
Actionable comments posted: 4
🤖 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 342-348: Update the post-patch verification around
WaitForBootImageControllerComplete to poll Get(automaticOCPVersionPath) with the
same Eventually-based strategy used by the restoration check below. Preserve the
existing error and non-empty assertions, logging, and controller wait while
allowing resetClusterBootImage status updates to settle asynchronously.
- Around line 313-348: Strengthen the assertions in the test flow around
machineConfiguration.Get and resetClusterBootImage: after reconcile-skip, verify
patchedOCPVersion equals the cluster install version from the ClusterVersion
desired version, rather than only checking it is non-empty. Preserve the
existing restoration checks and use the cluster-version retrieval mechanism
already established in the test or nearby helpers.
- Around line 353-357: Update the Eventually polling closure around
machineConfiguration.Get to return both its value and error directly instead of
discarding the error. Preserve the existing automatic.ocpVersion path and
equality assertion so Eventually retries on non-nil errors and validates the
restored version.
- Around line 335-339: Make the deferred cleanup around machineSetUnderTest
idempotent: before removing /metadata/ownerReferences, ensure the owner
reference exists or tolerate an already-removed path, while preserving the
existing restoration and MachineForBootImageControllerComplete wait behavior.
🪄 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: Pro Plus
Run ID: 1a0cf095-3672-4c2c-93c4-fa2f19bb9e7e
📒 Files selected for processing (1)
test/extended-priv/mco_bootimages_skew.go
| exutil.By("Get the automatic.ocpVersion before patch") | ||
| originalOCPVersion, err := machineConfiguration.Get(automaticOCPVersionPath) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Error getting automatic.ocpVersion from %s", machineConfiguration) | ||
| o.Expect(originalOCPVersion).NotTo(o.BeEmpty(), "automatic.ocpVersion should not be empty in Automatic mode") | ||
| logger.Infof("automatic.ocpVersion before patch: %s", originalOCPVersion) | ||
| logger.Infof("OK!\n") | ||
|
|
||
| exutil.By("Pick a MachineSet and add an OwnerReference to trigger reconcile-skip") | ||
| machineSetUnderTest := NewMachineSetList(oc.AsAdmin(), MachineAPINamespace).GetAllOrFail()[0] | ||
| logger.Infof("MachineSet under test: %s", machineSetUnderTest.name) | ||
|
|
||
| infraUID, err := oc.AsAdmin().WithoutNamespace().Run("get").Args( | ||
| "infrastructure", "cluster", "-o", "jsonpath={.metadata.uid}", | ||
| ).Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Failed to get Infrastructure UID") | ||
|
|
||
| ownerRefPatch := fmt.Sprintf( | ||
| `{"metadata":{"ownerReferences":[{"apiVersion":"config.openshift.io/v1","kind":"Infrastructure","name":"cluster","uid":"%s"}]}}`, | ||
| infraUID) | ||
| o.Expect(machineSetUnderTest.Patch("merge", ownerRefPatch)).To(o.Succeed()) | ||
| logger.Infof("Added OwnerReference to MachineSet %s to trigger reconcile-skip", machineSetUnderTest.name) | ||
|
|
||
| defer func() { | ||
| exutil.By("Restoring MachineSet by removing OwnerReference") | ||
| o.Expect(machineSetUnderTest.Patch("json", `[{"op":"remove","path":"/metadata/ownerReferences"}]`)).To(o.Succeed()) | ||
| machineConfiguration.WaitForBootImageControllerComplete() | ||
| }() | ||
| logger.Infof("OK!\n") | ||
|
|
||
| exutil.By("Wait for boot image controller to process and get automatic.ocpVersion after patch") | ||
| machineConfiguration.WaitForBootImageControllerComplete() | ||
| patchedOCPVersion, err := machineConfiguration.Get(automaticOCPVersionPath) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Error getting automatic.ocpVersion after patch") | ||
| o.Expect(patchedOCPVersion).NotTo(o.BeEmpty(), "automatic.ocpVersion should not be empty after reconcile-skip") | ||
| logger.Infof("automatic.ocpVersion after patch: %s", patchedOCPVersion) | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assertion only checks non-emptiness, not that resetClusterBootImage actually fired.
Per the controller contract (resetClusterBootImage, pkg/controller/bootimage/boot_image_controller.go:652-685), reconcile-skip should overwrite automatic.ocpVersion with the cluster's install version, not just "any non-empty string". If originalOCPVersion is already non-empty (the typical case) and this reset logic silently regresses, patchedOCPVersion would still be non-empty (because it never changed), and the final restoration check at lines 353-357 would then also trivially pass since the value never diverged from originalOCPVersion. The test could pass while the exact fix it's meant to validate (OCPBUGS-87962 / resetClusterBootImage) is broken.
Consider asserting that patchedOCPVersion equals the cluster's install version (e.g., via oc get clusterversion version -o jsonpath='{.status.desired.version}' or similar), or at minimum that it differs from originalOCPVersion when they're expected to diverge.
🤖 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 313 - 348, Strengthen
the assertions in the test flow around machineConfiguration.Get and
resetClusterBootImage: after reconcile-skip, verify patchedOCPVersion equals the
cluster install version from the ClusterVersion desired version, rather than
only checking it is non-empty. Preserve the existing restoration checks and use
the cluster-version retrieval mechanism already established in the test or
nearby helpers.
| defer func() { | ||
| exutil.By("Restoring MachineSet by removing OwnerReference") | ||
| o.Expect(machineSetUnderTest.Patch("json", `[{"op":"remove","path":"/metadata/ownerReferences"}]`)).To(o.Succeed()) | ||
| machineConfiguration.WaitForBootImageControllerComplete() | ||
| }() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Kubernetes API server reject a JSON Patch (RFC 6902) "remove" operation when the target path no longer exists?
💡 Result:
Yes, the Kubernetes API server rejects a JSON Patch (RFC 6902) remove operation if the target path does not exist [1][2]. According to the RFC 6902 specification, the target location for a "remove" operation must exist for the operation to be successful [3][4]. Because Kubernetes implements JSON Patch in accordance with this RFC, attempting to remove a non-existent path will result in a failure [1][5][2]. When such an error occurs, the API server rejects the request, typically returning an error (often a 422 Unprocessable Entity status code) [1]. This behavior is consistent with the standard's requirement that if any operation in a JSON Patch sequence fails, the entire patch is aborted [6][4]. If you need to avoid this error, you must ensure the path exists before attempting the removal, or handle the error on the client side [1][2]. Note that some specific Kubernetes-related tools or libraries may have their own wrapper implementations or configuration options (e.g., in admission controllers or specialized controllers) that might handle these operations differently, but the core Kubernetes API server enforces the strict RFC 6902 requirement [7][1][2].
Citations:
- 1: k8s_json_patch should not fail on op: remove ansible-collections/kubernetes.core#648
- 2: [BUG] patchesJson6902 remove operation fails on non-existant key kyverno/kyverno#1579
- 3: https://datatracker.ietf.org/doc/html/rfc6902
- 4: https://jsonic.io/guides/json-patch
- 5: null vs. empty array issues in json patch kubernetes/kubernetes#70281
- 6: https://jsonpatch.com/
- 7: kubeadm patch (type json - RFC 6902 conform) doesn't ensure path if not exists kubernetes/kubeadm#2770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching target:\n'
fd -a 'mco_bootimages_skew\.go$' . || true
printf '\nRelevant lines:\n'
file="$(fd 'mco_bootimages_skew\.go$' . | head -n1)"
if [ -n "${file:-}" ]; then
sed -n '320,365p' "$file" | nl -ba -v320
fi
printf '\nSearch for ownerReferences removal in test and ms helpers:\n'
rg -n 'ownerReferences|OwnerReference|reconcileSkipped|Automatic\.OCPVersion|ResetClusterBootImage|waitForBootImageControllerComplete|WaitForBootImageControllerComplete' test/extended-priv/mco_bootimages_skew.go pkg test || trueRepository: openshift/machine-config-operator
Length of output: 313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'mco_bootimages_skew\.go$' . | head -n1)"
echo "FILE=$file"
echo "== lines 320-365 =="
sed -n '320,365p' "$file" | awk '{printf "%7d\t%s\n", NR+319, $0}'
echo "== relevant searches =="
rg -n 'ownerReferences|OwnerReference|reconcileSkipped|Automatic\.OCPVersion|ResetClusterBootImage|waitForBootImageControllerComplete|WaitForBootImageControllerComplete' test/extended-priv/mco_bootimages_skew.go pkg || trueRepository: openshift/machine-config-operator
Length of output: 13527
Make the owner reference cleanup idempotent.
Line 351 removes /metadata/ownerReferences with a JSON patch, and the defer cleanup repeats the same removal. A JSON Patch remove on a path that no longer exists fails, so the deferred cleanup can make an otherwise passing spec fail.
🐛 Proposed fix: make cleanup idempotent
+ ownerRefRemoved := false
defer func() {
+ if ownerRefRemoved {
+ return
+ }
exutil.By("Restoring MachineSet by removing OwnerReference")
o.Expect(machineSetUnderTest.Patch("json", `[{"op":"remove","path":"/metadata/ownerReferences"}]`)).To(o.Succeed())
machineConfiguration.WaitForBootImageControllerComplete()
}()
...
exutil.By("Remove OwnerReference and verify automatic.ocpVersion is restored")
o.Expect(machineSetUnderTest.Patch("json", `[{"op":"remove","path":"/metadata/ownerReferences"}]`)).To(o.Succeed())
+ ownerRefRemoved = true
machineConfiguration.WaitForBootImageControllerComplete()📝 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.
| defer func() { | |
| exutil.By("Restoring MachineSet by removing OwnerReference") | |
| o.Expect(machineSetUnderTest.Patch("json", `[{"op":"remove","path":"/metadata/ownerReferences"}]`)).To(o.Succeed()) | |
| machineConfiguration.WaitForBootImageControllerComplete() | |
| }() | |
| ownerRefRemoved := false | |
| defer func() { | |
| if ownerRefRemoved { | |
| return | |
| } | |
| exutil.By("Restoring MachineSet by removing OwnerReference") | |
| o.Expect(machineSetUnderTest.Patch("json", `[{"op":"remove","path":"/metadata/ownerReferences"}]`)).To(o.Succeed()) | |
| machineConfiguration.WaitForBootImageControllerComplete() | |
| }() | |
| ... | |
| exutil.By("Remove OwnerReference and verify automatic.ocpVersion is restored") | |
| o.Expect(machineSetUnderTest.Patch("json", `[{"op":"remove","path":"/metadata/ownerReferences"}]`)).To(o.Succeed()) | |
| ownerRefRemoved = true | |
| machineConfiguration.WaitForBootImageControllerComplete() |
🤖 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 335 - 339, Make the
deferred cleanup around machineSetUnderTest idempotent: before removing
/metadata/ownerReferences, ensure the owner reference exists or tolerate an
already-removed path, while preserving the existing restoration and
MachineForBootImageControllerComplete wait behavior.
| exutil.By("Wait for boot image controller to process and get automatic.ocpVersion after patch") | ||
| machineConfiguration.WaitForBootImageControllerComplete() | ||
| patchedOCPVersion, err := machineConfiguration.Get(automaticOCPVersionPath) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Error getting automatic.ocpVersion after patch") | ||
| o.Expect(patchedOCPVersion).NotTo(o.BeEmpty(), "automatic.ocpVersion should not be empty after reconcile-skip") | ||
| logger.Infof("automatic.ocpVersion after patch: %s", patchedOCPVersion) | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Inconsistent wait strategy vs. the restoration check below.
This assertion uses a single synchronous o.Expect right after WaitForBootImageControllerComplete(), whereas the restoration check at lines 353-357 wraps the identical Get call in o.Eventually. Given resetClusterBootImage's status update can lag independently of the "controller complete" signal, this asymmetry risks flakiness here that the later check was clearly written to guard against.
🤖 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 342 - 348, Update the
post-patch verification around WaitForBootImageControllerComplete to poll
Get(automaticOCPVersionPath) with the same Eventually-based strategy used by the
restoration check below. Preserve the existing error and non-empty assertions,
logging, and controller wait while allowing resetClusterBootImage status updates
to settle asynchronously.
| o.Eventually(func() string { | ||
| v, _ := machineConfiguration.Get(automaticOCPVersionPath) | ||
| return v | ||
| }, "2m", "10s").Should(o.Equal(originalOCPVersion), | ||
| "automatic.ocpVersion should be restored to %s after removing OwnerReference", originalOCPVersion) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Error from Get is silently discarded.
v, _ := machineConfiguration.Get(...) drops the error inside the polling closure. Gomega's Eventually supports functions returning (value, error) directly and will retry automatically on a non-nil error, which is more idiomatic than swallowing it here.
♻️ Suggested simplification
- o.Eventually(func() string {
- v, _ := machineConfiguration.Get(automaticOCPVersionPath)
- return v
- }, "2m", "10s").Should(o.Equal(originalOCPVersion),
+ o.Eventually(func() (string, error) {
+ return machineConfiguration.Get(automaticOCPVersionPath)
+ }, "2m", "10s").Should(o.Equal(originalOCPVersion),
"automatic.ocpVersion should be restored to %s after removing OwnerReference", originalOCPVersion)📝 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.
| o.Eventually(func() string { | |
| v, _ := machineConfiguration.Get(automaticOCPVersionPath) | |
| return v | |
| }, "2m", "10s").Should(o.Equal(originalOCPVersion), | |
| "automatic.ocpVersion should be restored to %s after removing OwnerReference", originalOCPVersion) | |
| o.Eventually(func() (string, error) { | |
| return machineConfiguration.Get(automaticOCPVersionPath) | |
| }, "2m", "10s").Should(o.Equal(originalOCPVersion), | |
| "automatic.ocpVersion should be restored to %s after removing OwnerReference", originalOCPVersion) |
🤖 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 353 - 357, Update the
Eventually polling closure around machineConfiguration.Get to return both its
value and error directly instead of discarding the error. Preserve the existing
automatic.ocpVersion path and equality assertion so Eventually retries on
non-nil errors and validates the restored version.
Source: Path instructions
|
@ptalgulk01: 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
Summary
Approach
Manual verification
Question for dev
On a non-upgraded cluster the version doesn't visibly change because resetClusterBootImage() writes the same install version as updateClusterBootImage() writes from the configmap. Should the test assert a version difference (requiring an upgraded cluster), or is verifying the controller handles the skip path without degrading sufficient?
Summary by CodeRabbit