fix decommision, harden unit-tests

internal/controller/decomission_controller.go

@@ -29,7 +29,6 @@ import (
 	"github.com/gophercloud/gophercloud/v2/openstack/compute/v2/services"
 	"github.com/gophercloud/gophercloud/v2/openstack/placement/v1/resourceproviders"
 	corev1 "k8s.io/api/core/v1"
-	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -38,6 +37,7 @@ import (
 	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	logger "sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1"
 	"github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/openstack"
@@ -59,25 +59,24 @@ type NodeDecommissionReconciler struct {
 
 // +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)
 
-	hv := &kvmv1.Hypervisor{}
-	if err := r.Get(ctx, req.NamespacedName, hv); err != nil {
-		// ignore not found errors, could be deleted
+	node := &corev1.Node{}
+	if err := r.Get(ctx, req.NamespacedName, node); err != nil {
 		return ctrl.Result{}, k8sclient.IgnoreNotFound(err)
 	}
 
-	node := &corev1.Node{}
-	if err := r.Get(ctx, k8sclient.ObjectKey{Name: hv.Name}, node); err != nil {
-		if k8serrors.IsNotFound(err) {
-			return ctrl.Result{}, nil // Node not found, nothing to do
-		}
-		return ctrl.Result{}, 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 {
+		// ignore not found errors, could be deleted
+		return ctrl.Result{}, k8sclient.IgnoreNotFound(err)
 	}
-
 	if !hv.Spec.LifecycleEnabled {
 		// Get out of the way
 		return r.removeFinalizer(ctx, node)
@@ -213,9 +212,18 @@ func (r *NodeDecommissionReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	}
 	r.placementClient.Microversion = "1.39" // yoga, or later
 
+	// add predicate to only reconcile nodes with DeletionTimestamp set (i.e. being deleted)
+	predicateFilter := predicate.NewPredicateFuncs(func(object k8sclient.Object) bool {
+		node, ok := object.(*corev1.Node)
+		if !ok {
+			return false
+		}
+		return !node.DeletionTimestamp.IsZero() || controllerutil.ContainsFinalizer(node, decommissionFinalizerName)
+	})
+
 	return ctrl.NewControllerManagedBy(mgr).
 		Named("nodeDecommission").
-		For(&kvmv1.Hypervisor{}).
-		Owns(&corev1.Node{}).
+		For(&corev1.Node{}).
+		WithEventFilter(predicateFilter).
 		Complete(r)
 }

internal/controller/decomission_controller_test.go

@@ -28,61 +28,64 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1"
 )
 
 var _ = Describe("Decommission Controller", func() {
 	var decomReconciler *NodeDecommissionReconciler
+	const nodeName = "node-test"
 
-	Context("When reconciling a node", func() {
-		const nodeName = "node-test"
-
-		ctx := context.Background()
-
-		reconcileNodeLoop := func(steps int) (res ctrl.Result, err error) {
-			req := ctrl.Request{
-				NamespacedName: types.NamespacedName{Name: nodeName},
-			}
-			for range steps {
-				res, err = decomReconciler.Reconcile(ctx, req)
-				if err != nil {
-					return
-				}
-			}
-			return
+	BeforeEach(func(ctx context.Context) {
+		decomReconciler = &NodeDecommissionReconciler{
+			Client: k8sClient,
+			Scheme: k8sClient.Scheme(),
 		}
 
-		BeforeEach(func() {
-			decomReconciler = &NodeDecommissionReconciler{
-				Client: k8sClient,
-				Scheme: k8sClient.Scheme(),
-			}
+		By("creating the namespace for the reconciler")
+		ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "monsoon3"}}
+		Expect(client.IgnoreAlreadyExists(k8sClient.Create(ctx, ns))).To(Succeed())
 
-			By("creating the namespace for the reconciler")
-			ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "monsoon3"}}
-			Expect(client.IgnoreAlreadyExists(k8sClient.Create(ctx, ns))).To(Succeed())
+		By("creating the core resource for the Kind Node")
+		resource := &corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:   nodeName,
+				Labels: map[string]string{labelEvictionRequired: "true"}, //nolint:goconst
+			},
+		}
+		Expect(k8sClient.Create(ctx, resource)).To(Succeed())
+
+		By("Create the hypervisor resource")
+		hypervisor := &kvmv1.Hypervisor{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: nodeName,
+			},
+			Spec: kvmv1.HypervisorSpec{
+				LifecycleEnabled: true,
+			},
+		}
+		Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed())
+	})
 
-			By("creating the core resource for the Kind Node")
-			resource := &corev1.Node{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:   nodeName,
-					Labels: map[string]string{labelEvictionRequired: "true"}, //nolint:goconst
-				},
-			}
-			Expect(k8sClient.Create(ctx, resource)).To(Succeed())
-		})
+	AfterEach(func(ctx context.Context) {
+		node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}
+		By("Cleanup the specific node and hypervisor resource")
+		Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, node))).To(Succeed())
+		hypervisor := &kvmv1.Hypervisor{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}
+		Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, hypervisor))).To(Succeed())
+	})
 
