GardenerNodeLifecycleController: add timeout for HA-disabled wait

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

@@ -21,6 +21,7 @@ import (
 	"context"
 	"fmt"
 	"maps"
+	"time"
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -32,58 +33,63 @@ import (
 	corev1ac "k8s.io/client-go/applyconfigurations/core/v1"
 	v1 "k8s.io/client-go/applyconfigurations/meta/v1"
 	policyv1ac "k8s.io/client-go/applyconfigurations/policy/v1"
+	"k8s.io/utils/clock"
 	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"
 )
 
+const defaultHaDisabledTimeout = 15 * time.Minute
+
 type GardenerNodeLifecycleController struct {
 	k8sclient.Client
 	Scheme    *runtime.Scheme
 	namespace string
+	// HaDisabledTimeout is the maximum time to wait for ConditionTypeHaEnabled to
+	// become False after the node is offboarded, measured from the lastTransitionTime
+	// of the ConditionTypeOffboarded condition. After the deadline, Unknown and unset
+	// are also accepted. Defaults to defaultHaDisabledTimeout when zero.
+	HaDisabledTimeout time.Duration
+	// Clock is used to determine the current time. Defaults to clock.RealClock{}.
+	Clock clock.Clock
 }
 
 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
+	hv := &kvmv1.Hypervisor{}
+	if err := r.Get(ctx, req.NamespacedName, hv); err != nil {
 		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
+	if !hv.Spec.LifecycleEnabled {
+		// Nothing to be done
 		return ctrl.Result{}, nil
 	}
 
-	if !hv.Spec.LifecycleEnabled {
-		// Nothing to be done
+	// Fetch the node to apply the offboarding taint if needed.
+	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 not found yet, nothing to do
 		return ctrl.Result{}, nil
 	}
 
@@ -100,30 +106,47 @@ func (r *GardenerNodeLifecycleController) Reconcile(ctx context.Context, req ctr
 		}
 	}
 
-	// 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
 
+		onboardingCompleted := meta.IsStatusConditionFalse(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding)
 		if onboardingCompleted && isTerminating(node) {
-			// Wait for HypervisorInstanceHa controller to disable HA
+			// Wait for HypervisorInstanceHa controller to disable HA.
+			// After the deadline (measured from the lastTransitionTime of the
+			// Offboarded condition) we also accept Unknown and unset, so that a
+			// stalled HA controller cannot block node termination indefinitely.
 			if !meta.IsStatusConditionFalse(hv.Status.Conditions, kvmv1.ConditionTypeHaEnabled) {
-				return ctrl.Result{}, nil // Will be reconciled again when condition changes
+				offboardedCondition := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeOffboarded)
+				timeout := r.HaDisabledTimeout
+				if timeout == 0 {
+					timeout = defaultHaDisabledTimeout
+				}
+				now := r.Clock.Now()
+				offboardedAt := offboardedCondition.LastTransitionTime.Time
+				deadline := offboardedAt.Add(timeout)
+				if now.Before(deadline) {
+					return ctrl.Result{RequeueAfter: deadline.Sub(now)}, nil
+				}
+				log.Info("HA disabled timeout exceeded, proceeding without waiting for HaEnabled=False",
+					"timeout", timeout,
+					"offboardedAt", offboardedAt)
 			}
 		}
 	}
 
-	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,24 +177,6 @@ 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)
-	if err != nil {
-		return err
-	}
-
-	podDisruptionBudget := policyv1ac.PodDisruptionBudget(name, MaintenanceNamespace).
-		WithLabels(nodeLabels).
-		WithOwnerReferences(OwnerReference(node, &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 {
@@ -184,23 +189,40 @@ func isTerminating(node *corev1.Node) bool {
 			return true
 		}
 	}
-
 	return false
 }
 
-func nameForNode(node *corev1.Node) string {
-	return fmt.Sprintf("maint-%v", node.Name)
+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(hypervisor, &gvk)).
+		WithSpec(policyv1ac.PodDisruptionBudgetSpec().
+			WithMinAvailable(intstr.FromInt32(minAvailable)).
+			WithSelector(v1.LabelSelector().WithMatchLabels(nodeLabels)))
+
+	return r.Apply(ctx, podDisruptionBudget, k8sclient.FieldOwner(MaintenanceControllerName))
 }
 
-func labelsForNode(node *corev1.Node) map[string]string {
+func nameForHypervisor(hypervisor *kvmv1.Hypervisor) string {
+	return fmt.Sprintf("maint-%v", hypervisor.Name)
+}
+
+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 +234,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 +255,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(
@@ -264,49 +286,14 @@ func (r *GardenerNodeLifecycleController) SetupWithManager(mgr ctrl.Manager, nam
 	ctx := context.Background()
 	_ = 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{},
-	)
+	if r.Clock == nil {
+		r.Clock = clock.RealClock{}
+	}
 
 	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
-	}
-}