feat(reset): add cleanup volumes and cleanup-load-balancers flag #3507

rajaSahil · 2024-12-19T22:58:11Z

What this PR does / why we need it:

add remove-volumes and remove-lb-services flag to delete dynamically provisioned and unretained volumes and load balancers services from the cluster before resetting.

Which issue(s) this PR fixes:

What type of PR is this?

/kind feature

Does this PR introduce a user-facing change? Then add your Release Note here:

Add `remove-volumes` and `remove-lb-services` flags to delete dynamically provisioned and unretained PersistentVolumes and LoadBalancer Services before resetting the cluster

Documentation:

NONE

rajaSahil · 2024-12-20T09:45:12Z

/assign @xmudrii
Please review.

kubermatic-bot · 2024-12-20T10:29:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from xmudrii. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

Signed-off-by: rajaSahil <[email protected]>

xmudrii

I did some initial review and testing, I'll have some more comments, but let's start with this and then we can follow up on some other stuff.

xmudrii · 2024-12-31T14:06:52Z

pkg/clientutil/service.go

@@ -0,0 +1,71 @@
+/*
+Copyright 2020 The KubeOne Authors.


Please replace 2020 with ~~2024~~ 2025 in all files that you added in this PR. :)

pkg/clientutil/service.go

xmudrii · 2024-12-31T15:53:26Z

pkg/clientutil/service.go

+func CleanupLBs(ctx context.Context, logger logrus.FieldLogger, c client.Client) error {
+	serviceList := &corev1.ServiceList{}
+	if err := c.List(ctx, serviceList); err != nil {
+		return fail.KubeClient(err, "failed to list Service.")


A couple of things:

we don't use uppercase letters and capitalization in error messages

we use gerund form of operation that we do for error messages, e.g. listing services instead of failed to list services

Suggested change

return fail.KubeClient(err, "failed to list Service.")

return fail.KubeClient(err, "listing services")

xmudrii · 2024-12-31T15:55:01Z

pkg/clientutil/service.go

+		}
+		// Only LoadBalancer services incur charges on cloud providers
+		if service.Spec.Type == corev1.ServiceTypeLoadBalancer {
+			logger.Infof("Deleting SVC : %s/%s\n", service.Namespace, service.Name)


Let's do this instead (because this might spam logs quite a lot):

Have a message before getting into the loop such as Cleaning up LoadBalancer Services...

Have this as a debug level message, e.g.:

Suggested change

logger.Infof("Deleting SVC : %s/%s\n", service.Namespace, service.Name)

logger.Debugf("Deleting LoadBalancer Service \"%s/%s\"", service.Namespace, service.Name)

xmudrii · 2024-12-31T15:55:33Z

pkg/clientutil/service.go

+}
+
+func WaitCleanupLbs(ctx context.Context, logger logrus.FieldLogger, c client.Client) error {
+	logger.Infoln("Waiting for all load balancer services to get deleted...")


Suggested change

logger.Infoln("Waiting for all load balancer services to get deleted...")

logger.Infoln("Waiting for all LoadBalancer Services to get deleted...")

xmudrii · 2024-12-31T16:04:12Z

pkg/state/context.go

+	CleanupVolumes            bool
+	CleanupLoadBalancers      bool


If you rename flags, make sure to remove this as well.

xmudrii · 2024-12-31T16:06:35Z

pkg/tasks/reset.go

+	if !s.CleanupLoadBalancers {
+		return nil
+	}


We should make this task conditional instead, see an example here

kubeone/pkg/tasks/tasks.go

Line 603 in 8b8d794

Predicate: func(s *state.State) bool { return s.Cluster.CloudProvider.Openstack != nil },

xmudrii · 2024-12-31T16:06:57Z

pkg/tasks/reset.go

+		return nil
+	}
+	var lastErr error
+	s.Logger.Infoln("Deleting load balancer services...")


Suggested change

s.Logger.Infoln("Deleting load balancer services...")

s.Logger.Infoln("Deleting LoadBalancer Services...")

xmudrii · 2024-12-31T16:08:01Z

pkg/tasks/reset.go

+	}
+	var lastErr error
+	s.Logger.Infoln("Deleting load balancer services...")
+	_ = wait.ExponentialBackoff(defaultRetryBackoff(3), func() (bool, error) {


We don't need exponential backoff here I think, each task already has 3 times backoff.

Removed exponential backoff from both functions.

xmudrii · 2024-12-31T16:08:17Z

pkg/tasks/reset.go

+		return true, nil
+	})
+
+	_ = wait.ExponentialBackoff(defaultRetryBackoff(3), func() (bool, error) {


Same here, this shouldn't be needed.

Signed-off-by: rajaSahil <[email protected]>

kron4eg · 2025-05-15T07:12:46Z

go.mod

@@ -26,7 +26,7 @@ require (
 	github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2
 	github.com/sirupsen/logrus v1.9.3
 	github.com/spf13/cobra v1.8.1
-	github.com/spf13/pflag v1.0.5
+	github.com/spf13/pflag v1.0.6-0.20210604193023-d5e0c0615ace


is this really necessary to use unreleased version?

kron4eg · 2025-05-15T07:14:06Z

pkg/clientutil/service.go

+		if service.DeletionTimestamp != nil {
+			continue
+		}
+		logger.Infof("Cleaning up LoadBalancer Services...")


This line will appear for each Service in the cluster (LB or not), I believe this is not what you wanted to do.

kron4eg · 2025-05-15T07:18:46Z

pkg/clientutil/volumes.go

+	creatorGetters := []reconciling.NamedValidatingWebhookConfigurationReconcilerFactory{
+		creationPreventingWebhook("", VolumeResources),
+	}
+	if err := reconciling.ReconcileValidatingWebhookConfigurations(ctx, creatorGetters, "", c); err != nil {


I'm pretty sure we can EASILY create a webhook without reconciling framework, please replace this with a simple object creation.

kron4eg · 2025-05-15T07:26:33Z

pkg/clientutil/volumes.go

+	// We disable the PV & PVC creation so nothing creates new PV's while we delete them
+	logger.Infoln("Creating ValidatingWebhookConfiguration to disable future PV & PVC creation...")
+	if err := disablePVCreation(ctx, c); err != nil {
+		return fail.KubeClient(err, "failed to disable future PV & PVC creation.")


In all (here and forward) fail.Xyz() calls please avoid using "failed to do something", use "doing something".

kron4eg · 2025-05-15T09:09:36Z

pkg/clientutil/volumes.go

+
+	for _, pod := range pvUsingPods {
+		if pod.DeletionTimestamp == nil {
+			if err := DeleteIfExists(ctx, c, pod); err != nil {


Please use pod eviction. For more info see:

kubeone/pkg/nodeutils/drain.go

Line 78 in fb64982

func (dr *drainer) drainHelper(ctx context.Context) (*drain.Helper, error) {

https://pkg.go.dev/k8s.io/kubectl/pkg/drain#Helper.EvictPod

kron4eg · 2025-05-15T09:19:23Z

pkg/clientutil/volumes.go

+
+		if (pvcList != nil && pvList != nil) && len(pvcList.Items) == 0 && len(pvList.Items) == 0 {
+			return true, nil
+		}


since we don't do anything useful with pvcList and pvList besides checking their length, I'm pretty sure we can return this info directly

kron4eg · 2025-05-15T09:20:43Z

pkg/tasks/reset.go

+	lastErr = clientutil.WaitCleanupLbs(s.Context, s.Logger, s.DynamicClient)
+	if lastErr != nil {
+		s.Logger.Warn("Waiting for all load balancer services to be deleted...")
+
+		return lastErr
+	}


Suggested change

lastErr = clientutil.WaitCleanupLbs(s.Context, s.Logger, s.DynamicClient)

if lastErr != nil {

s.Logger.Warn("Waiting for all load balancer services to be deleted...")

return lastErr

}

lastErr = clientutil.WaitCleanupLbs(s.Context, s.Logger, s.DynamicClient)

if lastErr != nil {

s.Logger.Warn("Waiting for all load balancer services to be deleted...")

}

lastErr will be returned anyway

kron4eg · 2025-05-15T09:21:36Z

pkg/tasks/reset.go

+	lastErr = clientutil.WaitCleanUpVolumes(s.Context, s.Logger, s.DynamicClient)
+	if lastErr != nil {
+		s.Logger.Warn("Waiting for all dynamically provisioned and unretained volumes to be deleted...")
+
+		return lastErr
+	}


Suggested change

lastErr = clientutil.WaitCleanUpVolumes(s.Context, s.Logger, s.DynamicClient)

if lastErr != nil {

s.Logger.Warn("Waiting for all dynamically provisioned and unretained volumes to be deleted...")

return lastErr

}

lastErr = clientutil.WaitCleanUpVolumes(s.Context, s.Logger, s.DynamicClient)

if lastErr != nil {

s.Logger.Warn("Waiting for all dynamically provisioned and unretained volumes to be deleted...")

}

rajaSahil force-pushed the destroy-volumes-svc branch 5 times, most recently from c7a1d0b to 906bca7 Compare December 20, 2024 09:39

kubermatic-bot assigned xmudrii Dec 20, 2024

rajaSahil force-pushed the destroy-volumes-svc branch from 906bca7 to 576aa4a Compare December 20, 2024 10:29

feat(reset): add destroy volumes and destroy-load-balancers flag

7c32a02

Signed-off-by: rajaSahil <[email protected]>

rajaSahil force-pushed the destroy-volumes-svc branch from 576aa4a to 7c32a02 Compare December 20, 2024 10:30

xmudrii requested changes Dec 31, 2024

View reviewed changes

kubermatic-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 12, 2025

feat(reset): addressed review comments

e0aa078

Signed-off-by: rajaSahil <[email protected]>

rajaSahil force-pushed the destroy-volumes-svc branch from b98e59c to e0aa078 Compare January 12, 2025 17:48

rajaSahil requested review from xmudrii and kron4eg January 12, 2025 17:51

kron4eg requested changes May 15, 2025

View reviewed changes

kubermatic-bot assigned kron4eg May 15, 2025

	return fail.KubeClient(err, "failed to list Service.")
	return fail.KubeClient(err, "listing services")

	logger.Infof("Deleting SVC : %s/%s\n", service.Namespace, service.Name)
	logger.Debugf("Deleting LoadBalancer Service \"%s/%s\"", service.Namespace, service.Name)

	logger.Infoln("Waiting for all load balancer services to get deleted...")
	logger.Infoln("Waiting for all LoadBalancer Services to get deleted...")

	s.Logger.Infoln("Deleting load balancer services...")
	s.Logger.Infoln("Deleting LoadBalancer Services...")

feat(reset): add cleanup volumes and cleanup-load-balancers flag #3507

Are you sure you want to change the base?

feat(reset): add cleanup volumes and cleanup-load-balancers flag #3507

Uh oh!

Conversation

rajaSahil commented Dec 19, 2024 • edited by xmudrii Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajaSahil commented Dec 20, 2024

Uh oh!

kubermatic-bot commented Dec 20, 2024

Uh oh!

xmudrii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rajaSahil commented Dec 19, 2024 •

edited by xmudrii

Loading