-		AfterEach(func() {
-			node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}
-			By("Cleanup the specific node")
-			Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, node))).To(Succeed())
-		})
+	Context("When reconciling a node", func() {
 
-		It("should successfully reconcile the resource", func() {
+		It("should successfully reconcile the resource", func(ctx context.Context) {
 			By("ConditionType the created resource")
 			testhelper.SetupHTTP()
 			defer testhelper.TeardownHTTP()
 
-			_, err := reconcileNodeLoop(1)
+			req := ctrl.Request{
+				NamespacedName: types.NamespacedName{Name: nodeName},
+			}
+			_, err := decomReconciler.Reconcile(ctx, req)
 			Expect(err).NotTo(HaveOccurred())
 		})
 	})

internal/controller/maintenance_controller_test.go

@@ -18,8 +18,6 @@ limitations under the License.
 package controller
 
 import (
-	"context"
-
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
@@ -30,55 +28,43 @@ import (
 )
 
 var _ = Describe("Maintenance Controller", func() {
+	const nodeName = "node-test"
 	var maintenanceController *MaintenanceController
 
-	Context("When reconciling a node", func() {
-		const nodeName = "node-test"
+	BeforeEach(func() {
+		maintenanceController = &MaintenanceController{
+			Client: k8sClient,
+			Scheme: k8sClient.Scheme(),
+		}
 
-		ctx := context.Background()
+		By("creating the namespace for the reconciler")
+		ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "monsoon3"}}
+		Expect(client.IgnoreAlreadyExists(k8sClient.Create(ctx, ns))).To(Succeed())
 
-		reconcileNodeLoop := func(steps int) (res ctrl.Result, err error) {
-			req := ctrl.Request{
-				NamespacedName: types.NamespacedName{Name: nodeName},
-			}
-			for range steps {
-				res, err = maintenanceController.Reconcile(ctx, req)
-				if err != nil {
-					return
-				}
-			}
-			return
+		By("creating the core resource for the Kind Node")
+		resource := &corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:   nodeName,
+				Labels: map[string]string{labelEvictionRequired: "true"}, //nolint:goconst
+			},
 		}
+		Expect(k8sClient.Create(ctx, resource)).To(Succeed())
+	})
 
-		BeforeEach(func() {
-			maintenanceController = &MaintenanceController{
-				Client: k8sClient,
-				Scheme: k8sClient.Scheme(),
-			}
-
-			By("creating the namespace for the reconciler")
-			ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "monsoon3"}}
-			Expect(client.IgnoreAlreadyExists(k8sClient.Create(ctx, ns))).To(Succeed())
+	AfterEach(func() {
+		node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}
+		By("Cleanup the specific node")
+		Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, node))).To(Succeed())
+	})
 
-			By("creating the core resource for the Kind Node")
-			resource := &corev1.Node{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:   nodeName,
-					Labels: map[string]string{labelEvictionRequired: "true"}, //nolint:goconst
-				},
+	Context("When reconciling a node", func() {
+		It("should successfully reconcile the resource", func() {
+			req := ctrl.Request{
+				NamespacedName: types.NamespacedName{Name: nodeName},
 			}
-			Expect(k8sClient.Create(ctx, resource)).To(Succeed())
-		})
-
-		AfterEach(func() {
-			node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}
-			By("Cleanup the specific node")
-			Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, node))).To(Succeed())
-		})
 
-		It("should successfully reconcile the resource", func() {
 			By("Reconciling the created resource")
-			_, err := reconcileNodeLoop(1)
+			_, err := maintenanceController.Reconcile(ctx, req)
 			Expect(err).NotTo(HaveOccurred())
 		})
 	})

internal/controller/onboarding_controller_test.go

@@ -20,14 +20,15 @@ package controller
 import (
 	"context"
 
-	kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1"
 	"github.com/gophercloud/gophercloud/v2/testhelper"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1"
 )
 
 var _ = Describe("Onboarding Controller", func() {