[TEST ONLY] test(e2e/v2): add upsert desired-state-hash E2E tests - #9236
[TEST ONLY] test(e2e/v2): add upsert desired-state-hash E2E tests #9236mgencur wants to merge 6 commits into
Conversation
DeepDerivative treats nil/empty fields as "don't care", making it impossible to detect when spec fields like nodeSelector, tolerations, or container args are explicitly removed. This has been a recurring class of bugs (e.g., OCPBUGS-65879). Add a SHA-256 hash of the desired manifest state as a lightweight annotation (~64 bytes). On each reconcile the hash is computed before metadata merging and compared against the stored value. When they differ, an update is forced — catching removals that DeepDerivative misses. This replaces the earlier full-JSON last-applied-configuration approach (PR openshift#7713) which was held due to etcd size explosion at fleet scale and security concerns from embedding Secret data in annotations. A hash has none of these costs. DeepDerivative is retained as a fallback for drift detection, and the existing label-count removal workaround is preserved since hash- based detection cannot drive three-way metadata merges. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a case where the hash still matches but the live object's spec was modified externally, verifying the DeepDerivative fallback still detects and corrects the drift.
Add four E2E v2 tests covering the desired-state-hash annotation introduced in support/upsert.ApplyManifest (PR openshift#7713): - VerifyDesiredStateHashAnnotationStamped: asserts every managed Deployment in the control plane namespace carries a valid 64-char hex desired-state-hash annotation (Scenario 1). - VerifyDesiredStateHashIdempotency: records Deployment resourceVersions, waits 2 minutes across multiple reconcile cycles, and asserts none changed — verifying no hot-loop updates on stable clusters (Scenario 3). - VerifyExternalDriftReverted: patches spec.replicas out-of-band and asserts the reconciler reverts the drift via the DeepDerivative fallback, confirming the hash annotation itself remains unchanged (Scenario 5). Marked Informing (non-blocking). - VerifyServiceAccountPullSecretsPreserved: asserts imagePullSecrets injected by Kubernetes are not wiped by the reconciler (Scenario 8b).
…-hash Add VerifySpecFieldRemovalDetected: patches HC.Spec.Tolerations to add a harmless test toleration, waits for it to reach a managed Deployment, then removes it from HC spec and asserts the Deployment is updated. Without the hash fix, DeepDerivative treats the desired slice as a prefix and misses the removal — the test's final Eventually would time out.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant HostedCluster
participant ApplyManifest
participant KubernetesAPI
participant ManagedDeployment
HostedCluster->>ApplyManifest: provide desired object
ApplyManifest->>ApplyManifest: normalize object and compute SHA-256 hash
ApplyManifest->>KubernetesAPI: get existing object
KubernetesAPI-->>ApplyManifest: return object and stored hash
ApplyManifest->>ManagedDeployment: compare hash and detect drift
ApplyManifest->>KubernetesAPI: create or update object with refreshed hash
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
|
/test e2e-v2-aws |
openshift-ci
Bot
added
area/platform/aws
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mgencur The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
area/platform/gcp
openshift-ci
Bot
added
area/platform/kubevirt
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
support/upsert/apply_test.go (2)
355-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the "When ... it should ..." naming format for the subtests.
The repository guideline for
**/*_test.gorequires the "When ... it should ..." format for unit test case descriptions. The subtest names inTestApplyManifest_DesiredStateHashuse a different form, for example"create stamps the hash annotation"and"trailing slice removal detected".Rename them, for example
"When the object does not exist it should stamp the hash annotation".The repository guideline states: "Always use 'When ... it should ...' format for describing test cases when creating unit tests".
🤖 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 `@support/upsert/apply_test.go` around lines 355 - 374, Rename the subtests in TestApplyManifest_DesiredStateHash, including “create stamps the hash annotation” and “trailing slice removal detected,” to follow the “When ... it should ...” naming format while preserving their existing test behavior.Source: Coding guidelines
534-555: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the label removal path.
The suite covers create, migration, idempotency, drift, and slice removal. It does not cover the interaction between the new hash comparison and the existing
RemoveLabelMarkerhandling at Lines 152-158 ofsupport/upsert/apply.go. That branch setsneedsUpdate = trueindependently of the hash. Add a subtest that removes a label and assertsOperationResultUpdated, and a follow-up reconcile that assertsOperationResultNone.The second assertion matters. The label-removal check compares the mutated label count against the existing label count, and the hash is stamped in the same update. A regression there would produce a permanent update loop that the current tests would not catch.
🤖 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 `@support/upsert/apply_test.go` around lines 534 - 555, Add a subtest near the existing ApplyManifest reconciliation tests that creates a deployment containing the RemoveLabelMarker, applies it, removes the marker, and asserts OperationResultUpdated. Reconcile the cleaned deployment again and assert OperationResultNone, using the existing deployment helpers and result symbols to verify label removal does not trigger a loop.Source: Coding guidelines
support/upsert/apply.go (1)
133-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe hash is not computed from pure desired state for ServiceAccount and Deployment.
The comment says the hash captures "our pure desired state". That is not accurate. The type switch at Lines 123-131 runs before Line 135 and mutates
obj:
preserveServiceAccountPullSecretscopies cluster-injectedimagePullSecretsfromexistingintoobj.- The Deployment case copies
Spec.Selectorfromexistingintoobj.The hash therefore includes live-cluster data. For a newly created ServiceAccount this produces one guaranteed extra update: the create-path hash has no pull secrets, and the first update-path hash has the token secret name that Kubernetes injected. It converges after that reconcile, so the impact is bounded. Update the comment to describe the actual inputs, so a later reader does not assume the hash is a pure function of the manifest.
📝 Proposed comment change
- // Compute desired hash BEFORE preserveOriginalMetadata merges existing - // metadata into obj. This captures our pure desired state. + // Compute the desired hash BEFORE preserveOriginalMetadata merges existing + // metadata into obj. Note that the type switch above has already copied a few + // immutable or cluster-injected fields (ServiceAccount imagePullSecrets, + // Deployment spec.selector) from the existing object into obj, so those are + // part of the hashed input by design. desiredHash := computeDesiredHash(obj)🤖 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 `@support/upsert/apply.go` around lines 133 - 150, Update the comment above computeDesiredHash in the apply flow to accurately state that the hash is computed after ServiceAccount pull secrets or Deployment selectors may be copied from existing, cluster-managed state. Remove the claim that it represents pure desired state, while preserving the existing hash computation and metadata-preservation behavior.test/e2e/v2/tests/control_plane_upsert_test.go (1)
40-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a doc comment to the exported
RegisterUpsertTestsfunction.The path instructions require comments on exported functions that describe actual behavior, including panic conditions.
RegisterUpsertTestsis exported and has no comment. Every other exported function in this file has one.The path instruction states: "Comments on exported functions must describe actual behavior, including panic conditions, not just intended behavior." and "Exported test helpers need comments describing actual behavior and panic conditions."
📝 Proposed comment
+// RegisterUpsertTests registers all upsert desired-state-hash Ginkgo containers. +// It must be called from inside a Describe body. getTestCtx is invoked inside each +// It block, not at registration time. func RegisterUpsertTests(getTestCtx internal.TestContextGetter) {🤖 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/e2e/v2/tests/control_plane_upsert_test.go` around lines 40 - 46, Add a Go doc comment immediately before the exported RegisterUpsertTests function, describing that it registers the upsert end-to-end tests through the provided TestContextGetter and documenting any actual panic conditions if applicable, consistent with the comments on other exported functions in the file.Source: Path instructions
hypershift-operator/controllers/nodepool/capi_test.go (1)
758-762: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
if okguard makes the annotation assertion tolerate a missing hash.If
ApplyManifeststops stampingupsert.DesiredStateHashAnnotation, the block is skipped. Both maps then contain onlynodePoolAnnotationand Line 762 still passes. Delete the expected annotation instead of copying it. That keeps this test focused on the NodePool annotation and removes the conditional.♻️ Proposed change
- // ApplyManifest adds a desired-state-hash annotation; copy it so the assertion compares the rest of the annotations. - if hash, ok := gotMachineTemplate.Annotations[upsert.DesiredStateHashAnnotation]; ok { - expectedMachineTemplate.Annotations[upsert.DesiredStateHashAnnotation] = hash - } - g.Expect(expectedMachineTemplate.ObjectMeta.Annotations).To(BeEquivalentTo(gotMachineTemplate.ObjectMeta.Annotations)) + // ApplyManifest adds a desired-state-hash annotation; drop it so the assertion compares the rest of the annotations. + gotAnnotations := maps.Clone(gotMachineTemplate.ObjectMeta.Annotations) + delete(gotAnnotations, upsert.DesiredStateHashAnnotation) + g.Expect(expectedMachineTemplate.ObjectMeta.Annotations).To(BeEquivalentTo(gotAnnotations))This requires the
mapsimport.🤖 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 `@hypershift-operator/controllers/nodepool/capi_test.go` around lines 758 - 762, Update the annotation comparison around gotMachineTemplate and expectedMachineTemplate to remove upsert.DesiredStateHashAnnotation from both maps before asserting equivalence, rather than conditionally copying it into the expected map. Add and use the maps import as needed, and remove the if/ok guard so the test remains focused on the NodePool annotation.
🤖 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 `@support/upsert/apply.go`:
- Around line 304-314: The hashing flow must propagate failures instead of
silently using empty values: update toUnstructured to check and return the
unstructured.SetNestedField error, change computeDesiredHash to return (string,
error) and wrap failures from toUnstructured and json.Marshal, then update its
callers at support/upsert/apply.go lines 78-80 and 135 to return the error
rather than continue. The anchor support/upsert/apply.go lines 304-314 and
sibling lines 290-302 both require these changes.
- Around line 290-302: Update the annotation cleanup block in the surrounding
function to handle the error returned by unstructured.SetNestedField instead of
discarding it; if the update fails, return that error immediately so the hash
annotation cannot remain in the value used for hashing and DeepDerivative.
In `@test/e2e/v2/tests/control_plane_upsert_test.go`:
- Around line 165-186: Move the spec containing the `hc.Spec.Tolerations` patch
into the lifecycle test set, or document on its enclosing `Describe` that the
upsert suite is approved to run as a lifecycle suite. Keep the existing
`DeferCleanup` cleanup behavior unchanged.
- Around line 133-138: Update both Deployment target-selection sites in
test/e2e/v2/tests/control_plane_upsert_test.go:133-138 and
test/e2e/v2/tests/control_plane_upsert_test.go:237-245. In the Step 2 flow,
iterate deployList.Items to select a Deployment that receives the HostedCluster
toleration, then assert that a matching target was found before Step 3. In the
later flow, filter deployments by upsert.DesiredStateHashAnnotation before
selecting the target, and Skip() when target.Spec.Replicas is nil rather than
assuming a default replica count.
- Around line 88-111: Replace the Deployment resourceVersion snapshot and
comparison in this test with metadata.generation so status-only writes do not
appear as spec changes; preserve the existing handling for newly appearing
Deployments. Remove the unconditional two-minute time.Sleep and wait with the
test’s existing polling/eventual mechanism for the stability window instead.
Address cross-test ordering at the surrounding Describe level, such as marking
the specs Ordered, so mutations from VerifySpecFieldRemovalDetected and
VerifyExternalDriftReverted cannot contaminate this idempotency check.
- Around line 296-317: Update VerifyServiceAccountPullSecretsPreserved to filter
the listed ServiceAccounts to those carrying upsert.DesiredStateHashAnnotation
and assert the filtered list is non-empty. Capture each managed account’s
imagePullSecret names, wait for at least one reconcile cycle using the existing
test utilities, then re-list the managed accounts and assert the captured names
remain present.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/capi_test.go`:
- Around line 758-762: Update the annotation comparison around
gotMachineTemplate and expectedMachineTemplate to remove
upsert.DesiredStateHashAnnotation from both maps before asserting equivalence,
rather than conditionally copying it into the expected map. Add and use the maps
import as needed, and remove the if/ok guard so the test remains focused on the
NodePool annotation.
In `@support/upsert/apply_test.go`:
- Around line 355-374: Rename the subtests in
TestApplyManifest_DesiredStateHash, including “create stamps the hash
annotation” and “trailing slice removal detected,” to follow the “When ... it
should ...” naming format while preserving their existing test behavior.
- Around line 534-555: Add a subtest near the existing ApplyManifest
reconciliation tests that creates a deployment containing the RemoveLabelMarker,
applies it, removes the marker, and asserts OperationResultUpdated. Reconcile
the cleaned deployment again and assert OperationResultNone, using the existing
deployment helpers and result symbols to verify label removal does not trigger a
loop.
In `@support/upsert/apply.go`:
- Around line 133-150: Update the comment above computeDesiredHash in the apply
flow to accurately state that the hash is computed after ServiceAccount pull
secrets or Deployment selectors may be copied from existing, cluster-managed
state. Remove the claim that it represents pure desired state, while preserving
the existing hash computation and metadata-preservation behavior.
In `@test/e2e/v2/tests/control_plane_upsert_test.go`:
- Around line 40-46: Add a Go doc comment immediately before the exported
RegisterUpsertTests function, describing that it registers the upsert end-to-end
tests through the provided TestContextGetter and documenting any actual panic
conditions if applicable, consistent with the comments on other exported
functions in the file.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
|
||
| // Remove the hash annotation to avoid self-referential comparison. | ||
| if annotations, ok, _ := unstructured.NestedMap(u, "metadata", "annotations"); ok { | ||
| delete(annotations, DesiredStateHashAnnotation) | ||
| if len(annotations) == 0 { | ||
| unstructured.RemoveNestedField(u, "metadata", "annotations") | ||
| } else { | ||
| _ = unstructured.SetNestedField(u, annotations, "metadata", "annotations") | ||
| } | ||
| } | ||
|
|
||
| return u, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check the SetNestedField error.
Line 297 discards the error with _ =. SetNestedField fails when the value contains types that are not valid deep-copy JSON values. If it fails here, the hash annotation stays in the map used for hashing and for DeepDerivative, which reintroduces the self-reference this block exists to remove. Return the error instead.
The repository guideline states "Always check errors — don't ignore them."
🛠️ Proposed fix
if annotations, ok, _ := unstructured.NestedMap(u, "metadata", "annotations"); ok {
delete(annotations, DesiredStateHashAnnotation)
if len(annotations) == 0 {
unstructured.RemoveNestedField(u, "metadata", "annotations")
} else {
- _ = unstructured.SetNestedField(u, annotations, "metadata", "annotations")
+ if err := unstructured.SetNestedField(u, annotations, "metadata", "annotations"); err != nil {
+ return nil, fmt.Errorf("failed to set normalized annotations: %w", err)
+ }
}
}📝 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.
| // Remove the hash annotation to avoid self-referential comparison. | |
| if annotations, ok, _ := unstructured.NestedMap(u, "metadata", "annotations"); ok { | |
| delete(annotations, DesiredStateHashAnnotation) | |
| if len(annotations) == 0 { | |
| unstructured.RemoveNestedField(u, "metadata", "annotations") | |
| } else { | |
| _ = unstructured.SetNestedField(u, annotations, "metadata", "annotations") | |
| } | |
| } | |
| return u, nil | |
| } | |
| // Remove the hash annotation to avoid self-referential comparison. | |
| if annotations, ok, _ := unstructured.NestedMap(u, "metadata", "annotations"); ok { | |
| delete(annotations, DesiredStateHashAnnotation) | |
| if len(annotations) == 0 { | |
| unstructured.RemoveNestedField(u, "metadata", "annotations") | |
| } else { | |
| if err := unstructured.SetNestedField(u, annotations, "metadata", "annotations"); err != nil { | |
| return nil, fmt.Errorf("failed to set normalized annotations: %w", err) | |
| } | |
| } | |
| } | |
| return u, nil | |
| } |
🤖 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 `@support/upsert/apply.go` around lines 290 - 302, Update the annotation
cleanup block in the surrounding function to handle the error returned by
unstructured.SetNestedField instead of discarding it; if the update fails,
return that error immediately so the hash annotation cannot remain in the value
used for hashing and DeepDerivative.
Source: Coding guidelines
| func computeDesiredHash(obj crclient.Object) string { | ||
| u, err := toUnstructured(obj) | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| data, err := json.Marshal(u) | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| return fmt.Sprintf("%x", sha256.Sum256(data)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The new hashing helpers discard errors instead of propagating them. Both sites drop an error and continue with a degraded value, so a hashing failure produces no log, no error, and a silently weaker reconcile. The repository guideline states "Always check errors — don't ignore them."
support/upsert/apply.go#L304-L314: changecomputeDesiredHashto return(string, error)and wrap thetoUnstructuredandjson.Marshalfailures; update the callers at Lines 78-80 and Line 135 to return the error rather than proceed with an empty hash, which currently leaves a stalestoredHashon the object forever because Line 181 skips stamping.support/upsert/apply.go#L290-L302: check theunstructured.SetNestedFielderror at Line 297 and return it fromtoUnstructured; if it fails, the hash annotation stays in the normalized map and reintroduces the self-reference this block removes.
📍 Affects 1 file
support/upsert/apply.go#L304-L314(this comment)support/upsert/apply.go#L290-L302
🤖 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 `@support/upsert/apply.go` around lines 304 - 314, The hashing flow must
propagate failures instead of silently using empty values: update toUnstructured
to check and return the unstructured.SetNestedField error, change
computeDesiredHash to return (string, error) and wrap failures from
toUnstructured and json.Marshal, then update its callers at
support/upsert/apply.go lines 78-80 and 135 to return the error rather than
continue. The anchor support/upsert/apply.go lines 304-314 and sibling lines
290-302 both require these changes.
Source: Coding guidelines
| initialVersions := make(map[string]string, len(deployList.Items)) | ||
| for i := range deployList.Items { | ||
| d := &deployList.Items[i] | ||
| initialVersions[d.Name] = d.ResourceVersion | ||
| } | ||
|
|
||
| // Wait long enough to cover at least two reconcile cycles. | ||
| time.Sleep(2 * time.Minute) | ||
|
|
||
| updatedList := &appsv1.DeploymentList{} | ||
| Expect(tc.MgmtClient.List(tc.Context, updatedList, crclient.InNamespace(tc.ControlPlaneNamespace))). | ||
| To(Succeed(), "failed to re-list Deployments in namespace %s after wait", tc.ControlPlaneNamespace) | ||
|
|
||
| for i := range updatedList.Items { | ||
| d := &updatedList.Items[i] | ||
| initialVersion, existed := initialVersions[d.Name] | ||
| if !existed { | ||
| continue // new Deployment appeared during the wait; skip | ||
| } | ||
| Expect(d.ResourceVersion).To(Equal(initialVersion), | ||
| "Deployment %s/%s had unexpected resourceVersion change — possible reconcile hot-loop", | ||
| d.Namespace, d.Name) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
resourceVersion is the wrong signal for spec-level idempotency, and the fixed sleep makes the test slow.
Two problems here:
resourceVersionchanges on any write to the object, including status subresource writes. The Deployment controller writes status routinely: replica counts,Available/Progressingconditions,observedGeneration. A single pod restart or a rolling node event during the 2-minute window changesresourceVersionon a healthy, stable Deployment. The test then reports a "reconcile hot-loop" that does not exist. Usemetadata.generation, which increments only on spec changes. That matches what the test intends to prove.time.Sleep(2 * time.Minute)blocks the suite unconditionally, even when the state is already stable.
Point 1 is the correctness issue. Point 2 is secondary once the signal is right.
Also note that this spec has an ordering dependency. VerifySpecFieldRemovalDetected and VerifyExternalDriftReverted mutate control-plane Deployments in the same namespace. If Ginkgo runs this spec after one of those, or after their DeferCleanup triggers a rollout, the observed change is expected rather than a defect. Consider adding Ordered at the Describe level, or restricting this spec to a Deployment set it owns.
🛠️ Proposed fix using `generation`
- initialVersions := make(map[string]string, len(deployList.Items))
+ initialGenerations := make(map[string]int64, len(deployList.Items))
for i := range deployList.Items {
d := &deployList.Items[i]
- initialVersions[d.Name] = d.ResourceVersion
+ initialGenerations[d.Name] = d.Generation
}
// Wait long enough to cover at least two reconcile cycles.
time.Sleep(2 * time.Minute)
updatedList := &appsv1.DeploymentList{}
Expect(tc.MgmtClient.List(tc.Context, updatedList, crclient.InNamespace(tc.ControlPlaneNamespace))).
To(Succeed(), "failed to re-list Deployments in namespace %s after wait", tc.ControlPlaneNamespace)
for i := range updatedList.Items {
d := &updatedList.Items[i]
- initialVersion, existed := initialVersions[d.Name]
+ initialGeneration, existed := initialGenerations[d.Name]
if !existed {
continue // new Deployment appeared during the wait; skip
}
- Expect(d.ResourceVersion).To(Equal(initialVersion),
- "Deployment %s/%s had unexpected resourceVersion change — possible reconcile hot-loop",
- d.Namespace, d.Name)
+ Expect(d.Generation).To(Equal(initialGeneration),
+ "Deployment %s/%s had an unexpected spec generation change (%d -> %d) — possible reconcile hot-loop",
+ d.Namespace, d.Name, initialGeneration, d.Generation)
}📝 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.
| initialVersions := make(map[string]string, len(deployList.Items)) | |
| for i := range deployList.Items { | |
| d := &deployList.Items[i] | |
| initialVersions[d.Name] = d.ResourceVersion | |
| } | |
| // Wait long enough to cover at least two reconcile cycles. | |
| time.Sleep(2 * time.Minute) | |
| updatedList := &appsv1.DeploymentList{} | |
| Expect(tc.MgmtClient.List(tc.Context, updatedList, crclient.InNamespace(tc.ControlPlaneNamespace))). | |
| To(Succeed(), "failed to re-list Deployments in namespace %s after wait", tc.ControlPlaneNamespace) | |
| for i := range updatedList.Items { | |
| d := &updatedList.Items[i] | |
| initialVersion, existed := initialVersions[d.Name] | |
| if !existed { | |
| continue // new Deployment appeared during the wait; skip | |
| } | |
| Expect(d.ResourceVersion).To(Equal(initialVersion), | |
| "Deployment %s/%s had unexpected resourceVersion change — possible reconcile hot-loop", | |
| d.Namespace, d.Name) | |
| } | |
| }) | |
| initialGenerations := make(map[string]int64, len(deployList.Items)) | |
| for i := range deployList.Items { | |
| d := &deployList.Items[i] | |
| initialGenerations[d.Name] = d.Generation | |
| } | |
| // Wait long enough to cover at least two reconcile cycles. | |
| time.Sleep(2 * time.Minute) | |
| updatedList := &appsv1.DeploymentList{} | |
| Expect(tc.MgmtClient.List(tc.Context, updatedList, crclient.InNamespace(tc.ControlPlaneNamespace))). | |
| To(Succeed(), "failed to re-list Deployments in namespace %s after wait", tc.ControlPlaneNamespace) | |
| for i := range updatedList.Items { | |
| d := &updatedList.Items[i] | |
| initialGeneration, existed := initialGenerations[d.Name] | |
| if !existed { | |
| continue // new Deployment appeared during the wait; skip | |
| } | |
| Expect(d.Generation).To(Equal(initialGeneration), | |
| "Deployment %s/%s had an unexpected spec generation change (%d -> %d) — possible reconcile hot-loop", | |
| d.Namespace, d.Name, initialGeneration, d.Generation) | |
| } |
🤖 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/e2e/v2/tests/control_plane_upsert_test.go` around lines 88 - 111,
Replace the Deployment resourceVersion snapshot and comparison in this test with
metadata.generation so status-only writes do not appear as spec changes;
preserve the existing handling for newly appearing Deployments. Remove the
unconditional two-minute time.Sleep and wait with the test’s existing
polling/eventual mechanism for the stability window instead. Address cross-test
ordering at the surrounding Describe level, such as marking the specs Ordered,
so mutations from VerifySpecFieldRemovalDetected and VerifyExternalDriftReverted
cannot contaminate this idempotency check.
| // Step 1: add the test toleration to HC spec. | ||
| hc := getHC() | ||
| base := hc.DeepCopy() | ||
| hc.Spec.Tolerations = append(hc.Spec.Tolerations, testToleration) | ||
| Expect(tc.MgmtClient.Patch(tc.Context, hc, crclient.MergeFrom(base))).To(Succeed(), | ||
| "failed to patch HostedCluster %s to add test toleration", hcKey) | ||
|
|
||
| // DeferCleanup removes the test toleration on all exit paths. | ||
| DeferCleanup(func() { | ||
| current := getHC() | ||
| cleaned := removeToleration(current.Spec.Tolerations) | ||
| if len(cleaned) == len(current.Spec.Tolerations) { | ||
| return // already removed | ||
| } | ||
| restore := current.DeepCopy() | ||
| current.Spec.Tolerations = cleaned | ||
| if err := tc.MgmtClient.Patch(tc.Context, current, crclient.MergeFrom(restore)); err != nil { | ||
| if !apierrors.IsNotFound(err) { | ||
| GinkgoLogr.Error(err, "cleanup: failed to remove test toleration from HostedCluster", "key", hcKey) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
A non-lifecycle test mutates the HostedCluster to create its precondition.
Step 1 patches hc.Spec.Tolerations to add a test toleration. The path instructions forbid this for non-lifecycle tests.
The path instruction states: "Non-lifecycle tests must not mutate the hosted cluster to create preconditions; if a required annotation, label, or configuration is absent, Skip() instead of setting it. Only lifecycle tests may mutate cluster state."
The mutation is central to what this test proves, so Skip() is not a useful alternative here. Two options:
- Move this spec into the lifecycle test set, where cluster mutation is permitted.
- Confirm with the maintainers that the upsert suite runs as a lifecycle suite, and record that decision in a comment on the
Describe.
The cleanup implementation itself is correct: DeferCleanup is registered right after the patch, it re-reads the current object, and it handles NotFound.
🤖 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/e2e/v2/tests/control_plane_upsert_test.go` around lines 165 - 186, Move
the spec containing the `hc.Spec.Tolerations` patch into the lifecycle test set,
or document on its enclosing `Describe` that the upsert suite is approved to run
as a lifecycle suite. Keep the existing `DeferCleanup` cleanup behavior
unchanged.
Source: Path instructions
| // VerifyServiceAccountPullSecretsPreserved verifies Scenario 8b: the reconciler does not wipe | ||
| // imagePullSecrets injected by Kubernetes into ServiceAccounts in the control plane namespace. | ||
| func VerifyServiceAccountPullSecretsPreserved(getTestCtx internal.TestContextGetter) { | ||
| Context("ServiceAccount imagePullSecrets are preserved across reconciles", func() { | ||
| It("should not clear imagePullSecrets on managed ServiceAccounts", func() { | ||
| tc := getTestCtx() | ||
|
|
||
| saList := &corev1.ServiceAccountList{} | ||
| Expect(tc.MgmtClient.List(tc.Context, saList, crclient.InNamespace(tc.ControlPlaneNamespace))). | ||
| To(Succeed(), "failed to list ServiceAccounts in namespace %s", tc.ControlPlaneNamespace) | ||
| Expect(saList.Items).NotTo(BeEmpty(), | ||
| "expected at least one ServiceAccount in namespace %s", tc.ControlPlaneNamespace) | ||
|
|
||
| for i := range saList.Items { | ||
| sa := &saList.Items[i] | ||
| Expect(sa.ImagePullSecrets).NotTo(BeEmpty(), | ||
| "ServiceAccount %s/%s has empty imagePullSecrets — the reconciler may have wiped Kubernetes-injected pull secrets", | ||
| sa.Namespace, sa.Name) | ||
| } | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The ServiceAccount test does not verify preservation, and its scope is too broad.
Two problems:
- The comment and the function name state that
imagePullSecretsare preserved "across reconciles". The body takes one snapshot and asserts non-emptiness. It never observes a reconcile. A reconciler that wipes the field would still pass whenever the snapshot lands before the wipe, and would fail for reasons unrelated to reconciliation whenever it lands after. To test preservation, capture the secret names, wait for at least one reconcile cycle, re-read, and assert the names are still present. - The loop asserts on every ServiceAccount in the control-plane namespace, including
defaultand any ServiceAccount created by a component other than the upsert reconciler. Pull secret injection is not guaranteed for all of them. Restrict the loop to ServiceAccounts that carry theupsert.DesiredStateHashAnnotation, and assert that the filtered list is non-empty.
Point 2 also aligns with the path instruction to avoid vacuous passes by asserting that a filtered list is non-empty.
🛠️ Proposed restructure
saList := &corev1.ServiceAccountList{}
Expect(tc.MgmtClient.List(tc.Context, saList, crclient.InNamespace(tc.ControlPlaneNamespace))).
To(Succeed(), "failed to list ServiceAccounts in namespace %s", tc.ControlPlaneNamespace)
Expect(saList.Items).NotTo(BeEmpty(),
"expected at least one ServiceAccount in namespace %s", tc.ControlPlaneNamespace)
- for i := range saList.Items {
- sa := &saList.Items[i]
- Expect(sa.ImagePullSecrets).NotTo(BeEmpty(),
- "ServiceAccount %s/%s has empty imagePullSecrets — the reconciler may have wiped Kubernetes-injected pull secrets",
- sa.Namespace, sa.Name)
- }
+ // Only ServiceAccounts reconciled through the upsert path are in scope.
+ initial := map[crclient.ObjectKey][]string{}
+ for i := range saList.Items {
+ sa := &saList.Items[i]
+ if _, managed := sa.Annotations[upsert.DesiredStateHashAnnotation]; !managed {
+ continue
+ }
+ if len(sa.ImagePullSecrets) == 0 {
+ continue
+ }
+ names := make([]string, 0, len(sa.ImagePullSecrets))
+ for _, ref := range sa.ImagePullSecrets {
+ names = append(names, ref.Name)
+ }
+ initial[crclient.ObjectKeyFromObject(sa)] = names
+ }
+ Expect(initial).NotTo(BeEmpty(),
+ "expected at least one managed ServiceAccount with injected imagePullSecrets in namespace %s",
+ tc.ControlPlaneNamespace)
+
+ // Observe across at least one reconcile cycle.
+ Consistently(func(g Gomega) {
+ for key, names := range initial {
+ sa := &corev1.ServiceAccount{}
+ g.Expect(tc.MgmtClient.Get(tc.Context, key, sa)).To(Succeed(),
+ "failed to fetch ServiceAccount %s", key)
+ got := make([]string, 0, len(sa.ImagePullSecrets))
+ for _, ref := range sa.ImagePullSecrets {
+ got = append(got, ref.Name)
+ }
+ g.Expect(got).To(ContainElements(names),
+ "ServiceAccount %s lost injected imagePullSecrets across a reconcile", key)
+ }
+ }, 2*time.Minute, 15*time.Second).Should(Succeed())🤖 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/e2e/v2/tests/control_plane_upsert_test.go` around lines 296 - 317,
Update VerifyServiceAccountPullSecretsPreserved to filter the listed
ServiceAccounts to those carrying upsert.DesiredStateHashAnnotation and assert
the filtered list is non-empty. Capture each managed account’s imagePullSecret
names, wait for at least one reconcile cycle using the existing test utilities,
then re-list the managed accounts and assert the captured names remain present.
Source: Path instructions
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9236 +/- ##
==========================================
+ Coverage 43.12% 44.07% +0.95%
==========================================
Files 766 778 +12
Lines 94872 97995 +3123
==========================================
+ Hits 40909 43193 +2284
- Misses 51115 51792 +677
- Partials 2848 3010 +162 see 96 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
VerifyDesiredStateHashIdempotency was failing because it checked all Deployments in the HCP namespace including OLM-managed ones that legitimately change resourceVersion. Filter by desired-state-hash annotation for idempotency, and by managed-by label for other tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The e2e-v2-aws failed but these were false positives: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_hypershift/9236/pull-ci-openshift-hypershift-main-e2e-v2-aws/2085292297674559488 |
|
@mgencur: The following tests 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. |
|
PR needs rebase. 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. |
openshift-ci
Bot
added
the
needs-rebase
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
|
|
|
This is a test-only PR. Closing. One of the test cases is being merged in #9257 |
3 participants
Additional tests for #7713
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit