Skip to content

OCPBUGS-87962: Add OCP-89593 test for boot image skew reset on reconcile-skip #6331

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.

Sign up for GitHub

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

Open
ptalgulk01 wants to merge 1 commit into
base: main
Choose a base branch
from
+62 −0
Open

OCPBUGS-87962: Add OCP-89593 test for boot image skew reset on reconcile-skip #6331

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions test/extended-priv/mco_bootimages_skew.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package extended

import (
"context"
"fmt"

g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"
Expand Down Expand Up @@ -296,4 +297,65 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/disruptive
// Check machine-config CO upgradeable status, should be set to true
o.Eventually(mcoCO, "1m", "10s").Should(BeUpgradeable(), "co/machine-config should be upgradeable when skew enforcement is disabled (None mode)")
})

// https://issues.redhat.com/browse/OCPBUGS-87962
g.It("[PolarionID:89593][OTP] Boot image skew check correctly reflects actual boot image version when MachineSets are reconcile-skipped [Disruptive]", g.Label("Platform:aws", "Platform:gce", "Platform:azure", "Platform:vsphere"), func() {
skipTestIfSupportedPlatformNotMatched(oc, GCPPlatform, AWSPlatform, AzurePlatform, VspherePlatform)

automaticOCPVersionPath := `{.status.bootImageSkewEnforcementStatus.automatic.ocpVersion}`

exutil.By("Ensure Automatic mode is active")
o.Expect(machineConfiguration.RemoveSkew()).To(o.Succeed())
machineConfiguration.WaitForBootImageSkewEnforcementStatusMode(SkewEnforcementAutomaticMode)
machineConfiguration.WaitForBootImageControllerComplete()
logger.Infof("OK!\n")

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()
}()
Comment on lines +335 to +339

@coderabbitai coderabbitai Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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:

🏁 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 || true

Repository: 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 || true

Repository: 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.

Suggested change
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.

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")
Comment on lines +313 to +348

@coderabbitai coderabbitai Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Comment on lines +342 to +348

@coderabbitai coderabbitai Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.


exutil.By("Remove OwnerReference and verify automatic.ocpVersion is restored")
o.Expect(machineSetUnderTest.Patch("json", `[{"op":"remove","path":"/metadata/ownerReferences"}]`)).To(o.Succeed())
machineConfiguration.WaitForBootImageControllerComplete()
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)
Comment on lines +353 to +357

@coderabbitai coderabbitai Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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)
As per path instructions, "Never ignore error returns".
📝 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.

Suggested change
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

logger.Infof("automatic.ocpVersion restored to: %s", originalOCPVersion)
logger.Infof("OK!\n")
})
})