KUBESAW-264: Implement TierTemplateRevision cleanup controller #1153

fbm3307 · 2025-03-07T10:14:49Z

This PR is to create a cleanup controller for TierTemplateRevision CRs so that it deletes all the TierTemplateRevisions CRs that are not used by any Space

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

rsoaresd

Great work! 🚀

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go

mfrancisc

Overall looks good, I still need to take a better look at the unit tests 👍
Nice job 🚀

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

mfrancisc

Great job ! Mostly LGTM.

I have only few last minor comments.

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

mfrancisc

Great job 🚀

Thanks for addressing my comments and for the additional changes 🙏

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

MatousJobanek

Looks good

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go

MatousJobanek · 2025-03-14T10:08:50Z

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go

+		if errors.IsNotFound(err) {
+			// if there is no related nstemplate tier, we can delete the TTR directly
+			return reconcile.Result{}, r.deleteTTR(ctx, ttr)
+		}


Btw, if you added a watcher on NSTemplateTier CRD with a predicate that would allow deletion events only, then you could remove the TTR-specific cleanup logic from the e2e tests.
Can be done as separate PR

You meant the watcher in this controller itselt?

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go

metlos · 2025-03-14T11:59:44Z

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go

+		// Error reading the object - requeue the request.
+		return reconcile.Result{}, fmt.Errorf("unable to get the current TierTemplateRevision: %w", err)
+	}
+


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?

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

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

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 ?

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.

@metlos I have updated it now PTAL.

@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.

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

openshift-ci · 2025-03-18T11:43:09Z

@fbm3307: No presubmit jobs available for codeready-toolchain/host-operator@master

In response to this:

/test e2e, looks like a flaky test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

fbm3307 · 2025-03-18T11:43:35Z

/test e2e

looks unrelated flaky test

openshift-ci · 2025-03-18T11:43:38Z

@fbm3307: No presubmit jobs available for codeready-toolchain/host-operator@master

In response to this:

/test e2e

looks unrelated flaky test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

fbm3307 · 2025-03-18T11:44:15Z

/test e2e

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

sonarqubecloud · 2025-03-19T06:28:07Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
6.0% Duplication on New Code

See analysis details on SonarQube Cloud

openshift-ci · 2025-03-20T09:30:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fbm3307, MatousJobanek, metlos, mfrancisc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [MatousJobanek,fbm3307,metlos,mfrancisc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2025-03-20T09:35:59Z

Codecov Report

Attention: Patch coverage is 84.09091% with 14 lines in your changes missing coverage. Please review.

Project coverage is 79.73%. Comparing base (13f3520) to head (f9ebadc).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...anup/nstemplatetier_revision_cleanup_controller.go	90.24%	7 Missing and 1 partial ⚠️
main.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1153      +/-   ##
==========================================
+ Coverage   79.68%   79.73%   +0.04%     
==========================================
  Files          82       83       +1     
  Lines        8245     8333      +88     
==========================================
+ Hits         6570     6644      +74     
- Misses       1479     1492      +13     
- Partials      196      197       +1

Files with missing lines	Coverage Δ
main.go	`0.00% <0.00%> (ø)`
...anup/nstemplatetier_revision_cleanup_controller.go	`90.24% <90.24%> (ø)`

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

fbm3307 added 2 commits February 26, 2025 14:57

KUBESAW-144: Implement TierTemplateRevision cleanup controller

83de6a7

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

KUBESAW-264: Implement TierTemplateRevision cleanup controller

b5534de

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

fbm3307 requested review from MatousJobanek, xcoulon, alexeykazakov, rajivnathan, ranakan19, mfrancisc, rsoaresd, metlos and jrosental as code owners March 7, 2025 10:14

openshift-ci bot added the approved label Mar 7, 2025

fbm3307 added 6 commits March 7, 2025 15:44

Merge branch 'master' into ks144ttr_cleanup_controller

f44718b

golint

7612388

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

some cleanup

01dbe62

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

golint spelling

726947e

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

adding it to main

ffc4f83

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

Merge branch 'master' into ks144ttr_cleanup_controller

20bae4e

MatousJobanek reviewed Mar 10, 2025

View reviewed changes

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go Outdated Show resolved Hide resolved

review comments

81d0768

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

rsoaresd reviewed Mar 10, 2025

View reviewed changes

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller.go Show resolved Hide resolved

mfrancisc reviewed Mar 10, 2025

View reviewed changes

fbm3307 added 2 commits March 11, 2025 12:16

review comments part 2

87707b0

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

golanglint ci

e898e6c

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

fbm3307 requested review from MatousJobanek, mfrancisc and rsoaresd March 11, 2025 07:21

nstemplatetier not found rc

7307ee7

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

mfrancisc reviewed Mar 13, 2025

View reviewed changes

adding test case and rc

879809a

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

fbm3307 requested a review from mfrancisc March 13, 2025 13:04

mfrancisc approved these changes Mar 13, 2025

View reviewed changes

controllers/nstemplatetierrevisioncleanup/nstemplatetier_revision_cleanup_controller_test.go Outdated Show resolved Hide resolved

removing extra

8d33377

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

MatousJobanek reviewed Mar 13, 2025

View reviewed changes

loggers and comments

588ef62

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

fbm3307 requested a review from MatousJobanek March 13, 2025 16:04

MatousJobanek reviewed Mar 13, 2025

View reviewed changes

changes to test cases

c53009d

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

fbm3307 requested a review from MatousJobanek March 13, 2025 16:40

mfrancisc and others added 2 commits March 13, 2025 19:08

Merge branch 'master' into ks144ttr_cleanup_controller

cfc17cf

Merge branch 'master' into ks144ttr_cleanup_controller

1268cbd

MatousJobanek approved these changes Mar 14, 2025

View reviewed changes

metlos reviewed Mar 14, 2025

View reviewed changes

metlos and others added 3 commits March 14, 2025 13:34

Merge branch 'master' into ks144ttr_cleanup_controller

0eaa6cf

review comments

a48d2d1

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

golint

7e79da5

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

fbm3307 requested a review from metlos March 18, 2025 05:38

fbm3307 added 2 commits March 19, 2025 10:53

Merge branch 'master' into ks144ttr_cleanup_controller

433f758

adding deletion stamp check

f9ebadc

Signed-off-by: Feny Mehta <fbm3307@gmail.com>

metlos approved these changes Mar 20, 2025

View reviewed changes

fbm3307 merged commit 825a78b into codeready-toolchain:master Mar 20, 2025
12 of 14 checks passed

KUBESAW-264: Implement TierTemplateRevision cleanup controller #1153

KUBESAW-264: Implement TierTemplateRevision cleanup controller #1153

Uh oh!

Conversation

fbm3307 commented Mar 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rsoaresd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mfrancisc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mfrancisc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mfrancisc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MatousJobanek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!