Skip to content

KUBESAW-264: Implement TierTemplateRevision cleanup controller #1153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
83de6a7
KUBESAW-144: Implement TierTemplateRevision cleanup controller
fbm3307 Jan 29, 2025
b5534de
KUBESAW-264: Implement TierTemplateRevision cleanup controller
fbm3307 Mar 7, 2025
f44718b
Merge branch 'master' into ks144ttr_cleanup_controller
fbm3307 Mar 7, 2025
7612388
golint
fbm3307 Mar 7, 2025
01dbe62
some cleanup
fbm3307 Mar 7, 2025
726947e
golint spelling
fbm3307 Mar 7, 2025
ffc4f83
adding it to main
fbm3307 Mar 10, 2025
20bae4e
Merge branch 'master' into ks144ttr_cleanup_controller
fbm3307 Mar 10, 2025
81d0768
review comments
fbm3307 Mar 10, 2025
87707b0
review comments part 2
fbm3307 Mar 11, 2025
e898e6c
golanglint ci
fbm3307 Mar 11, 2025
7307ee7
nstemplatetier not found rc
fbm3307 Mar 11, 2025
1be1970
requeu bool true
fbm3307 Mar 11, 2025
467980e
deleteing ttr if tier-name not there
fbm3307 Mar 11, 2025
6059ce5
coverage
fbm3307 Mar 11, 2025
ab49a42
reque bool
fbm3307 Mar 12, 2025
879809a
adding test case and rc
fbm3307 Mar 13, 2025
8d33377
removing extra
fbm3307 Mar 13, 2025
588ef62
loggers and comments
fbm3307 Mar 13, 2025
c53009d
changes to test cases
fbm3307 Mar 13, 2025
cfc17cf
Merge branch 'master' into ks144ttr_cleanup_controller
mfrancisc Mar 13, 2025
1268cbd
Merge branch 'master' into ks144ttr_cleanup_controller
MatousJobanek Mar 14, 2025
0eaa6cf
Merge branch 'master' into ks144ttr_cleanup_controller
metlos Mar 14, 2025
a48d2d1
review comments
fbm3307 Mar 17, 2025
7e79da5
golint
fbm3307 Mar 17, 2025
433f758
Merge branch 'master' into ks144ttr_cleanup_controller
fbm3307 Mar 19, 2025
f9ebadc
adding deletion stamp check
fbm3307 Mar 19, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
package nstemplatetierrevisioncleanup

import (
"context"
"fmt"
"time"

"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/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"
)

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.TierTemplateRevision{}).
Complete(r)
}

type Reconciler struct {
Client runtimeclient.Client
}

func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) {
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.
return reconcile.Result{}, fmt.Errorf("unable to get the current TierTemplateRevision: %w", err)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a check for a non-nil ttr.DeletionTimeStamp - am I right in assuming we don't want to continue the reconciliation if we detect the TTR is already in the process of being deleted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, but there is no finalizer which means that it should be deleted immediately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and if we want to check if it's not being deleted, then we should use util.IsBeingDeleted(ttr) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay just curious, if we don't have finalizers, then any particular reason on why we should still check isBeingDeleted, there won't be any delay as per my understanding, may be i missed some case ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, if you detect the resource is being deleted, you just stop the reconcile by returning early without an error. If there are finalizers, you first do the clean up logic, remove the finalizer and then return, again not performing the "normal" reconciliation logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metlos I have updated it now PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metlos the point was that if there is no finalizer added, then as soon as the resource has the deletion timestamp set, then it should be immediately deleted.

//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}, 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
}
// fetch the related NSTemplateTier tier
tier := &toolchainv1alpha1.NSTemplateTier{}
if err := r.Client.Get(ctx, types.NamespacedName{
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
if err := r.deleteTTR(ctx, ttr); err != nil {
return reconcile.Result{}, 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)
}

// 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 {
return reconcile.Result{}, err
}

// Delete the unused revision
if isTTrUnused {
return reconcile.Result{}, r.deleteTTR(ctx, ttr)
}

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.
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", "ttr.name", rev.Name)
return false, nil
}
}

// get the outdated matching label to list outdated spaces
matchOutdated, err := nstemplatetier.OutdatedTierSelector(nsTmplTier)
if err != nil {
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, err
}

//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 there should be no outdated spaces.
if len(spaces.Items) > 0 {
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 {
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
}
Loading
Loading