GardenerNodeLifecycle: Base reconcile on Hypervisor CRD, not Node

api/v1/hypervisor_types.go

@@ -60,7 +60,9 @@ const (
 	// ConditionTypeReady is the type of condition for ready status of a hypervisor
 	ConditionTypeReady = "Ready"
 
-	// ConditionTypeTerminating is the type of condition for terminating status of a hypervisor
+	// ConditionTypeTerminating is set when the underlying Node is terminating —
+	// either because the legacy gardener node condition "Terminating" is present,
+	// or because the Node's deletionTimestamp has been set (gardener's future path).
 	ConditionTypeTerminating = "Terminating"
 
 	// ConditionTypeTainted is the type of condition for tainted status of a hypervisor

charts/openstack-hypervisor-operator/templates/role.yaml

@@ -12,7 +12,6 @@ rules:
   - get
   - list
   - patch
-  - update
   - watch
 - apiGroups:
   - ""

internal/controller/gardener_node_lifecycle_controller.go

@@ -33,13 +33,9 @@ import (
 	v1 "k8s.io/client-go/applyconfigurations/meta/v1"
 	policyv1ac "k8s.io/client-go/applyconfigurations/policy/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
-	"sigs.k8s.io/controller-runtime/pkg/builder"
 	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
-	"sigs.k8s.io/controller-runtime/pkg/event"
-	"sigs.k8s.io/controller-runtime/pkg/handler"
 	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"
 )
@@ -52,34 +48,23 @@ type GardenerNodeLifecycleController struct {
 
 const (
 	labelCriticalComponent    = "node.gardener.cloud/critical-component"
-	valueReasonTerminating    = "terminating"
 	MaintenanceControllerName = "maintenance"
 )
 
 // 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;watch
+// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors,verbs=get;list;watch;update;patch
 // +kubebuilder:rbac:groups="apps",resources=deployments,verbs=create;delete;get;list;patch;update;watch
 // +kubebuilder:rbac:groups="policy",resources=poddisruptionbudgets,verbs=create;delete;get;list;patch;update;watch
 
 func (r *GardenerNodeLifecycleController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := logger.FromContext(ctx).WithName(req.Name)
 	ctx = logger.IntoContext(ctx, log)
 
-	node := &corev1.Node{}
-	if err := r.Get(ctx, req.NamespacedName, node); err != nil {
-		// ignore not found errors, could be deleted
-		return ctrl.Result{}, k8sclient.IgnoreNotFound(err)
-	}
-
 	hv := &kvmv1.Hypervisor{}
-	if err := r.Get(ctx, k8sclient.ObjectKey{Name: req.Name}, hv); err != nil {
-		if k8sclient.IgnoreNotFound(err) != nil {
-			return ctrl.Result{}, err
-		}
-		// Hypervisor not found, nothing to do
-		return ctrl.Result{}, nil
+	if err := r.Get(ctx, req.NamespacedName, hv); err != nil {
+		return ctrl.Result{}, k8sclient.IgnoreNotFound(err)
 	}
 
 	if !hv.Spec.LifecycleEnabled {
@@ -91,39 +76,48 @@ func (r *GardenerNodeLifecycleController) Reconcile(ctx context.Context, req ctr
 	// to avoid racing with live-migration.
 	if hv.Spec.Maintenance == kvmv1.MaintenanceTermination &&
 		meta.IsStatusConditionFalse(hv.Status.Conditions, kvmv1.ConditionTypeEvicting) {
-		patched, err := r.ensureOffboardingTaint(ctx, node)
-		if err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to ensure offboarding taint: %w", err)
-		}
-		if patched {
-			return ctrl.Result{}, nil
+		node := &corev1.Node{}
+		if err := r.Get(ctx, k8sclient.ObjectKey{Name: hv.Name}, node); err != nil {
+			if k8sclient.IgnoreNotFound(err) != nil {
+				return ctrl.Result{}, err
+			}
+			// Node already gone, nothing to taint
+		} else {
+			patched, err := r.ensureOffboardingTaint(ctx, node)
+			if err != nil {
+				return ctrl.Result{}, fmt.Errorf("failed to ensure offboarding taint: %w", err)
+			}
+			if patched {
+				return ctrl.Result{}, nil
+			}
 		}
 	}
 
-	// We do not care about the particular value, as long as it isn't an error
-	var minAvailable int32 = 1
+	var minAvailable int32 = 0
+	if !meta.IsStatusConditionFalse(hv.Status.Conditions, kvmv1.ConditionTypeEvicting) {
+		// Evicting condition is either not present or is true (i.e. ongoing)
+		minAvailable = 1 // Do not allow draining of the pod
+	}
 
-	// Onboarding is not in progress anymore, i.e. the host is onboarded
-	onboardingCompleted := meta.IsStatusConditionFalse(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding)
-	// Evicting is not in progress anymore, i.e. the host is empty
 	offboarded := meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeOffboarded)
-
 	if offboarded {
 		minAvailable = 0
 
-		if onboardingCompleted && isTerminating(node) {
+		onboardingCompleted := meta.IsStatusConditionFalse(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding)
+		if onboardingCompleted && hv.Spec.Maintenance == kvmv1.MaintenanceTermination {
 			// Wait for HypervisorInstanceHa controller to disable HA
 			if !meta.IsStatusConditionFalse(hv.Status.Conditions, kvmv1.ConditionTypeHaEnabled) {
 				return ctrl.Result{}, nil // Will be reconciled again when condition changes
 			}
 		}
 	}
 
-	if err := r.ensureBlockingPodDisruptionBudget(ctx, node, minAvailable); err != nil {
+	if err := r.ensureBlockingPodDisruptionBudget(ctx, hv, minAvailable); err != nil {
 		return ctrl.Result{}, err
 	}
 
-	if err := r.ensureSignallingDeployment(ctx, node, minAvailable, onboardingCompleted); err != nil {
+	onboardingCompleted := meta.IsStatusConditionFalse(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding)
+	if err := r.ensureSignallingDeployment(ctx, hv, minAvailable, onboardingCompleted); err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -154,53 +148,37 @@ func (r *GardenerNodeLifecycleController) ensureOffboardingTaint(ctx context.Con
 	return true, r.Patch(ctx, node, patch, k8sclient.FieldOwner(MaintenanceControllerName))
 }
 
-func (r *GardenerNodeLifecycleController) ensureBlockingPodDisruptionBudget(ctx context.Context, node *corev1.Node, minAvailable int32) error {
-	name := nameForNode(node)
-	nodeLabels := labelsForNode(node)
-	gvk, err := apiutil.GVKForObject(node, r.Scheme)
+func (r *GardenerNodeLifecycleController) ensureBlockingPodDisruptionBudget(ctx context.Context, hypervisor *kvmv1.Hypervisor, minAvailable int32) error {
+	name := nameForHypervisor(hypervisor)
+	nodeLabels := labelsForHypervisor(hypervisor)
+	gvk, err := apiutil.GVKForObject(hypervisor, r.Scheme)
 	if err != nil {
 		return err
 	}
 
 	podDisruptionBudget := policyv1ac.PodDisruptionBudget(name, MaintenanceNamespace).
 		WithLabels(nodeLabels).
-		WithOwnerReferences(OwnerReference(node, &gvk)).
+		WithOwnerReferences(OwnerReference(hypervisor, &gvk)).
 		WithSpec(policyv1ac.PodDisruptionBudgetSpec().
 			WithMinAvailable(intstr.FromInt32(minAvailable)).
 			WithSelector(v1.LabelSelector().WithMatchLabels(nodeLabels)))
 
 	return r.Apply(ctx, podDisruptionBudget, k8sclient.FieldOwner(MaintenanceControllerName))
 }
 
-func isTerminating(node *corev1.Node) bool {
-	conditions := node.Status.Conditions
-	if conditions == nil {
-		return false
-	}
-
-	// See: https://github.com/gardener/machine-controller-manager/blob/rel-v0.56/pkg/util/provider/machinecontroller/machine.go#L658-L659
-	for _, condition := range conditions {
-		if condition.Type == "Terminating" {
-			return true
-		}
-	}
-
-	return false
-}
-
-func nameForNode(node *corev1.Node) string {
-	return fmt.Sprintf("maint-%v", node.Name)
+func nameForHypervisor(hypervisor *kvmv1.Hypervisor) string {
+	return fmt.Sprintf("maint-%v", hypervisor.Name)
 }
 
-func labelsForNode(node *corev1.Node) map[string]string {
+func labelsForHypervisor(hypervisor *kvmv1.Hypervisor) map[string]string {
 	return map[string]string{
-		MaintenanceLabelKey: nameForNode(node),
+		MaintenanceLabelKey: nameForHypervisor(hypervisor),
 	}
 }
 
-func (r *GardenerNodeLifecycleController) ensureSignallingDeployment(ctx context.Context, node *corev1.Node, scale int32, ready bool) error {
-	name := nameForNode(node)
-	labels := labelsForNode(node)
+func (r *GardenerNodeLifecycleController) ensureSignallingDeployment(ctx context.Context, hypervisor *kvmv1.Hypervisor, scale int32, ready bool) error {
+	name := nameForHypervisor(hypervisor)
+	labels := labelsForHypervisor(hypervisor)
 
 	podLabels := maps.Clone(labels)
 	podLabels[labelCriticalComponent] = "true"
@@ -212,13 +190,13 @@ func (r *GardenerNodeLifecycleController) ensureSignallingDeployment(ctx context
 		command = "/bin/false"
 	}
 
-	gvk, err := apiutil.GVKForObject(node, r.Scheme)
+	gvk, err := apiutil.GVKForObject(hypervisor, r.Scheme)
 	if err != nil {
 		return err
 	}
 
 	deployment := apps1ac.Deployment(name, MaintenanceNamespace).
-		WithOwnerReferences(OwnerReference(node, &gvk)).
+		WithOwnerReferences(OwnerReference(hypervisor, &gvk)).
 		WithLabels(labels).
 		WithSpec(apps1ac.DeploymentSpec().
 			WithReplicas(scale).
@@ -233,7 +211,7 @@ func (r *GardenerNodeLifecycleController) ensureSignallingDeployment(ctx context
 				WithSpec(corev1ac.PodSpec().
 					WithHostNetwork(true).
 					WithNodeSelector(map[string]string{
-						corev1.LabelHostname: node.Labels[corev1.LabelHostname],
+						corev1.LabelHostname: hypervisor.Labels[corev1.LabelHostname],
 					}).
 					WithTerminationGracePeriodSeconds(1).
 					WithTolerations(
@@ -265,48 +243,10 @@ func (r *GardenerNodeLifecycleController) SetupWithManager(mgr ctrl.Manager, nam
 	_ = logger.FromContext(ctx)
 	r.namespace = namespace
 
-	hypervisorToNode := handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &corev1.Node{})
-
-	// Maintenance=termination bumps generation; Evicting status changes do not.
-	hypervisorRelevantChange := predicate.Or(
-		predicate.GenerationChangedPredicate{},
-		evictingConditionChangedPredicate{},
-	)
-
 	return ctrl.NewControllerManagedBy(mgr).
 		Named(MaintenanceControllerName).
-		For(&corev1.Node{}).
-		Watches(&kvmv1.Hypervisor{}, hypervisorToNode,
-			builder.WithPredicates(hypervisorRelevantChange),
-		).
+		For(&kvmv1.Hypervisor{}).
 		Owns(&appsv1.Deployment{}).
 		Owns(&policyv1.PodDisruptionBudget{}).
 		Complete(r)
 }
-
-// evictingConditionChangedPredicate complements GenerationChangedPredicate,
-// which ignores status-only updates.
-type evictingConditionChangedPredicate struct {
-	predicate.Funcs
-}
-
-func (evictingConditionChangedPredicate) Update(e event.UpdateEvent) bool {
-	if e.ObjectOld == nil || e.ObjectNew == nil {
-		return false
-	}
-	oldHv, ok1 := e.ObjectOld.(*kvmv1.Hypervisor)
-	newHv, ok2 := e.ObjectNew.(*kvmv1.Hypervisor)
-	if !ok1 || !ok2 {
-		return false
-	}
-	oldCond := meta.FindStatusCondition(oldHv.Status.Conditions, kvmv1.ConditionTypeEvicting)
-	newCond := meta.FindStatusCondition(newHv.Status.Conditions, kvmv1.ConditionTypeEvicting)
-	switch {
-	case oldCond == nil && newCond == nil:
-		return false
-	case oldCond == nil || newCond == nil:
-		return true
-	default:
-		return oldCond.Status != newCond.Status
-	}
-}

internal/controller/gardener_node_lifecycle_controller_test.go

@@ -58,7 +58,7 @@ var _ = Describe("Gardener Maintenance Controller", func() {
 		Expect(k8sClient.Create(ctx, node)).To(Succeed())
 		DeferCleanup(func(ctx SpecContext) {
 			By("Cleanup the specific node")
-			Expect(k8sClient.Delete(ctx, node)).To(Succeed())
+			Expect(k8sclient.IgnoreNotFound(k8sClient.Delete(ctx, node))).To(Succeed())
 		})
 
 		By("creating the core resource for the Kind hypervisor")
@@ -116,6 +116,26 @@ var _ = Describe("Gardener Maintenance Controller", func() {
 			})
 		})
 
+		When("evicting has completed (Evicting=False)", func() {
+			BeforeEach(func(ctx SpecContext) {
+				hypervisor := &kvmv1.Hypervisor{}
+				Expect(k8sClient.Get(ctx, name, hypervisor)).To(Succeed())
+				meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
+					Type:    kvmv1.ConditionTypeEvicting,
+					Status:  metav1.ConditionFalse,
+					Reason:  "Succeeded",
+					Message: "All VMs evicted",
+				})
+				Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
+			})
+
+			It("should set the pdb to minAvailable 0 without waiting for Offboarded", func(ctx SpecContext) {
+				pdb := &policyv1.PodDisruptionBudget{}
+				Expect(k8sClient.Get(ctx, maintenanceName, pdb)).To(Succeed())
+				Expect(pdb.Spec.MinAvailable).To(HaveField("IntVal", BeNumerically("==", int32(0))))
+			})
+		})
+
 		When("the node has been offboarded", func() {
 			BeforeEach(func(ctx SpecContext) {
 				hypervisor := &kvmv1.Hypervisor{}
@@ -150,6 +170,17 @@ var _ = Describe("Gardener Maintenance Controller", func() {
 		})
 	})
 
+	Context("When node does not exist", func() {
+		It("should succeed without error", func(ctx SpecContext) {
+			node := &corev1.Node{}
+			Expect(k8sClient.Get(ctx, name, node)).To(Succeed())
+			Expect(k8sClient.Delete(ctx, node)).To(Succeed())
+
+			_, err := controller.Reconcile(ctx, reconcileReq)
+			Expect(err).NotTo(HaveOccurred())
+		})
+	})
+
 	Context("When lifecycle is not enabled", func() {
 		BeforeEach(func(ctx SpecContext) {
 			hypervisor := &kvmv1.Hypervisor{}
@@ -164,31 +195,30 @@ var _ = Describe("Gardener Maintenance Controller", func() {
 		})
 	})
 
-	Context("When node is terminating and offboarded", func() {
+	Context("When hypervisor is in termination maintenance and offboarded", func() {
 		BeforeEach(func(ctx SpecContext) {
-			// Set node as terminating and add required labels for disableInstanceHA
-			node := &corev1.Node{}
-			Expect(k8sClient.Get(ctx, name, node)).To(Succeed())
-			node.Labels = map[string]string{
-				corev1.LabelHostname:          nodeName,
-				"topology.kubernetes.io/zone": "test-zone",
-			}
-			node.Status.Conditions = append(node.Status.Conditions, corev1.NodeCondition{
-				Type:   "Terminating",
-				Status: corev1.ConditionTrue,
-			})
-			Expect(k8sClient.Update(ctx, node)).To(Succeed())
-			Expect(k8sClient.Status().Update(ctx, node)).To(Succeed())
-
-			// Set hypervisor as onboarded and offboarded
 			hypervisor := &kvmv1.Hypervisor{}
 			Expect(k8sClient.Get(ctx, name, hypervisor)).To(Succeed())
+			hypervisor.Spec.Maintenance = kvmv1.MaintenanceTermination
+			Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())
+			meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
+				Type:    kvmv1.ConditionTypeEvicting,
+				Status:  metav1.ConditionFalse,
+				Reason:  "Succeeded",
+				Message: "All VMs evicted",
+			})
 			meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
 				Type:    kvmv1.ConditionTypeOnboarding,
 				Status:  metav1.ConditionFalse,
 				Reason:  "Onboarded",
 				Message: "Onboarding completed",
 			})
+			meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
+				Type:    kvmv1.ConditionTypeHaEnabled,
+				Status:  metav1.ConditionFalse,
+				Reason:  "Evicted",
+				Message: "HA disabled",
+			})
 			meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
 				Type:    kvmv1.ConditionTypeOffboarded,
 				Status:  metav1.ConditionTrue,
@@ -199,8 +229,12 @@ var _ = Describe("Gardener Maintenance Controller", func() {
 		})
 
 		It("should allow pod eviction by setting the PDB to minAvailable 0", func(ctx SpecContext) {
+			// First reconcile applies the offboarding taint and returns early.
 			_, err := controller.Reconcile(ctx, reconcileReq)
 			Expect(err).NotTo(HaveOccurred())
+			// Second reconcile proceeds to update the PDB.
+			_, err = controller.Reconcile(ctx, reconcileReq)
+			Expect(err).NotTo(HaveOccurred())
 
 			pdb := &policyv1.PodDisruptionBudget{}
 			Expect(k8sClient.Get(ctx, maintenanceName, pdb)).To(Succeed())