remove the bundled NSTemplateTiers that are no longer used #1180
Conversation
metlos
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
mfrancisc,
rsoaresd,
fbm3307 and
jrosental
as code owners
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: metlos 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:
Approvers can indicate their approval by writing |
metlos
force-pushed
the
remove-unused-bundled-tiers
branch
from
c3ffb22 to
a2f74ef
Compare
metlos
force-pushed
the
remove-unused-bundled-tiers
branch
from
b9e078d to
05bb8c9
Compare
|
/retest |
…s a finalizer to control the deletion
metlos
force-pushed
the
remove-unused-bundled-tiers
branch
from
0cb9c19 to
cd13c5d
Compare
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1180 +/- ##
==========================================
- Coverage 83.40% 83.22% -0.18%
==========================================
Files 82 82
Lines 7940 8007 +67
==========================================
+ Hits 6622 6664 +42
- Misses 1112 1131 +19
- Partials 206 212 +6
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Let's clarify the goals because the PR includes two things that are indepentent:
Finalizers:
- each NSTemplateTier should have finalizer
- the finalizer should block the removal of the NSTemplateTier until there is at least one Space that uses the tier (no matter if it's bundled or not)
- each NSTemplateTier is deletable, which means that anyone can call "delete" on the CR (no matter if it's bundled or not). The tier should be removed only when it's not used (see the previous point).
Deletion of obsolete tiers
- we want to delete tiers that are not bundle anymore, this means that the generator
- adds annotation to each bundled tier
- calls delete on NSTemplateTiers that have the annotation but are not bundled
| bundled, err := nstemplatetiers.IsBundled(runtimeclient.ObjectKeyFromObject(tier)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to find if a tier is bundled or not (this shouldn't happen): %w", err) | ||
| } | ||
|
|
||
| if bundled { | ||
| // let's just keep the bundled tier around even if someone wants to delete it. It'd be recreated at | ||
| // the next start of the operator anyway. | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I don't think that we should really check if it's bundled or not as part of the finalizer. If you would block the deletion, then the CR would end up in the terminating state forever.
If someone want's to delete bundled tier, then just delete it if it's not used by a space. As you mentioned, then next operator start will recreated it anyway.
|
|
||
| func isTierUsed(ctx context.Context, client runtimeclient.Client, tier *toolchainv1alpha1.NSTemplateTier) (bool, error) { | ||
| list := &toolchainv1alpha1.SpaceList{} | ||
| if err := client.List(ctx, list, runtimeclient.InNamespace(tier.Namespace), runtimeclient.HasLabels{hash.TemplateTierHashLabelKey(tier.Name)}); err != nil { |
There was a problem hiding this comment.
only an optimization tip (in this particular case it doesn't matter much) - you can use runtimeclient.Limit(1) as a list option to get only one result from the list. The idea is that you don't need to get all Spaces with the label, but only one
| // BundledTierKeys is the list of NSTemplateTiers that are bundled with the operator binary. | ||
| // This list is initialized during the SyncResources call. DO NOT MODIFY it unless you're in | ||
| // tests and need to setup a custom list of bundled tiers. | ||
| var BundledTierKeys []runtimeclient.ObjectKey |
There was a problem hiding this comment.
related to my comment above, I don't think that we need this global variable
| if util.IsBeingDeleted(tier) { | ||
| return reconcile.Result{}, nil | ||
| return reconcile.Result{}, r.handleDeletion(ctx, tier) | ||
| } |
There was a problem hiding this comment.
the code is still missing a logic that would add a finalizer
|
|
||
| var allErrors []error | ||
| for _, tier := range allTiers.Items { | ||
| if controllerutil.ContainsFinalizer(&tier, constants.BundledNSTemplateTierFinalizerName) && |
There was a problem hiding this comment.
why do you check the finalizer here?
| // initialize tier generator, loads templates from assets | ||
| return nstemplatetiers.GenerateTiers(s, func(toEnsure runtimeclient.Object, canUpdate bool, _ string) (bool, error) { | ||
| err = nstemplatetiers.GenerateTiers(s, func(toEnsure runtimeclient.Object, canUpdate bool, _ string) (bool, error) { | ||
| controllerutil.AddFinalizer(toEnsure, constants.BundledNSTemplateTierFinalizerName) |
There was a problem hiding this comment.
ok, now I found the line with the finalizer - let's move it to the controller to keep it consistent with all other controllers & CRDs.
Apart from that, it's a responsibility of the controller to add/remove the finalizer, not of the client that creates/deletes the CRs
There was a problem hiding this comment.
I maybe now understand why you added the finalizer here - to add it only to the bundled tiers. Well, it's not really needed to differentiate between bundled and non-bundled tiers. All the tiers should contain the finalizer and the tier that is being deleted can be removed only if it's not used by any space.
This compiles the list of the
NSTemplateTiers that are bundled in the binary and makes sure that anyNSTemplateTierthat is annotated as bundled and is NOT in that list, is automatically deleted if no longer used by any space.This will provide a nice cleanup mechanism of the bundled
NSTemplateTierthat we no longer use.It does not affect any user-defined
NSTemplateTiers.Related PRs: