From 35d8cf10fd932ab56246a540e1310a10bba58923 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Thu, 7 May 2026 16:04:02 +0100 Subject: [PATCH 1/3] Fix test race deleting CAPA machine template --- .../machineset_sync_controller_test.go | 126 ++++++++++-------- 1 file changed, 73 insertions(+), 53 deletions(-) diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller_test.go b/pkg/controllers/machinesetsync/machineset_sync_controller_test.go index b17df5822..a16a66cb5 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller_test.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller_test.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "errors" + "slices" "sync" . "github.com/onsi/ginkgo/v2" @@ -882,7 +883,7 @@ var _ = Describe("With a running MachineSetSync controller", func() { }) }) - Context("when the CAPI machine set has a non-zero deletion timestamp", func() { + Context("when the CAPI machine set is deleted", func() { BeforeEach(func() { // We expect deletion logic to intract with the template, so create it here By("Creating the CAPI infra machine template") @@ -902,74 +903,93 @@ var _ = Describe("With a running MachineSetSync controller", func() { HaveField("ObjectMeta.Finalizers", ContainElement(clusterv1.MachineSetFinalizer)), ), ) - Eventually(kDelete(ctx, capiMachineSet)).To(Succeed()) }) - Context("when the CAPI finalizer is removed", func() { + // Do the actual deletion in JustBeforeEach so sub-contexts can + // further modify capiMachineSet before deletion. + JustBeforeEach(func() { + Eventually(kDelete(ctx, capiMachineSet)).To(Succeed()) + // Mock the CAPI machine set controller removing the // finalizer that goes once all machines have been deleted. - BeforeEach(func() { - Eventually(k.Update(capiMachineSet, func() { - capiMachineSet.SetFinalizers([]string{machinesync.SyncFinalizer}) - })).Should(Succeed()) - }) + Eventually(k.Update(capiMachineSet, func() { + filteredFinalizers := slices.DeleteFunc(capiMachineSet.GetFinalizers(), func(finalizer string) bool { + return finalizer == clusterv1.MachineSetFinalizer + }) + capiMachineSet.SetFinalizers(filteredFinalizers) + })).Should(Succeed()) + }) - It("should delete the MAPI machine set", func() { - Eventually(k.Get(mapiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "eventually mapiMachineSet should not be found") + It("should delete the MAPI machine set", func() { + Eventually(k.Get(mapiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "eventually mapiMachineSet should not be found") - // We don't want to re-create the machineset just deleted - Consistently(k.Get(mapiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "the mapiMachineSet should not be recreated") - }) + // We don't want to re-create the machineset just deleted + Consistently(k.Get(mapiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "the mapiMachineSet should not be recreated") + }) - It("should delete the CAPI machine set", func() { - Eventually(k.Get(capiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "eventually capiMachineSet should not be found") - // We don't want to re-create the machineset just deleted - Consistently(k.Get(capiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "the capiMachineSet should not be recreated") - }) + It("should delete the CAPI machine set", func() { + Eventually(k.Get(capiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "eventually capiMachineSet should not be found") + // We don't want to re-create the machineset just deleted + Consistently(k.Get(capiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "the capiMachineSet should not be recreated") + }) - It("should not delete the CAPA machine template because it does not have MachineSet label", func() { - uid := capaMachineTemplate.GetUID() - // Both the MAPI and CAPI machine sets should be deleted - Eventually(k.Get(mapiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "eventually mapiMachineSet should not be found") - Eventually(k.Get(capiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "eventually capiMachineSet should not be found") + It("should not delete the CAPA machine template because it does not have MachineSet label", func() { + uid := capaMachineTemplate.GetUID() + // Both the MAPI and CAPI machine sets should be deleted + Eventually(k.Get(mapiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "eventually mapiMachineSet should not be found") + Eventually(k.Get(capiMachineSet), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "eventually capiMachineSet should not be found") - // The CAPA machine template should still exist after the MAPI and CAPI machine sets are deleted - Eventually(k.Object(capaMachineTemplate)).To(SatisfyAll( - HaveField("ObjectMeta.UID", Equal(uid)), - HaveField("ObjectMeta.DeletionTimestamp", BeNil()), - )) - }) + // The CAPA machine template should still exist after the MAPI and CAPI machine sets are deleted + Eventually(k.Object(capaMachineTemplate)).To(SatisfyAll( + HaveField("ObjectMeta.UID", Equal(uid)), + HaveField("ObjectMeta.DeletionTimestamp", BeNil()), + )) + }) - Context("when the CAPA machine template is updated to contain the machine set label", func() { - BeforeEach(func() { - Eventually(k.Update(capaMachineTemplate, func() { - if capaMachineTemplate.Labels == nil { - capaMachineTemplate.Labels = make(map[string]string) - } + Context("when the CAPA machine template has the machine set label", func() { + BeforeEach(func(ctx context.Context) { + Eventually(k.Update(capaMachineTemplate, func() { + if capaMachineTemplate.Labels == nil { + capaMachineTemplate.Labels = make(map[string]string) + } - capaMachineTemplate.Labels[consts.MachineSetOpenshiftLabelKey] = mapiMachineSet.Name - })).Should(Succeed()) + capaMachineTemplate.Labels[consts.MachineSetOpenshiftLabelKey] = mapiMachineSet.Name + })).Should(Succeed()) + + // Because this is a change to capaMachineTemplate but the + // reconcile is triggered by deletion of capiMachineSet + // (i.e. this is change to a different object in a different + // cache), to avoid a race we need to ensure that the + // manager's informer cache has observed this change before + // we do the delete. + By("Waiting for the manager's informer to observe the updated finalizers", func() { + Eventually(func(g Gomega) *awsv1.AWSMachineTemplate { + template := &awsv1.AWSMachineTemplate{} + g.Expect(mgr.GetClient().Get(ctx, client.ObjectKeyFromObject(capaMachineTemplate), template)).To(Succeed()) + + return template + }, timeout).Should(HaveField("Labels", HaveKeyWithValue(consts.MachineSetOpenshiftLabelKey, mapiMachineSet.Name))) }) + }) - It("should delete the CAPA machine template", func() { - By("checking that there are no templates with the machine set label") - Eventually(func() []awsv1.AWSMachineTemplate { - templateList := &awsv1.AWSMachineTemplateList{} - listOptions := []client.ListOption{ - client.InNamespace(capiNamespace.Name), - client.MatchingLabels(map[string]string{consts.MachineSetOpenshiftLabelKey: mapiMachineSet.Name}), - } + It("should delete the CAPA machine template", func() { + By("checking that there are no templates with the machine set label") + Eventually(func() []awsv1.AWSMachineTemplate { + templateList := &awsv1.AWSMachineTemplateList{} + listOptions := []client.ListOption{ + client.InNamespace(capiNamespace.Name), + client.MatchingLabels(map[string]string{consts.MachineSetOpenshiftLabelKey: mapiMachineSet.Name}), + } - if err := k8sClient.List(ctx, templateList, listOptions...); err != nil { - return nil - } + if err := k8sClient.List(ctx, templateList, listOptions...); err != nil { + return nil + } - return templateList.Items - }, timeout).Should(BeEmpty(), "all associated AWS machine templates should be deleted") + return templateList.Items + }, timeout).Should(BeEmpty(), "all associated AWS machine templates should be deleted") - By("checking that the CAPA machine template is deleted") - Eventually(k.Get(capaMachineTemplate), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "eventually CAPA machine template should not be found") - }) + By("checking that the CAPA machine template is deleted") + Eventually(k.Get(capaMachineTemplate), timeout).Should(WithTransform(apierrors.IsNotFound, BeTrue()), "eventually CAPA machine template should not be found") }) }) }) From 434cfaf15962516ac925d4abad829e6dafae8af7 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Thu, 7 May 2026 16:21:47 +0100 Subject: [PATCH 2/3] Fix incorrect uses of k8sClient.Create in Eventually --- .../machineset_sync_controller_test.go | 4 +- .../machinesetsync/machineset_vap_test.go | 82 +++++++++---------- 2 files changed, 39 insertions(+), 47 deletions(-) diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller_test.go b/pkg/controllers/machinesetsync/machineset_sync_controller_test.go index a16a66cb5..62a9e8c46 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller_test.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller_test.go @@ -1446,9 +1446,9 @@ var _ = Describe("applySynchronizedConditionWithPatch", func() { }) // kCreate is a komega equivalent for create. -func kCreate(ctx context.Context, obj client.Object) func() error { +func kCreate(ctx context.Context, obj client.Object, opts ...client.CreateOption) func() error { return func() error { - return k8sClient.Create(ctx, obj) + return k8sClient.Create(ctx, obj, opts...) } } diff --git a/pkg/controllers/machinesetsync/machineset_vap_test.go b/pkg/controllers/machinesetsync/machineset_vap_test.go index c2ff364aa..c67b6654f 100644 --- a/pkg/controllers/machinesetsync/machineset_vap_test.go +++ b/pkg/controllers/machinesetsync/machineset_vap_test.go @@ -28,7 +28,6 @@ import ( admissionregistrationv1 "k8s.io/api/admissionregistration/v1" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" mapiv1beta1 "github.com/openshift/api/machine/v1beta1" @@ -72,11 +71,11 @@ var _ = Describe("MachineSet VAP Tests", func() { mapiNamespace = corev1resourcebuilder.Namespace(). WithGenerateName("openshift-machine-api-").Build() - Eventually(k8sClient.Create(ctx, mapiNamespace)).Should(Succeed(), "mapi namespace should be able to be created") + Eventually(kCreate(ctx, mapiNamespace)).Should(Succeed(), "mapi namespace should be able to be created") capiNamespace = corev1resourcebuilder.Namespace(). WithGenerateName("openshift-cluster-api-").Build() - Eventually(k8sClient.Create(ctx, capiNamespace)).Should(Succeed(), "capi namespace should be able to be created") + Eventually(kCreate(ctx, capiNamespace)).Should(Succeed(), "capi namespace should be able to be created") infrastructureName := "cluster-foo" @@ -85,12 +84,12 @@ var _ = Describe("MachineSet VAP Tests", func() { capaClusterBuilder := awsv1resourcebuilder.AWSCluster(). WithNamespace(capiNamespace.GetName()). WithName(infrastructureName) - Eventually(k8sClient.Create(ctx, capaClusterBuilder.Build())).Should(Succeed(), "capa cluster should be able to be created") + Eventually(kCreate(ctx, capaClusterBuilder.Build())).Should(Succeed(), "capa cluster should be able to be created") capiClusterBuilder := clusterv1resourcebuilder.Cluster(). WithNamespace(capiNamespace.GetName()). WithName(infrastructureName) - Eventually(k8sClient.Create(ctx, capiClusterBuilder.Build())).Should(Succeed(), "capi cluster should be able to be created") + Eventually(kCreate(ctx, capiClusterBuilder.Build())).Should(Succeed(), "capi cluster should be able to be created") capaMachineTemplateBuilder := awsv1resourcebuilder.AWSMachineTemplate(). WithNamespace(capiNamespace.GetName()). @@ -108,7 +107,7 @@ var _ = Describe("MachineSet VAP Tests", func() { }, } - Eventually(k8sClient.Create(ctx, capaMachineTemplate)).Should(Succeed(), "capa machine template should be able to be created") + Eventually(kCreate(ctx, capaMachineTemplate)).Should(Succeed(), "capa machine template should be able to be created") capiMachineSetBuilder = clusterv1resourcebuilder.MachineSet(). WithNamespace(capiNamespace.GetName()). @@ -128,14 +127,7 @@ var _ = Describe("MachineSet VAP Tests", func() { newObj, ok := obj.DeepCopyObject().(client.Object) Expect(ok).To(BeTrue()) - Eventually(func() error { - err := k8sClient.Create(ctx, newObj) - if err != nil && !apierrors.IsAlreadyExists(err) { - return err - } - - return nil - }, timeout).Should(Succeed()) + Eventually(kCreate(ctx, newObj), timeout).Should(WithTransform(client.IgnoreAlreadyExists, Succeed())) } }) @@ -210,17 +202,17 @@ var _ = Describe("MachineSet VAP Tests", func() { }, }). Build() - Eventually(k8sClient.Create(ctx, sentinelMachineSet)).Should(Succeed(), "sentinel machineset should be able to be created") + Eventually(kCreate(ctx, sentinelMachineSet)).Should(Succeed(), "sentinel machineset should be able to be created") admissiontestutils.VerifySentinelValidation(k, sentinelMachineSet, setupTimeout) }) It("should allow creating a MachineSet without forbidden fields", func() { - Eventually(k8sClient.Create(ctx, capiMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, capiMachineSet)).Should(Succeed()) }) It("should allow updating a MachineSet without changing forbidden fields", func() { - Eventually(k8sClient.Create(ctx, capiMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, capiMachineSet)).Should(Succeed()) Eventually(k.Update(capiMachineSet, func() { replicas := int32(3) @@ -232,11 +224,11 @@ var _ = Describe("MachineSet VAP Tests", func() { testVersion := "1" capiMachineSet.Spec.Template.Spec.Version = testVersion - Eventually(k8sClient.Create(ctx, capiMachineSet), timeout).Should(MatchError(ContainSubstring(".version is a forbidden field"))) + Eventually(kCreate(ctx, capiMachineSet, client.DryRunAll), timeout).Should(MatchError(ContainSubstring(".version is a forbidden field"))) }) It("should deny updating spec.template.spec.version on an existing MachineSet", func() { - Eventually(k8sClient.Create(ctx, capiMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, capiMachineSet)).Should(Succeed()) Eventually(k.Update(capiMachineSet, func() { testVersion := "1" @@ -247,11 +239,11 @@ var _ = Describe("MachineSet VAP Tests", func() { It("should deny creating a MachineSet with spec.template.spec.readinessGates", func() { capiMachineSet.Spec.Template.Spec.ReadinessGates = []clusterv1.MachineReadinessGate{{ConditionType: "foo"}} - Eventually(k8sClient.Create(ctx, capiMachineSet), timeout).Should(MatchError(ContainSubstring(".readinessGates is a forbidden field"))) + Eventually(kCreate(ctx, capiMachineSet, client.DryRunAll), timeout).Should(MatchError(ContainSubstring(".readinessGates is a forbidden field"))) }) It("should deny updating spec.template.spec.readinessGates on an existing MachineSet", func() { - Eventually(k8sClient.Create(ctx, capiMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, capiMachineSet)).Should(Succeed()) Eventually(k.Update(capiMachineSet, func() { capiMachineSet.Spec.Template.Spec.ReadinessGates = []clusterv1.MachineReadinessGate{{ConditionType: "foo"}} @@ -315,7 +307,7 @@ var _ = Describe("MachineSet VAP Tests", func() { WithName("sentinel-machineset"). WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI). Build() - Eventually(k8sClient.Create(ctx, sentinelMachineSet), setupTimeout).Should(Succeed()) + Eventually(kCreate(ctx, sentinelMachineSet), setupTimeout).Should(Succeed()) capiSentinelMachineSet := clusterv1resourcebuilder.MachineSet(). WithName("sentinel-machineset"). @@ -326,7 +318,7 @@ var _ = Describe("MachineSet VAP Tests", func() { }, }). Build() - Eventually(k8sClient.Create(ctx, capiSentinelMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, capiSentinelMachineSet)).Should(Succeed()) Eventually(k.Get(capiSentinelMachineSet)).Should(Succeed()) @@ -335,7 +327,7 @@ var _ = Describe("MachineSet VAP Tests", func() { It("Does not allow creation of a MAPI MachineSet with spec.authoritativeAPI: MachineAPI and the same name", func() { By("Create the CAPI MachineSet") - Eventually(k8sClient.Create(ctx, capiMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, capiMachineSet)).Should(Succeed()) By("Create the MAPI MachineSet") @@ -343,13 +335,13 @@ var _ = Describe("MachineSet VAP Tests", func() { WithName("test-machineset"). WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityMachineAPI). Build() - Eventually(k8sClient.Create(ctx, newMapiMachineSet), timeout).Should( + Eventually(kCreate(ctx, newMapiMachineSet, client.DryRunAll), timeout).Should( MatchError(ContainSubstring("with spec.authoritativeAPI: MachineAPI because a Cluster API MachineSet with the same name already exists."))) }) It("Does allow creation of a MAPI machineset with authoritative API ClusterAPI and the same name", func() { By("Create the CAPI MachineSet") - Eventually(k8sClient.Create(ctx, capiMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, capiMachineSet)).Should(Succeed()) By("Create the MAPI MachineSet") @@ -357,7 +349,7 @@ var _ = Describe("MachineSet VAP Tests", func() { WithName("test-machineset"). WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI). Build() - Eventually(k8sClient.Create(ctx, newMapiMachineSet), timeout).Should(Succeed()) + Eventually(kCreate(ctx, newMapiMachineSet), timeout).Should(Succeed()) }) It("Does allow creation of a MAPI MachineSet when no matching CAPI MachineSet exists (parameterNotFoundAction)", func() { @@ -367,7 +359,7 @@ var _ = Describe("MachineSet VAP Tests", func() { WithName("no-capi-equivalent"). WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityMachineAPI). Build() - Eventually(k8sClient.Create(ctx, newMapiMachineSet), timeout).Should(Succeed()) + Eventually(kCreate(ctx, newMapiMachineSet), timeout).Should(Succeed()) }) }) @@ -426,7 +418,7 @@ var _ = Describe("MachineSet VAP Tests", func() { WithName("sentinel-machineset"). WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI). Build() - Eventually(k8sClient.Create(ctx, sentinelMachineSet), setupTimeout).Should(Succeed()) + Eventually(kCreate(ctx, sentinelMachineSet), setupTimeout).Should(Succeed()) Eventually(k.UpdateStatus(sentinelMachineSet, func() { sentinelMachineSet.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI @@ -444,7 +436,7 @@ var _ = Describe("MachineSet VAP Tests", func() { }, }). Build() - Eventually(k8sClient.Create(ctx, capiSentinelMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, capiSentinelMachineSet)).Should(Succeed()) Eventually(k.Get(capiSentinelMachineSet)).Should(Succeed()) @@ -466,7 +458,7 @@ var _ = Describe("MachineSet VAP Tests", func() { "machine.openshift.io/vCPU": "4", }) mapiMachineSet = mapiMachineSetBuilder.Build() - Eventually(k8sClient.Create(ctx, mapiMachineSet), setupTimeout).Should(Succeed()) + Eventually(kCreate(ctx, mapiMachineSet), setupTimeout).Should(Succeed()) capiMachineSet = capiMachineSetBuilder.WithLabels(map[string]string{ "machine.openshift.io/cluster-api-cluster": "ci-op-gs2k97d6-c9e33-2smph", @@ -476,7 +468,7 @@ var _ = Describe("MachineSet VAP Tests", func() { "capacity.cluster-autoscaler.kubernetes.io/labels": "kubernetes.io/arch=amd64", }).Build() - Eventually(k8sClient.Create(ctx, capiMachineSet), setupTimeout).Should(Succeed()) + Eventually(kCreate(ctx, capiMachineSet), setupTimeout).Should(Succeed()) }) Context("with status.AuthoritativeAPI: Machine API", func() { @@ -678,10 +670,10 @@ var _ = Describe("MachineSet VAP Tests", func() { By("Creating a throwaway MAPI machine set") sentinelMachineSet := mapiMachineSetBuilder.WithName("sentinel-machineset").WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityClusterAPI).Build() - Eventually(k8sClient.Create(ctx, sentinelMachineSet), setupTimeout).Should(Succeed()) + Eventually(kCreate(ctx, sentinelMachineSet), setupTimeout).Should(Succeed()) capiSentinelMachine := clusterv1resourcebuilder.MachineSet().WithName("sentinel-machineset").WithNamespace(capiNamespace.Name).Build() - Eventually(k8sClient.Create(ctx, capiSentinelMachine)).Should(Succeed()) + Eventually(kCreate(ctx, capiSentinelMachine)).Should(Succeed()) Eventually(k.Get(capiSentinelMachine)).Should(Succeed()) @@ -696,7 +688,7 @@ var _ = Describe("MachineSet VAP Tests", func() { WithName("no-mapi-counterpart"). WithNamespace(capiNamespace.Name). Build() - Eventually(k8sClient.Create(ctx, newCapiMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, newCapiMachineSet)).Should(Succeed()) }) }) @@ -705,7 +697,7 @@ var _ = Describe("MachineSet VAP Tests", func() { By("Creating MAPI machineset with authoritativeAPI=MachineAPI") mapiMachineSet := mapiMachineSetBuilder.WithName("validation-machineset").Build() - Eventually(k8sClient.Create(ctx, mapiMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, mapiMachineSet)).Should(Succeed()) Eventually(k.UpdateStatus(mapiMachineSet, func() { mapiMachineSet.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI @@ -720,7 +712,7 @@ var _ = Describe("MachineSet VAP Tests", func() { WithNamespace(capiNamespace.Name). Build() - Eventually(k8sClient.Create(ctx, newCapiMachineSet), timeout).Should( + Eventually(kCreate(ctx, newCapiMachineSet, client.DryRunAll), timeout).Should( MatchError(ContainSubstring("in an un-paused state"))) }) @@ -735,7 +727,7 @@ var _ = Describe("MachineSet VAP Tests", func() { }). Build() - Eventually(k8sClient.Create(ctx, newCapiMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, newCapiMachineSet)).Should(Succeed()) }) }) @@ -746,7 +738,7 @@ var _ = Describe("MachineSet VAP Tests", func() { By("Creating MAPI machineset with authoritativeAPI=ClusterAPI") mapiMachineSet = mapiMachineSetBuilder.WithName("validation-machineset").Build() - Eventually(k8sClient.Create(ctx, mapiMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, mapiMachineSet)).Should(Succeed()) Eventually(k.UpdateStatus(mapiMachineSet, func() { mapiMachineSet.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI @@ -766,7 +758,7 @@ var _ = Describe("MachineSet VAP Tests", func() { // Use DryRunAll to ensure the object is never actually // persisted. This means we can test again if it initially // succeeds - Eventually(func() error { return k8sClient.Create(ctx, newCapiMachineSet, client.DryRunAll) }, longerTimeout).Should( + Eventually(kCreate(ctx, newCapiMachineSet, client.DryRunAll), longerTimeout).Should( MatchError(ContainSubstring("already exists and is not paused"))) }) @@ -788,7 +780,7 @@ var _ = Describe("MachineSet VAP Tests", func() { Build() // Give this eventually extra timeout to allow the VAP to catch up with the cross-object reference - Eventually(func() error { return k8sClient.Create(ctx, newCapiMachineSet) }, longerTimeout).Should(Succeed()) + Eventually(kCreate(ctx, newCapiMachineSet), longerTimeout).Should(Succeed()) }) }) }) @@ -848,7 +840,7 @@ var _ = Describe("MachineSet VAP Tests", func() { WithName("sentinel-machineset"). WithAuthoritativeAPI(mapiv1beta1.MachineAuthorityMachineAPI). Build() - Eventually(k8sClient.Create(ctx, sentinelMachineSet), setupTimeout).Should(Succeed()) + Eventually(kCreate(ctx, sentinelMachineSet), setupTimeout).Should(Succeed()) Eventually(k.UpdateStatus(sentinelMachineSet, func() { sentinelMachineSet.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI @@ -866,7 +858,7 @@ var _ = Describe("MachineSet VAP Tests", func() { }, }). Build() - Eventually(k8sClient.Create(ctx, capiSentinelMachineSet)).Should(Succeed()) + Eventually(kCreate(ctx, capiSentinelMachineSet)).Should(Succeed()) Eventually(k.Get(capiSentinelMachineSet)).Should(Succeed()) @@ -888,7 +880,7 @@ var _ = Describe("MachineSet VAP Tests", func() { "machine.openshift.io/vCPU": "4", }) mapiMachineSet = mapiMachineSetBuilder.Build() - Eventually(k8sClient.Create(ctx, mapiMachineSet), setupTimeout).Should(Succeed()) + Eventually(kCreate(ctx, mapiMachineSet), setupTimeout).Should(Succeed()) capiMachineSet = capiMachineSetBuilder.WithLabels(map[string]string{ "machine.openshift.io/cluster-api-cluster": "ci-op-gs2k97d6-c9e33-2smph", @@ -902,7 +894,7 @@ var _ = Describe("MachineSet VAP Tests", func() { "machine.openshift.io/vCPU": "4", }).Build() - Eventually(k8sClient.Create(ctx, capiMachineSet), setupTimeout).Should(Succeed()) + Eventually(kCreate(ctx, capiMachineSet), setupTimeout).Should(Succeed()) }) Context("with status.authoritativeAPI: Machine API (on MAPI MachineSet)", func() { BeforeEach(func() { From 16ba30fb11d762346411fcf5320993f9a91643e2 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Thu, 7 May 2026 17:44:41 +0100 Subject: [PATCH 3/3] Fix machineset sync test races between MAPI and CAPI object creation --- .../machineset_sync_controller_test.go | 175 ++++++++++++------ 1 file changed, 120 insertions(+), 55 deletions(-) diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller_test.go b/pkg/controllers/machinesetsync/machineset_sync_controller_test.go index 62a9e8c46..855cf8189 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller_test.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller_test.go @@ -42,6 +42,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" capierrors "sigs.k8s.io/cluster-api/errors" + gomegatypes "github.com/onsi/gomega/types" "github.com/openshift/cluster-api-actuator-pkg/testutils" consts "github.com/openshift/cluster-capi-operator/pkg/controllers" "github.com/openshift/cluster-capi-operator/pkg/controllers/machinesync" @@ -156,6 +157,19 @@ var _ = Describe("With a running MachineSetSync controller", func() { <-mgrDone } + // eventuallyManagerInformerCache is a helper function to wait for the + // manager's informer cache to observe an object in a particular state. This + // prevents a class of race condition where the test is trying to assert + // behaviour of the controller in the presence of existing state. We need to + // ensure that the controller has had a chance to observe the state the test + // just created first. + eventuallyManagerInformerCache := func(obj client.Object) gomegatypes.AsyncAssertion { + return Eventually(func(g Gomega) client.Object { + g.Expect(mgr.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed()) + return obj + }) + } + BeforeEach(func() { var err error @@ -547,23 +561,44 @@ var _ = Describe("With a running MachineSetSync controller", func() { }) Context("when the MAPI machine set has MachineAuthority set to Cluster API", func() { + // afterMAPIUpdate can be set in a sub-context's BeforeEach to register + // initialisation to happen after the MAPI machine set is updated. + // + // NOTE: This is a workaround for the deeply nested test structure + // which makes it difficult to control the order of setup operations. + // Do not copy this pattern to new tests or extend its usage. New + // tests should be structured so that this kind of deferred setup is + // not necessary. + var afterMAPIUpdate func() + BeforeEach(func() { By("Creating the MAPI machine set") mapiMachineSet = mapiMachineSetBuilder.Build() Eventually(kCreate(ctx, mapiMachineSet)).Should(Succeed()) + }) + // This triggers reconciliation, so we do it in JustBeforeEach to allow + // sub-contexts to make additional changes first without racing. + JustBeforeEach(func() { By("Setting the MAPI machine set AuthoritativeAPI to ClusterAPI") Eventually(k.UpdateStatus(mapiMachineSet, func() { mapiMachineSet.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityClusterAPI - })).Should(Succeed()) - // We need to set the observed generation to the metadata generation - // to ensure the status is updated as that's a prerequisite for the status to be updated by the machinesetsync controller. - By("Setting the MAPI machine set observed generation to its metadata generation") - Eventually(k.UpdateStatus(mapiMachineSet, func() { + // We need to set the observed generation to the metadata + // generation to ensure the status is updated as that's a + // prerequisite for the status to be updated by the + // machinesetsync controller. mapiMachineSet.Status.ObservedGeneration = mapiMachineSet.Generation })).Should(Succeed()) + + if afterMAPIUpdate != nil { + afterMAPIUpdate() + + // afterMAPIUpdate is not reinitialised between tests in this + // context, so we must clear it after use. + afterMAPIUpdate = nil + } }) Context("when the CAPI machine set exists and the spec differs (replica count)", func() { @@ -583,6 +618,12 @@ var _ = Describe("With a running MachineSetSync controller", func() { Eventually(k.UpdateStatus(capiMachineSet, func() { capiMachineSet.Status.ObservedGeneration = capiMachineSet.Generation })).Should(Succeed()) + + By("Waiting for the manager's informer cache to observe the CAPI machine set", func() { + eventuallyManagerInformerCache(capiMachineSet).Should( + HaveField("Status.ObservedGeneration", Equal(capiMachineSet.Generation)), + ) + }) }) It("should set the sync finalizer on both the mapi and capi machine sets", func() { @@ -627,7 +668,12 @@ var _ = Describe("With a running MachineSetSync controller", func() { // We expect a sync, so we require the infra template By("Creating the CAPI infra machine template") Eventually(kCreate(ctx, capaMachineTemplate)).Should(Succeed(), "capa machine template should be able to be created") + + By("Waiting for the manager's informer cache to observe the CAPA machine template", func() { + eventuallyManagerInformerCache(capaMachineTemplate).ShouldNot(BeNil()) + }) }) + Context("where the field is meant to be copied", func() { BeforeEach(func() { By("Creating the MAPI machine set with differing object meta in relevant field") @@ -641,6 +687,12 @@ var _ = Describe("With a running MachineSetSync controller", func() { Eventually(k.UpdateStatus(capiMachineSet, func() { capiMachineSet.Status.ObservedGeneration = capiMachineSet.Generation })).Should(Succeed()) + + By("Waiting for the manager's informer cache to observe the CAPI machine set", func() { + eventuallyManagerInformerCache(capiMachineSet).Should( + HaveField("Status.ObservedGeneration", Equal(capiMachineSet.Generation)), + ) + }) }) It("should update the synchronized condition on the MAPI machine set to True", func() { @@ -676,6 +728,12 @@ var _ = Describe("With a running MachineSetSync controller", func() { Eventually(k.UpdateStatus(capiMachineSet, func() { capiMachineSet.Status.ObservedGeneration = capiMachineSet.Generation })).Should(Succeed()) + + By("Waiting for the manager's informer cache to observe the CAPI machine set", func() { + eventuallyManagerInformerCache(capiMachineSet).Should( + HaveField("Status.ObservedGeneration", Equal(capiMachineSet.Generation)), + ) + }) }) It("should update the synchronized condition on the MAPI machine set to True", func() { @@ -788,19 +846,6 @@ var _ = Describe("With a running MachineSetSync controller", func() { var newCapaMachineTemplate *awsv1.AWSMachineTemplate BeforeEach(func() { - capiMachineSet = capiMachineSetBuilder.Build() - - By("Waiting for initial synchronization") - Eventually(k.Object(mapiMachineSet), timeout).Should( - HaveField("Status.Conditions", ContainElement( - SatisfyAll( - HaveField("Type", Equal(consts.SynchronizedCondition)), - HaveField("Status", Equal(corev1.ConditionTrue)), - HaveField("Reason", Equal("ResourceSynchronized")), - HaveField("Message", Equal("Successfully synchronized CAPI MachineSet to MAPI")), - ))), - ) - By("Creating a new CAPA machine template with different instanceType") newCapaMachineTemplate = capav1builder.AWSMachineTemplate(). @@ -810,24 +855,41 @@ var _ = Describe("With a running MachineSetSync controller", func() { Build() Eventually(kCreate(ctx, newCapaMachineTemplate)).Should(Succeed()) - By("Updating the CAPI machine set to reference the new template") - Eventually(k.Update(capiMachineSet, func() { - capiMachineSet.Spec.Template.Spec.InfrastructureRef.Name = newCapaMachineTemplate.Name - })).Should(Succeed()) + By("Waiting for the manager's informer cache to observe the CAPA machine template", func() { + eventuallyManagerInformerCache(newCapaMachineTemplate).ShouldNot(BeNil()) + }) - // We need to set the observed generation to the metadata generation - // to ensure the status is updated as that's a prerequisite for the status to be updated by the machinesetsync controller. - By("Setting the CAPI machine set observed generation to its metadata generation") - Eventually(k.UpdateStatus(capiMachineSet, func() { - capiMachineSet.Status.ObservedGeneration = capiMachineSet.Generation - })).Should(Succeed()) + afterMAPIUpdate = func() { + capiMachineSet = capiMachineSetBuilder.Build() + + By("Waiting for initial synchronization") + Eventually(k.Object(mapiMachineSet), timeout).Should( + HaveField("Status.Conditions", ContainElement( + SatisfyAll( + HaveField("Type", Equal(consts.SynchronizedCondition)), + HaveField("Status", Equal(corev1.ConditionTrue)), + HaveField("Reason", Equal("ResourceSynchronized")), + HaveField("Message", Equal("Successfully synchronized CAPI MachineSet to MAPI")), + ))), + ) + + By("Updating the CAPI machine set to reference the new template") + Eventually(k.Update(capiMachineSet, func() { + capiMachineSet.Spec.Template.Spec.InfrastructureRef.Name = newCapaMachineTemplate.Name + })).Should(Succeed()) + + // We need to set the observed generation to the metadata generation + // to ensure the status is updated as that's a prerequisite for the status to be updated by the machinesetsync controller. + By("Setting the CAPI machine set observed generation to its metadata generation") + Eventually(k.UpdateStatus(capiMachineSet, func() { + capiMachineSet.Status.ObservedGeneration = capiMachineSet.Generation + })).Should(Succeed()) + } }) It("should update the MAPI machine set instanceType to match the new template", func() { - Eventually(func() (string, error) { - if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(mapiMachineSet), mapiMachineSet); err != nil { - return "", err - } + Eventually(func(g Gomega) (string, error) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(mapiMachineSet), mapiMachineSet)).To(Succeed()) providerSpec, err := mapi2capi.AWSProviderSpecFromRawExtension(mapiMachineSet.Spec.Template.Spec.ProviderSpec.Value) if err != nil { @@ -893,16 +955,19 @@ var _ = Describe("With a running MachineSetSync controller", func() { capiMachineSet.SetFinalizers([]string{clusterv1.MachineSetFinalizer}) Eventually(kCreate(ctx, capiMachineSet)).Should(Succeed()) - // Expect to see the finalizers, so they're in place before - // we Expect logic that relies on them to work - Eventually(k.Object(mapiMachineSet), timeout).Should( - HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)), - ) - Eventually(k.Object(capiMachineSet), timeout).Should(SatisfyAll( - HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)), - HaveField("ObjectMeta.Finalizers", ContainElement(clusterv1.MachineSetFinalizer)), - ), - ) + afterMAPIUpdate = func() { + // Expect to see the finalizers, so they're in place before + // we Expect logic that relies on them to work + Eventually(k.Object(mapiMachineSet), timeout).Should( + HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)), + ) + Eventually(k.Object(capiMachineSet), timeout).Should( + HaveField("ObjectMeta.Finalizers", SatisfyAll( + ContainElement(machinesync.SyncFinalizer), + ContainElement(clusterv1.MachineSetFinalizer), + )), + ) + } }) // Do the actual deletion in JustBeforeEach so sub-contexts can @@ -963,12 +1028,9 @@ var _ = Describe("With a running MachineSetSync controller", func() { // manager's informer cache has observed this change before // we do the delete. By("Waiting for the manager's informer to observe the updated finalizers", func() { - Eventually(func(g Gomega) *awsv1.AWSMachineTemplate { - template := &awsv1.AWSMachineTemplate{} - g.Expect(mgr.GetClient().Get(ctx, client.ObjectKeyFromObject(capaMachineTemplate), template)).To(Succeed()) - - return template - }, timeout).Should(HaveField("Labels", HaveKeyWithValue(consts.MachineSetOpenshiftLabelKey, mapiMachineSet.Name))) + eventuallyManagerInformerCache(capaMachineTemplate).Should( + HaveField("ObjectMeta.Labels", HaveKeyWithValue(consts.MachineSetOpenshiftLabelKey, mapiMachineSet.Name)), + ) }) }) @@ -998,13 +1060,16 @@ var _ = Describe("With a running MachineSetSync controller", func() { BeforeEach(func() { capiMachineSet = capiMachineSetBuilder.Build() Eventually(kCreate(ctx, capiMachineSet)).Should(Succeed()) - Eventually(k.Object(mapiMachineSet), timeout).Should( - HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)), - ) - Eventually(k.Object(capiMachineSet), timeout).Should( - HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)), - ) - Eventually(kDelete(ctx, mapiMachineSet)).To(Succeed()) + + afterMAPIUpdate = func() { + Eventually(k.Object(mapiMachineSet), timeout).Should( + HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)), + ) + Eventually(k.Object(capiMachineSet), timeout).Should( + HaveField("ObjectMeta.Finalizers", ContainElement(machinesync.SyncFinalizer)), + ) + Eventually(kDelete(ctx, mapiMachineSet)).To(Succeed()) + } }) It("should remove the machinesync finalizer from the CAPI machine set", func() {