add finalizer directly to the GPUCluster manifest#2673
Conversation
93d90be to
758abd2
Compare
|
coverage: 33.481%. remained the same — add-finalizer-to-manifest into main |
| if !instance.DeletionTimestamp.IsZero() { | ||
| return r.reconcileDelete(ctx, instance) | ||
| } | ||
| if !controllerutil.ContainsFinalizer(instance, gpuClusterFinalizer) { |
There was a problem hiding this comment.
If a user doesn't specify finalizer by default, we should add it. So we should keep this logic as it is and just add finalizer by default in our templates.
There was a problem hiding this comment.
Yeah, that's fair. I was thinking about this just after pushing the commit
There was a problem hiding this comment.
Addressed your comment. Please check now
Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com>
758abd2 to
dc6ebca
Compare
| "name": "gpu-cluster" | ||
| "name": "gpu-cluster", | ||
| "finalizers": [ | ||
| "gpucluster.nvidia.com/dra-resourceclaim" |
There was a problem hiding this comment.
This is somewhat of a behavior change. We're pre-adding the finalizer instead of adding it after the operator's first reconcile. If the operator never comes up during helm install, you cannot delete this CR without manually removing the finalizer. I'd rather have one way to add and remove the finalizer. Not a strong opinion, but I'm not entirely sure this change is worth it.
This also means we need to maintain this finalizer in multiple places instead of a single constant in the controller.
|
I agree with @rajathagasthya here. If we are testing things with DRA and operator fails to come up, cleaning of gpucluster resource would require manually removing the finalizer, which is not great. |
|
Thanks for the reviews. I agree with the comments. Closing... |
4 participants
This PR adds the finaliser inline to the GPUCluster resource manifests. This is done for readability reasons and it also shaves off an kube client UPDATE call