Decomission: Base it on the hypervisor

This also needs some manual migration work: The finalizer
in the node needs to get manually removed, and the name
change of the controller-name requires a reset of the
field owners.

Author: fwiesel

internal/controller/decomission_controller.go

@@ -28,7 +28,6 @@ import (
 	"github.com/gophercloud/gophercloud/v2/openstack/compute/v2/aggregates"
 	"github.com/gophercloud/gophercloud/v2/openstack/compute/v2/services"
 	"github.com/gophercloud/gophercloud/v2/openstack/placement/v1/resourceproviders"
-	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -44,7 +43,7 @@ import (
 
 const (
 	decommissionFinalizerName  = "cobaltcore.cloud.sap/decommission-hypervisor"
-	DecommissionControllerName = "nodeDecommission"
+	DecommissionControllerName = "decommission"
 )
 
 type NodeDecommissionReconciler struct {
@@ -57,20 +56,13 @@ type NodeDecommissionReconciler struct {
 // The counter-side in gardener is here:
 // https://github.com/gardener/machine-controller-manager/blob/rel-v0.56/pkg/util/provider/machinecontroller/machine.go#L646
 
-// +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch;patch;update
-// +kubebuilder:rbac:groups="",resources=nodes/finalizers,verbs=update
 // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors,verbs=get;list;watch
 // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;list;watch;update;patch
 func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	hostname := req.Name
 	log := logger.FromContext(ctx).WithName(req.Name).WithValues("hostname", hostname)
 	ctx = logger.IntoContext(ctx, log)
 
-	node := &corev1.Node{}
-	if err := r.Get(ctx, req.NamespacedName, node); err != nil {
-		return ctrl.Result{}, k8sclient.IgnoreNotFound(err)
-	}
-
 	// Fetch HV to check if lifecycle management is enabled
 	hv := &kvmv1.Hypervisor{}
 	if err := r.Get(ctx, k8sclient.ObjectKey{Name: hostname}, hv); err != nil {
@@ -79,14 +71,14 @@ func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	}
 	if !hv.Spec.LifecycleEnabled {
 		// Get out of the way
-		return r.removeFinalizer(ctx, node)
+		return r.removeFinalizer(ctx, hv)
 	}
 
-	if !controllerutil.ContainsFinalizer(node, decommissionFinalizerName) {
+	if !controllerutil.ContainsFinalizer(hv, decommissionFinalizerName) {
 		return ctrl.Result{}, retry.RetryOnConflict(retry.DefaultRetry, func() error {
-			patch := k8sclient.MergeFrom(node.DeepCopy())
-			controllerutil.AddFinalizer(node, decommissionFinalizerName)
-			if err := r.Patch(ctx, node, patch); err != nil {
+			patch := k8sclient.MergeFrom(hv.DeepCopy())
+			controllerutil.AddFinalizer(hv, decommissionFinalizerName)
+			if err := r.Patch(ctx, hv, patch); err != nil {
 				return fmt.Errorf("failed to add finalizer due to %w", err)
 			}
 			log.Info("Added finalizer")
@@ -95,16 +87,16 @@ func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	}
 
 	// Not yet deleting hv, nothing more to do
-	if node.DeletionTimestamp.IsZero() {
+	if hv.DeletionTimestamp.IsZero() {
 		return ctrl.Result{}, nil
 	}
 
 	// Someone is just deleting the hv, without going through termination
 	// See: https://github.com/gardener/machine-controller-manager/blob/rel-v0.56/pkg/util/provider/machinecontroller/machine.go#L658-L659
-	if !IsNodeConditionTrue(node.Status.Conditions, "Terminating") {
+	if !meta.IsStatusConditionTrue(hv.Status.Conditions, "Terminating") {
 		log.Info("removing finalizer since not terminating")
 		// So we just get out of the way for now
-		return r.removeFinalizer(ctx, node)
+		return r.removeFinalizer(ctx, hv)
 	}
 
 	if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeReady) {
@@ -116,7 +108,7 @@ func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	hypervisor, err := openstack.GetHypervisorByName(ctx, r.computeClient, hostname, true)
 	if errors.Is(err, openstack.ErrNoHypervisor) {
 		// We are (hopefully) done
-		return r.removeFinalizer(ctx, node)
+		return r.removeFinalizer(ctx, hv)
 	}
 
 	// TODO: remove since RunningVMs is only available until micro-version 2.87, and also is updated asynchronously
@@ -141,7 +133,7 @@ func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Req
 		return r.setDecommissioningCondition(ctx, hv, fmt.Sprintf("cannot list aggregates due to %v", err))
 	}
 
-	host := node.Name
+	host := hv.Name
 	for name, aggregate := range aggs {
 		if slices.Contains(aggregate.Hosts, host) {
 			opts := aggregates.RemoveHostOpts{Host: host}
@@ -168,17 +160,17 @@ func (r *NodeDecommissionReconciler) Reconcile(ctx context.Context, req ctrl.Req
 		return r.setDecommissioningCondition(ctx, hv, fmt.Sprintf("cannot clean up resource provider: %v", err))
 	}
 
-	return r.removeFinalizer(ctx, node)
+	return r.removeFinalizer(ctx, hv)
 }
 
-func (r *NodeDecommissionReconciler) removeFinalizer(ctx context.Context, node *corev1.Node) (ctrl.Result, error) {
-	if !controllerutil.ContainsFinalizer(node, decommissionFinalizerName) {
+func (r *NodeDecommissionReconciler) removeFinalizer(ctx context.Context, hypervisor *kvmv1.Hypervisor) (ctrl.Result, error) {
+	if !controllerutil.ContainsFinalizer(hypervisor, decommissionFinalizerName) {
 		return ctrl.Result{}, nil
 	}
 
-	nodeBase := node.DeepCopy()
-	controllerutil.RemoveFinalizer(node, decommissionFinalizerName)
-	err := r.Patch(ctx, node, k8sclient.MergeFromWithOptions(nodeBase,
+	base := hypervisor.DeepCopy()
+	controllerutil.RemoveFinalizer(hypervisor, decommissionFinalizerName)
+	err := r.Patch(ctx, hypervisor, k8sclient.MergeFromWithOptions(base,
 		k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(DecommissionControllerName))
 	return ctrl.Result{}, err
 }
@@ -195,7 +187,7 @@ func (r *NodeDecommissionReconciler) setDecommissioningCondition(ctx context.Con
 		k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(DecommissionControllerName)); err != nil {
 		return ctrl.Result{}, fmt.Errorf("cannot update hypervisor status due to %w", err)
 	}
-	return ctrl.Result{RequeueAfter: shortRetryTime}, nil
+	return ctrl.Result{}, nil
 }
 
 // SetupWithManager sets up the controller with the Manager.
@@ -217,6 +209,6 @@ func (r *NodeDecommissionReconciler) SetupWithManager(mgr ctrl.Manager) error {
 
 	return ctrl.NewControllerManagedBy(mgr).
 		Named(DecommissionControllerName).
-		For(&corev1.Node{}).
+		For(&kvmv1.Hypervisor{}).
 		Complete(r)
 }

internal/controller/decomission_controller_test.go

@@ -26,7 +26,6 @@ import (
 	"github.com/gophercloud/gophercloud/v2/testhelper/client"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -68,9 +67,9 @@ const (
 var _ = Describe("Decommission Controller", func() {
 	var (
 		r            *NodeDecommissionReconciler
-		nodeName     = types.NamespacedName{Name: hypervisorName}
+		name         = types.NamespacedName{Name: hypervisorName}
 		reconcileReq = ctrl.Request{
-			NamespacedName: nodeName,
+			NamespacedName: name,
 		}
 		fakeServer testhelper.FakeServer
 	)
@@ -91,90 +90,70 @@ var _ = Describe("Decommission Controller", func() {
 			placementClient: client.ServiceClient(fakeServer),
 		}
 
-		By("creating the namespace for the reconciler")
-		ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespaceName}}
-		Expect(k8sclient.IgnoreAlreadyExists(k8sClient.Create(ctx, ns))).To(Succeed())
-
-		DeferCleanup(func(ctx SpecContext) {
-			Expect(k8sClient.Delete(ctx, ns)).To(Succeed())
-		})
-
-		By("creating the core resource for the Kind Node")
-		node := &corev1.Node{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:   nodeName.Name,
-				Labels: map[string]string{labelEvictionRequired: "true"},
-			},
-		}
-		Expect(k8sClient.Create(ctx, node)).To(Succeed())
-		DeferCleanup(func(ctx SpecContext) {
-			node := &corev1.Node{}
-			Expect(k8sclient.IgnoreNotFound(k8sClient.Get(ctx, nodeName, node))).To(Succeed())
-			if len(node.Finalizers) > 0 {
-				node.Finalizers = make([]string, 0)
-				Expect(k8sClient.Update(ctx, node)).To(Succeed())
-			}
-			if node.Name != "" {
-				Expect(k8sclient.IgnoreNotFound(k8sClient.Delete(ctx, node))).To(Succeed())
-			}
-		})
-
-		By("Create the hypervisor resource with lifecycle enabled")
+		By("Create the hypervisor to be reconciled with lifecycle enabled")
 		hypervisor := &kvmv1.Hypervisor{
 			ObjectMeta: metav1.ObjectMeta{
-				Name: nodeName.Name,
+				Name: name.Name,
 			},
 			Spec: kvmv1.HypervisorSpec{
 				LifecycleEnabled: true,
 			},
 		}
 		Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed())
 		DeferCleanup(func(ctx SpecContext) {
-			Expect(k8sClient.Delete(ctx, hypervisor)).To(Succeed())
+			hypervisor := &kvmv1.Hypervisor{}
+			Expect(k8sclient.IgnoreNotFound(k8sClient.Get(ctx, name, hypervisor))).To(Succeed())
+			if len(hypervisor.Finalizers) > 0 {
+				hypervisor.Finalizers = make([]string, 0)
+				Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())
+			}
+			if hypervisor.Name != "" {
+				Expect(k8sclient.IgnoreNotFound(k8sClient.Delete(ctx, hypervisor))).To(Succeed())
+			}
 		})
 	})
 
-	Context("When reconciling a node", func() {
+	Context("When reconciling a hypervisor", func() {
 		It("should set the finalizer", func(ctx SpecContext) {
 			By("reconciling the created resource")
 			_, err := r.Reconcile(ctx, reconcileReq)
 			Expect(err).NotTo(HaveOccurred())
-			node := &corev1.Node{}
+			hypervisor := &kvmv1.Hypervisor{}
 
-			Expect(k8sClient.Get(ctx, nodeName, node)).To(Succeed())
-			Expect(node.Finalizers).To(ContainElement(decommissionFinalizerName))
+			Expect(k8sClient.Get(ctx, name, hypervisor)).To(Succeed())
+			Expect(hypervisor.Finalizers).To(ContainElement(decommissionFinalizerName))
 		})
 	})
 
-	Context("When terminating a node", func() {
+	Context("When terminating a hypervisor", func() {
 		JustBeforeEach(func(ctx SpecContext) {
 			By("reconciling first reconciling the to add the finalizer")
 			_, err := r.Reconcile(ctx, reconcileReq)
 			Expect(err).NotTo(HaveOccurred())
-			node := &corev1.Node{}
+			hypervisor := &kvmv1.Hypervisor{}
 
-			Expect(k8sClient.Get(ctx, nodeName, node)).To(Succeed())
-			Expect(node.Finalizers).To(ContainElement(decommissionFinalizerName))
+			Expect(k8sClient.Get(ctx, name, hypervisor)).To(Succeed())
+			Expect(hypervisor.Finalizers).To(ContainElement(decommissionFinalizerName))
 
-			By("and then terminating then node")
-			node.Status.Conditions = append(node.Status.Conditions, corev1.NodeCondition{
+			By("and then terminating then hypervisor")
+			meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
 				Type:    "Terminating",
-				Status:  corev1.ConditionTrue,
+				Status:  metav1.ConditionTrue,
 				Reason:  "dontcare",
 				Message: "dontcare",
 			})
-			Expect(k8sClient.Status().Update(ctx, node)).To(Succeed())
-			Expect(k8sClient.Delete(ctx, node)).To(Succeed())
-			nodelist := &corev1.NodeList{}
-			Expect(k8sClient.List(ctx, nodelist)).To(Succeed())
-			Expect(nodelist.Items).NotTo(BeEmpty())
+			Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
+			Expect(k8sClient.Delete(ctx, hypervisor)).To(Succeed())
+			hypervisorList := &kvmv1.HypervisorList{}
+			Expect(k8sClient.List(ctx, hypervisorList)).To(Succeed())
+			Expect(hypervisorList.Items).NotTo(BeEmpty())
 		})
 
 		When("the hypervisor was set to ready", func() {
 			getHypervisorsCalled := 0
 			BeforeEach(func(ctx SpecContext) {
 				hv := &kvmv1.Hypervisor{}
-				Expect(k8sClient.Get(ctx, nodeName, hv)).To(Succeed())
+				Expect(k8sClient.Get(ctx, name, hv)).To(Succeed())
 				meta.SetStatusCondition(&hv.Status.Conditions,
 					metav1.Condition{
 						Type:    kvmv1.ConditionTypeReady,
@@ -244,7 +223,7 @@ var _ = Describe("Decommission Controller", func() {
 				_, err := r.Reconcile(ctx, reconcileReq)
 				Expect(err).NotTo(HaveOccurred())
 				hypervisor := &kvmv1.Hypervisor{}
-				Expect(k8sClient.Get(ctx, nodeName, hypervisor)).To(Succeed())
+				Expect(k8sClient.Get(ctx, name, hypervisor)).To(Succeed())
 				Expect(hypervisor.Status.Conditions).To(ContainElement(
 					SatisfyAll(
 						HaveField("Type", kvmv1.ConditionTypeReady),
@@ -262,8 +241,8 @@ var _ = Describe("Decommission Controller", func() {
 				}
 				Expect(getHypervisorsCalled).To(BeNumerically(">", 0))
 
-				node := &corev1.Node{}
-				err := k8sClient.Get(ctx, nodeName, node)
+				hypervisor := &kvmv1.Hypervisor{}
+				err := k8sClient.Get(ctx, name, hypervisor)
 				Expect(err).To(HaveOccurred())
 				Expect(k8sclient.IgnoreNotFound(err)).To(Succeed())
 			})