update: removal finalizer on DSCI #1477
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
zdtsw
requested review from
lburgazzoli and
VaishnaviHire
and removed request for
etirelli and
CFSNM
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1477 +/- ##
==========================================
- Coverage 20.18% 20.05% -0.13%
==========================================
Files 162 162
Lines 11147 11133 -14
==========================================
- Hits 2250 2233 -17
- Misses 8665 8673 +8
+ Partials 232 227 -5 ☔ View full report in Codecov by Sentry. |
|
We would probably need to add an e2e test for this |
@zdtsw are you going to add e2e tests in this PR ? |
i will, once i get all other stuff done first. |
ok, can we mark such PRs as a draft/WIP ? |
zdtsw
force-pushed
the
chore_146
branch
2 times, most recently
from
02dfda5 to
502e5b1
Compare
| @@ -105,38 +104,23 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re | |||
| } | |||
| } | |||
|
|
|||
| if instance.ObjectMeta.DeletionTimestamp.IsZero() { | |||
There was a problem hiding this comment.
if I got this right, we are not adding any finalizer, however feature handler have some explicit delete action https://github.com/opendatahub-io/opendatahub-operator/blob/main/controllers/dscinitialization/servicemesh_setup.go#L209-L214, so by removing the finalizer, I think we may enter in a conditions where those explicit delete actions are not executed successfully
There was a problem hiding this comment.
took some time to refresh my memory,
so the OnDelete() on this case is used for:
servicemesh is Unmanaged, when user delete DSCI, we unset externalprovider in SMCP but still keep SMCP in the cluster.
but in reality, this will not happen, because
- we do not have a management flag on authorino itself. it is created as part of enabled servicemesh or not created at all.
so if user has SMCP before ODH, then they set servicemesh to Unmanaged, we will not even patch SMCP with authorino or create authorino CR.
There was a problem hiding this comment.
so that logic can be removed ?
There was a problem hiding this comment.
i believe so.
it was put there for future but the future did not come
There was a problem hiding this comment.
ok, so maybe we have to remove it, and then remove the finalizer ?
There was a problem hiding this comment.
#1621 track changes here for removal
There was a problem hiding this comment.
the removal PR has been merged.
- do not add finalizer on DSCI during instance creation - only perform removal finalizer on DSCI if it has finalizer already applied (this is more for an upgrade case: if DSCI was created when we forced to add finalizer) Signed-off-by: Wen Zhou <[email protected]>
Signed-off-by: Wen Zhou <[email protected]>
Description
How Has This Been Tested?
local build quay.io/wenzhou/opendatahub-operator-catalog:v2.17.20250106
Screenshot or short clip
Merge criteria