Improve IBMPowerVSCluster deletion #1825

dharaneeshvrd · 2024-06-04T04:24:22Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1805

Special notes for your reviewer:

/area provider/ibmcloud

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

netlify · 2024-06-04T04:24:37Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name	Link
🔨 Latest commit	`726be6b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/66729e2be7e23a00081c2bfd
😎 Deploy Preview	https://deploy-preview-1825--kubernetes-sigs-cluster-api-ibmcloud.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Amulyam24 · 2024-06-04T09:37:34Z

controllers/ibmpowervscluster_controller.go

@@ -311,7 +311,8 @@ func (r *IBMPowerVSClusterReconciler) reconcileDelete(ctx context.Context, clust

 	clusterScope.Info("Deleting DHCP server")
 	if err := clusterScope.DeleteDHCPServer(); err != nil {
-		allErrs = append(allErrs, errors.Wrapf(err, "failed to delete DHCP server"))
+		clusterScope.Error(err, "failed to delete DHCP server")
+		return reconcile.Result{RequeueAfter: 30 * time.Second}, nil


@dharaneeshvrd, if the DHCP server deletion constantly fails, won't this keep requeuing the request and not proceed with PowerVS service instance deletion causing cluster deletion to be stuck?

Yes, currently also same thing will happen with this block https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/blob/main/cloud/scope/powervs_cluster.go#L2543toL2554
We need to delete before proceeding to the powervs instance right? or we can try deleting powervs instance after few retries anyway?

Yes, currently also same thing will happen with this block https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/blob/main/cloud/scope/powervs_cluster.go#L2543toL2554

Yes, currently it's doing the same. I'm trying to understand how this change will help

we can try deleting powervs instance after few retries anyway?

right, I feel we need to proceed with PowerVS service instance deletion despite failure with DHCP, wdyt?

I have not tested the scenario but in case of DHCP failure, will the service instance deletion also get stuck?

Yes if the dhcp server did not get deleted, I don't think powervs instance will get deleted successfully.

Got it, Thanks. @Amulyam24 was there any particular reason for explicitly deleting DHCP server in CAPI?

In the initial flow before adding proper state management of resources, we added the DHCP deletion and check to make sure PowerVS workspace deletion succeeds. We can consider removing it now and proceed with workspace deletion and retry until it is succeeds. We need to test it thoroughly to make sure it is cleaned up. @dharaneeshvrd can you please try this out and let us know?

Sure will try and see!

Tried many times in different region, the deletion went through without explicitly deleting the DHCP server.
Modified the code accordingly, please check!

Nice, Thanks for verifying.

Prajyot-Parab · 2024-06-13T17:16:03Z

/cc @Karthik-K-N

Karthik-K-N · 2024-06-17T13:10:32Z

cloud/scope/powervs_cluster.go

@@ -2492,32 +2490,6 @@ func (s *PowerVSClusterScope) deleteTransitGatewayConnections(tg *tgapiv1.Transi
 	return requeue, nil
 }

-// DeleteDHCPServer deletes DHCP server.
-func (s *PowerVSClusterScope) DeleteDHCPServer() error {
-	if !s.isResourceCreatedByController(infrav1beta2.ResourceTypeDHCPServer) {


Do you think there will be scenario where user provides existing workspace and expects the DHCP server to be created? If so in that case we need to delete only DHCP server.

True, we need to handle that. Will do the changes.

Addressed this, ptal!

Karthik-K-N · 2024-06-18T06:29:20Z

cloud/scope/powervs_cluster.go

@@ -2498,6 +2498,10 @@ func (s *PowerVSClusterScope) DeleteDHCPServer() error {
 		s.Info("Skipping DHP server deletion as resource is not created by controller")
 		return nil
 	}
+	if s.isResourceCreatedByController(infrav1beta2.ResourceTypeServiceInstance) {
+		s.Info("Skipping DHCP server deletion as PowerVS service instance is created by controller, will directly delete the PowerVS service instance")


I think user may not know PowerVS service instance delete will also delete DHCP server and may get confused after seeing word Skipping, I think its better to either use V(3) or update the log message.

Updated the log, let's have this log IMO, since its skipping a major action in delete flow.

Karthik-K-N · 2024-06-18T06:29:44Z

cloud/scope/powervs_cluster.go

 			ID: subnet.ID,
 		})

 		if err != nil {
-			if strings.Contains(err.Error(), string(VPCSubnetNotFound)) {
+			if resp.StatusCode == ResourceNotFoundCode {


So is it safe to assume that resp wont be nil?

It seems there is a possibility for it to be nil, added a nil check.

Karthik-K-N

/lgtm

Amulyam24

minor nit, otherwise LGTM. Thanks for verifying the deletion with DHCP server!

Amulyam24 · 2024-06-19T08:46:38Z

cloud/scope/types.go

-
-	// TransitGatewayNotFound is the error returned when a transit gateway is not found.
-	TransitGatewayNotFound = ResourceNotFound("gateway was not found")
+	// ResourceNotFoundCode indicates the http status code used to denote that the resource not exists.


how about simplifying as?

Suggested change

// ResourceNotFoundCode indicates the http status code used to denote that the resource not exists.

// ResourceNotFoundCode indicates the http status code when a resource does not exist.

Skip DHCP server deletion when PowerVS service instance is created by controller Since PowerVS service instance deletion will take care of deleting the DHCP server as well

dharaneeshvrd · 2024-06-21T05:02:31Z

@Karthik-K-N @Amulyam24 addressed the comments, please take a look!

Amulyam24

/lgtm

Karthik-K-N

/lgtm

mkumatag

/lgtm

k8s-ci-robot · 2024-06-21T09:26:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Amulyam24, dharaneeshvrd, Karthik-K-N, mkumatag

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

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

k8s-ci-robot added area/provider/ibmcloud Issues or PRs related to ibmcloud provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 4, 2024

k8s-ci-robot requested review from Amulyam24 and mkumatag June 4, 2024 04:24

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 4, 2024

Amulyam24 reviewed Jun 4, 2024

View reviewed changes

k8s-ci-robot requested a review from Karthik-K-N June 13, 2024 17:16

dharaneeshvrd force-pushed the improve-deletion branch 2 times, most recently from 644791b to 0730fa7 Compare June 17, 2024 12:51

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 17, 2024

Karthik-K-N reviewed Jun 17, 2024

View reviewed changes

dharaneeshvrd force-pushed the improve-deletion branch 2 times, most recently from 06c5ad5 to c795492 Compare June 18, 2024 04:05

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 18, 2024

dharaneeshvrd force-pushed the improve-deletion branch from c795492 to aa1c6fc Compare June 18, 2024 04:10

Karthik-K-N reviewed Jun 18, 2024

View reviewed changes

dharaneeshvrd force-pushed the improve-deletion branch 2 times, most recently from 2b7a3fd to dbe9618 Compare June 18, 2024 13:14

Karthik-K-N reviewed Jun 19, 2024

View reviewed changes

k8s-ci-robot assigned Karthik-K-N Jun 19, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2024

Amulyam24 reviewed Jun 19, 2024

View reviewed changes

Improve IBMPowerVSCluster deletion

726be6b

Skip DHCP server deletion when PowerVS service instance is created by controller Since PowerVS service instance deletion will take care of deleting the DHCP server as well

dharaneeshvrd force-pushed the improve-deletion branch from dbe9618 to 726be6b Compare June 19, 2024 09:00

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2024

Amulyam24 approved these changes Jun 21, 2024

View reviewed changes

k8s-ci-robot assigned Amulyam24 Jun 21, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2024

Karthik-K-N approved these changes Jun 21, 2024

View reviewed changes

mkumatag approved these changes Jun 21, 2024

View reviewed changes

k8s-ci-robot assigned mkumatag Jun 21, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2024

k8s-ci-robot merged commit af94a98 into kubernetes-sigs:main Jun 21, 2024
13 checks passed

dharaneeshvrd mentioned this pull request Aug 6, 2024

Cluster deletion stuck with DHCP server deletion failure #1815

Closed

Karthik-K-N mentioned this pull request Sep 17, 2024

EPIC - Introduce new API Specs and infra creation flow #1611

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve IBMPowerVSCluster deletion #1825

Improve IBMPowerVSCluster deletion #1825

dharaneeshvrd commented Jun 4, 2024

netlify bot commented Jun 4, 2024 •

edited

Loading

Amulyam24 Jun 4, 2024 •

edited

Loading

dharaneeshvrd Jun 4, 2024

Amulyam24 Jun 5, 2024

Amulyam24 Jun 5, 2024

dharaneeshvrd Jun 5, 2024

Karthik-K-N Jun 14, 2024

Amulyam24 Jun 14, 2024 •

edited

Loading

dharaneeshvrd Jun 17, 2024

dharaneeshvrd Jun 17, 2024

Karthik-K-N Jun 17, 2024

Prajyot-Parab commented Jun 13, 2024

Karthik-K-N Jun 17, 2024

dharaneeshvrd Jun 17, 2024

dharaneeshvrd Jun 18, 2024

Karthik-K-N Jun 18, 2024 •

edited

Loading

dharaneeshvrd Jun 19, 2024

Karthik-K-N Jun 18, 2024

dharaneeshvrd Jun 19, 2024

Karthik-K-N left a comment

Amulyam24 left a comment

Amulyam24 Jun 19, 2024

dharaneeshvrd commented Jun 21, 2024

Amulyam24 left a comment

Karthik-K-N left a comment

mkumatag left a comment

k8s-ci-robot commented Jun 21, 2024

	// ResourceNotFoundCode indicates the http status code used to denote that the resource not exists.
	// ResourceNotFoundCode indicates the http status code when a resource does not exist.

Improve IBMPowerVSCluster deletion #1825

Improve IBMPowerVSCluster deletion #1825

Conversation

dharaneeshvrd commented Jun 4, 2024

netlify bot commented Jun 4, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Amulyam24 Jun 4, 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

Amulyam24 Jun 14, 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

Prajyot-Parab commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karthik-K-N Jun 18, 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

Karthik-K-N left a comment

Choose a reason for hiding this comment

Amulyam24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharaneeshvrd commented Jun 21, 2024

Amulyam24 left a comment

Choose a reason for hiding this comment

Karthik-K-N left a comment

Choose a reason for hiding this comment

mkumatag left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 21, 2024

netlify bot commented Jun 4, 2024 •

edited

Loading

Amulyam24 Jun 4, 2024 •

edited

Loading

Amulyam24 Jun 14, 2024 •

edited

Loading

Karthik-K-N Jun 18, 2024 •

edited

Loading