Migration uninstall configmap from DSC to setup controller #1438

biswassri · 2024-12-10T01:15:14Z

Description

Created a new setup-controller and moved the below functionalities to uninstallation.go if HasDeleteConfigMap exists

Deletion of the DataScienceCluster instance
Operator uninstall
Existing workflow

How Has This Been Tested?

Testing process :

install cluster
create dsc and dsci
create delete-configmap with

labels:
    api.openshift.com/addon-managed-odh-delete: 'true'

check dsc and dsci deletion and logs for deletion of other resources

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

ykaliuta · 2024-12-10T06:27:11Z

Could you put description in the commit message?

ykaliuta · 2024-12-10T06:30:29Z

controllers/setupcontroller/setup_controller.go

+	}
+
+	// Fetch the DataScienceCluster instance
+	if len(instances.Items) == 0 {


ykaliuta · 2024-12-10T06:36:27Z

controllers/setupcontroller/setup_controller.go

+	platform := currentOperatorRelease.Name
+	instances := &dscv1.DataScienceClusterList{}
+
+	// Check if the delete ConfigMap exists


Obvious from the function name actually

ykaliuta · 2024-12-10T06:36:40Z

controllers/setupcontroller/setup_controller.go

+		return reconcile.Result{}, nil
+	}
+
+	// Fetch the DataScienceCluster instance


I do not see that

The comment should go above this right ? instances := &dscv1.DataScienceClusterList{}

ykaliuta · 2024-12-10T06:37:32Z

controllers/setupcontroller/setup_controller.go

+
+	// Check if the delete ConfigMap exists
+	if !upgrade.HasDeleteConfigMap(ctx, r.Client) {
+		log.Info("No delete ConfigMap found, skipping reconciliation")


Probably, loglevels can be revisited

Sure, maybe in separate PR or probably after we decide the workflow for this intended task is good.

ykaliuta · 2024-12-10T06:42:23Z

controllers/setupcontroller/setup_controller.go

+			}
+		}
+		// Delete the DataScienceCluster instance
+		if err := r.Client.Delete(ctx, instance, []client.DeleteOption{}...); err != nil {


what is the trick with the options?

Just picked it up from here: https://github.com/opendatahub-io/opendatahub-operator/blob/main/controllers/datasciencecluster/datasciencecluster_controller.go#L127

Just picked it up from here: https://github.com/opendatahub-io/opendatahub-operator/blob/main/controllers/datasciencecluster/datasciencecluster_controller.go#L127

Introduced by 9227687 ("Fix: uninstallation for operator (#796)")
@zdtsw ? :)

biswassri · 2024-12-10T09:07:31Z

@ykaliuta Thanks for the review. Updated the description Let me know if you have any other questions

Additionally,
/cc @lburgazzoli @VaishnaviHire I took a first go at the possible solution of the above mentioned task. Let me know wdyt or we can discuss further in the channel/gmeet (if you think a longer conversation is needed)
I haven't worked on getting past the tests, I'll work on that once we are good with the workflow

controllers/setupcontroller/setup_controller.go

controllers/datasciencecluster/datasciencecluster_controller.go

controllers/setupcontroller/setup_controller.go

Makefile

codecov · 2024-12-16T07:26:01Z

Codecov Report

Attention: Patch coverage is 0% with 58 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@9704854). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
controllers/setupcontroller/setup_controller.go	0.00%	47 Missing ⚠️
pkg/upgrade/uninstallation.go	0.00%	11 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1438   +/-   ##
=======================================
  Coverage        ?   19.10%           
=======================================
  Files           ?      158           
  Lines           ?    10362           
  Branches        ?        0           
=======================================
  Hits            ?     1980           
  Misses          ?     8158           
  Partials        ?      224

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lburgazzoli · 2024-12-16T14:18:45Z

controllers/datasciencecluster/datasciencecluster_controller.go

