Merge pull request #1295 from theobarberbany/machineset-fix-flake

NO-JIRA: Updates MachineSet tests to use GenerateName

Author: openshift-merge-bot[bot]

pkg/controller/machine/machine_controller_suite_test.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/klog/v2/textlogger"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
+	"sigs.k8s.io/controller-runtime/pkg/envtest/komega"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -61,13 +62,19 @@ func TestMachineController(t *testing.T) {
 	RunSpecs(t, "Machine Controller Suite")
 }
 
-var _ = BeforeSuite(func(ctx SpecContext) {
+var _ = BeforeSuite(func() {
 	By("bootstrapping test environment")
 	var err error
 	cfg, testEnv, err = StartEnvTest()
 	Expect(err).ToNot(HaveOccurred())
 	Expect(cfg).ToNot(BeNil())
 
+	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
+	Expect(err).ToNot(HaveOccurred())
+
+	komega.SetContext(ctx)
+	komega.SetClient(k8sClient)
+
 })
 
 var _ = AfterSuite(func() {

pkg/controller/machine/machine_controller_test.go

@@ -28,9 +28,10 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 
+	testutils "github.com/openshift/cluster-api-actuator-pkg/testutils"
 	machinev1resourcebuilder "github.com/openshift/cluster-api-actuator-pkg/testutils/resourcebuilder/machine/v1beta1"
 	"github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework"
-	testutils "github.com/openshift/machine-api-operator/pkg/util/testing"
+	"github.com/openshift/machine-api-operator/pkg/util/testing"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
@@ -55,11 +56,10 @@ var _ = Describe("Machine Reconciler", func() {
 		})
 		Expect(err).NotTo(HaveOccurred())
 
-		k8sClient = mgr.GetClient()
 		k = komega.New(k8sClient)
 
 		By("Setting up feature gates")
-		gate, err := testutils.NewDefaultMutableFeatureGate()
+		gate, err := testing.NewDefaultMutableFeatureGate()
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Setting up a new reconciler")
@@ -96,15 +96,15 @@ var _ = Describe("Machine Reconciler", func() {
 	})
 
 	AfterEach(func() {
-		By("Deleting the machines")
-		Expect(cleanResources(namespace.Name)).To(Succeed())
-
-		By("Deleting the namespace")
-		Expect(k8sClient.Delete(ctx, namespace)).To(Succeed())
-
 		By("Closing the manager")
 		mgrCtxCancel()
 		Eventually(mgrStopped, timeout).WithTimeout(20 * time.Second).Should(BeClosed())
+
+		By("Cleaning up test resources")
+		testutils.CleanupResources(Default, ctx, cfg, k8sClient, namespace.GetName(),
+			&machinev1.Machine{},
+		)
+
 	})
 
 	It("Should reconcile a Machine", func() {
@@ -127,99 +127,98 @@ var _ = Describe("Machine Reconciler", func() {
 
 	})
 
-	It("Should set the Paused condition appropriately", func() {
-		instance := machineBuilder.Build()
-
-		By("Creating the Machine")
-		Expect(k8sClient.Create(ctx, instance)).To(Succeed())
-
-		By("Setting the AuthoritativeAPI to ClusterAPI")
-		Eventually(k.UpdateStatus(instance, func() {
-			instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityClusterAPI
-		})).Should(Succeed())
-
-		By("Verifying that the AuthoritativeAPI is set to Cluster API")
-		Eventually(k.Object(instance), timeout).Should(HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityClusterAPI)))
-
-		By("Verifying the paused condition is approproately set to true")
-		Eventually(k.Object(instance), timeout).Should(HaveField("Status.Conditions", ContainElement(SatisfyAll(
-			HaveField("Type", Equal(PausedCondition)),
-			HaveField("Status", Equal(corev1.ConditionTrue)),
-		))))
-
-		// The condition should remain true whilst transitioning through 'Migrating'
-		// Run this in a goroutine so we don't block
-
-		// Copy the instance before starting the goroutine, to avoid data races
-		instanceCopy := instance.DeepCopy()
-		go func() {
-			defer GinkgoRecover()
-			framework.RunCheckUntil(
-				ctx,
-				// Check that we consistently have the Paused condition true
-				func(_ context.Context, g framework.GomegaAssertions) bool {
-					By("Checking that the paused condition is consistently true whilst migrating to MachineAPI")
-
-					localInstance := instanceCopy.DeepCopy()
-					if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(localInstance), localInstance); err != nil {
-						return g.Expect(err).Should(WithTransform(testutils.IsRetryableAPIError, BeTrue()), "expected temporary error while getting machine: %v", err)
-					}
-
-					return g.Expect(localInstance.Status.Conditions).Should(ContainElement(SatisfyAll(
-						HaveField("Type", Equal(PausedCondition)),
-						HaveField("Status", Equal(corev1.ConditionTrue)),
-					)))
-
-				},
-				// Condition / until function: until we observe the MachineAuthority being MAPI
-				func(_ context.Context, g framework.GomegaAssertions) bool {
-					By("Checking that the AuthoritativeAPI is not MachineAPI")
-
-					localInstance := instanceCopy.DeepCopy()
-					if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(localInstance), localInstance); err != nil {
-						return g.Expect(err).Should(WithTransform(testutils.IsRetryableAPIError, BeTrue()), "expected temporary error while getting machine: %v", err)
-					}
-
-					return g.Expect(localInstance.Status.AuthoritativeAPI).To(Equal(machinev1.MachineAuthorityMachineAPI))
-				})
-		}()
-
-		By("Transitioning the AuthoritativeAPI though 'Migrating' to MachineAPI")
-		Eventually(k.UpdateStatus(instance, func() {
-			instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMigrating
-		})).Should(Succeed())
-
-		By("Updating the AuthoritativeAPI from Migrating to MachineAPI")
-		Eventually(k.UpdateStatus(instance, func() {
-			instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
-		})).Should(Succeed())
-
-		Eventually(k.Object(instance), timeout).Should(HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityMachineAPI)))
-
-		By("Verifying the paused condition is approproately set to false")
-		Eventually(k.Object(instance), timeout).Should(HaveField("Status.Conditions", ContainElement(SatisfyAll(
-			HaveField("Type", Equal(PausedCondition)),
-			HaveField("Status", Equal(corev1.ConditionFalse)),
-		))))
-
-		By("Unsetting the AuthoritativeAPI field in the status")
-		Eventually(k.UpdateStatus(instance, func() {
-			instance.Status.AuthoritativeAPI = ""
-		})).Should(Succeed())
+	Describe("Paused condition", func() {
+		var instance *machinev1.Machine
+		BeforeEach(func() {
+			instance = machineBuilder.Build()
+		})
 
-		By("Verifying the paused condition is still approproately set to false")
-		Eventually(k.Object(instance), timeout).Should(HaveField("Status.Conditions", ContainElement(SatisfyAll(
-			HaveField("Type", Equal(PausedCondition)),
-			HaveField("Status", Equal(corev1.ConditionFalse)),
-		))))
+		It("Should set the Paused condition appropriately", func() {
+
+			By("Creating the Machine")
+			Expect(k8sClient.Create(ctx, instance)).To(Succeed())
+
+			By("Setting the AuthoritativeAPI to ClusterAPI")
+			Eventually(k.UpdateStatus(instance, func() {
+				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityClusterAPI
+			})).Should(Succeed())
+
+			By("Verifying the paused condition is approproately set to true")
+			Eventually(k.Object(instance), timeout).Should(SatisfyAll(
+				HaveField("Status.Conditions", ContainElement(SatisfyAll(
+					HaveField("Type", Equal(PausedCondition)),
+					HaveField("Status", Equal(corev1.ConditionTrue)),
+				))),
+				HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityClusterAPI)),
+			))
+
+			// The condition should remain true whilst transitioning through 'Migrating'
+			// Run this in a goroutine so we don't block
+
+			// Copy the instance before starting the goroutine, to avoid data races
+			instanceCopy := instance.DeepCopy()
+			go func() {
+				defer GinkgoRecover()
+				framework.RunCheckUntil(
+					ctx,
+					// Check that we consistently have the Paused condition true
+					func(_ context.Context, g framework.GomegaAssertions) bool {
+						By("Checking that the paused condition is consistently true whilst migrating to MachineAPI")
+
+						localInstance := instanceCopy.DeepCopy()
+						if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(localInstance), localInstance); err != nil {
+							return g.Expect(err).Should(WithTransform(testing.IsRetryableAPIError, BeTrue()), "expected temporary error while getting machine: %v", err)
+						}
+
+						return g.Expect(localInstance.Status.Conditions).Should(ContainElement(SatisfyAll(
+							HaveField("Type", Equal(PausedCondition)),
+							HaveField("Status", Equal(corev1.ConditionTrue)),
+						)))
+
+					},
+					// Condition / until function: until we observe the MachineAuthority being MAPI
+					func(_ context.Context, g framework.GomegaAssertions) bool {
+						By("Checking that the AuthoritativeAPI is not MachineAPI")
+
+						localInstance := instanceCopy.DeepCopy()
+						if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(localInstance), localInstance); err != nil {
+							return g.Expect(err).Should(WithTransform(testing.IsRetryableAPIError, BeTrue()), "expected temporary error while getting machine: %v", err)
+						}
+
+						return g.Expect(localInstance.Status.AuthoritativeAPI).To(Equal(machinev1.MachineAuthorityMachineAPI))
+					})
+			}()
+
+			By("Transitioning the AuthoritativeAPI though 'Migrating' to MachineAPI")
+			Eventually(k.UpdateStatus(instance, func() {
+				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMigrating
+			})).Should(Succeed())
+
+			By("Updating the AuthoritativeAPI from Migrating to MachineAPI")
+			Eventually(k.UpdateStatus(instance, func() {
+				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
+			})).Should(Succeed())
+
+			By("Verifying the paused condition is approproately set to false")
+			Eventually(k.Object(instance), timeout).Should(SatisfyAll(
+				HaveField("Status.Conditions", ContainElement(SatisfyAll(
+					HaveField("Type", Equal(PausedCondition)),
+					HaveField("Status", Equal(corev1.ConditionFalse)),
+				))),
+				HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityMachineAPI)),
+			))
+
+			By("Unsetting the AuthoritativeAPI field in the status")
+			Eventually(k.UpdateStatus(instance, func() {
+				instance.Status.AuthoritativeAPI = ""
+			})).Should(Succeed())
+
+			By("Verifying the paused condition is still approproately set to false")
+			Eventually(k.Object(instance), timeout).Should(HaveField("Status.Conditions", ContainElement(SatisfyAll(
+				HaveField("Type", Equal(PausedCondition)),
+				HaveField("Status", Equal(corev1.ConditionFalse)),
+				HaveField("Message", Equal("The AuthoritativeAPI is not set")),
+			))))
+		})
 	})
 })
