RHOAIENG-2974: Restrict DSCI deletion before DSC #1094

Sara4994 · 2024-07-02T20:05:21Z

Description

This PR fixes a race condition that happens while uninstalling the odh-operator. The issue occurs while uninstalling, at times DSCI instance gets removed first and DSC instance hangs over with error status expecting DSCI instance to be created. Manual deletion also does not remove the hanging DSC instance. This PR address the issue by blocking the deletion of DSCI before DSC through webhooks.

JIRA issue: https://issues.redhat.com/browse/RHOAIENG-2974

How Has This Been Tested?

Issue was reported as a race condition. To reproduce the race condition manually, followed the below steps

Built and deployed the operator with this code changes.
Created DSCI and DSC instances and checked pod logs if they are fine without errors.
Deleted DSCI instance first and checked the status of DSC to be in error status
Manually tried to delete the DSC instance and was able to remove the instance.

Screenshot or short clip

Merge criteria

You have read the contributors guide.
Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

zdtsw

need test case updates in the webhook_suite_test.go

controllers/webhook/webhook.go

zdtsw

two small comments, otherwise i am fine with the change

controllers/webhook/webhook_suite_test.go

zdtsw · 2024-07-04T04:17:09Z

controllers/webhook/webhook.go

@@ -101,12 +103,24 @@ func (w *OpenDataHubWebhook) checkDupCreation(ctx context.Context, req admission
 		fmt.Sprintf("Only one instance of %s object is allowed", req.Kind.Kind))
 }