-		return ctrl.Result{}, nil
-	case err != nil:
-		return ctrl.Result{}, err
+	if err := r.Client.Get(ctx, req.NamespacedName, instance); err != nil {


a check fr "NotFound" error is probably needed, in such case, the reconciliation loop can return early

Yeap makes sense. I'll update this.

controllers/datasciencecluster/datasciencecluster_controller.go

biswassri · 2024-12-16T15:23:40Z

/retest

biswassri · 2024-12-16T18:01:09Z

/retest

lburgazzoli · 2024-12-16T19:29:47Z

controllers/setupcontroller/setup_controller.go

+
+func (r *SetupControllerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	log := logf.FromContext(ctx).WithName("SetupController")
+	log.Info("Reconciling setup controller", "Request.Name", req.Name) // log.V(1).Info(...)


"Request.Name", req.Name is probably redundant as it should be included already in the log passed to the within the context. Can you confirm ?

Yeah you're correct it does get passed from within the context.
This is the log for this bit

{"level":"info","ts":"2024-12-16T14:00:12Z","logger":"SetupContoller","msg":"Reconciling setup controller","controller":"configmap","controllerGroup":"","controllerKind":"ConfigMap","ConfigMap":{"name":"delete-configmap","namespace":"opendatahub-operator-system"},"namespace":"opendatahub-operator-system","name":"delete-configmap","reconcileID":"1c11083d-f086-4198-bfde-ba26a7f7fc38","Request.Name":"delete-configmap"}

This is similar to that in DSC controller too. But maybe it makes sense for the DSC and not for the setup controller? Or should it be removed from there too.

Probably it should be revisited in the DSC controller too, for the moment I would just fix it here

Done 👍 Updated the setup controller only

biswassri · 2024-12-18T15:21:11Z

/retest

biswassri · 2024-12-18T15:29:59Z

/retest

lburgazzoli · 2024-12-18T17:12:56Z

LGTM, @grdryn anything missing ?

grdryn

/lgtm 👍

openshift-ci · 2024-12-18T17:25:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grdryn

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

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

lburgazzoli · 2024-12-18T17:33:50Z

/lgtm

biswassri · 2024-12-18T18:09:03Z

/retest

biswassri · 2024-12-18T19:41:58Z

/retest

biswassri · 2024-12-19T00:19:35Z

/retest

pkg/upgrade/uninstallation.go

controllers/datasciencecluster/kubebuilder_rbac.go

openshift-ci bot added the do-not-merge/work-in-progress label Dec 10, 2024

openshift-ci bot requested review from StevenTobin and ykaliuta December 10, 2024 01:15

biswassri marked this pull request as draft December 10, 2024 01:15

ykaliuta reviewed Dec 10, 2024

View reviewed changes

lburgazzoli reviewed Dec 11, 2024

View reviewed changes

controllers/setupcontroller/setup_controller.go Outdated Show resolved Hide resolved

lburgazzoli reviewed Dec 11, 2024

View reviewed changes

controllers/setupcontroller/setup_controller.go Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase label Dec 12, 2024

biswassri force-pushed the migration-uninstall branch from ec46ccf to 0a5c917 Compare December 12, 2024 22:03

openshift-merge-robot removed the needs-rebase label Dec 12, 2024

biswassri force-pushed the migration-uninstall branch from 0a5c917 to 56f8901 Compare December 13, 2024 00:43

biswassri marked this pull request as ready for review December 13, 2024 06:47

openshift-ci bot requested review from grdryn and mattmahoneyrh December 13, 2024 06:47

biswassri changed the title ~~[WIP] Migration uninstall configmap from DSC to setup controller~~ Migration uninstall configmap from DSC to setup controller Dec 13, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 13, 2024

lburgazzoli reviewed Dec 13, 2024

View reviewed changes

controllers/datasciencecluster/datasciencecluster_controller.go Show resolved Hide resolved

lburgazzoli reviewed Dec 13, 2024

View reviewed changes

controllers/setupcontroller/setup_controller.go Show resolved Hide resolved

biswassri force-pushed the migration-uninstall branch from f48a300 to 58d79a2 Compare December 16, 2024 06:04

biswassri commented Dec 16, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

lburgazzoli reviewed Dec 16, 2024

View reviewed changes

biswassri requested a review from lburgazzoli December 16, 2024 14:19

biswassri commented Dec 16, 2024

View reviewed changes

controllers/datasciencecluster/datasciencecluster_controller.go Show resolved Hide resolved

lburgazzoli reviewed Dec 16, 2024

View reviewed changes

grdryn approved these changes Dec 18, 2024

View reviewed changes

openshift-ci bot assigned grdryn Dec 18, 2024

openshift-ci bot added the lgtm label Dec 18, 2024

openshift-ci bot added the approved label Dec 18, 2024

biswassri added 12 commits December 18, 2024 12:30

Migration uninstall configmap from DSC to setup controller

e776e0a

Updating after rebase

71c1cb7

Fix lint + add comments

9fb3f15

Adding predicate to watch configMap

5c6d14e

Changes to debug

bed238f

Deletion update + rbac change

4b116f3

Adding IsNotFound() check in DSC_controller

8668a62

Changing k8serr.IsNotFound error from if to switch logic

bd85515

Changes from review

52b15cd

Removing HasDeleteConfig from DSC_controller

749c1c2

Remove comment + change deleteErr name

37263e6

Updating error message

b2af7e2

biswassri force-pushed the migration-uninstall branch from 1b0ec88 to b2af7e2 Compare December 18, 2024 17:30

openshift-ci bot removed the lgtm label Dec 18, 2024

openshift-ci bot assigned lburgazzoli Dec 18, 2024

openshift-ci bot added the lgtm label Dec 18, 2024

openshift-merge-bot bot merged commit 56109a7 into opendatahub-io:main Dec 19, 2024
10 checks passed

zdtsw reviewed Jan 2, 2025

View reviewed changes

pkg/upgrade/uninstallation.go Show resolved Hide resolved

controllers/datasciencecluster/kubebuilder_rbac.go Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration uninstall configmap from DSC to setup controller #1438

Migration uninstall configmap from DSC to setup controller #1438

biswassri commented Dec 10, 2024 •

edited

Loading

ykaliuta commented Dec 10, 2024

ykaliuta Dec 10, 2024

ykaliuta Dec 10, 2024

ykaliuta Dec 10, 2024

biswassri Dec 10, 2024 •

edited

Loading

ykaliuta Dec 10, 2024

biswassri Dec 10, 2024

ykaliuta Dec 10, 2024

biswassri Dec 10, 2024

ykaliuta Dec 10, 2024

biswassri Dec 10, 2024

biswassri commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 16, 2024 •

edited

Loading

lburgazzoli Dec 16, 2024

biswassri Dec 16, 2024

biswassri commented Dec 16, 2024

biswassri commented Dec 16, 2024

lburgazzoli Dec 16, 2024

biswassri Dec 16, 2024 •

edited

Loading

biswassri Dec 16, 2024

lburgazzoli Dec 17, 2024

biswassri Dec 17, 2024 •

edited

Loading

biswassri commented Dec 18, 2024

biswassri commented Dec 18, 2024

lburgazzoli commented Dec 18, 2024

grdryn left a comment

openshift-ci bot commented Dec 18, 2024

lburgazzoli commented Dec 18, 2024

biswassri commented Dec 18, 2024

biswassri commented Dec 18, 2024

biswassri commented Dec 19, 2024

Migration uninstall configmap from DSC to setup controller #1438

Migration uninstall configmap from DSC to setup controller #1438

Conversation

biswassri commented Dec 10, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

ykaliuta commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biswassri Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biswassri commented Dec 10, 2024 • edited Loading

codecov bot commented Dec 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biswassri commented Dec 16, 2024

biswassri commented Dec 16, 2024

Choose a reason for hiding this comment

biswassri Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biswassri Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

biswassri commented Dec 18, 2024

biswassri commented Dec 18, 2024

lburgazzoli commented Dec 18, 2024

grdryn left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 18, 2024

lburgazzoli commented Dec 18, 2024

biswassri commented Dec 18, 2024

biswassri commented Dec 18, 2024

biswassri commented Dec 19, 2024

biswassri commented Dec 10, 2024 •

edited

Loading

biswassri Dec 10, 2024 •

edited

Loading

biswassri commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 16, 2024 •

edited

Loading

biswassri Dec 16, 2024 •

edited

Loading

biswassri Dec 17, 2024 •

edited

Loading