OCPBUGS-100155: fix flaky MachineSet sync test by atomically verifying InfraTemplate - #647
OCPBUGS-100155: fix flaky MachineSet sync test by atomically verifying InfraTemplate#647simkam 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
|
@simkam: This pull request references Jira Issue OCPBUGS-100155, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
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: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. WalkthroughThe migration helper refreshes MachineSet state before updating the provider specification. The migration test validates replacement templates through direct client polling. The synchronization controller delays reconciliation while outdated infrastructure templates remain pending deletion. ChangesMachineSet migration synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR makes the MachineSet rotation test atomic and adds a bounded requeue safeguard while waiting for outdated templates to be deleted; no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 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 NOT APPROVED This pull-request has been approved by: 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 |
|
@simkam: This pull request references Jira Issue OCPBUGS-100155, 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. |
|
/test e2e-aws-capi-techpreview |
|
/pipeline required |
|
Scheduling tests matching the |
| } else if shouldRequeue { | ||
| logger.Info("Waiting for Cluster API infrastructure machine templates to be deleted") | ||
| return ctrl.Result{}, nil | ||
| return ctrl.Result{RequeueAfter: time.Second}, nil |
There was a problem hiding this comment.
Not a fan of this. There's no reason we should ever miss a watch event, even until intense load, even if the watch is cancelled and restarted at any point. This just hides bugs.
| if err := cl.Get(ctx, client.ObjectKeyFromObject(mapiMachineSet), mapiMachineSet); err != nil { | ||
| return fmt.Errorf("failed to refresh MachineSet %s: %w", mapiMachineSet.Name, err) | ||
| } |
There was a problem hiding this comment.
Was this strictly required? If not I prefer to remove it. I think this could make tests harder to debug if the helper might not actually be using the same object state it was passed.
There was a problem hiding this comment.
Right, it is not required, removed.
| By("Waiting for new InfraTemplate with updated InstanceType to be created") | ||
| originalAWSMachineTemplateName := capiMachineSet.Spec.Template.Spec.InfrastructureRef.Name | ||
| capiMachineSet = capiframework.GetMachineSetWithRetry(mapiMSAuthMAPIName, capiframework.CAPINamespace) | ||
| Eventually(k.Object(capiMachineSet), capiframework.WaitMedium, capiframework.RetryMedium).Should(HaveField("Spec.Template.Spec.InfrastructureRef.Name", Not(Equal(originalAWSMachineTemplateName))), "Should have InfraTemplate name changed") | ||
| Eventually(func(g Gomega) { | ||
| g.Expect(cl.Get(ctx, client.ObjectKeyFromObject(capiMachineSet), capiMachineSet)).To(Succeed()) | ||
|
|
||
| currentTemplateName := capiMachineSet.Spec.Template.Spec.InfrastructureRef.Name | ||
| g.Expect(currentTemplateName).ToNot(Equal(originalAWSMachineTemplateName), | ||
| "InfraTemplate name should have changed from %s", originalAWSMachineTemplateName) | ||
|
|
||
| By("Verifying new InfraTemplate has the updated InstanceType", func() { | ||
| newAWSMachineTemplate = &awsv1.AWSMachineTemplate{} | ||
| newAWSMachineTemplate.Name = capiMachineSet.Spec.Template.Spec.InfrastructureRef.Name | ||
| newAWSMachineTemplate.Name = currentTemplateName | ||
| newAWSMachineTemplate.Namespace = capiMachineSet.Namespace | ||
|
|
||
| Eventually(k.Object(newAWSMachineTemplate), capiframework.WaitShort, capiframework.RetryShort).Should( | ||
| HaveField("Spec.Template.Spec.InstanceType", Equal(newInstanceType)), | ||
| ) | ||
| }) | ||
| g.Expect(cl.Get(ctx, client.ObjectKeyFromObject(newAWSMachineTemplate), newAWSMachineTemplate)).To(Succeed()) | ||
| g.Expect(newAWSMachineTemplate.Spec.Template.Spec.InstanceType).To(Equal(newInstanceType), | ||
| "AWSMachineTemplate %s should have InstanceType %s", currentTemplateName, newInstanceType) | ||
| }, capiframework.WaitMedium, capiframework.RetryMedium).Should(Succeed(), | ||
| "Should have a new InfraTemplate with InstanceType %s", newInstanceType) |
https://redhat.atlassian.net/browse/OCPBUGS-100155 The test that verifies `AWSMachineTemplate` rotation after a MAPI MachineSet `providerSpec` update was racy: it captured the new `InfrastructureRef` name in one `Eventually` block, then verified the template content in a separate block. Between these two steps, the sync controller could reconcile again and replace the intermediate template, leaving the test looking for a deleted object. Fix by merging the two `Eventually` blocks into a single atomic poll that reads the current CAPI MachineSet, captures the `InfrastructureRef` name, and verifies the referenced template's `InstanceType` in the same iteration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bbd73f9 to
4b9a045
Compare
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest |
1 similar comment
|
/retest |
|
@simkam: all tests passed! 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. |
3 participants
https://redhat.atlassian.net/browse/OCPBUGS-100155
The test that verifies
AWSMachineTemplaterotation after a MAPI MachineSetproviderSpecupdate was racy: it captured the newInfrastructureRefname in oneEventuallyblock, then verified the template content in a separate block. Between these two steps, the sync controller could reconcile again and replace the intermediate template, leaving the test looking for a deleted object.Fix by merging the two
Eventuallyblocks into a single atomic poll that reads the current CAPI MachineSet, captures theInfrastructureRefname, and verifies the referenced template'sInstanceTypein the same iteration.Also refresh the MAPI MachineSet from the API server before patching its
providerSpecto avoid staleResourceVersionconflicts.Add an explicit
RequeueAfter: time.Secondwhen themachinesetsynccontroller is waiting for outdatedInfraMachineTemplatesto be deleted, providing a safety-net requeue instead of relying solely on watch events under CI load.Summary by CodeRabbit