-
-func cleanResources(namespace string) error {
-	machine := &machinev1.Machine{}
-	if err := k8sClient.DeleteAllOf(ctx, machine, client.InNamespace(namespace)); err != nil {
-		return err
-	}
-
-	return nil
-}

pkg/controller/machineset/controller.go

@@ -188,12 +188,12 @@ func (r *ReconcileMachineSet) Reconcile(ctx context.Context, request reconcile.R
 				"The AuthoritativeAPI is set to %s", string(machineSet.Status.AuthoritativeAPI),
 			))
 
-			_, err := updateMachineSetStatus(r.Client, machineSet, machineSetCopy.Status)
+			machineSet, err := updateMachineSetStatus(r.Client, machineSet, machineSetCopy.Status)
 			if err != nil {
 				klog.Errorf("%v: error updating status: %v", machineSet.Name, err)
 			}
 
-			klog.Infof("%v: machine is paused, taking no further action", machineSet.Name)
+			klog.Infof("%v: machine set is paused, taking no further action", machineSet.Name)
 			return reconcile.Result{}, nil
 		}
 
@@ -211,7 +211,7 @@ func (r *ReconcileMachineSet) Reconcile(ctx context.Context, request reconcile.R
 			machinev1.ConditionSeverityInfo,
 			pausedFalseReason,
 		))
