🐛 Fix flaky tests

.golangci.yaml

@@ -101,9 +101,13 @@ linters-settings:
     settings:
       hugeParam:
         sizeThreshold: 120
+      rangeValCopy:
+        sizeThreshold: 512
   revive:
     enable-all-rules: true
     rules:
+      - name: range-val-address
+        disabled: true
       - name: import-alias-naming
         disabled: true
       - name: redundant-import-alias

internal/controller/clusteraddon_controller.go

@@ -138,7 +138,6 @@ func (r *ClusterAddonReconciler) Reconcile(ctx context.Context, req reconcile.Re
 	if err := r.Get(ctx, clusterName, cluster); err != nil {
 		if apierrors.IsNotFound(err) && !clusterAddon.DeletionTimestamp.IsZero() {
 			controllerutil.RemoveFinalizer(clusterAddon, csov1alpha1.ClusterAddonFinalizer)
-
 			return reconcile.Result{}, nil
 		}
 

internal/controller/clusterstack_controller.go

@@ -157,7 +157,6 @@ func (r *ClusterStackReconciler) Reconcile(ctx context.Context, req reconcile.Re
 	if err != nil {
 		return reconcile.Result{}, fmt.Errorf("failed to get ClusterStackReleases from clusterstack.spec.versions: %w", err)
 	}
-
 	toCreate, toDelete := makeDiff(existingClusterStackReleases, latest, latestReady, inSpec, inUse)
 
 	// delete all cluster stack releases
@@ -442,12 +441,10 @@ func (r *ClusterStackReconciler) getExistingClusterStackReleases(ctx context.Con
 
 	existingClusterStackReleases := make([]*csov1alpha1.ClusterStackRelease, 0, len(csrList.Items))
 
-	for i := range csrList.Items {
-		csr := csrList.Items[i]
-		for j := range csr.GetOwnerReferences() {
-			ownerRef := csr.GetOwnerReferences()[j]
+	for _, csr := range csrList.Items {
+		for _, ownerRef := range csr.GetOwnerReferences() {
 			if matchesOwnerRef(&ownerRef, clusterStack) {
-				existingClusterStackReleases = append(existingClusterStackReleases, &csrList.Items[i])
+				existingClusterStackReleases = append(existingClusterStackReleases, &csr)
 				break
 			}
 		}
@@ -463,29 +460,28 @@ func makeDiff(clusterStackReleases []*csov1alpha1.ClusterStackRelease, latest, l
 	mapToCreate := make(map[string]struct{})
 
 	// decide whether existing clusterStackReleases should be kept or not
-	for i, cs := range clusterStackReleases {
+	for _, csr := range clusterStackReleases {
 		var shouldCreate bool
-
 		// if clusterStackRelease is either the latest or the latest that is ready, we want to have it
-		if latest != nil && cs.Name == *latest || latestReady != nil && cs.Name == *latestReady {
+		if latest != nil && csr.Name == *latest || latestReady != nil && csr.Name == *latestReady {
 			shouldCreate = true
 		}
 
 		// if the clusterStackRelease is listed in spec, then we want to keep it
-		if _, found := inSpec[cs.Name]; found {
+		if _, found := inSpec[csr.Name]; found {
 			shouldCreate = true
 		}
 
 		// if the clusterStackRelease is in use, then we want to keep it
-		if _, found := inUse[cs.Name]; found {
+		if _, found := inUse[csr.Name]; found {
 			shouldCreate = true
 		}
 
 		if shouldCreate {
-			toCreate = append(toCreate, clusterStackReleases[i])
-			mapToCreate[cs.Name] = struct{}{}
+			toCreate = append(toCreate, csr)
+			mapToCreate[csr.Name] = struct{}{}
 		} else {
-			toDelete = append(toDelete, clusterStackReleases[i])
+			toDelete = append(toDelete, csr)
 		}
 	}
 
@@ -506,7 +502,6 @@ func makeDiff(clusterStackReleases []*csov1alpha1.ClusterStackRelease, latest, l
 			toCreate = append(toCreate, csr)
 		}
 	}
-
 	return toCreate, toDelete
 }
 

internal/controller/clusterstack_controller_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package controller
 
 import (
+	"fmt"
 	"reflect"
 	"sort"
 	"testing"
@@ -534,6 +535,7 @@ var _ = Describe("ClusterStackReconciler", func() {
 
 				clusterStackReleaseTagV1Key = types.NamespacedName{Name: clusterStackReleaseTagV1, Namespace: testNs.Name}
 				clusterStackReleaseTagV2Key = types.NamespacedName{Name: clusterStackReleaseTagV2, Namespace: testNs.Name}
+				waitUntilChildObjectsAreCreated(clusterStack)
 			})
 
 			AfterEach(func() {
@@ -593,26 +595,78 @@ var _ = Describe("ClusterStackReconciler", func() {
 				ph, err := patch.NewHelper(clusterStack, testEnv)
 				Expect(err).ShouldNot(HaveOccurred())
 
+				providerclusterStackReleaseRefV2 := &corev1.ObjectReference{
+					APIVersion: "infrastructure.clusterstack.x-k8s.io/v1alpha1",
+					Kind:       "TestInfrastructureProviderClusterStackRelease",
+					Name:       clusterStackReleaseTagV2,
+					Namespace:  testNs.Name,
+				}
+
+				// Wait until the ProviderClusterStackRelease is created
+				// Check for the correct ownerRef
+				Eventually(func() bool {
+					obj, err := external.Get(ctx, testEnv.GetClient(), providerclusterStackReleaseRefV2, testNs.Name)
+					if apierrors.IsNotFound(err) {
+						fmt.Printf("   providerclusterStackReleaseRefV2 not found (retry). %s\n", err.Error())
+						return false
+					}
+					if err != nil {
+						fmt.Printf("   get providerclusterStackReleaseRefV2: error (retry). %s\n", err.Error())
+						return false
+					}
+					if obj.GetDeletionTimestamp() != nil {
+						// in envTests there is not GC: If the parent-objects gets deleted, the child-objects are not updated..
+						fmt.Printf("   I am confused. providerclusterStackReleaseRefV2 has a deletionTimestamp? (retry). %s\n", obj.GetDeletionTimestamp())
+						return false
+					}
+					owners := obj.GetOwnerReferences()
+					fmt.Printf("    providerclusterStackReleaseRefV2 found: Finalizers %+v OwnerRefs: %+v Kind: %s, uuid: %s\n", obj.GetFinalizers(), owners,
+						obj.GetKind(), obj.GetUID())
+					if len(obj.GetOwnerReferences()) == 0 {
+						fmt.Printf("    Missing finalizer/ownerRefs\n")
+						return false
+					}
+					if len(obj.GetOwnerReferences()) > 1 {
+						fmt.Printf("    Too many ownerRefs?\n")
+						return false
+					}
+					ownerRef := obj.GetOwnerReferences()[0]
+					if ownerRef.APIVersion != "clusterstack.x-k8s.io/v1alpha1" || ownerRef.Kind != "ClusterStackRelease" ||
+						ownerRef.Name != "docker-ferrol-1-27-v2" || !*ownerRef.Controller || !*ownerRef.BlockOwnerDeletion {
+						fmt.Printf("    I am confused, wrong ownerRef: %+v\n", ownerRef)
+						return false
+					}
+					return true
+				}, timeout, interval).Should(BeTrue())
+
+				fmt.Printf("old %+v new %+v\n", clusterStack.Spec.Versions, []string{"v1"})
 				clusterStack.Spec.Versions = []string{"v1"}
 
+				// remove v2 from CS
 				Eventually(func() error {
-					return ph.Patch(ctx, clusterStack)
+					err := ph.Patch(ctx, clusterStack)
+					return err
 				}, timeout, interval).Should(BeNil())
 
+				// wait until clusterStackRelease V2 is deleted
+				// This delete propagation (From CS spec.versions --> ClusterStackRelease) is done by our controller (not Kubernetes GC)
+				// We can't check for the deletion of the providerClusterStackRelease, because in envTests there is no GC.
 				Eventually(func() bool {
-					return apierrors.IsNotFound(testEnv.Get(ctx, clusterStackReleaseTagV2Key, &csov1alpha1.ClusterStackRelease{}))
-				}, timeout, interval).Should(BeTrue())
+					obj := csov1alpha1.ClusterStackRelease{}
+					err := testEnv.Get(ctx, clusterStackReleaseTagV2Key, &obj)
 
-				Eventually(func() bool {
-					foundProviderclusterStackReleaseRef := &corev1.ObjectReference{
-						APIVersion: "infrastructure.clusterstack.x-k8s.io/v1alpha1",
-						Kind:       "TestInfrastructureProviderClusterStackRelease",
-						Name:       clusterStackReleaseTagV2,
-						Namespace:  testNs.Name,
+					if apierrors.IsNotFound(err) {
+						fmt.Printf("    clusterStackReleaseTagV2Key was deleted (good): %s\n", err.Error())
+						return true
 					}
 
-					_, err := external.Get(ctx, testEnv.GetClient(), foundProviderclusterStackReleaseRef, testNs.Name)
-					return apierrors.IsNotFound(err)
+					if err != nil {
+						fmt.Printf("    clusterStackReleaseTagV2Key error (retry). %s\n", err.Error())
+						return false
+					}
+					fmt.Printf("    clusterStackReleaseTagV2Key is found (will retry). %s Finalizers: %+v OwnerRefs %+v DelTimestamp: %v\n", obj.GetName(), obj.GetFinalizers(), obj.GetOwnerReferences(),
+						obj.GetDeletionTimestamp())
+					return false
 				}, timeout, interval).Should(BeTrue())
 			})
 		})
@@ -872,3 +926,40 @@ var _ = Describe("clusterStack validation", func() {
 		})
 	})
 })
+
+func waitUntilChildObjectsAreCreated(cs *csov1alpha1.ClusterStack) {
+	providerKind := "TestInfrastructureProviderClusterStackRelease"
+	for _, csrVersion := range cs.Spec.Versions {
+		csoClusterStack, err := clusterstack.New(cs.Spec.Provider, cs.Spec.Name, cs.Spec.KubernetesVersion, csrVersion)
+		Expect(err).To(BeNil())
+		key := types.NamespacedName{Name: csoClusterStack.String(), Namespace: cs.Namespace}
+		Eventually(func() error {
+			return testEnv.Get(ctx, key, &csov1alpha1.ClusterStackRelease{})
+		}, 3*timeout, interval).Should(BeNil())
+		Eventually(func() bool {
+			providerObj, err := external.Get(ctx, testEnv.GetClient(), &corev1.ObjectReference{
+				APIVersion: "infrastructure.clusterstack.x-k8s.io/v1alpha1",
+				Kind:       providerKind,
+				Name:       csoClusterStack.String(),
+				Namespace:  cs.Namespace,
+			}, cs.Namespace)
+			if err != nil {
+				fmt.Printf("%s %s not found. %s\n", providerKind, csoClusterStack.String(), err.Error())
+				return false
+			}
+			fmt.Printf("foundProviderclusterStackReleaseRef is found. %s Finalizers: %+v OwnerRefs: %+v\n",
+				providerObj.GetName(), providerObj.GetFinalizers(), providerObj.GetOwnerReferences())
+			if len(providerObj.GetOwnerReferences()) == 0 {
+				fmt.Printf("    Missing ownerRefs\n")
+				return false
+			}
+			csoObj := &csov1alpha1.ClusterStackRelease{}
+			err = testEnv.Get(ctx, key, csoObj)
+			if err != nil {
+				fmt.Printf("ClusterStackRelease %s not found. %s\n", csoClusterStack.Name, err.Error())
+				return false
+			}
+			return true
+		}, timeout, interval).Should(BeTrue())
+	}
+}

internal/test/helpers/envtest.go

@@ -290,6 +290,8 @@ func (t *TestEnvironment) Stop() error {
 
 // Cleanup deletes client objects.
 func (t *TestEnvironment) Cleanup(ctx context.Context, objs ...client.Object) error {
+	// Deletion of Namespaces has limitations in envTests:
+	// https://book.kubebuilder.io/reference/envtest.html#namespace-usage-limitation
 	errs := make([]error, 0, len(objs))
 	for _, o := range objs {
 		err := t.Client.Delete(ctx, o)