+func (w *OpenDataHubWebhook) checkDeletion(ctx context.Context, req admission.Request) admission.Response {
+	if req.Kind.Kind != "DSCInitialization" {


if we want to have a check here, we can do a if kind == DSC ,be more clear

Sara4994 · 2024-07-04T11:44:27Z

/retest

zdtsw · 2024-07-04T12:36:23Z

there are some failure in "linter" @Sara4994 i will approve once that is fixed.

Sara4994 · 2024-07-04T14:29:24Z

there are some failure in "linter" @Sara4994 i will approve once that is fixed.

@zdtsw fixed now :)

openshift-ci · 2024-07-04T14:52:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zdtsw

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~~ [zdtsw]

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

bartoszmajsak · 2024-07-07T14:20:10Z

I might be missing something fundamental, but what I instantly thought when seeing this PR is - why not cascade deletion using owner ref and finalizers? DSCI would be an owner of DSC so deleting the former (DSCI) would result in always deleting the latter (DSC). With the approach brought by this PR, we force the user to trigger those two deletions in order manually.

adelton

I have the same question as @bartoszmajsak in #1094 (comment) -- why are we not using owner ref and finalizers?

zdtsw · 2024-07-08T11:55:01Z

I think one reason to do this is: if user mistakenly deleted DSCI (as origin issue described) this cascading would automatically delete DSC as well which in turn all components' resource etc.
the effect of this ^ would similar to uninstall operator.
do we want this? if so, we need someone document this change to the user.
A missing/broken DSCI can somehow mean operator is nonfunctional, but still enabled components can run as-is.

adelton · 2024-07-09T06:45:43Z

Conceptually, what is the reason for DSC and DSCI being separate entities?

If deleting the DSCI CR produces a broken cluster, aren't the two actually linked so tightly together that it makes sense to treat them as very tightly connected? If the user/customer has the Operator nonfunctional but the components (seem to) work, is that a supported configuration?

And yes, I understand that implications of this would need to be figured out / investigated. So the webhook approach might be a reasonable stop gap.

zdtsw · 2024-07-09T07:53:12Z

Conceptually, what is the reason for DSC and DSCI being separate entities?

If deleting the DSCI CR produces a broken cluster, aren't the two actually linked so tightly together that it makes sense to treat them as very tightly connected? If the user/customer has the Operator nonfunctional but the components (seem to) work, is that a supported configuration?

And yes, I understand that implications of this would need to be figured out / investigated. So the webhook approach might be a reasonable stop gap.

The very old idea (i was not involved) is to have one DSCI for one cluster, but we could have multiple DSC:s in one cluster (I can only guess it is prep for the multi-tenant case) . But later on, this had been changed which makes it only supports one DSC and one DSCI in one cluster.
I do not know if any plan to continue with multiple DSC in the (near) future or we continue with 1:1:1 map, but yes, use webhook is only for "DSC deletion not hanging if no DSCI exist" case , till we re-define the relation between DSC and DSCI, e.g using Ownerreference or not.
cc @VaishnaviHire / @etirelli

* RHOAIENG-2974: Restrict DSCI deletion before DSC * Added tests and minor enhancements * Admission allow DSC * lint fix (cherry picked from commit 2439104) Add DELETE operation for webhook in CSV, upstream 60a44c2 ("update: cleanup remove kfdef during uninstallation (opendatahub-io#1100)")

* RHOAIENG-2974: Restrict DSCI deletion before DSC * Added tests and minor enhancements * Admission allow DSC * lint fix (cherry picked from commit 2439104) Add DELETE operation for webhook in CSV, upstream 60a44c2 ("update: cleanup remove kfdef during uninstallation (opendatahub-io#1100)") Amend client -> Client due to already applied 03c1abc ("upgrade: controller-runtime and code change accordingly (opendatahub-io#1189)")

RHOAIENG-2974: Restrict DSCI deletion before DSC

d60781d

openshift-ci bot requested review from adelton and VaishnaviHire July 2, 2024 20:05

zdtsw reviewed Jul 3, 2024

View reviewed changes

controllers/webhook/webhook.go Outdated Show resolved Hide resolved

controllers/webhook/webhook.go Outdated Show resolved Hide resolved

Sara4994 force-pushed the uninstall-racecondition-fix branch from 7016ad6 to b83b9cb Compare July 3, 2024 16:53

Added tests and minor enhancements

c6e8f45

Sara4994 force-pushed the uninstall-racecondition-fix branch from b83b9cb to c6e8f45 Compare July 3, 2024 17:16

zdtsw reviewed Jul 4, 2024

View reviewed changes

Admission allow DSC

3432a5f

lint fix

3850351

zdtsw approved these changes Jul 4, 2024

View reviewed changes

openshift-ci bot assigned zdtsw Jul 4, 2024

openshift-ci bot added the lgtm label Jul 4, 2024

openshift-ci bot added the approved label Jul 4, 2024

Sara4994 mentioned this pull request Jul 4, 2024

RHOAIENG-2974: Fix race condition while uninstalling operator #1030

Closed

3 tasks

openshift-merge-bot bot merged commit 2439104 into opendatahub-io:incubation Jul 4, 2024
8 checks passed

adelton reviewed Jul 8, 2024

View reviewed changes

ykaliuta mentioned this pull request Aug 28, 2024

WIP: [RHOAIENG-4485] Fix feature tracker errors while uninstalling operator #1192

Draft

5 tasks

zdtsw mentioned this pull request Sep 13, 2024

DSC, DSCI: add validating webhook (#711) red-hat-data-services/rhods-operator#322

Merged

5 tasks

ykaliuta mentioned this pull request Sep 17, 2024

Webhook backport red-hat-data-services/rhods-operator#357

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RHOAIENG-2974: Restrict DSCI deletion before DSC #1094

RHOAIENG-2974: Restrict DSCI deletion before DSC #1094

Sara4994 commented Jul 2, 2024 •

edited

Loading

zdtsw left a comment

zdtsw left a comment

zdtsw Jul 4, 2024

Sara4994 Jul 4, 2024

Sara4994 commented Jul 4, 2024

zdtsw commented Jul 4, 2024

Sara4994 commented Jul 4, 2024

openshift-ci bot commented Jul 4, 2024

bartoszmajsak commented Jul 7, 2024 •

edited

Loading

adelton left a comment

zdtsw commented Jul 8, 2024

adelton commented Jul 9, 2024 •

edited

Loading

zdtsw commented Jul 9, 2024

RHOAIENG-2974: Restrict DSCI deletion before DSC #1094

RHOAIENG-2974: Restrict DSCI deletion before DSC #1094

Conversation

Sara4994 commented Jul 2, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

zdtsw left a comment

Choose a reason for hiding this comment

zdtsw left a comment

Choose a reason for hiding this comment

zdtsw Jul 4, 2024

Choose a reason for hiding this comment

Sara4994 Jul 4, 2024

Choose a reason for hiding this comment

Sara4994 commented Jul 4, 2024

zdtsw commented Jul 4, 2024

Sara4994 commented Jul 4, 2024

openshift-ci bot commented Jul 4, 2024

bartoszmajsak commented Jul 7, 2024 • edited Loading

adelton left a comment

Choose a reason for hiding this comment

zdtsw commented Jul 8, 2024

adelton commented Jul 9, 2024 • edited Loading

zdtsw commented Jul 9, 2024

Sara4994 commented Jul 2, 2024 •

edited

Loading

bartoszmajsak commented Jul 7, 2024 •

edited

Loading

adelton commented Jul 9, 2024 •

edited

Loading