Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: uninstallation for operator #796

Merged
merged 9 commits into from
Feb 8, 2024
Merged

Fix: uninstallation for operator #796

merged 9 commits into from
Feb 8, 2024

Conversation

zdtsw
Copy link
Member

@zdtsw zdtsw commented Jan 11, 2024 •
edited

  • first to remove finalizer from DSC CR in case it prevents DSC CR to be deleted
  • add function to cleanup subscription before deletion CSV otherwise hanging subs will prevent cluster being install again with same version
  • removed reconcile on DSCI for Prometheus when DSC CR is removed
  • FT CR should be cleaned up
  • DS projects should be still in the cluster

ref:

Description

How Has This Been Tested?

live build: quay.io/wenzhou/opendatahub-operator-catalog:v2.6.4-499

test I:

  1. install operator
  2. create DSCI CR
  3. create DSC CR
  4. create CM + add label to the CM
  5. check pod logs: see
  6. check operator is gone
  7. install same operator again

test II:

  1. install operator
  2. create DSCI CR
  3. create CM + add label to the CM
  4. check pod logs: see
  5. check operator is gone
  6. install same operator again

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • 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

@bartoszmajsak
Copy link
Contributor

How Has This Been Tested?

this list of steps looks to me like they could have been automated in a test?

@zdtsw
Copy link
Member Author

zdtsw commented Jan 12, 2024

How Has This Been Tested?

this list of steps looks to me like they could have been automated in a test?

unless i am wrong, we have cfgMapDeletionTestSuite() which does this test

@zdtsw zdtsw force-pushed the issue/499 branch 2 times, most recently from 546a8d8 to 97ce1ce Compare February 6, 2024 12:28
@zdtsw zdtsw changed the title Fix: add back logic to deal with uninstallation Fix: uninstallation for operator Feb 6, 2024
@zdtsw zdtsw force-pushed the issue/499 branch 2 times, most recently from b8e73a0 to c048880 Compare February 7, 2024 16:13
@zdtsw
Fix: add back logic to deal with uninstallation
65dd887 
- when CM exist and DSC CR exists

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw
update: cleanup finalzlier
9f0e676
@zdtsw
fix
38b37f4 
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw
update: uninstallation process
e09876d 
- add logic to remove subscription
- do not call backup in DSCI during uninstallation
- add permisison of delete subscripton
- change function SubscriptionExists() return sub

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw
update: add deletion for DS project as well during uninstallation
5cb0bfb 
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw
update: add calls of component.Cleanup() esp for kserve for now for
0fd5b66 
uninstall

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw
revert: deletion ds project during uninstall
b5444d0 
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw
fix: extend timeout in unit-test till we update testcase
68b32dc 
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw
cleanup: remove commented block
502bbdd 
Signed-off-by: Wen Zhou <wenzhou@redhat.com>
VaishnaviHire
VaishnaviHire approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@VaishnaviHire VaishnaviHire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 8, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Feb 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VaishnaviHire

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Feb 8, 2024
@VaishnaviHire VaishnaviHire merged commit 9227687 into opendatahub-io:incubation Feb 8, 2024
6 of 7 checks passed
bartoszmajsak
bartoszmajsak reviewed Feb 9, 2024
View reviewed changes
controllers/datasciencecluster/datasciencecluster_controller.go
// If DSC CR exist and deletion CM exist
// delete DSC CR and let reconcile requeue
// sometimes with finalzier DSC CR wont get deleted, force to remove finalizer here
if upgrade.HasDeleteConfigMap(r.Client) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this deserves its own func with a descriptive name which would self-explain the step which is going to be taken?

The reconciliation func body is becoming lengthy and it's getting a bit challenging to quickly skim trough the code to get understanding of the overall flow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder however, wouldn't it be better to have a controller watching for the existence of this exact config map instead of checking for every DSCI reconcile? If exists -> delete DSCI which will trigger DSCI reconcile reacting on delete event.

controllers/dscinitialization/suite_test.go
@@ -63,7 +63,7 @@ var (
)

const (
timeout = 20 * time.Second
timeout = 30 * time.Second // change this from original 20 to 30 because we often failed in post cleanup job
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe move this to the commit message instead?

pkg/deploy/deploy.go Show resolved Hide resolved
pkg/upgrade/upgrade.go Show resolved Hide resolved
tests/e2e/dsc_cfmap_deletion_test.go Show resolved Hide resolved
zdtsw added a commit to zdtsw-forking/rhods-operator that referenced this pull request Feb 9, 2024
@zdtsw
Fix: uninstallation for operator (opendatahub-io#796)
a5c4b8b 
* Fix: add back logic to deal with uninstallation

- when CM exist and DSC CR exists

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: cleanup finalzlier

* fix

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: uninstallation process

- add logic to remove subscription
- do not call backup in DSCI during uninstallation
- add permisison of delete subscripton
- change function SubscriptionExists() return sub

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: add deletion for DS project as well during uninstallation

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: add calls of component.Cleanup() esp for kserve for now for
uninstall

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* revert: deletion ds project during uninstall

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* fix: extend timeout in unit-test till we update testcase

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* cleanup: remove commented block

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@zdtsw zdtsw mentioned this pull request Feb 9, 2024
Merged
3 tasks
VaishnaviHire pushed a commit to VaishnaviHire/opendatahub-operator that referenced this pull request Feb 22, 2024
@zdtsw @VaishnaviHire
Fix: uninstallation for operator (opendatahub-io#796)
6c4a7fc 
* Fix: add back logic to deal with uninstallation

- when CM exist and DSC CR exists

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: cleanup finalzlier

* fix

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: uninstallation process

- add logic to remove subscription
- do not call backup in DSCI during uninstallation
- add permisison of delete subscripton
- change function SubscriptionExists() return sub

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: add deletion for DS project as well during uninstallation

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: add calls of component.Cleanup() esp for kserve for now for
uninstall

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* revert: deletion ds project during uninstall

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* fix: extend timeout in unit-test till we update testcase

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* cleanup: remove commented block

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
(cherry picked from commit 9227687)
@zdtsw zdtsw mentioned this pull request Apr 30, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartoszmajsak bartoszmajsak bartoszmajsak left review comments

@VaishnaviHire VaishnaviHire VaishnaviHire approved these changes

@etirelli etirelli Awaiting requested review from etirelli

Assignees

@VaishnaviHire VaishnaviHire

Labels
approved lgtm rhoai-2.7
Projects
ODH Platform Planning
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zdtsw @bartoszmajsak @VaishnaviHire