-		_, err := updateMachineSetStatus(r.Client, machineSet, machineSetCopy.Status)
+		machineSet, err := updateMachineSetStatus(r.Client, machineSet, machineSetCopy.Status)
 		if err != nil {
 			klog.Errorf("%v: error updating status: %v", machineSet.Name, err)
 		}

pkg/controller/machineset/machineset_controller_suite_test.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/klog/v2/textlogger"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
+	"sigs.k8s.io/controller-runtime/pkg/envtest/komega"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -57,11 +58,10 @@ var (
 
 func TestMachinesetController(t *testing.T) {
 	RegisterFailHandler(Fail)
-
 	RunSpecs(t, "MachineSet Controller Suite")
 }
 
-var _ = BeforeSuite(func(ctx SpecContext) {
+var _ = BeforeSuite(func() {
 	By("bootstrapping test environment")
 
 	testEnv = &envtest.Environment{
@@ -80,6 +80,12 @@ var _ = BeforeSuite(func(ctx SpecContext) {
 	cfg, err = testEnv.Start()
 	Expect(err).ToNot(HaveOccurred())
 	Expect(cfg).ToNot(BeNil())
+
+	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
+	Expect(err).ToNot(HaveOccurred())
+
+	komega.SetContext(ctx)
+	komega.SetClient(k8sClient)
 })
 
 var _ = AfterSuite(func() {