Add a finalizer to the NSTemplateTier to prevent its deletion

[metlos]

Add a finalizer to the NSTemplateTier to prevent its deletion as long as some spaces are still derived from it.

Related PR:

• toolchain-common: introduce a simple abstraction for easier handling of finalizers toolchain-common#485
• toolchain-e2e: clean up the spaces after the test so that the ns template tier is deletable toolchain-e2e#1161

JIRA: https://issues.redhat.com/browse/SANDBOX-1213

[MatousJobanek]

Thanks 👍

[rsoaresd]

🚀

[rsoaresd]

I saw that the e2e tests are failing here:

=== NAME  TestFeatureToggles/provision_space_with_enabled_feature
    clean.go:188: 
        	Error Trace:	/tmp/toolchain-e2e/testsupport/cleanup/clean.go:188
        	            				/usr/local/go/src/sync/once.go:74
        	            				/usr/local/go/src/sync/once.go:65
        	            				/tmp/toolchain-e2e/testsupport/cleanup/clean.go:100
        	            				/tmp/toolchain-e2e/testsupport/cleanup/clean.go:77
        	            				/usr/local/go/src/runtime/asm_amd64.s:1695
        	Error:      	Received unexpected error:
        	            	context deadline exceeded
        	Test:       	TestFeatureToggles/provision_space_with_enabled_feature
        	Messages:   	The proper cleanup of the NSTemplateTier 'ftier' and related resources wasn't finished within the given timeout
        	            	the NSTemplateTier 'ftier' is still present in the cluster: &{TypeMeta:{Kind: APIVersion:} ObjectMeta:{Name:ftier GenerateName: Namespace:

[MatousJobanek]

OK, this means that we cannot leave the tier to wait for the next scheduled reconcile if it's pending deletion (in X hours).
Let's return an error from the controller when it's blocked for the deletion. I guess that it's fine. This shouldn't happen often in production (so it won't trigger too many additional reconciles) and it will solve the problem in e2e tests.

[metlos]

OK, this means that we cannot leave the tier to wait for the next scheduled reconcile if it's pending deletion (in X hours).
Let's return an error from the controller when it's blocked for the deletion. I guess that it's fine. This shouldn't happen often in production (so it won't trigger too many additional reconciles) and it will solve the problem in e2e tests.

Btw, this is the exact logic that the finalizer handling abstraction from the controller runtime would have forced us to use.

[MatousJobanek]

but in most of the cases (if not all except for this one) we don't return an error if the CR is pending deletion because of some dependent CRs. The reason is that the other controllers watch the CRDs it depends on, this is an exception. So it's rather the logic of the finalizer you would need to implement anyway.

[metlos]

/retest

[MatousJobanek]

I see that the e2e tests are still failing, According to the logs it's a timing issue because the returned error triggers postponed reconcile loops with exponential increase of the time gaps. In other words, returning an error is not enough.
Multiple options from the top of my head:

• in the finalizer logic, ignore Spaces that are already being deleted. In other words, list all Spaces with the label, filter out those that have the deletion timestamp set and return the error only when there is a Space with the label that is not being deleted. This should help and it's completely fine to ignore Spaces that are being deleted.
• modify the cleanup logic in e2e tests so the deletion of NSTemplateTiers is triggered as the last one after all other cleanups are completed (this seems to be pretty complicated)
• modify the cleanup logic so it keeps updating for example an annotation in the NSTemplateTier CR to trigger the reconcile every time when the NSTemplateTier is pending deletion (this sounds much simpler)
• add a watch to the controller with a predicate & mapper so it listens to deletion of Spaces (in my opinion it's an overkill to workaround problem in e2e tests)

Anything else?

[metlos]

/retest

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[fbm3307]

Great Work ,
just a curiosity question .

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [MatousJobanek,fbm3307,metlos,rsoaresd]

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

[codecov]

Codecov Report

Attention: Patch coverage is 64.44444% with 16 lines in your changes missing coverage. Please review.

Project coverage is 71.24%. Comparing base (69afa0e) to head (e5dcda5).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...ollers/nstemplatetier/nstemplatetier_controller.go	70.73%	9 Missing and 3 partials ⚠️
test/nstemplatetier/nstemplatetier.go	0.00%	4 Missing ⚠️

❌ Your changes status has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1182       +/-   ##
===========================================
- Coverage   83.40%   71.24%   -12.16%     
===========================================
  Files          82       82               
  Lines        7940     7984       +44     
===========================================
- Hits         6622     5688      -934     
- Misses       1112     2092      +980     
+ Partials      206      204        -2     

Files with missing lines	Coverage Δ
test/nstemplatetier/nstemplatetier.go	0.00% <0.00%> (-76.55%)	⬇️
...ollers/nstemplatetier/nstemplatetier_controller.go	75.00% <70.73%> (-1.71%)	⬇️

... and 20 files with indirect coverage changes

🚀 New features to boost your workflow:

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