diff --git a/charts/openstack-hypervisor-operator/templates/role.yaml b/charts/openstack-hypervisor-operator/templates/role.yaml index 3b1ecda..60dca46 100644 --- a/charts/openstack-hypervisor-operator/templates/role.yaml +++ b/charts/openstack-hypervisor-operator/templates/role.yaml @@ -12,7 +12,6 @@ rules: - get - list - patch - - update - watch - apiGroups: - "" diff --git a/internal/controller/gardener_node_lifecycle_controller.go b/internal/controller/gardener_node_lifecycle_controller.go index 3cd803b..bfb9ea0 100644 --- a/internal/controller/gardener_node_lifecycle_controller.go +++ b/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,34 +33,39 @@ 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 @@ -67,23 +73,23 @@ func (r *GardenerNodeLifecycleController) Reconcile(ctx context.Context, req ctr 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 - } -} diff --git a/internal/controller/gardener_node_lifecycle_controller_test.go b/internal/controller/gardener_node_lifecycle_controller_test.go index ace31b8..eabe473 100644 --- a/internal/controller/gardener_node_lifecycle_controller_test.go +++ b/internal/controller/gardener_node_lifecycle_controller_test.go @@ -19,6 +19,7 @@ package controller import ( "fmt" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -28,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/clock" ctrl "sigs.k8s.io/controller-runtime" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -47,6 +49,7 @@ var _ = Describe("Gardener Maintenance Controller", func() { controller = &GardenerNodeLifecycleController{ Client: k8sClient, Scheme: k8sClient.Scheme(), + Clock: clock.RealClock{}, } By("creating the core resource for the Kind Node") @@ -150,6 +153,84 @@ var _ = Describe("Gardener Maintenance Controller", func() { }) }) + Context("setting the pod disruption budget", func() { + JustBeforeEach(func(ctx SpecContext) { + _, err := controller.Reconcile(ctx, reconcileReq) + Expect(err).NotTo(HaveOccurred()) + }) + + When("not evicted", func() { + It("should set the pdb to minimum 1", func(ctx SpecContext) { + pdb := &policyv1.PodDisruptionBudget{} + Expect(k8sClient.Get(ctx, maintenanceName, pdb)).To(Succeed()) + Expect(pdb.Spec.MinAvailable).To(HaveValue(HaveField("IntVal", BeEquivalentTo(1)))) + }) + }) + When("evicted", func() { + BeforeEach(func(ctx SpecContext) { + hv := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, name, hv)).To(Succeed()) + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeEvicting, + Status: metav1.ConditionFalse, + Reason: "dontcare", + Message: "dontcare", + }) + Expect(k8sClient.Status().Update(ctx, hv)).To(Succeed()) + }) + + It("should set the pdb to minimum 0", func(ctx SpecContext) { + pdb := &policyv1.PodDisruptionBudget{} + Expect(k8sClient.Get(ctx, maintenanceName, pdb)).To(Succeed()) + Expect(pdb.Spec.MinAvailable).To(HaveValue(HaveField("IntVal", BeEquivalentTo(0)))) + }) + }) + }) + + Context("create a signalling deployment", func() { + JustBeforeEach(func(ctx SpecContext) { + _, err := controller.Reconcile(ctx, reconcileReq) + Expect(err).NotTo(HaveOccurred()) + }) + + When("onboarding not completed", func() { + It("should create a failing deployment for the node", func(ctx SpecContext) { + deployment := &appsv1.Deployment{} + Expect(k8sClient.Get(ctx, maintenanceName, deployment)).To(Succeed()) + Expect(deployment.Spec.Template.Spec.Containers).To( + ContainElement( + HaveField("StartupProbe", + HaveField("ProbeHandler", + HaveField("Exec", + HaveField("Command", ContainElements("/bin/false"))))))) + }) + }) + When("onboarding is completed", func() { + BeforeEach(func(ctx SpecContext) { + hv := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, name, hv)).To(Succeed()) + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOnboarding, + Status: metav1.ConditionFalse, + Reason: "dontcare", + Message: "dontcare", + }) + Expect(k8sClient.Status().Update(ctx, hv)).To(Succeed()) + }) + + It("should create a succeeding deployment for the node", func(ctx SpecContext) { + deployment := &appsv1.Deployment{} + Expect(k8sClient.Get(ctx, maintenanceName, deployment)).To(Succeed()) + Expect(deployment.Spec.Template.Spec.Containers).To( + ContainElement( + HaveField("StartupProbe", + HaveField("ProbeHandler", + HaveField("Exec", + HaveField("Command", ContainElements("/bin/true"))))))) + }) + }) + }) + Context("When lifecycle is not enabled", func() { BeforeEach(func(ctx SpecContext) { hypervisor := &kvmv1.Hypervisor{} @@ -166,18 +247,21 @@ var _ = Describe("Gardener Maintenance Controller", func() { Context("When node is terminating and offboarded", func() { BeforeEach(func(ctx SpecContext) { - // Set node as terminating and add required labels for disableInstanceHA + // Set node labels (spec/metadata update) 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", } + Expect(k8sClient.Update(ctx, node)).To(Succeed()) + // Set node Terminating condition separately via the status subresource, + // using a fresh Get to avoid the spec Update overwriting the status. + Expect(k8sClient.Get(ctx, name, node)).To(Succeed()) 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 @@ -198,13 +282,70 @@ var _ = Describe("Gardener Maintenance Controller", func() { Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) }) - It("should allow pod eviction by setting the PDB to minAvailable 0", func(ctx SpecContext) { - _, err := controller.Reconcile(ctx, reconcileReq) - Expect(err).NotTo(HaveOccurred()) + When("HaEnabled is explicitly 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.ConditionTypeHaEnabled, + Status: metav1.ConditionFalse, + Reason: "Evicted", + Message: "HA disabled due to eviction", + }) + Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) + }) - pdb := &policyv1.PodDisruptionBudget{} - Expect(k8sClient.Get(ctx, maintenanceName, pdb)).To(Succeed()) - Expect(pdb.Spec.MinAvailable).To(HaveField("IntVal", BeNumerically("==", int32(0)))) + It("should allow pod eviction immediately by setting the PDB to minAvailable 0", func(ctx SpecContext) { + result, err := controller.Reconcile(ctx, reconcileReq) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeZero()) + + pdb := &policyv1.PodDisruptionBudget{} + Expect(k8sClient.Get(ctx, maintenanceName, pdb)).To(Succeed()) + Expect(pdb.Spec.MinAvailable).To(HaveField("IntVal", BeNumerically("==", int32(0)))) + }) + }) + + When("HaEnabled is not yet False and the timeout has not elapsed", func() { + BeforeEach(func() { + // LastTransitionTime ≈ now (set by meta.SetStatusCondition above), + // so deadline = now + 1h is in the future. + controller.HaDisabledTimeout = time.Hour + }) + + It("should requeue and not proceed", func(ctx SpecContext) { + result, err := controller.Reconcile(ctx, reconcileReq) + Expect(err).NotTo(HaveOccurred()) + // Should requeue before the deadline rather than returning immediately. + Expect(result.RequeueAfter).To(BeNumerically(">", 0)) + }) + }) + + When("HaEnabled is not False but the timeout has elapsed", func() { + BeforeEach(func(ctx SpecContext) { + // Push LastTransitionTime 2h into the past so that + // deadline = (now - 2h) + 1h = now - 1h, which has already elapsed. + hypervisor := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, name, hypervisor)).To(Succeed()) + for i := range hypervisor.Status.Conditions { + if hypervisor.Status.Conditions[i].Type == kvmv1.ConditionTypeOffboarded { + hypervisor.Status.Conditions[i].LastTransitionTime = metav1.NewTime(time.Now().Add(-2 * time.Hour)) + break + } + } + Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed()) + controller.HaDisabledTimeout = time.Hour + }) + + It("should allow pod eviction by setting the PDB to minAvailable 0", func(ctx SpecContext) { + result, err := controller.Reconcile(ctx, reconcileReq) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(BeZero()) + + pdb := &policyv1.PodDisruptionBudget{} + Expect(k8sClient.Get(ctx, maintenanceName, pdb)).To(Succeed()) + Expect(pdb.Spec.MinAvailable).To(HaveField("IntVal", BeNumerically("==", int32(0)))) + }) }) })