From 83de6a7550bab415540c7823681d21d08695ab08 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Wed, 29 Jan 2025 14:55:46 +0530 Subject: [PATCH 01/21] KUBESAW-144: Implement TierTemplateRevision cleanup controller Signed-off-by: Feny Mehta --- ...emplatetier_revision_cleanup_controller.go | 115 ++++++++++++++++++ ...tetier_revision_cleanup_controller_test.go | 79 ++++++++++++ 2 files changed, 194 insertions(+) create mode 100644 controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go create mode 100644 controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go new file mode 100644 index 000000000..b24b5739c --- /dev/null +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -0,0 +1,115 @@ +package nstemplatetierrevisioncleanup + +import ( + "context" + "fmt" + "time" + + "sigs.k8s.io/controller-runtime/pkg/log" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + ctrl "sigs.k8s.io/controller-runtime" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + +var AgeOld = 24 * time.Hour + +// SetupWithManager sets up the controller with the Manager. +func (r *Reconciler) SetupWithManager(mgr manager.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + For(&toolchainv1alpha1.NSTemplateTier{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). + Complete(r) +} + +type Reconciler struct { + Client runtimeclient.Client + Scheme *runtime.Scheme +} + +func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { + logger := log.FromContext(ctx) + + // fetch the NSTemplateTier tier + tier := &toolchainv1alpha1.NSTemplateTier{} + if err := r.Client.Get(ctx, request.NamespacedName, tier); err != nil { + if errors.IsNotFound(err) { + logger.Info("NSTemplateTier not found") + return reconcile.Result{}, nil + } + // Error reading the object - requeue the request. + logger.Error(err, "unable to get the current NSTemplateTier") + return reconcile.Result{}, fmt.Errorf("unable to get the current NSTemplateTier: %w", err) + } + + // Get the list of tier template revisions which are unsused(not referenced by any tiers and whose creation date is older that 24hours(1 day)) + ttrevisions, err := r.ListUnusedTTRs(ctx, tier, AgeOld) + if err != nil { + return reconcile.Result{}, fmt.Errorf("unable to get the current unused Tier Template Revisions List: %w", err) + } + + // Delete the unused revisions + if err := r.DeleteRevision(ctx, ttrevisions); err != nil { + return reconcile.Result{}, fmt.Errorf("unable to delete the current Tier Template Revision: %w", err) + } + return reconcile.Result{}, nil +} + +// ListTTRS function first lists all the tier template revisions, then compare it with the nsTemplatetierRevisions map list, and gets the revisions +// which are no longer referenced/used by any space and whose creation time stamp is older than 1 day(24hours) and returns this list of unused revisions +func (r *Reconciler) ListUnusedTTRs(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, ageOld time.Duration) ([]toolchainv1alpha1.TierTemplateRevision, error) { + + ttrDelList := []toolchainv1alpha1.TierTemplateRevision{} + ttr := toolchainv1alpha1.TierTemplateRevision{} + ttRevList := toolchainv1alpha1.TierTemplateRevisionList{} + unUsedRevisions := make(map[string]bool) + + //list all the template revisions in a given NStemplate tier + if err := r.Client.List(ctx, &ttRevList, runtimeclient.InNamespace(nsTmplTier.GetNamespace()), + runtimeclient.MatchingLabels{toolchainv1alpha1.TierLabelKey: nsTmplTier.GetName()}); err != nil { + return nil, err + } + + //finding the tier revisions which are not refernced/used by the nstemplatetier.status.revisions + for _, ttrs := range ttRevList.Items { + unUsedRevisions[ttrs.Name] = true + for _, revCR := range nsTmplTier.Status.Revisions { + if ttrs.Name == revCR { + unUsedRevisions[ttrs.Name] = false + } + } + } + + //list the not refernced/unused tier revisions whose creation time stamp is older than 1 day. + for _, ttrs := range ttRevList.Items { + for ttrname, value := range unUsedRevisions { + if ttrs.Name == ttrname && value { + ttCreateTime, err := time.Parse(time.RFC3339, ttrs.CreationTimestamp.String()) + if err != nil { + return nil, err + } + duration := -time.Until(ttCreateTime) + if duration > ageOld { + ttrDelList = append(ttrDelList, ttr) + } + } + } + } + + return ttrDelList, nil +} + +func (r *Reconciler) DeleteRevision(ctx context.Context, ttrevisions []toolchainv1alpha1.TierTemplateRevision) error { + for _, revision := range ttrevisions { + if err := r.Client.Delete(ctx, &revision); err != nil { + return fmt.Errorf("%s was not deleted : %w", revision.Name, err) + } + } + return nil +} diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go new file mode 100644 index 000000000..618ca39be --- /dev/null +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -0,0 +1,79 @@ +package nstemplatetierrevisioncleanup + +import ( + "fmt" + "strings" + "testing" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/codeready-toolchain/host-operator/pkg/apis" + "github.com/codeready-toolchain/toolchain-common/pkg/test" + templatev1 "github.com/openshift/api/template/v1" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/client-go/kubernetes/scheme" +) + +const ( + operatorNamespace = "toolchain-host-operator" +) + +func TestReconcile(t *testing.T) { + +} + +func createTierTemplate(t *testing.T, typeName string, withTemplateObjects []runtime.RawExtension, tierName string) *toolchainv1alpha1.TierTemplate { + var ( + ns test.TemplateObject = ` +- apiVersion: v1 + kind: Namespace + metadata: + name: ${SPACE_NAME} +` + spacename test.TemplateParam = ` +- name: SPACE_NAME + value: johnsmith` + ) + s := scheme.Scheme + err := apis.AddToScheme(s) + require.NoError(t, err) + codecFactory := serializer.NewCodecFactory(s) + decoder := codecFactory.UniversalDeserializer() + tmpl := templatev1.Template{} + _, _, err = decoder.Decode([]byte(test.CreateTemplate(test.WithObjects(ns), test.WithParams(spacename))), nil, &tmpl) + require.NoError(t, err) + + revision := "123456new" + // we can set the template field to something empty as it is not relevant for the tests + tt := &toolchainv1alpha1.TierTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: strings.ToLower(fmt.Sprintf("%s-%s-%s", tierName, typeName, revision)), + Namespace: test.HostOperatorNs, + }, + Spec: toolchainv1alpha1.TierTemplateSpec{ + TierName: tierName, + Type: typeName, + Revision: revision, + Template: tmpl, + }, + } + + // just copy the raw objects to the templateObjects field + // TODO this will be removed once we switch on using templateObjects only in the TierTemplates + if withTemplateObjects != nil { + tt.Spec.TemplateObjects = withTemplateObjects + } + + return tt +} + +func withTemplateObjects(templates ...unstructured.Unstructured) []runtime.RawExtension { + var templateObjects []runtime.RawExtension + for i := range templates { + templateObjects = append(templateObjects, runtime.RawExtension{Object: &templates[i]}) + } + return templateObjects +} From b5534de2a994ee69ea2981493c28796bd023f189 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Fri, 7 Mar 2025 15:42:01 +0530 Subject: [PATCH 02/21] KUBESAW-264: Implement TierTemplateRevision cleanup controller Signed-off-by: Feny Mehta --- ...emplatetier_revision_cleanup_controller.go | 117 ++++---- ...tetier_revision_cleanup_controller_test.go | 250 ++++++++++++++---- 2 files changed, 265 insertions(+), 102 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index b24b5739c..3e7ac047a 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -8,23 +8,23 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/codeready-toolchain/host-operator/controllers/nstemplatetier" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" - "sigs.k8s.io/controller-runtime/pkg/builder" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/reconcile" ctrl "sigs.k8s.io/controller-runtime" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" - "sigs.k8s.io/controller-runtime/pkg/predicate" ) -var AgeOld = 24 * time.Hour +const deletionTimeThreshold = 30 * time.Second // SetupWithManager sets up the controller with the Manager. func (r *Reconciler) SetupWithManager(mgr manager.Manager) error { return ctrl.NewControllerManagedBy(mgr). - For(&toolchainv1alpha1.NSTemplateTier{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). + For(&toolchainv1alpha1.TierTemplateRevision{}). Complete(r) } @@ -37,8 +37,27 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. logger := log.FromContext(ctx) // fetch the NSTemplateTier tier + ttr := &toolchainv1alpha1.TierTemplateRevision{} + if err := r.Client.Get(ctx, request.NamespacedName, ttr); err != nil { + if errors.IsNotFound(err) { + logger.Info("TierTemplateRevision not found") + return reconcile.Result{}, nil + } + // Error reading the object - requeue the request. + logger.Error(err, "unable to get the current TierTemplateRevision") + return reconcile.Result{}, fmt.Errorf("unable to get the current TierTemplateRevision: %w", err) + } + tierName, ok := ttr.GetLabels()[toolchainv1alpha1.TierLabelKey] + if !ok { + return reconcile.Result{}, fmt.Errorf("tier-name label not found in tiertemplaterevision") + + } + // fetch the related NSTemplateTier tier tier := &toolchainv1alpha1.NSTemplateTier{} - if err := r.Client.Get(ctx, request.NamespacedName, tier); err != nil { + if err := r.Client.Get(ctx, types.NamespacedName{ + Namespace: ttr.GetNamespace(), + Name: tierName, + }, tier); err != nil { if errors.IsNotFound(err) { logger.Info("NSTemplateTier not found") return reconcile.Result{}, nil @@ -48,68 +67,62 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, fmt.Errorf("unable to get the current NSTemplateTier: %w", err) } - // Get the list of tier template revisions which are unsused(not referenced by any tiers and whose creation date is older that 24hours(1 day)) - ttrevisions, err := r.ListUnusedTTRs(ctx, tier, AgeOld) + // verify that the tier template revision which is unsused(not referenced by any tiers and whose creation date is older that 30secs + // and all the spaces are up-to-date) + UnusedRevisionBool, requeue, reqAft, err := r.VerifyUnusedTTR(ctx, tier, ttr, deletionTimeThreshold) if err != nil { - return reconcile.Result{}, fmt.Errorf("unable to get the current unused Tier Template Revisions List: %w", err) + return reconcile.Result{RequeueAfter: reqAft, Requeue: requeue}, fmt.Errorf("cannot delete the current TTR,requing :%w", err) } - // Delete the unused revisions - if err := r.DeleteRevision(ctx, ttrevisions); err != nil { - return reconcile.Result{}, fmt.Errorf("unable to delete the current Tier Template Revision: %w", err) + // Delete the unused revision + if UnusedRevisionBool { + if err := r.Client.Delete(ctx, ttr); err != nil { + return reconcile.Result{}, fmt.Errorf("unable to delete the current Tier Template Revision %s: %w", ttr.Name, err) + } } + return reconcile.Result{}, nil } -// ListTTRS function first lists all the tier template revisions, then compare it with the nsTemplatetierRevisions map list, and gets the revisions -// which are no longer referenced/used by any space and whose creation time stamp is older than 1 day(24hours) and returns this list of unused revisions -func (r *Reconciler) ListUnusedTTRs(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, ageOld time.Duration) ([]toolchainv1alpha1.TierTemplateRevision, error) { +// VerifyUnusedTTR function verifies that the tier template revision's creation time stamp is older than 30sec, +// it is not present/refernced in status.revisions field, and there are no outdated spaces. +func (r *Reconciler) VerifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, + rev *toolchainv1alpha1.TierTemplateRevision, deletionTimeThreshold time.Duration) (bool, bool, time.Duration, error) { - ttrDelList := []toolchainv1alpha1.TierTemplateRevision{} - ttr := toolchainv1alpha1.TierTemplateRevision{} - ttRevList := toolchainv1alpha1.TierTemplateRevisionList{} - unUsedRevisions := make(map[string]bool) + //get the tier template revision creation time stamp and the duration + timeSinceCreation := time.Since(rev.GetCreationTimestamp().Time) - //list all the template revisions in a given NStemplate tier - if err := r.Client.List(ctx, &ttRevList, runtimeclient.InNamespace(nsTmplTier.GetNamespace()), - runtimeclient.MatchingLabels{toolchainv1alpha1.TierLabelKey: nsTmplTier.GetName()}); err != nil { - return nil, err + //the ttr age should be greater than 30 seconds + if timeSinceCreation < deletionTimeThreshold { + requeAfter := deletionTimeThreshold - timeSinceCreation + return false, true, requeAfter, fmt.Errorf("the revision creation timestamp is less than 30 sec(new revision)") } - //finding the tier revisions which are not refernced/used by the nstemplatetier.status.revisions - for _, ttrs := range ttRevList.Items { - unUsedRevisions[ttrs.Name] = true - for _, revCR := range nsTmplTier.Status.Revisions { - if ttrs.Name == revCR { - unUsedRevisions[ttrs.Name] = false - } + //check if the ttr name is present status.revisions + for _, ttstatusrev := range nsTmplTier.Status.Revisions { + if ttstatusrev == rev.Name { + return false, false, 0, fmt.Errorf("the revision is still being referenced in status.revisions") } } - //list the not refernced/unused tier revisions whose creation time stamp is older than 1 day. - for _, ttrs := range ttRevList.Items { - for ttrname, value := range unUsedRevisions { - if ttrs.Name == ttrname && value { - ttCreateTime, err := time.Parse(time.RFC3339, ttrs.CreationTimestamp.String()) - if err != nil { - return nil, err - } - duration := -time.Until(ttCreateTime) - if duration > ageOld { - ttrDelList = append(ttrDelList, ttr) - } - } - } - } + // get the outdated matchig label to list outdated spaces + matchOutdated, err := nstemplatetier.OutdatedTierSelector(nsTmplTier) + if err != nil { + return false, false, 0, err - return ttrDelList, nil -} + } + // look-up all spaces associated with the NSTemplateTier which are outdated + spaces := &toolchainv1alpha1.SpaceList{} + if err := r.Client.List(ctx, spaces, runtimeclient.InNamespace(nsTmplTier.Namespace), + matchOutdated, runtimeclient.Limit(1)); err != nil { + return false, false, 0, err + } -func (r *Reconciler) DeleteRevision(ctx context.Context, ttrevisions []toolchainv1alpha1.TierTemplateRevision) error { - for _, revision := range ttrevisions { - if err := r.Client.Delete(ctx, &revision); err != nil { - return fmt.Errorf("%s was not deleted : %w", revision.Name, err) - } + //there should not be any outdated spaces + if len(spaces.Items) > 0 { + return false, false, 0, fmt.Errorf("there are still some spaces which are outdated") } - return nil + + return true, false, 0, nil + } diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 618ca39be..ef4349684 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -1,79 +1,229 @@ -package nstemplatetierrevisioncleanup +package nstemplatetierrevisioncleanup_test import ( + "context" "fmt" - "strings" + "os" "testing" + "time" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/codeready-toolchain/host-operator/controllers/nstemplatetierrevisioncleanup" "github.com/codeready-toolchain/host-operator/pkg/apis" + tiertest "github.com/codeready-toolchain/host-operator/test/nstemplatetier" + "github.com/codeready-toolchain/host-operator/test/tiertemplaterevision" "github.com/codeready-toolchain/toolchain-common/pkg/test" - templatev1 "github.com/openshift/api/template/v1" + spacetest "github.com/codeready-toolchain/toolchain-common/pkg/test/space" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/serializer" - "k8s.io/client-go/kubernetes/scheme" + "k8s.io/apimachinery/pkg/types" + "k8s.io/kubectl/pkg/scheme" + controllerruntime "sigs.k8s.io/controller-runtime" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ) const ( operatorNamespace = "toolchain-host-operator" ) -func TestReconcile(t *testing.T) { +func TestTTRDeletionReconcile(t *testing.T) { + //given + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + s := createSpace(nsTemplateTier) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + //when + res, err := r.Reconcile(context.TODO(), req) + //then + require.NoError(t, err) + require.Equal(t, controllerruntime.Result{}, res) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).DoNotExist() + + t.Run("Failure", func(t *testing.T) { + t.Run("the creation timestamp is less than 30 sec", func(t *testing.T) { + // given + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.PreviousBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttr"), metav1.NewTime(time.Now())) + s := createSpace(nsTemplateTier) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + + // when + res, err := r.Reconcile(context.TODO(), req) + + // then + require.EqualError(t, err, "cannot delete the current TTR,requing :the revision creation timestamp is less than 30 sec(new revision)") + require.Equal(t, true, res.Requeue) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + + }) + t.Run("ttr is still being referenced in status.revisions", func(t *testing.T) { + // given + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.PreviousBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttr"), metav1.NewTime(time.Now().Add(-time.Minute))) + s := createSpace(nsTemplateTier) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + + // when + res, err := r.Reconcile(context.TODO(), req) + + // then + require.EqualError(t, err, "cannot delete the current TTR,requing :the revision is still being referenced in status.revisions") + require.Equal(t, false, res.Requeue) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + + }) + + t.Run("spaces are still being updated", func(t *testing.T) { + // given + nsTemplateTier.Status.Revisions = map[string]string{ + "base1ns-code-123456new": "base1ns-code-123456new-ttr", + "base1ns-clusterresources-123456new": "base1ns-clusterresources-123456new-ttr", + } + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + + // when + res, err := r.Reconcile(context.TODO(), req) + + // then + require.EqualError(t, err, "cannot delete the current TTR,requing :there are still some spaces which are outdated") + require.Equal(t, false, res.Requeue) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + }) + t.Run("Error while deleting the TTR", func(t *testing.T) { + // given + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + s := createSpace(nsTemplateTier) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { + return fmt.Errorf("some error cannot delete") + } + // when + res, err := r.Reconcile(context.TODO(), req) + + // then + require.EqualError(t, err, "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete") + require.Equal(t, false, res.Requeue) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + + }) + + t.Run("error while getting revision", func(t *testing.T) { + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + s := createSpace(nsTemplateTier) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + cl.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { + return fmt.Errorf("some error cannot get") + } + // when + res, err := r.Reconcile(context.TODO(), req) + + // then + require.EqualError(t, err, "unable to get the current TierTemplateRevision: some error cannot get") + require.Equal(t, false, res.Requeue) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + }) + + t.Run("error while listing outdated spaces", func(t *testing.T) { + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + s := createSpace(nsTemplateTier) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + cl.MockList = func(ctx context.Context, list runtimeclient.ObjectList, opts ...runtimeclient.ListOption) error { + return fmt.Errorf("some error") + } + // when + res, err := r.Reconcile(context.TODO(), req) + + // then + require.EqualError(t, err, "cannot delete the current TTR,requing :some error") + require.Equal(t, false, res.Requeue) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + }) + + t.Run("revision not found", func(t *testing.T) { + r, req, _ := prepareReconcile(t, "base") + //when + res, err := r.Reconcile(context.TODO(), req) + //then + require.NoError(t, err) + require.Equal(t, false, res.Requeue) + + }) + + t.Run("NsTemplate Tier not found", func(t *testing.T) { + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + r, req, _ := prepareReconcile(t, ttr.Name, ttr) + //when + res, err := r.Reconcile(context.TODO(), req) + //then + require.NoError(t, err) + require.Equal(t, false, res.Requeue) + + }) + + t.Run("tier label not found", func(t *testing.T) { + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + ttr.Labels = map[string]string{} + r, req, _ := prepareReconcile(t, ttr.Name, ttr) + //when + res, err := r.Reconcile(context.TODO(), req) + //then + require.EqualError(t, err, "tier-name label not found in tiertemplaterevision") + require.Equal(t, false, res.Requeue) + + }) + + }) } -func createTierTemplate(t *testing.T, typeName string, withTemplateObjects []runtime.RawExtension, tierName string) *toolchainv1alpha1.TierTemplate { - var ( - ns test.TemplateObject = ` -- apiVersion: v1 - kind: Namespace - metadata: - name: ${SPACE_NAME} -` - spacename test.TemplateParam = ` -- name: SPACE_NAME - value: johnsmith` - ) +func prepareReconcile(t *testing.T, name string, initObjs ...runtimeclient.Object) (*nstemplatetierrevisioncleanup.Reconciler, reconcile.Request, *test.FakeClient) { + os.Setenv("WATCH_NAMESPACE", test.HostOperatorNs) s := scheme.Scheme err := apis.AddToScheme(s) require.NoError(t, err) - codecFactory := serializer.NewCodecFactory(s) - decoder := codecFactory.UniversalDeserializer() - tmpl := templatev1.Template{} - _, _, err = decoder.Decode([]byte(test.CreateTemplate(test.WithObjects(ns), test.WithParams(spacename))), nil, &tmpl) - require.NoError(t, err) - - revision := "123456new" - // we can set the template field to something empty as it is not relevant for the tests - tt := &toolchainv1alpha1.TierTemplate{ + cl := test.NewFakeClient(t, initObjs...) + r := &nstemplatetierrevisioncleanup.Reconciler{ + Client: cl, + Scheme: s, + } + return r, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: name, + Namespace: operatorNamespace, + }, + }, cl +} +func createttr(nsTTier toolchainv1alpha1.NSTemplateTier, name string, crtime metav1.Time) *toolchainv1alpha1.TierTemplateRevision { + labels := map[string]string{ + toolchainv1alpha1.TierLabelKey: nsTTier.Name, + } + ttr := &toolchainv1alpha1.TierTemplateRevision{ ObjectMeta: metav1.ObjectMeta{ - Name: strings.ToLower(fmt.Sprintf("%s-%s-%s", tierName, typeName, revision)), - Namespace: test.HostOperatorNs, + Namespace: operatorNamespace, + Name: name, + Labels: labels, + CreationTimestamp: crtime, }, - Spec: toolchainv1alpha1.TierTemplateSpec{ - TierName: tierName, - Type: typeName, - Revision: revision, - Template: tmpl, + Spec: toolchainv1alpha1.TierTemplateRevisionSpec{ + Parameters: nsTTier.Spec.Parameters, }, } - - // just copy the raw objects to the templateObjects field - // TODO this will be removed once we switch on using templateObjects only in the TierTemplates - if withTemplateObjects != nil { - tt.Spec.TemplateObjects = withTemplateObjects - } - - return tt + return ttr } -func withTemplateObjects(templates ...unstructured.Unstructured) []runtime.RawExtension { - var templateObjects []runtime.RawExtension - for i := range templates { - templateObjects = append(templateObjects, runtime.RawExtension{Object: &templates[i]}) - } - return templateObjects +func createSpace(nsTTier *toolchainv1alpha1.NSTemplateTier) *toolchainv1alpha1.Space { + testSpace := spacetest.NewSpace(test.HostOperatorNs, "oddity1", + spacetest.WithTierNameAndHashLabelFor(nsTTier), + spacetest.WithSpecTargetCluster("member-1"), + spacetest.WithStatusTargetCluster("member-1"), + spacetest.WithFinalizer(), + spacetest.WithCondition(spacetest.Ready())) + return testSpace } From 76123883844e7015efe0648166bd69d958ab593b Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Fri, 7 Mar 2025 15:50:47 +0530 Subject: [PATCH 03/21] golint Signed-off-by: Feny Mehta --- ...atetier_revision_cleanup_controller_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index ef4349684..36120fca1 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -53,7 +53,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.EqualError(t, err, "cannot delete the current TTR,requing :the revision creation timestamp is less than 30 sec(new revision)") - require.Equal(t, true, res.Requeue) + require.True(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -69,7 +69,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.EqualError(t, err, "cannot delete the current TTR,requing :the revision is still being referenced in status.revisions") - require.Equal(t, false, res.Requeue) + require.False(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -88,7 +88,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.EqualError(t, err, "cannot delete the current TTR,requing :there are still some spaces which are outdated") - require.Equal(t, false, res.Requeue) + require.False(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) t.Run("Error while deleting the TTR", func(t *testing.T) { @@ -105,7 +105,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.EqualError(t, err, "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete") - require.Equal(t, false, res.Requeue) + require.False(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -123,7 +123,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.EqualError(t, err, "unable to get the current TierTemplateRevision: some error cannot get") - require.Equal(t, false, res.Requeue) + require.False(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -140,7 +140,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.EqualError(t, err, "cannot delete the current TTR,requing :some error") - require.Equal(t, false, res.Requeue) + require.False(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -150,7 +150,7 @@ func TestTTRDeletionReconcile(t *testing.T) { res, err := r.Reconcile(context.TODO(), req) //then require.NoError(t, err) - require.Equal(t, false, res.Requeue) + require.False(t, res.Requeue) }) @@ -162,7 +162,7 @@ func TestTTRDeletionReconcile(t *testing.T) { res, err := r.Reconcile(context.TODO(), req) //then require.NoError(t, err) - require.Equal(t, false, res.Requeue) + require.False(t, res.Requeue) }) @@ -175,7 +175,7 @@ func TestTTRDeletionReconcile(t *testing.T) { res, err := r.Reconcile(context.TODO(), req) //then require.EqualError(t, err, "tier-name label not found in tiertemplaterevision") - require.Equal(t, false, res.Requeue) + require.False(t, res.Requeue) }) From 01dbe62aa5e49c9b8b86325334e450c028cd5324 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Fri, 7 Mar 2025 16:00:59 +0530 Subject: [PATCH 04/21] some cleanup Signed-off-by: Feny Mehta --- ...tetier_revision_cleanup_controller_test.go | 45 +++++++------------ 1 file changed, 17 insertions(+), 28 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 36120fca1..7ca4d47f6 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -28,24 +28,26 @@ const ( ) func TestTTRDeletionReconcile(t *testing.T) { - //given - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) - s := createSpace(nsTemplateTier) - r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) - //when - res, err := r.Reconcile(context.TODO(), req) - //then - require.NoError(t, err) - require.Equal(t, controllerruntime.Result{}, res) - tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).DoNotExist() - + t.Run("TTR Deleted Successfuly", func(t *testing.T) { + //given + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + s := createSpace(nsTemplateTier) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + //when + res, err := r.Reconcile(context.TODO(), req) + //then + require.NoError(t, err) + require.Equal(t, controllerruntime.Result{}, res) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).DoNotExist() + }) t.Run("Failure", func(t *testing.T) { + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + s := createSpace(nsTemplateTier) t.Run("the creation timestamp is less than 30 sec", func(t *testing.T) { // given - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.PreviousBase1nsTemplates, tiertest.WithStatusRevisions()) ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttr"), metav1.NewTime(time.Now())) - s := createSpace(nsTemplateTier) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) // when @@ -59,9 +61,7 @@ func TestTTRDeletionReconcile(t *testing.T) { }) t.Run("ttr is still being referenced in status.revisions", func(t *testing.T) { // given - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.PreviousBase1nsTemplates, tiertest.WithStatusRevisions()) ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttr"), metav1.NewTime(time.Now().Add(-time.Minute))) - s := createSpace(nsTemplateTier) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) // when @@ -91,6 +91,7 @@ func TestTTRDeletionReconcile(t *testing.T) { require.False(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) + t.Run("Error while deleting the TTR", func(t *testing.T) { // given nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) @@ -107,13 +108,9 @@ func TestTTRDeletionReconcile(t *testing.T) { require.EqualError(t, err, "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete") require.False(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) - }) t.Run("error while getting revision", func(t *testing.T) { - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) - s := createSpace(nsTemplateTier) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) cl.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { return fmt.Errorf("some error cannot get") @@ -128,9 +125,6 @@ func TestTTRDeletionReconcile(t *testing.T) { }) t.Run("error while listing outdated spaces", func(t *testing.T) { - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) - s := createSpace(nsTemplateTier) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) cl.MockList = func(ctx context.Context, list runtimeclient.ObjectList, opts ...runtimeclient.ListOption) error { return fmt.Errorf("some error") @@ -155,8 +149,6 @@ func TestTTRDeletionReconcile(t *testing.T) { }) t.Run("NsTemplate Tier not found", func(t *testing.T) { - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) r, req, _ := prepareReconcile(t, ttr.Name, ttr) //when res, err := r.Reconcile(context.TODO(), req) @@ -167,8 +159,6 @@ func TestTTRDeletionReconcile(t *testing.T) { }) t.Run("tier label not found", func(t *testing.T) { - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) ttr.Labels = map[string]string{} r, req, _ := prepareReconcile(t, ttr.Name, ttr) //when @@ -176,7 +166,6 @@ func TestTTRDeletionReconcile(t *testing.T) { //then require.EqualError(t, err, "tier-name label not found in tiertemplaterevision") require.False(t, res.Requeue) - }) }) From 726947e3959a91d6d8e922bbd541ab231280a62b Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Fri, 7 Mar 2025 16:32:03 +0530 Subject: [PATCH 05/21] golint spelling Signed-off-by: Feny Mehta --- .../nstemplatetier_revision_cleanup_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 7ca4d47f6..617d7efbc 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -28,7 +28,7 @@ const ( ) func TestTTRDeletionReconcile(t *testing.T) { - t.Run("TTR Deleted Successfuly", func(t *testing.T) { + t.Run("TTR Deleted Successfully", func(t *testing.T) { //given nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) From ffc4f8360478d31537c7a082e921101eacc4fe08 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Mon, 10 Mar 2025 12:31:12 +0530 Subject: [PATCH 06/21] adding it to main Signed-off-by: Feny Mehta --- main.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/main.go b/main.go index cd8326cc1..2119ba51b 100644 --- a/main.go +++ b/main.go @@ -6,14 +6,16 @@ import ( "net/http" "os" goruntime "runtime" + "time" + "sigs.k8s.io/controller-runtime/pkg/cache" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" - "time" "github.com/codeready-toolchain/host-operator/controllers/deactivation" "github.com/codeready-toolchain/host-operator/controllers/masteruserrecord" "github.com/codeready-toolchain/host-operator/controllers/notification" "github.com/codeready-toolchain/host-operator/controllers/nstemplatetier" + "github.com/codeready-toolchain/host-operator/controllers/nstemplatetierrevisioncleanup" "github.com/codeready-toolchain/host-operator/controllers/socialevent" "github.com/codeready-toolchain/host-operator/controllers/space" "github.com/codeready-toolchain/host-operator/controllers/spacebindingcleanup" @@ -279,6 +281,13 @@ func main() { // nolint:gocyclo setupLog.Error(err, "unable to create controller", "controller", "NSTemplateTier") os.Exit(1) } + if err = (&nstemplatetierrevisioncleanup.Reconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "NSTemplatTierRevisionCleanup") + os.Exit(1) + } if err := (&toolchainconfig.Reconciler{ Client: mgr.GetClient(), GetMembersFunc: commoncluster.GetMemberClusters, From 81d0768f045a7fab54ead330dad72f3451c8b0f8 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Mon, 10 Mar 2025 16:14:49 +0530 Subject: [PATCH 07/21] review comments Signed-off-by: Feny Mehta --- ...emplatetier_revision_cleanup_controller.go | 55 ++++++++++--------- ...tetier_revision_cleanup_controller_test.go | 13 ++--- main.go | 1 - 3 files changed, 36 insertions(+), 33 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index 3e7ac047a..5c6be3d18 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -10,7 +10,6 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/controllers/nstemplatetier" "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -30,7 +29,6 @@ func (r *Reconciler) SetupWithManager(mgr manager.Manager) error { type Reconciler struct { Client runtimeclient.Client - Scheme *runtime.Scheme } func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { @@ -47,9 +45,22 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. logger.Error(err, "unable to get the current TierTemplateRevision") return reconcile.Result{}, fmt.Errorf("unable to get the current TierTemplateRevision: %w", err) } + //there is no point in fetching the NStemplateTier, if the TTR is just created + + //get the tier template revision creation time stamp and the duration + timeSinceCreation := time.Since(ttr.GetCreationTimestamp().Time) + + //the ttr age should be greater than 30 seconds + if timeSinceCreation < deletionTimeThreshold { + requeAfter := deletionTimeThreshold - timeSinceCreation + return reconcile.Result{RequeueAfter: requeAfter, Requeue: true}, nil + } + + //check if there is tier-name label available tierName, ok := ttr.GetLabels()[toolchainv1alpha1.TierLabelKey] if !ok { - return reconcile.Result{}, fmt.Errorf("tier-name label not found in tiertemplaterevision") + logger.Info("tier-name label not found in tiertemplaterevision") + return reconcile.Result{}, nil } // fetch the related NSTemplateTier tier @@ -60,7 +71,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. }, tier); err != nil { if errors.IsNotFound(err) { logger.Info("NSTemplateTier not found") - return reconcile.Result{}, nil } // Error reading the object - requeue the request. logger.Error(err, "unable to get the current NSTemplateTier") @@ -69,13 +79,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. // verify that the tier template revision which is unsused(not referenced by any tiers and whose creation date is older that 30secs // and all the spaces are up-to-date) - UnusedRevisionBool, requeue, reqAft, err := r.VerifyUnusedTTR(ctx, tier, ttr, deletionTimeThreshold) + unusedRevisionBool, err := r.VerifyUnusedTTR(ctx, tier, ttr) if err != nil { - return reconcile.Result{RequeueAfter: reqAft, Requeue: requeue}, fmt.Errorf("cannot delete the current TTR,requing :%w", err) + return reconcile.Result{}, err } // Delete the unused revision - if UnusedRevisionBool { + if unusedRevisionBool { if err := r.Client.Delete(ctx, ttr); err != nil { return reconcile.Result{}, fmt.Errorf("unable to delete the current Tier Template Revision %s: %w", ttr.Name, err) } @@ -87,42 +97,37 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. // VerifyUnusedTTR function verifies that the tier template revision's creation time stamp is older than 30sec, // it is not present/refernced in status.revisions field, and there are no outdated spaces. func (r *Reconciler) VerifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, - rev *toolchainv1alpha1.TierTemplateRevision, deletionTimeThreshold time.Duration) (bool, bool, time.Duration, error) { - - //get the tier template revision creation time stamp and the duration - timeSinceCreation := time.Since(rev.GetCreationTimestamp().Time) - - //the ttr age should be greater than 30 seconds - if timeSinceCreation < deletionTimeThreshold { - requeAfter := deletionTimeThreshold - timeSinceCreation - return false, true, requeAfter, fmt.Errorf("the revision creation timestamp is less than 30 sec(new revision)") - } + rev *toolchainv1alpha1.TierTemplateRevision) (bool, error) { + logger := log.FromContext(ctx) //check if the ttr name is present status.revisions - for _, ttstatusrev := range nsTmplTier.Status.Revisions { - if ttstatusrev == rev.Name { - return false, false, 0, fmt.Errorf("the revision is still being referenced in status.revisions") + for _, ttStatusRev := range nsTmplTier.Status.Revisions { + if ttStatusRev == rev.Name { + logger.Info("the revision is still being referenced in status.revisions") + return false, nil } } // get the outdated matchig label to list outdated spaces matchOutdated, err := nstemplatetier.OutdatedTierSelector(nsTmplTier) if err != nil { - return false, false, 0, err + return false, err } // look-up all spaces associated with the NSTemplateTier which are outdated spaces := &toolchainv1alpha1.SpaceList{} if err := r.Client.List(ctx, spaces, runtimeclient.InNamespace(nsTmplTier.Namespace), matchOutdated, runtimeclient.Limit(1)); err != nil { - return false, false, 0, err + return false, err } - //there should not be any outdated spaces + //If there has been an update on nstemplatetier, it might be in a process to update all the spaces. + // so we need to check that that there should not be any outdated spaces. if len(spaces.Items) > 0 { - return false, false, 0, fmt.Errorf("there are still some spaces which are outdated") + logger.Info("there are still some spaces which are outdated") + return false, nil } - return true, false, 0, nil + return true, nil } diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 617d7efbc..4363682f4 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -54,7 +54,7 @@ func TestTTRDeletionReconcile(t *testing.T) { res, err := r.Reconcile(context.TODO(), req) // then - require.EqualError(t, err, "cannot delete the current TTR,requing :the revision creation timestamp is less than 30 sec(new revision)") + require.NoError(t, err) require.True(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) @@ -68,7 +68,7 @@ func TestTTRDeletionReconcile(t *testing.T) { res, err := r.Reconcile(context.TODO(), req) // then - require.EqualError(t, err, "cannot delete the current TTR,requing :the revision is still being referenced in status.revisions") + require.NoError(t, err) require.False(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) @@ -87,7 +87,7 @@ func TestTTRDeletionReconcile(t *testing.T) { res, err := r.Reconcile(context.TODO(), req) // then - require.EqualError(t, err, "cannot delete the current TTR,requing :there are still some spaces which are outdated") + require.NoError(t, err) require.False(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -133,7 +133,7 @@ func TestTTRDeletionReconcile(t *testing.T) { res, err := r.Reconcile(context.TODO(), req) // then - require.EqualError(t, err, "cannot delete the current TTR,requing :some error") + require.EqualError(t, err, "some error") require.False(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -153,7 +153,7 @@ func TestTTRDeletionReconcile(t *testing.T) { //when res, err := r.Reconcile(context.TODO(), req) //then - require.NoError(t, err) + require.Error(t, err) require.False(t, res.Requeue) }) @@ -164,7 +164,7 @@ func TestTTRDeletionReconcile(t *testing.T) { //when res, err := r.Reconcile(context.TODO(), req) //then - require.EqualError(t, err, "tier-name label not found in tiertemplaterevision") + require.NoError(t, err) require.False(t, res.Requeue) }) @@ -180,7 +180,6 @@ func prepareReconcile(t *testing.T, name string, initObjs ...runtimeclient.Objec cl := test.NewFakeClient(t, initObjs...) r := &nstemplatetierrevisioncleanup.Reconciler{ Client: cl, - Scheme: s, } return r, reconcile.Request{ NamespacedName: types.NamespacedName{ diff --git a/main.go b/main.go index 2119ba51b..fa1af4caa 100644 --- a/main.go +++ b/main.go @@ -283,7 +283,6 @@ func main() { // nolint:gocyclo } if err = (&nstemplatetierrevisioncleanup.Reconciler{ Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "NSTemplatTierRevisionCleanup") os.Exit(1) From 87707b010fd3f6f1faadafc27cf0d2af9e29b489 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Tue, 11 Mar 2025 12:16:27 +0530 Subject: [PATCH 08/21] review comments part 2 Signed-off-by: Feny Mehta --- ...emplatetier_revision_cleanup_controller.go | 23 ++++++++----------- ...tetier_revision_cleanup_controller_test.go | 2 +- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index 5c6be3d18..bc56039b6 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -59,8 +59,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. //check if there is tier-name label available tierName, ok := ttr.GetLabels()[toolchainv1alpha1.TierLabelKey] if !ok { - logger.Info("tier-name label not found in tiertemplaterevision") - return reconcile.Result{}, nil + return reconcile.Result{}, fmt.Errorf("tier-name label not found in tiertemplaterevision") } // fetch the related NSTemplateTier tier @@ -69,23 +68,19 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. Namespace: ttr.GetNamespace(), Name: tierName, }, tier); err != nil { - if errors.IsNotFound(err) { - logger.Info("NSTemplateTier not found") - } // Error reading the object - requeue the request. - logger.Error(err, "unable to get the current NSTemplateTier") return reconcile.Result{}, fmt.Errorf("unable to get the current NSTemplateTier: %w", err) } - // verify that the tier template revision which is unsused(not referenced by any tiers and whose creation date is older that 30secs + // verify that the tier template revision which is unused(not referenced by any tiers and whose creation date is older that 30secs // and all the spaces are up-to-date) - unusedRevisionBool, err := r.VerifyUnusedTTR(ctx, tier, ttr) + isTTrUnused, err := r.verifyUnusedTTR(ctx, tier, ttr) if err != nil { return reconcile.Result{}, err } // Delete the unused revision - if unusedRevisionBool { + if isTTrUnused { if err := r.Client.Delete(ctx, ttr); err != nil { return reconcile.Result{}, fmt.Errorf("unable to delete the current Tier Template Revision %s: %w", ttr.Name, err) } @@ -95,20 +90,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. } // VerifyUnusedTTR function verifies that the tier template revision's creation time stamp is older than 30sec, -// it is not present/refernced in status.revisions field, and there are no outdated spaces. -func (r *Reconciler) VerifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, +// it is not present/referenced in status.revisions field, and there are no outdated spaces. +func (r *Reconciler) verifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, rev *toolchainv1alpha1.TierTemplateRevision) (bool, error) { logger := log.FromContext(ctx) //check if the ttr name is present status.revisions for _, ttStatusRev := range nsTmplTier.Status.Revisions { if ttStatusRev == rev.Name { - logger.Info("the revision is still being referenced in status.revisions") + logger.Info("the revision %s is still being referenced in status.revisions", rev.Name) return false, nil } } - // get the outdated matchig label to list outdated spaces + // get the outdated matching label to list outdated spaces matchOutdated, err := nstemplatetier.OutdatedTierSelector(nsTmplTier) if err != nil { return false, err @@ -122,7 +117,7 @@ func (r *Reconciler) VerifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv } //If there has been an update on nstemplatetier, it might be in a process to update all the spaces. - // so we need to check that that there should not be any outdated spaces. + // so we need to check that there should be no outdated spaces. if len(spaces.Items) > 0 { logger.Info("there are still some spaces which are outdated") return false, nil diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 4363682f4..8a755923b 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -164,7 +164,7 @@ func TestTTRDeletionReconcile(t *testing.T) { //when res, err := r.Reconcile(context.TODO(), req) //then - require.NoError(t, err) + require.EqualError(t, err, "tier-name label not found in tiertemplaterevision") require.False(t, res.Requeue) }) From e898e6c12acfb7b3b64fc72699572faa93371492 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Tue, 11 Mar 2025 12:24:08 +0530 Subject: [PATCH 09/21] golanglint ci Signed-off-by: Feny Mehta --- .../nstemplatetier_revision_cleanup_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index bc56039b6..92881e79e 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -98,7 +98,7 @@ func (r *Reconciler) verifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv //check if the ttr name is present status.revisions for _, ttStatusRev := range nsTmplTier.Status.Revisions { if ttStatusRev == rev.Name { - logger.Info("the revision %s is still being referenced in status.revisions", rev.Name) + logger.Info("the revision is still being referenced in status.revisions", "ttr.name", rev.Name) return false, nil } } From 7307ee7c9674f32911b9cb10d1de68d2234e817c Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Tue, 11 Mar 2025 17:09:21 +0530 Subject: [PATCH 10/21] nstemplatetier not found rc Signed-off-by: Feny Mehta --- ...emplatetier_revision_cleanup_controller.go | 16 +++++-- ...tetier_revision_cleanup_controller_test.go | 43 ++++++++++++++++--- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index 92881e79e..796edd261 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -18,7 +18,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" ) -const deletionTimeThreshold = 30 * time.Second +const DeletionTimeThreshold = 30 * time.Second // SetupWithManager sets up the controller with the Manager. func (r *Reconciler) SetupWithManager(mgr manager.Manager) error { @@ -51,9 +51,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. timeSinceCreation := time.Since(ttr.GetCreationTimestamp().Time) //the ttr age should be greater than 30 seconds - if timeSinceCreation < deletionTimeThreshold { - requeAfter := deletionTimeThreshold - timeSinceCreation - return reconcile.Result{RequeueAfter: requeAfter, Requeue: true}, nil + if timeSinceCreation < DeletionTimeThreshold { + requeAfter := DeletionTimeThreshold - timeSinceCreation + return reconcile.Result{RequeueAfter: requeAfter}, nil } //check if there is tier-name label available @@ -68,6 +68,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. Namespace: ttr.GetNamespace(), Name: tierName, }, tier); err != nil { + if errors.IsNotFound(err) { + // if there is no related nstemplate tier, we can delete the TTR directly + logger.Info("NSTemplateTier not found, proceeding to delete it ") + if err := r.Client.Delete(ctx, ttr); err != nil { + return reconcile.Result{}, fmt.Errorf("unable to delete the current Tier Template Revision %s: %w", ttr.Name, err) + } + return reconcile.Result{}, nil + } // Error reading the object - requeue the request. return reconcile.Result{}, fmt.Errorf("unable to get the current NSTemplateTier: %w", err) } diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 8a755923b..66bbcf82b 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -14,6 +14,7 @@ import ( "github.com/codeready-toolchain/host-operator/test/tiertemplaterevision" "github.com/codeready-toolchain/toolchain-common/pkg/test" spacetest "github.com/codeready-toolchain/toolchain-common/pkg/test/space" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -47,7 +48,7 @@ func TestTTRDeletionReconcile(t *testing.T) { s := createSpace(nsTemplateTier) t.Run("the creation timestamp is less than 30 sec", func(t *testing.T) { // given - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttr"), metav1.NewTime(time.Now())) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttr"), metav1.NewTime(time.Now().Add(-29*time.Second))) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) // when @@ -55,7 +56,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.NoError(t, err) - require.True(t, res.Requeue) + assert.LessOrEqual(t, res.RequeueAfter, time.Second) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -124,6 +125,22 @@ func TestTTRDeletionReconcile(t *testing.T) { tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) + t.Run("error while getting NSTemplate Tier", func(t *testing.T) { + r, req, cl := prepareReconcile(t, ttr.Name, ttr, nsTemplateTier) + + cl.MockGet = func(ctx context.Context, key types.NamespacedName, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { + if _, ok := obj.(*toolchainv1alpha1.NSTemplateTier); ok { + return fmt.Errorf("mock error") + } + return cl.Client.Get(ctx, key, obj, opts...) + } + //when + _, err := r.Reconcile(context.TODO(), req) + //then + require.EqualError(t, err, "unable to get the current NSTemplateTier: mock error") + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + + }) t.Run("error while listing outdated spaces", func(t *testing.T) { r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) cl.MockList = func(ctx context.Context, list runtimeclient.ObjectList, opts ...runtimeclient.ListOption) error { @@ -148,16 +165,30 @@ func TestTTRDeletionReconcile(t *testing.T) { }) - t.Run("NsTemplate Tier not found", func(t *testing.T) { - r, req, _ := prepareReconcile(t, ttr.Name, ttr) + t.Run("NSTemplate Tier not found", func(t *testing.T) { + r, req, cl := prepareReconcile(t, ttr.Name, ttr) //when res, err := r.Reconcile(context.TODO(), req) //then - require.Error(t, err) - require.False(t, res.Requeue) + require.NoError(t, err) + require.Equal(t, controllerruntime.Result{}, res) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).DoNotExist() }) + t.Run("NSTemplate Tier not found, but error deleting ttr", func(t *testing.T) { + r, req, cl := prepareReconcile(t, ttr.Name, ttr) + cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { + return fmt.Errorf("some error cannot delete") + } + //when + res, err := r.Reconcile(context.TODO(), req) + //then + require.EqualError(t, err, "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete") + require.False(t, res.Requeue) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + }) + t.Run("tier label not found", func(t *testing.T) { ttr.Labels = map[string]string{} r, req, _ := prepareReconcile(t, ttr.Name, ttr) From 1be1970f581291291238e2b482fb8ef8832a0f07 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Tue, 11 Mar 2025 17:33:39 +0530 Subject: [PATCH 11/21] requeu bool true Signed-off-by: Feny Mehta --- .../nstemplatetier_revision_cleanup_controller.go | 2 +- .../nstemplatetier_revision_cleanup_controller_test.go | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index 796edd261..0f5133fb2 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -53,7 +53,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. //the ttr age should be greater than 30 seconds if timeSinceCreation < DeletionTimeThreshold { requeAfter := DeletionTimeThreshold - timeSinceCreation - return reconcile.Result{RequeueAfter: requeAfter}, nil + return reconcile.Result{RequeueAfter: requeAfter, Requeue: true}, nil } //check if there is tier-name label available diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 66bbcf82b..9a0a7dec0 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -14,7 +14,6 @@ import ( "github.com/codeready-toolchain/host-operator/test/tiertemplaterevision" "github.com/codeready-toolchain/toolchain-common/pkg/test" spacetest "github.com/codeready-toolchain/toolchain-common/pkg/test/space" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -29,6 +28,7 @@ const ( ) func TestTTRDeletionReconcile(t *testing.T) { + t.Run("TTR Deleted Successfully", func(t *testing.T) { //given nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) @@ -42,6 +42,7 @@ func TestTTRDeletionReconcile(t *testing.T) { require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).DoNotExist() }) + t.Run("Failure", func(t *testing.T) { nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) @@ -56,7 +57,8 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.NoError(t, err) - assert.LessOrEqual(t, res.RequeueAfter, time.Second) + require.LessOrEqual(t, res.RequeueAfter, time.Second) + require.True(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) From 467980e538b2743c74af00afed747283677c8b12 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Tue, 11 Mar 2025 20:16:02 +0530 Subject: [PATCH 12/21] deleteing ttr if tier-name not there Signed-off-by: Feny Mehta --- ...emplatetier_revision_cleanup_controller.go | 27 ++++++++++++------- ...tetier_revision_cleanup_controller_test.go | 20 +++++++++++--- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index 0f5133fb2..0045ffe87 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -59,8 +59,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. //check if there is tier-name label available tierName, ok := ttr.GetLabels()[toolchainv1alpha1.TierLabelKey] if !ok { - return reconcile.Result{}, fmt.Errorf("tier-name label not found in tiertemplaterevision") - + //if tier-name label is not found we should delete the TTR + if err := r.deleteTTR(ctx, ttr); err != nil { + return reconcile.Result{}, err + } + return reconcile.Result{}, nil } // fetch the related NSTemplateTier tier tier := &toolchainv1alpha1.NSTemplateTier{} @@ -70,9 +73,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. }, tier); err != nil { if errors.IsNotFound(err) { // if there is no related nstemplate tier, we can delete the TTR directly - logger.Info("NSTemplateTier not found, proceeding to delete it ") - if err := r.Client.Delete(ctx, ttr); err != nil { - return reconcile.Result{}, fmt.Errorf("unable to delete the current Tier Template Revision %s: %w", ttr.Name, err) + if err := r.deleteTTR(ctx, ttr); err != nil { + return reconcile.Result{}, err } return reconcile.Result{}, nil } @@ -89,15 +91,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. // Delete the unused revision if isTTrUnused { - if err := r.Client.Delete(ctx, ttr); err != nil { - return reconcile.Result{}, fmt.Errorf("unable to delete the current Tier Template Revision %s: %w", ttr.Name, err) + if err := r.deleteTTR(ctx, ttr); err != nil { + return reconcile.Result{}, err } + return reconcile.Result{}, nil } return reconcile.Result{}, nil } -// VerifyUnusedTTR function verifies that the tier template revision's creation time stamp is older than 30sec, +// verifyUnusedTTR function verifies that the tier template revision's creation time stamp is older than 30sec, // it is not present/referenced in status.revisions field, and there are no outdated spaces. func (r *Reconciler) verifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, rev *toolchainv1alpha1.TierTemplateRevision) (bool, error) { @@ -130,7 +133,13 @@ func (r *Reconciler) verifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv logger.Info("there are still some spaces which are outdated") return false, nil } - return true, nil +} +// deleteTTR function delete the TTR +func (r *Reconciler) deleteTTR(ctx context.Context, ttr *toolchainv1alpha1.TierTemplateRevision) error { + if err := r.Client.Delete(ctx, ttr); err != nil { + return fmt.Errorf("unable to delete the current Tier Template Revision %s: %w", ttr.Name, err) + } + return nil } diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 9a0a7dec0..e181cf631 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -167,6 +167,16 @@ func TestTTRDeletionReconcile(t *testing.T) { }) + t.Run("tier label not found", func(t *testing.T) { + ttr.Labels = map[string]string{} + r, req, cl := prepareReconcile(t, ttr.Name, ttr) + //when + res, err := r.Reconcile(context.TODO(), req) + //then + require.NoError(t, err) + require.Equal(t, controllerruntime.Result{}, res) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).DoNotExist() + }) t.Run("NSTemplate Tier not found", func(t *testing.T) { r, req, cl := prepareReconcile(t, ttr.Name, ttr) //when @@ -191,14 +201,18 @@ func TestTTRDeletionReconcile(t *testing.T) { tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) - t.Run("tier label not found", func(t *testing.T) { + t.Run("tier label not found but error deleting ttr", func(t *testing.T) { ttr.Labels = map[string]string{} - r, req, _ := prepareReconcile(t, ttr.Name, ttr) + r, req, cl := prepareReconcile(t, ttr.Name, ttr) + cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { + return fmt.Errorf("some error cannot delete") + } //when res, err := r.Reconcile(context.TODO(), req) //then - require.EqualError(t, err, "tier-name label not found in tiertemplaterevision") + require.EqualError(t, err, "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete") require.False(t, res.Requeue) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) }) From 6059ce5a023bf4b6e75b54b28f1fa59f1197602e Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Tue, 11 Mar 2025 20:45:23 +0530 Subject: [PATCH 13/21] coverage Signed-off-by: Feny Mehta --- ...tetier_revision_cleanup_controller_test.go | 39 ++++++++++++------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index e181cf631..98e3f8d76 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -15,7 +15,9 @@ import ( "github.com/codeready-toolchain/toolchain-common/pkg/test" spacetest "github.com/codeready-toolchain/toolchain-common/pkg/test/space" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/kubectl/pkg/scheme" controllerruntime "sigs.k8s.io/controller-runtime" @@ -166,19 +168,14 @@ func TestTTRDeletionReconcile(t *testing.T) { require.False(t, res.Requeue) }) - - t.Run("tier label not found", func(t *testing.T) { - ttr.Labels = map[string]string{} - r, req, cl := prepareReconcile(t, ttr.Name, ttr) - //when - res, err := r.Reconcile(context.TODO(), req) - //then - require.NoError(t, err) - require.Equal(t, controllerruntime.Result{}, res) - tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).DoNotExist() - }) t.Run("NSTemplate Tier not found", func(t *testing.T) { - r, req, cl := prepareReconcile(t, ttr.Name, ttr) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, nsTemplateTier) + cl.MockGet = func(ctx context.Context, key types.NamespacedName, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { + if _, ok := obj.(*toolchainv1alpha1.NSTemplateTier); ok { + return errors.NewNotFound(schema.GroupResource{}, key.Name) + } + return cl.Client.Get(ctx, key, obj, opts...) + } //when res, err := r.Reconcile(context.TODO(), req) //then @@ -189,7 +186,13 @@ func TestTTRDeletionReconcile(t *testing.T) { }) t.Run("NSTemplate Tier not found, but error deleting ttr", func(t *testing.T) { - r, req, cl := prepareReconcile(t, ttr.Name, ttr) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, nsTemplateTier) + cl.MockGet = func(ctx context.Context, key types.NamespacedName, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { + if _, ok := obj.(*toolchainv1alpha1.NSTemplateTier); ok { + return errors.NewNotFound(schema.GroupResource{}, key.Name) + } + return cl.Client.Get(ctx, key, obj, opts...) + } cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { return fmt.Errorf("some error cannot delete") } @@ -200,6 +203,16 @@ func TestTTRDeletionReconcile(t *testing.T) { require.False(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) + t.Run("tier label not found", func(t *testing.T) { + ttr.Labels = map[string]string{} + r, req, cl := prepareReconcile(t, ttr.Name, ttr) + //when + res, err := r.Reconcile(context.TODO(), req) + //then + require.NoError(t, err) + require.Equal(t, controllerruntime.Result{}, res) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).DoNotExist() + }) t.Run("tier label not found but error deleting ttr", func(t *testing.T) { ttr.Labels = map[string]string{} From ab49a422d75d1daaac55e00667a13a312f617f54 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Wed, 12 Mar 2025 13:36:25 +0530 Subject: [PATCH 14/21] reque bool Signed-off-by: Feny Mehta --- ...stemplatetier_revision_cleanup_controller.go | 2 +- ...latetier_revision_cleanup_controller_test.go | 17 ++++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index 0045ffe87..d210a69fa 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -53,7 +53,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. //the ttr age should be greater than 30 seconds if timeSinceCreation < DeletionTimeThreshold { requeAfter := DeletionTimeThreshold - timeSinceCreation - return reconcile.Result{RequeueAfter: requeAfter, Requeue: true}, nil + return reconcile.Result{RequeueAfter: requeAfter}, nil } //check if there is tier-name label available diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 98e3f8d76..f8a277390 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -60,7 +60,6 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.NoError(t, err) require.LessOrEqual(t, res.RequeueAfter, time.Second) - require.True(t, res.Requeue) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -74,7 +73,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.NoError(t, err) - require.False(t, res.Requeue) + require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -93,7 +92,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.NoError(t, err) - require.False(t, res.Requeue) + require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -111,7 +110,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.EqualError(t, err, "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete") - require.False(t, res.Requeue) + require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -125,7 +124,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.EqualError(t, err, "unable to get the current TierTemplateRevision: some error cannot get") - require.False(t, res.Requeue) + require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -155,7 +154,7 @@ func TestTTRDeletionReconcile(t *testing.T) { // then require.EqualError(t, err, "some error") - require.False(t, res.Requeue) + require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) @@ -165,7 +164,7 @@ func TestTTRDeletionReconcile(t *testing.T) { res, err := r.Reconcile(context.TODO(), req) //then require.NoError(t, err) - require.False(t, res.Requeue) + require.Equal(t, controllerruntime.Result{}, res) }) t.Run("NSTemplate Tier not found", func(t *testing.T) { @@ -200,7 +199,7 @@ func TestTTRDeletionReconcile(t *testing.T) { res, err := r.Reconcile(context.TODO(), req) //then require.EqualError(t, err, "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete") - require.False(t, res.Requeue) + require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) t.Run("tier label not found", func(t *testing.T) { @@ -224,7 +223,7 @@ func TestTTRDeletionReconcile(t *testing.T) { res, err := r.Reconcile(context.TODO(), req) //then require.EqualError(t, err, "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete") - require.False(t, res.Requeue) + require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) From 879809a38e7b9eb689377063486672d090347d21 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Thu, 13 Mar 2025 18:27:01 +0530 Subject: [PATCH 15/21] adding test case and rc Signed-off-by: Feny Mehta --- ...emplatetier_revision_cleanup_controller.go | 17 +++++----- ...tetier_revision_cleanup_controller_test.go | 34 +++++++++++++------ 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index d210a69fa..b115ec591 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -42,11 +42,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, nil } // Error reading the object - requeue the request. - logger.Error(err, "unable to get the current TierTemplateRevision") return reconcile.Result{}, fmt.Errorf("unable to get the current TierTemplateRevision: %w", err) } - //there is no point in fetching the NStemplateTier, if the TTR is just created + //there is no point in fetching the NStemplateTier, if the TTR is just created //get the tier template revision creation time stamp and the duration timeSinceCreation := time.Since(ttr.GetCreationTimestamp().Time) @@ -82,7 +81,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, fmt.Errorf("unable to get the current NSTemplateTier: %w", err) } - // verify that the tier template revision which is unused(not referenced by any tiers and whose creation date is older that 30secs + // verify that the tier template revision which is unused(not referenced by any tiers // and all the spaces are up-to-date) isTTrUnused, err := r.verifyUnusedTTR(ctx, tier, ttr) if err != nil { @@ -91,17 +90,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. // Delete the unused revision if isTTrUnused { - if err := r.deleteTTR(ctx, ttr); err != nil { - return reconcile.Result{}, err - } - return reconcile.Result{}, nil + return reconcile.Result{}, r.deleteTTR(ctx, ttr) } return reconcile.Result{}, nil } -// verifyUnusedTTR function verifies that the tier template revision's creation time stamp is older than 30sec, -// it is not present/referenced in status.revisions field, and there are no outdated spaces. +// verifyUnusedTTR function verifies that the tier template revision +// is not present/referenced in status.revisions field, and there are no outdated spaces. func (r *Reconciler) verifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, rev *toolchainv1alpha1.TierTemplateRevision) (bool, error) { @@ -139,6 +135,9 @@ func (r *Reconciler) verifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv // deleteTTR function delete the TTR func (r *Reconciler) deleteTTR(ctx context.Context, ttr *toolchainv1alpha1.TierTemplateRevision) error { if err := r.Client.Delete(ctx, ttr); err != nil { + if errors.IsNotFound(err) { + return nil // was already deleted + } return fmt.Errorf("unable to delete the current Tier Template Revision %s: %w", ttr.Name, err) } return nil diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index f8a277390..7cdbcf600 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -80,6 +80,7 @@ func TestTTRDeletionReconcile(t *testing.T) { t.Run("spaces are still being updated", func(t *testing.T) { // given + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) nsTemplateTier.Status.Revisions = map[string]string{ "base1ns-code-123456new": "base1ns-code-123456new-ttr", "base1ns-clusterresources-123456new": "base1ns-clusterresources-123456new-ttr", @@ -98,7 +99,6 @@ func TestTTRDeletionReconcile(t *testing.T) { t.Run("Error while deleting the TTR", func(t *testing.T) { // given - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) s := createSpace(nsTemplateTier) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) @@ -114,6 +114,23 @@ func TestTTRDeletionReconcile(t *testing.T) { tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) + t.Run("Is Not Found Error-already deleted, while deleting the TTR", func(t *testing.T) { + // given + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + s := createSpace(nsTemplateTier) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { + return errors.NewNotFound(schema.GroupResource{}, ttr.Name) + } + + // when + res, err := r.Reconcile(context.TODO(), req) + + // then + require.NoError(t, err) + require.Equal(t, controllerruntime.Result{}, res) + }) t.Run("error while getting revision", func(t *testing.T) { r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) cl.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { @@ -167,7 +184,7 @@ func TestTTRDeletionReconcile(t *testing.T) { require.Equal(t, controllerruntime.Result{}, res) }) - t.Run("NSTemplate Tier not found", func(t *testing.T) { + t.Run("NSTemplate Tier not found - ttr gets deleted", func(t *testing.T) { r, req, cl := prepareReconcile(t, ttr.Name, ttr, nsTemplateTier) cl.MockGet = func(ctx context.Context, key types.NamespacedName, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { if _, ok := obj.(*toolchainv1alpha1.NSTemplateTier); ok { @@ -202,7 +219,8 @@ func TestTTRDeletionReconcile(t *testing.T) { require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) - t.Run("tier label not found", func(t *testing.T) { + t.Run("tier label not found - ttr gets deleted", func(t *testing.T) { + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) ttr.Labels = map[string]string{} r, req, cl := prepareReconcile(t, ttr.Name, ttr) //when @@ -214,6 +232,7 @@ func TestTTRDeletionReconcile(t *testing.T) { }) t.Run("tier label not found but error deleting ttr", func(t *testing.T) { + ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) ttr.Labels = map[string]string{} r, req, cl := prepareReconcile(t, ttr.Name, ttr) cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { @@ -258,19 +277,12 @@ func createttr(nsTTier toolchainv1alpha1.NSTemplateTier, name string, crtime met Labels: labels, CreationTimestamp: crtime, }, - Spec: toolchainv1alpha1.TierTemplateRevisionSpec{ - Parameters: nsTTier.Spec.Parameters, - }, } return ttr } func createSpace(nsTTier *toolchainv1alpha1.NSTemplateTier) *toolchainv1alpha1.Space { testSpace := spacetest.NewSpace(test.HostOperatorNs, "oddity1", - spacetest.WithTierNameAndHashLabelFor(nsTTier), - spacetest.WithSpecTargetCluster("member-1"), - spacetest.WithStatusTargetCluster("member-1"), - spacetest.WithFinalizer(), - spacetest.WithCondition(spacetest.Ready())) + spacetest.WithTierNameAndHashLabelFor(nsTTier)) return testSpace } From 8d3337748ce7bfdd8b4467a49fdf139488fdc7b1 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Thu, 13 Mar 2025 19:24:04 +0530 Subject: [PATCH 16/21] removing extra Signed-off-by: Feny Mehta --- .../nstemplatetier_revision_cleanup_controller_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 7cdbcf600..519956b7e 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -116,7 +116,6 @@ func TestTTRDeletionReconcile(t *testing.T) { t.Run("Is Not Found Error-already deleted, while deleting the TTR", func(t *testing.T) { // given - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) s := createSpace(nsTemplateTier) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) From 588ef620241c09e93de3638ccd89d286f8b63dee Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Thu, 13 Mar 2025 21:25:52 +0530 Subject: [PATCH 17/21] loggers and comments Signed-off-by: Feny Mehta --- ...emplatetier_revision_cleanup_controller.go | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index b115ec591..941ced036 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -51,18 +51,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. //the ttr age should be greater than 30 seconds if timeSinceCreation < DeletionTimeThreshold { - requeAfter := DeletionTimeThreshold - timeSinceCreation - return reconcile.Result{RequeueAfter: requeAfter}, nil + requeueAfter := DeletionTimeThreshold - timeSinceCreation + logger.Info("the TierTemplateRevision is not old enough", "requeue-after", requeueAfter) + return reconcile.Result{RequeueAfter: requeueAfter}, nil } //check if there is tier-name label available tierName, ok := ttr.GetLabels()[toolchainv1alpha1.TierLabelKey] if !ok { //if tier-name label is not found we should delete the TTR - if err := r.deleteTTR(ctx, ttr); err != nil { - return reconcile.Result{}, err - } - return reconcile.Result{}, nil + return reconcile.Result{}, r.deleteTTR(ctx, ttr) } // fetch the related NSTemplateTier tier tier := &toolchainv1alpha1.NSTemplateTier{} @@ -72,17 +70,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. }, tier); err != nil { if errors.IsNotFound(err) { // if there is no related nstemplate tier, we can delete the TTR directly - if err := r.deleteTTR(ctx, ttr); err != nil { - return reconcile.Result{}, err - } - return reconcile.Result{}, nil + return reconcile.Result{}, r.deleteTTR(ctx, ttr) } // Error reading the object - requeue the request. return reconcile.Result{}, fmt.Errorf("unable to get the current NSTemplateTier: %w", err) } - // verify that the tier template revision which is unused(not referenced by any tiers - // and all the spaces are up-to-date) + // let's make sure that we delete the TTR only if it's not used isTTrUnused, err := r.verifyUnusedTTR(ctx, tier, ttr) if err != nil { return reconcile.Result{}, err @@ -96,8 +90,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, nil } -// verifyUnusedTTR function verifies that the tier template revision -// is not present/referenced in status.revisions field, and there are no outdated spaces. +// verifyUnusedTTR function verifies that the TTR is not used (returns true if it's used). +// this is done by: +// - checking the NSTemplateTier.status.revisions field, if the TTR is referenced there or not +// - checking if all Spaces are up-to-date. In case there are some outdated space, we could risk that the TTR is still being used func (r *Reconciler) verifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, rev *toolchainv1alpha1.TierTemplateRevision) (bool, error) { @@ -105,7 +101,7 @@ func (r *Reconciler) verifyUnusedTTR(ctx context.Context, nsTmplTier *toolchainv //check if the ttr name is present status.revisions for _, ttStatusRev := range nsTmplTier.Status.Revisions { if ttStatusRev == rev.Name { - logger.Info("the revision is still being referenced in status.revisions", "ttr.name", rev.Name) + logger.Info("the TierTemplateRevision is still being referenced in NSTemplateTier.Status.Revisions") return false, nil } } @@ -140,5 +136,6 @@ func (r *Reconciler) deleteTTR(ctx context.Context, ttr *toolchainv1alpha1.TierT } return fmt.Errorf("unable to delete the current Tier Template Revision %s: %w", ttr.Name, err) } + log.FromContext(ctx).Info("The TierTemplateRevision has been deleted") return nil } From c53009d321baa932b7e920afe71f11fd385543c4 Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Thu, 13 Mar 2025 22:06:17 +0530 Subject: [PATCH 18/21] changes to test cases Signed-off-by: Feny Mehta --- ...tetier_revision_cleanup_controller_test.go | 135 ++++++++---------- 1 file changed, 59 insertions(+), 76 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 519956b7e..fcf1ab299 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -3,7 +3,6 @@ package nstemplatetierrevisioncleanup_test import ( "context" "fmt" - "os" "testing" "time" @@ -25,17 +24,13 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -const ( - operatorNamespace = "toolchain-host-operator" -) - func TestTTRDeletionReconcile(t *testing.T) { - + oldCreationTime := metav1.NewTime(time.Now().Add(-time.Minute)) + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + s := newSpace(nsTemplateTier) t.Run("TTR Deleted Successfully", func(t *testing.T) { //given - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) - s := createSpace(nsTemplateTier) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) //when res, err := r.Reconcile(context.TODO(), req) @@ -45,62 +40,62 @@ func TestTTRDeletionReconcile(t *testing.T) { tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).DoNotExist() }) - t.Run("Failure", func(t *testing.T) { - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) - s := createSpace(nsTemplateTier) - t.Run("the creation timestamp is less than 30 sec", func(t *testing.T) { - // given - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttr"), metav1.NewTime(time.Now().Add(-29*time.Second))) - r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + t.Run("the creation timestamp is less than 30 sec", func(t *testing.T) { + // given + ttr := newTTR(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttr"), metav1.NewTime(time.Now().Add(-29*time.Second))) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) - // when - res, err := r.Reconcile(context.TODO(), req) + // when + res, err := r.Reconcile(context.TODO(), req) - // then - require.NoError(t, err) - require.LessOrEqual(t, res.RequeueAfter, time.Second) - tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + // then + require.NoError(t, err) + require.LessOrEqual(t, res.RequeueAfter, time.Second) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) - }) - t.Run("ttr is still being referenced in status.revisions", func(t *testing.T) { - // given - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttr"), metav1.NewTime(time.Now().Add(-time.Minute))) - r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + }) + t.Run("ttr is still being referenced in status.revisions", func(t *testing.T) { + // given + ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttr", oldCreationTime) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) - // when - res, err := r.Reconcile(context.TODO(), req) + // when + res, err := r.Reconcile(context.TODO(), req) - // then - require.NoError(t, err) - require.Equal(t, controllerruntime.Result{}, res) - tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + // then + require.NoError(t, err) + require.Equal(t, controllerruntime.Result{}, res) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) - }) + }) - t.Run("spaces are still being updated", func(t *testing.T) { - // given - nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) - nsTemplateTier.Status.Revisions = map[string]string{ - "base1ns-code-123456new": "base1ns-code-123456new-ttr", - "base1ns-clusterresources-123456new": "base1ns-clusterresources-123456new-ttr", - } - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) - r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + t.Run("spaces are still being updated", func(t *testing.T) { + // given + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + nsTemplateTier.Status.Revisions = map[string]string{ + "base1ns-code-123456new": "base1ns-code-123456new-ttr", + "base1ns-clusterresources-123456new": "base1ns-clusterresources-123456new-ttr", + } + ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) - // when - res, err := r.Reconcile(context.TODO(), req) + // when + res, err := r.Reconcile(context.TODO(), req) - // then - require.NoError(t, err) - require.Equal(t, controllerruntime.Result{}, res) - tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) - }) + // then + require.NoError(t, err) + require.Equal(t, controllerruntime.Result{}, res) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + }) + t.Run("Failure", func(t *testing.T) { + nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) + ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + s := newSpace(nsTemplateTier) t.Run("Error while deleting the TTR", func(t *testing.T) { // given - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) - s := createSpace(nsTemplateTier) + ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + s := newSpace(nsTemplateTier) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { return fmt.Errorf("some error cannot delete") @@ -116,8 +111,8 @@ func TestTTRDeletionReconcile(t *testing.T) { t.Run("Is Not Found Error-already deleted, while deleting the TTR", func(t *testing.T) { // given - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) - s := createSpace(nsTemplateTier) + ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + s := newSpace(nsTemplateTier) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { return errors.NewNotFound(schema.GroupResource{}, ttr.Name) @@ -184,13 +179,8 @@ func TestTTRDeletionReconcile(t *testing.T) { }) t.Run("NSTemplate Tier not found - ttr gets deleted", func(t *testing.T) { - r, req, cl := prepareReconcile(t, ttr.Name, ttr, nsTemplateTier) - cl.MockGet = func(ctx context.Context, key types.NamespacedName, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { - if _, ok := obj.(*toolchainv1alpha1.NSTemplateTier); ok { - return errors.NewNotFound(schema.GroupResource{}, key.Name) - } - return cl.Client.Get(ctx, key, obj, opts...) - } + //given + r, req, cl := prepareReconcile(t, ttr.Name, ttr) //when res, err := r.Reconcile(context.TODO(), req) //then @@ -201,13 +191,7 @@ func TestTTRDeletionReconcile(t *testing.T) { }) t.Run("NSTemplate Tier not found, but error deleting ttr", func(t *testing.T) { - r, req, cl := prepareReconcile(t, ttr.Name, ttr, nsTemplateTier) - cl.MockGet = func(ctx context.Context, key types.NamespacedName, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { - if _, ok := obj.(*toolchainv1alpha1.NSTemplateTier); ok { - return errors.NewNotFound(schema.GroupResource{}, key.Name) - } - return cl.Client.Get(ctx, key, obj, opts...) - } + r, req, cl := prepareReconcile(t, ttr.Name, ttr) cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { return fmt.Errorf("some error cannot delete") } @@ -219,7 +203,7 @@ func TestTTRDeletionReconcile(t *testing.T) { tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) t.Run("tier label not found - ttr gets deleted", func(t *testing.T) { - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) ttr.Labels = map[string]string{} r, req, cl := prepareReconcile(t, ttr.Name, ttr) //when @@ -231,7 +215,7 @@ func TestTTRDeletionReconcile(t *testing.T) { }) t.Run("tier label not found but error deleting ttr", func(t *testing.T) { - ttr := createttr(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr"), metav1.NewTime(time.Now().Add(-time.Minute))) + ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) ttr.Labels = map[string]string{} r, req, cl := prepareReconcile(t, ttr.Name, ttr) cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { @@ -250,7 +234,6 @@ func TestTTRDeletionReconcile(t *testing.T) { } func prepareReconcile(t *testing.T, name string, initObjs ...runtimeclient.Object) (*nstemplatetierrevisioncleanup.Reconciler, reconcile.Request, *test.FakeClient) { - os.Setenv("WATCH_NAMESPACE", test.HostOperatorNs) s := scheme.Scheme err := apis.AddToScheme(s) require.NoError(t, err) @@ -261,17 +244,17 @@ func prepareReconcile(t *testing.T, name string, initObjs ...runtimeclient.Objec return r, reconcile.Request{ NamespacedName: types.NamespacedName{ Name: name, - Namespace: operatorNamespace, + Namespace: test.HostOperatorNs, }, }, cl } -func createttr(nsTTier toolchainv1alpha1.NSTemplateTier, name string, crtime metav1.Time) *toolchainv1alpha1.TierTemplateRevision { +func newTTR(nsTTier toolchainv1alpha1.NSTemplateTier, name string, crtime metav1.Time) *toolchainv1alpha1.TierTemplateRevision { labels := map[string]string{ toolchainv1alpha1.TierLabelKey: nsTTier.Name, } ttr := &toolchainv1alpha1.TierTemplateRevision{ ObjectMeta: metav1.ObjectMeta{ - Namespace: operatorNamespace, + Namespace: test.HostOperatorNs, Name: name, Labels: labels, CreationTimestamp: crtime, @@ -280,7 +263,7 @@ func createttr(nsTTier toolchainv1alpha1.NSTemplateTier, name string, crtime met return ttr } -func createSpace(nsTTier *toolchainv1alpha1.NSTemplateTier) *toolchainv1alpha1.Space { +func newSpace(nsTTier *toolchainv1alpha1.NSTemplateTier) *toolchainv1alpha1.Space { testSpace := spacetest.NewSpace(test.HostOperatorNs, "oddity1", spacetest.WithTierNameAndHashLabelFor(nsTTier)) return testSpace From a48d2d11cdcd19f692f7857ac3588890bfa72daf Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Mon, 17 Mar 2025 13:36:10 +0530 Subject: [PATCH 19/21] review comments Signed-off-by: Feny Mehta --- ...emplatetier_revision_cleanup_controller.go | 15 +++++++----- ...tetier_revision_cleanup_controller_test.go | 24 ++++++++++--------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index 941ced036..1425f7a74 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -18,7 +18,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" ) -const DeletionTimeThreshold = 30 * time.Second +const minTTRAge = 30 * time.Second // SetupWithManager sets up the controller with the Manager. func (r *Reconciler) SetupWithManager(mgr manager.Manager) error { @@ -34,24 +34,27 @@ type Reconciler struct { func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { logger := log.FromContext(ctx) - // fetch the NSTemplateTier tier + // fetch the TTR ttr := &toolchainv1alpha1.TierTemplateRevision{} if err := r.Client.Get(ctx, request.NamespacedName, ttr); err != nil { if errors.IsNotFound(err) { logger.Info("TierTemplateRevision not found") return reconcile.Result{}, nil } + // Error reading the object - requeue the request. return reconcile.Result{}, fmt.Errorf("unable to get the current TierTemplateRevision: %w", err) } - //there is no point in fetching the NStemplateTier, if the TTR is just created + //there is no point in fetching the NStemplateTier, if the TTR is just created, + // as it is a new TTR created due to changes in NSTemplate Tier, + // and the referneces are still being updated to nstemplatetier //get the tier template revision creation time stamp and the duration timeSinceCreation := time.Since(ttr.GetCreationTimestamp().Time) //the ttr age should be greater than 30 seconds - if timeSinceCreation < DeletionTimeThreshold { - requeueAfter := DeletionTimeThreshold - timeSinceCreation + if timeSinceCreation < minTTRAge { + requeueAfter := minTTRAge - timeSinceCreation logger.Info("the TierTemplateRevision is not old enough", "requeue-after", requeueAfter) return reconcile.Result{RequeueAfter: requeueAfter}, nil } @@ -90,7 +93,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, nil } -// verifyUnusedTTR function verifies that the TTR is not used (returns true if it's used). +// verifyUnusedTTR function verifies that the TTR is not used (returns true if it's NOT used). // this is done by: // - checking the NSTemplateTier.status.revisions field, if the TTR is referenced there or not // - checking if all Spaces are up-to-date. In case there are some outdated space, we could risk that the TTR is still being used diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index fcf1ab299..76fbe314f 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -27,7 +27,8 @@ import ( func TestTTRDeletionReconcile(t *testing.T) { oldCreationTime := metav1.NewTime(time.Now().Add(-time.Minute)) nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) - ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + ttrName := nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttrcr" + ttr := newTTR(*nsTemplateTier, ttrName, oldCreationTime) s := newSpace(nsTemplateTier) t.Run("TTR Deleted Successfully", func(t *testing.T) { //given @@ -42,7 +43,7 @@ func TestTTRDeletionReconcile(t *testing.T) { t.Run("the creation timestamp is less than 30 sec", func(t *testing.T) { // given - ttr := newTTR(*nsTemplateTier, (nsTemplateTier.Spec.ClusterResources.TemplateRef + "-ttr"), metav1.NewTime(time.Now().Add(-29*time.Second))) + ttr := newTTR(*nsTemplateTier, ttrName, metav1.NewTime(time.Now().Add(-29*time.Second))) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) // when @@ -76,7 +77,7 @@ func TestTTRDeletionReconcile(t *testing.T) { "base1ns-code-123456new": "base1ns-code-123456new-ttr", "base1ns-clusterresources-123456new": "base1ns-clusterresources-123456new-ttr", } - ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + ttr := newTTR(*nsTemplateTier, ttrName, oldCreationTime) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) // when @@ -89,12 +90,13 @@ func TestTTRDeletionReconcile(t *testing.T) { }) t.Run("Failure", func(t *testing.T) { + deleteErr := "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete" nsTemplateTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithStatusRevisions()) - ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + ttr := newTTR(*nsTemplateTier, ttrName, oldCreationTime) s := newSpace(nsTemplateTier) t.Run("Error while deleting the TTR", func(t *testing.T) { // given - ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + ttr := newTTR(*nsTemplateTier, ttrName, oldCreationTime) s := newSpace(nsTemplateTier) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { @@ -104,14 +106,14 @@ func TestTTRDeletionReconcile(t *testing.T) { res, err := r.Reconcile(context.TODO(), req) // then - require.EqualError(t, err, "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete") + require.EqualError(t, err, deleteErr) require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) t.Run("Is Not Found Error-already deleted, while deleting the TTR", func(t *testing.T) { // given - ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + ttr := newTTR(*nsTemplateTier, ttrName, oldCreationTime) s := newSpace(nsTemplateTier) r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { @@ -198,12 +200,12 @@ func TestTTRDeletionReconcile(t *testing.T) { //when res, err := r.Reconcile(context.TODO(), req) //then - require.EqualError(t, err, "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete") + require.EqualError(t, err, deleteErr) require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) t.Run("tier label not found - ttr gets deleted", func(t *testing.T) { - ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + ttr := newTTR(*nsTemplateTier, ttrName, oldCreationTime) ttr.Labels = map[string]string{} r, req, cl := prepareReconcile(t, ttr.Name, ttr) //when @@ -215,7 +217,7 @@ func TestTTRDeletionReconcile(t *testing.T) { }) t.Run("tier label not found but error deleting ttr", func(t *testing.T) { - ttr := newTTR(*nsTemplateTier, nsTemplateTier.Spec.ClusterResources.TemplateRef+"-ttrcr", oldCreationTime) + ttr := newTTR(*nsTemplateTier, ttrName, oldCreationTime) ttr.Labels = map[string]string{} r, req, cl := prepareReconcile(t, ttr.Name, ttr) cl.MockDelete = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.DeleteOption) error { @@ -224,7 +226,7 @@ func TestTTRDeletionReconcile(t *testing.T) { //when res, err := r.Reconcile(context.TODO(), req) //then - require.EqualError(t, err, "unable to delete the current Tier Template Revision base1ns-clusterresources-123456new-ttrcr: some error cannot delete") + require.EqualError(t, err, deleteErr) require.Equal(t, controllerruntime.Result{}, res) tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) }) From 7e79da5df966db4d29c0079785ada68dd892b4df Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Mon, 17 Mar 2025 14:23:06 +0530 Subject: [PATCH 20/21] golint Signed-off-by: Feny Mehta --- .../nstemplatetier_revision_cleanup_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index 1425f7a74..7a4596c08 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -48,7 +48,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. //there is no point in fetching the NStemplateTier, if the TTR is just created, // as it is a new TTR created due to changes in NSTemplate Tier, - // and the referneces are still being updated to nstemplatetier + // and the references are still being updated to nstemplatetier //get the tier template revision creation time stamp and the duration timeSinceCreation := time.Since(ttr.GetCreationTimestamp().Time) From f9ebadcc92e35b0eac99ddde4bd65941efea014f Mon Sep 17 00:00:00 2001 From: Feny Mehta Date: Wed, 19 Mar 2025 11:57:15 +0530 Subject: [PATCH 21/21] adding deletion stamp check Signed-off-by: Feny Mehta --- ...stemplatetier_revision_cleanup_controller.go | 6 ++++++ ...latetier_revision_cleanup_controller_test.go | 17 +++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go index 7a4596c08..8330f0752 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go @@ -9,6 +9,7 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/controllers/nstemplatetier" + "github.com/redhat-cop/operator-utils/pkg/util" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -46,6 +47,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, fmt.Errorf("unable to get the current TierTemplateRevision: %w", err) } + // if the TTR has deletion time stamp, return, no need to do anything as its already marked for deleteion + if util.IsBeingDeleted(ttr) { + logger.Info("TierTemplateRevision already marked for deletion") + return reconcile.Result{}, nil + } //there is no point in fetching the NStemplateTier, if the TTR is just created, // as it is a new TTR created due to changes in NSTemplate Tier, // and the references are still being updated to nstemplatetier diff --git a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go index 76fbe314f..0dd7ac422 100644 --- a/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go +++ b/controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/codeready-toolchain/api/api/v1alpha1" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/controllers/nstemplatetierrevisioncleanup" "github.com/codeready-toolchain/host-operator/pkg/apis" @@ -21,6 +22,7 @@ import ( "k8s.io/kubectl/pkg/scheme" controllerruntime "sigs.k8s.io/controller-runtime" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -41,6 +43,21 @@ func TestTTRDeletionReconcile(t *testing.T) { tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).DoNotExist() }) + t.Run("TTR has deletion time Stamp", func(t *testing.T) { + //given + ttr.DeletionTimestamp = &metav1.Time{ + Time: time.Now(), + } + controllerutil.AddFinalizer(ttr, v1alpha1.FinalizerName) + r, req, cl := prepareReconcile(t, ttr.Name, ttr, s, nsTemplateTier) + //when + res, err := r.Reconcile(context.TODO(), req) + //then + require.NoError(t, err) + require.Equal(t, controllerruntime.Result{}, res) + tiertemplaterevision.AssertThatTTRs(t, cl, nsTemplateTier.GetNamespace()).ExistFor(nsTemplateTier.Name) + }) + t.Run("the creation timestamp is less than 30 sec", func(t *testing.T) { // given ttr := newTTR(*nsTemplateTier, ttrName, metav1.NewTime(time.Now().Add(-29*time.Second)))