diff --git a/.golangci.yaml b/.golangci.yaml index d14076dfb..217909370 100644 --- a/.golangci.yaml +++ b/.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 diff --git a/internal/controller/clusteraddon_controller.go b/internal/controller/clusteraddon_controller.go index e670b0273..94dd4a178 100644 --- a/internal/controller/clusteraddon_controller.go +++ b/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 } diff --git a/internal/controller/clusterstack_controller.go b/internal/controller/clusterstack_controller.go index 808eb62f7..38b448a90 100644 --- a/internal/controller/clusterstack_controller.go +++ b/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 } diff --git a/internal/controller/clusterstack_controller_test.go b/internal/controller/clusterstack_controller_test.go index a6fcca3e7..f1d77295e 100644 --- a/internal/controller/clusterstack_controller_test.go +++ b/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()) + } +} diff --git a/internal/test/helpers/envtest.go b/internal/test/helpers/envtest.go index eb33c1288..fcee85f6c 100644 --- a/internal/test/helpers/envtest.go +++ b/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)