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 cleanup volumes and cleanup-load-balancers 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 cleanup-volumes and cleanup-load-balancers flag to delete `dynamically provisioned and unretained volumes` and `load balancer 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. :)

xmudrii · 2024-12-31T15:51:57Z

pkg/clientutil/service.go

+
+package clientutil
+
+import (


Take a look here how we order imports:

kubeone/pkg/nodeutils/drain.go

Lines 19 to 31 in 8b8d794

import (

"context"

"github.com/sirupsen/logrus"

"k8c.io/kubeone/pkg/fail"

corev1 "k8s.io/api/core/v1"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"k8s.io/client-go/kubernetes"

"k8s.io/client-go/rest"

"k8s.io/kubectl/pkg/drain"

)

It should be stdlib -> external dependencies -> k8c.io -> k8s.io

This will fix all the imports

go run go.xrstf.de/gimps@latest .

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.

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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rajaSahil commented Dec 19, 2024 •

edited

Loading

rajaSahil commented Dec 20, 2024

kubermatic-bot commented Dec 20, 2024

xmudrii left a comment

xmudrii Dec 31, 2024

xmudrii Dec 31, 2024

kron4eg Jan 2, 2025

xmudrii Dec 31, 2024

xmudrii Dec 31, 2024

xmudrii Dec 31, 2024

xmudrii Dec 31, 2024

xmudrii Dec 31, 2024

xmudrii Dec 31, 2024

xmudrii Dec 31, 2024

xmudrii Dec 31, 2024

	import (
	"context"

	"github.com/sirupsen/logrus"

	"k8c.io/kubeone/pkg/fail"

	corev1 "k8s.io/api/core/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/client-go/kubernetes"
	"k8s.io/client-go/rest"
	"k8s.io/kubectl/pkg/drain"
	)

	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

Conversation

rajaSahil commented Dec 19, 2024 • edited Loading

rajaSahil commented Dec 20, 2024

kubermatic-bot commented Dec 20, 2024

xmudrii left a 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

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

rajaSahil commented Dec 19, 2024 •

edited

Loading