diff --git a/api/hypershift/v1beta1/nodepool_types.go b/api/hypershift/v1beta1/nodepool_types.go index dc17f42ee9e6..280f4e1faf03 100644 --- a/api/hypershift/v1beta1/nodepool_types.go +++ b/api/hypershift/v1beta1/nodepool_types.go @@ -29,6 +29,20 @@ const ( // NodePoolReleaseVersionAnnotation is set on userdata Secrets (Replace strategy) and on Machines (InPlace strategy) // to track the OCP release version associated with each machine. NodePoolReleaseVersionAnnotation = "hypershift.openshift.io/release-version" + + // NodeScaleDownAnnotation is applied to Nodes in the hosted cluster by users. + // When set to exactly "true" (case-sensitive), the HCCO Node Controller sets + // cluster.x-k8s.io/delete-machine=yes on the corresponding Machine, giving it + // top priority for deletion during NodePool scale-down. + // + // This annotation is a deletion-priority hint, not a trigger: the annotated + // Machine is only removed when a scale-down actually occurs (e.g., the NodePool + // replica count is reduced or the cluster autoscaler decides to shrink the pool). + // + // Removing this annotation from the Node causes the controller to remove the + // delete-machine annotation from the Machine, allowing the user to change + // their mind before scale-down occurs. + NodeScaleDownAnnotation = "hypershift.openshift.io/scale-down" ) // ImageType specifies the type of image to use for node instances. diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/node/node.go b/control-plane-operator/hostedclusterconfigoperator/controllers/node/node.go index d43a6d48e7d6..f9338a3c53e8 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/node/node.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/node/node.go @@ -47,29 +47,33 @@ func (r *reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return ctrl.Result{}, fmt.Errorf("failed to get Node: %w", err) } - var apiErr *apierrors.StatusError - nodePoolName, err := r.nodeToNodePoolName(ctx, node) + machine, err := r.getMachineForNode(ctx, node) if err != nil { + var apiErr *apierrors.StatusError if errors.As(err, &apiErr) && !apierrors.IsNotFound(err) { - // Return error and retry only if the API interaction failed. Other errors are because the nodeToNodePoolName expected - // annotations are not in place yet, so we'll reconcile triggered by the event which sets them in the Node. return ctrl.Result{}, err - } else { - log.Error(err, "failed to get nodePool name from Node") - return ctrl.Result{}, nil } + log.Error(err, "failed to get Machine for Node, CAPI annotations may not be set yet") + return ctrl.Result{}, nil + } + + // Must run before labelsHaveSynced: scale-down annotation sync applies to all nodes, not just unsynchronized ones. + if err := r.reconcileDeleteMachineAnnotation(ctx, node, machine); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to reconcile delete-machine annotation: %w", err) } if labelsHaveSynced(node) { return reconcile.Result{}, nil } - machine, err := r.getMachineForNode(ctx, node) - if err != nil { - return reconcile.Result{}, err + nodePoolName, ok := machine.Annotations[nodePoolAnnotation] + if !ok || nodePoolName == "" { + log.Info("Missing nodePoolAnnotation on Machine, skipping label sync", "machine", machine.Name) + return ctrl.Result{}, nil } + labelsToSync := getManagedLabels(machine.Labels) - labelsToSync[hyperv1.NodePoolLabel] = nodePoolName + labelsToSync[hyperv1.NodePoolLabel] = supportutil.ParseNamespacedName(nodePoolName).Name var taints []corev1.Taint taintsInJSON := machine.Annotations[nodePoolAnnotationTaints] @@ -99,6 +103,38 @@ func (r *reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return reconcile.Result{}, nil } +func (r *reconciler) reconcileDeleteMachineAnnotation(ctx context.Context, node *corev1.Node, machine *capiv1.Machine) error { + if machine.DeletionTimestamp != nil { + return nil + } + + nodeWantsScaleDown := node.Annotations[hyperv1.NodeScaleDownAnnotation] == "true" + _, machineHasDelete := machine.Annotations[capiv1.DeleteMachineAnnotation] + + if nodeWantsScaleDown && !machineHasDelete { + machineToPatch := machine.DeepCopy() + patch := client.MergeFrom(machine.DeepCopy()) + if machineToPatch.Annotations == nil { + machineToPatch.Annotations = make(map[string]string) + } + machineToPatch.Annotations[capiv1.DeleteMachineAnnotation] = "yes" + if err := r.client.Patch(ctx, machineToPatch, patch); err != nil { + return fmt.Errorf("failed to set delete-machine annotation on Machine %s/%s: %w", machine.Namespace, machine.Name, err) + } + ctrl.LoggerFrom(ctx).Info("Set delete-machine annotation on Machine", "machine", machine.Name) + } else if !nodeWantsScaleDown && machineHasDelete { + machineToPatch := machine.DeepCopy() + patch := client.MergeFrom(machine.DeepCopy()) + delete(machineToPatch.Annotations, capiv1.DeleteMachineAnnotation) + if err := r.client.Patch(ctx, machineToPatch, patch); err != nil { + return fmt.Errorf("failed to remove delete-machine annotation from Machine %s/%s: %w", machine.Namespace, machine.Name, err) + } + ctrl.LoggerFrom(ctx).Info("Removed delete-machine annotation from Machine", "machine", machine.Name) + } + + return nil +} + func getManagedLabels(labels map[string]string) map[string]string { managedLabels := make(map[string]string) for k, v := range labels { @@ -142,16 +178,3 @@ func labelsHaveSynced(node *corev1.Node) bool { return false } -func (r *reconciler) nodeToNodePoolName(ctx context.Context, node *corev1.Node) (string, error) { - machine, err := r.getMachineForNode(ctx, node) - if err != nil { - return "", err - } - - nodePoolName, ok := machine.Annotations[nodePoolAnnotation] - if !ok || nodePoolName == "" { - return "", fmt.Errorf("failed to find nodePoolAnnotation on Machine %q", machine.Name) - } - - return supportutil.ParseNamespacedName(nodePoolName).Name, nil -} diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/node/node_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/node/node_test.go index eddda9f6fd89..d6729c4cde24 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/node/node_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/node/node_test.go @@ -1,10 +1,15 @@ package node import ( + "context" + "fmt" + "os" "testing" + "time" . "github.com/onsi/gomega" + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" supportutil "github.com/openshift/hypershift/support/util" corev1 "k8s.io/api/core/v1" @@ -12,38 +17,205 @@ import ( "k8s.io/client-go/kubernetes/scheme" capiv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) -func TestNodeToNodePoolName(t *testing.T) { +func TestMain(m *testing.M) { _ = capiv1.AddToScheme(scheme.Scheme) + os.Exit(m.Run()) +} - machineNamespace := "test" - nodePoolName := "ns/name" - machineWithNodePoolAnnotation := &capiv1.Machine{ +func TestReconcileDeleteMachineAnnotation(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + nodeAnnotations map[string]string + machineAnnotations map[string]string + machineDeletionTimestamp *metav1.Time + expectDeleteMachineOnMachine bool + }{ + { + name: "When Node has scale-down=true and Machine lacks delete-machine, it should set delete-machine on Machine", + nodeAnnotations: map[string]string{ + hyperv1.NodeScaleDownAnnotation: "true", + }, + machineAnnotations: map[string]string{}, + expectDeleteMachineOnMachine: true, + }, + { + name: "When Node lacks scale-down and Machine has delete-machine, it should remove delete-machine from Machine", + nodeAnnotations: map[string]string{}, + machineAnnotations: map[string]string{ + capiv1.DeleteMachineAnnotation: "yes", + }, + expectDeleteMachineOnMachine: false, + }, + { + name: "When Node has scale-down=true and Machine already has delete-machine, it should not modify Machine", + nodeAnnotations: map[string]string{ + hyperv1.NodeScaleDownAnnotation: "true", + }, + machineAnnotations: map[string]string{ + capiv1.DeleteMachineAnnotation: "yes", + }, + expectDeleteMachineOnMachine: true, + }, + { + name: "When neither annotation is present, it should not modify Machine", + nodeAnnotations: map[string]string{}, + machineAnnotations: map[string]string{}, + expectDeleteMachineOnMachine: false, + }, + { + name: "When Node scale-down value is not true, it should not set delete-machine", + nodeAnnotations: map[string]string{ + hyperv1.NodeScaleDownAnnotation: "false", + }, + machineAnnotations: map[string]string{}, + expectDeleteMachineOnMachine: false, + }, + { + name: "When Node scale-down value is yes, it should not set delete-machine", + nodeAnnotations: map[string]string{ + hyperv1.NodeScaleDownAnnotation: "yes", + }, + machineAnnotations: map[string]string{}, + expectDeleteMachineOnMachine: false, + }, + { + name: "When Node scale-down value is empty string, it should not set delete-machine", + nodeAnnotations: map[string]string{ + hyperv1.NodeScaleDownAnnotation: "", + }, + machineAnnotations: map[string]string{}, + expectDeleteMachineOnMachine: false, + }, + { + name: "When Node has nil annotations, it should not modify Machine", + nodeAnnotations: nil, + machineAnnotations: map[string]string{}, + expectDeleteMachineOnMachine: false, + }, + { + name: "When Node has scale-down=true and Machine has nil annotations, it should set delete-machine on Machine", + nodeAnnotations: map[string]string{ + hyperv1.NodeScaleDownAnnotation: "true", + }, + machineAnnotations: nil, + expectDeleteMachineOnMachine: true, + }, + { + name: "When Machine is being deleted, it should skip", + nodeAnnotations: map[string]string{ + hyperv1.NodeScaleDownAnnotation: "true", + }, + machineAnnotations: map[string]string{}, + machineDeletionTimestamp: &metav1.Time{Time: time.Now()}, + expectDeleteMachineOnMachine: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + machine := &capiv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-ns", + Name: "test-machine", + Annotations: tc.machineAnnotations, + DeletionTimestamp: tc.machineDeletionTimestamp, + }, + } + if tc.machineDeletionTimestamp != nil { + machine.Finalizers = []string{"test-finalizer"} + } + + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + Annotations: tc.nodeAnnotations, + }, + } + + c := fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithObjects(machine). + Build() + + r := &reconciler{client: c} + err := r.reconcileDeleteMachineAnnotation(t.Context(), node, machine) + g.Expect(err).ToNot(HaveOccurred()) + + updatedMachine := &capiv1.Machine{} + err = c.Get(t.Context(), client.ObjectKeyFromObject(machine), updatedMachine) + g.Expect(err).ToNot(HaveOccurred()) + + _, hasDeleteAnnotation := updatedMachine.Annotations[capiv1.DeleteMachineAnnotation] + g.Expect(hasDeleteAnnotation).To(Equal(tc.expectDeleteMachineOnMachine)) + }) + } +} + +func TestReconcileDeleteMachineAnnotation_PatchError(t *testing.T) { + t.Parallel() + + machine := &capiv1.Machine{ ObjectMeta: metav1.ObjectMeta{ - Namespace: machineNamespace, - Name: "hasNodePoolAnnotation", + Namespace: "test-ns", + Name: "test-machine", + Annotations: map[string]string{}, + }, + } + + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", Annotations: map[string]string{ - nodePoolAnnotation: nodePoolName, + hyperv1.NodeScaleDownAnnotation: "true", }, }, } - machineWithOutNodePoolAnnotation := &capiv1.Machine{ + + c := fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithObjects(machine). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + return fmt.Errorf("simulated patch failure") + }, + }). + Build() + + r := &reconciler{client: c} + g := NewWithT(t) + err := r.reconcileDeleteMachineAnnotation(t.Context(), node, machine) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to set delete-machine annotation")) +} + +func TestGetMachineForNode(t *testing.T) { + t.Parallel() + + machineNamespace := "test" + machine := &capiv1.Machine{ ObjectMeta: metav1.ObjectMeta{ Namespace: machineNamespace, - Name: "DoNotHaveNodePoolAnnotation", + Name: "test-machine", }, } testCases := []struct { - name string - node *corev1.Node - expectedNodePoolName string - error bool + name string + node *corev1.Node + expectError bool }{ { - name: "When MachineAnnotation does not exist in Node it should fail", + name: "When MachineAnnotation does not exist in Node, it should fail", node: &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -51,36 +223,21 @@ func TestNodeToNodePoolName(t *testing.T) { }, }, }, - expectedNodePoolName: "", - error: true, - }, - { - name: "When ClusterNamespaceAnnotation does not exist in Node it should fail", - node: &corev1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - capiv1.MachineAnnotation: machineWithNodePoolAnnotation.Name, - }, - }, - }, - expectedNodePoolName: "", - error: true, + expectError: true, }, { - name: "When nodePoolAnnotation does not exist in Machine it should fail", + name: "When ClusterNamespaceAnnotation does not exist in Node, it should fail", node: &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - capiv1.ClusterNamespaceAnnotation: machineNamespace, - capiv1.MachineAnnotation: machineWithOutNodePoolAnnotation.Name, + capiv1.MachineAnnotation: machine.Name, }, }, }, - expectedNodePoolName: "", - error: true, + expectError: true, }, { - name: "When Machine does not exist it should fail", + name: "When Machine does not exist, it should fail", node: &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -89,39 +246,47 @@ func TestNodeToNodePoolName(t *testing.T) { }, }, }, - expectedNodePoolName: "", - error: true, + expectError: true, }, { - name: "When all annotations and Machine exist it should return the NodePool Name", + name: "When all annotations and Machine exist, it should return the Machine", node: &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ capiv1.ClusterNamespaceAnnotation: machineNamespace, - capiv1.MachineAnnotation: machineWithNodePoolAnnotation.Name, + capiv1.MachineAnnotation: machine.Name, }, }, }, - expectedNodePoolName: supportutil.ParseNamespacedName(nodePoolName).Name, - error: false, + expectError: false, }, } - c := fake.NewClientBuilder().WithObjects(machineWithNodePoolAnnotation, machineWithOutNodePoolAnnotation).Build() - r := &reconciler{ - client: c, - } + c := fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithObjects(machine). + Build() + r := &reconciler{client: c} + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() g := NewWithT(t) - got, err := r.nodeToNodePoolName(t.Context(), tc.node) - g.Expect(got).To(Equal(tc.expectedNodePoolName)) - g.Expect(err != nil).To(Equal(tc.error)) + got, err := r.getMachineForNode(t.Context(), tc.node) + if tc.expectError { + g.Expect(err).To(HaveOccurred()) + g.Expect(got).To(BeNil()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got.Name).To(Equal(machine.Name)) + g.Expect(got.Namespace).To(Equal(machine.Namespace)) + } }) } } func TestGetManagedLabels(t *testing.T) { + t.Parallel() g := NewWithT(t) labels := map[string]string{ labelManagedPrefix + "." + "foo": "bar", @@ -133,3 +298,10 @@ func TestGetManagedLabels(t *testing.T) { "foo": "bar", })) } + +// Verify supportutil.ParseNamespacedName extracts the Name portion from a "namespace/name" string. +func TestParseNamespacedNameForNodePoolLabel(t *testing.T) { + t.Parallel() + g := NewWithT(t) + g.Expect(supportutil.ParseNamespacedName("ns/name").Name).To(Equal("name")) +} diff --git a/docs/content/how-to/automated-machine-management/targeted-scale-down.md b/docs/content/how-to/automated-machine-management/targeted-scale-down.md new file mode 100644 index 000000000000..051ff4b29230 --- /dev/null +++ b/docs/content/how-to/automated-machine-management/targeted-scale-down.md @@ -0,0 +1,57 @@ +--- +title: Targeted node scale-down for NodePools +--- + +# Targeted node scale-down for NodePools + +By default, when a NodePool is scaled down, CAPI selects machines for deletion based on creation timestamp (newest first). This page explains how to target specific nodes for removal, giving you control over which machines are deleted during a scale-down operation. + +## How it works + +You annotate one or more Nodes in the hosted cluster with `hypershift.openshift.io/scale-down=true`. The HCCO Node Controller detects this annotation and sets `cluster.x-k8s.io/delete-machine=yes` on the corresponding Machine in the management cluster. When a scale-down occurs (either by reducing `spec.replicas` on the NodePool or via the cluster autoscaler), CAPI prioritizes machines with the `delete-machine` annotation for deletion. + +!!! important + + This annotation is a **deletion-priority hint**, not a trigger. Annotating a Node does **not** cause it to be deleted immediately. The machine is only removed when a scale-down actually occurs. + +## Usage + +### Marking a node for preferential deletion + +```bash +oc --kubeconfig annotate node hypershift.openshift.io/scale-down=true +``` + +Then scale down the NodePool: + +```bash +oc -n scale nodepool --replicas= +``` + +The annotated node will be selected for deletion before any non-annotated nodes. + +### Changing your mind + +If you annotated a node but decide you no longer want it removed, simply remove the annotation **before** the scale-down completes: + +```bash +oc --kubeconfig annotate node hypershift.openshift.io/scale-down- +``` + +The controller will remove the `delete-machine` annotation from the corresponding Machine, returning it to normal deletion priority. + +### Marking multiple nodes + +You can annotate multiple nodes simultaneously. When a scale-down occurs, CAPI will prioritize all annotated machines for deletion. If the scale-down removes fewer machines than are annotated, CAPI selects among the annotated machines using its default ordering (newest first). + +```bash +oc --kubeconfig annotate node node-1 node-2 hypershift.openshift.io/scale-down=true +``` + +## Important details + +- **Value must be exactly `true`**: The annotation value is case-sensitive. Values like `"True"`, `"yes"`, `"1"`, or `"false"` will **not** activate the feature. +- **Works with all platforms**: This feature operates at the CAPI layer and works with AWS, Azure, KubeVirt, Agent, OpenStack, and IBM Cloud. +- **Works with autoscaling**: When the cluster autoscaler decides to shrink the pool, annotated machines are prioritized for deletion. +- **Node draining is respected**: The standard CAPI drain process (including `NodeDrainTimeout`) applies to annotated machines just like any other. +- **Unidirectional sync**: The controller syncs from Node to Machine only. If `delete-machine` is set directly on a Machine without the corresponding Node annotation, the controller will remove it on the next reconciliation cycle. diff --git a/docs/content/reference/aggregated-docs.md b/docs/content/reference/aggregated-docs.md index ee120f8aa3d4..71195a9099ce 100644 --- a/docs/content/reference/aggregated-docs.md +++ b/docs/content/reference/aggregated-docs.md @@ -4564,6 +4564,69 @@ scale_down_nodepool After these steps, you will see how the (in the AWS case) instances will be terminated instantly, but Openshift will take some time until the nodes get deleted because of the default timeouts set on the platforms. +--- + +## Source: docs/content/how-to/automated-machine-management/targeted-scale-down.md + +--- +title: Targeted node scale-down for NodePools +--- + +# Targeted node scale-down for NodePools + +By default, when a NodePool is scaled down, CAPI selects machines for deletion based on creation timestamp (newest first). This page explains how to target specific nodes for removal, giving you control over which machines are deleted during a scale-down operation. + +## How it works + +You annotate one or more Nodes in the hosted cluster with `hypershift.openshift.io/scale-down=true`. The HCCO Node Controller detects this annotation and sets `cluster.x-k8s.io/delete-machine=yes` on the corresponding Machine in the management cluster. When a scale-down occurs (either by reducing `spec.replicas` on the NodePool or via the cluster autoscaler), CAPI prioritizes machines with the `delete-machine` annotation for deletion. + +!!! important + + This annotation is a **deletion-priority hint**, not a trigger. Annotating a Node does **not** cause it to be deleted immediately. The machine is only removed when a scale-down actually occurs. + +## Usage + +### Marking a node for preferential deletion + +```bash +oc --kubeconfig annotate node hypershift.openshift.io/scale-down=true +``` + +Then scale down the NodePool: + +```bash +oc -n scale nodepool --replicas= +``` + +The annotated node will be selected for deletion before any non-annotated nodes. + +### Changing your mind + +If you annotated a node but decide you no longer want it removed, simply remove the annotation **before** the scale-down completes: + +```bash +oc --kubeconfig annotate node hypershift.openshift.io/scale-down- +``` + +The controller will remove the `delete-machine` annotation from the corresponding Machine, returning it to normal deletion priority. + +### Marking multiple nodes + +You can annotate multiple nodes simultaneously. When a scale-down occurs, CAPI will prioritize all annotated machines for deletion. If the scale-down removes fewer machines than are annotated, CAPI selects among the annotated machines using its default ordering (newest first). + +```bash +oc --kubeconfig annotate node node-1 node-2 hypershift.openshift.io/scale-down=true +``` + +## Important details + +- **Value must be exactly `true`**: The annotation value is case-sensitive. Values like `"True"`, `"yes"`, `"1"`, or `"false"` will **not** activate the feature. +- **Works with all platforms**: This feature operates at the CAPI layer and works with AWS, Azure, KubeVirt, Agent, OpenStack, and IBM Cloud. +- **Works with autoscaling**: When the cluster autoscaler decides to shrink the pool, annotated machines are prioritized for deletion. +- **Node draining is respected**: The standard CAPI drain process (including `NodeDrainTimeout`) applies to annotated machines just like any other. +- **Unidirectional sync**: The controller syncs from Node to Machine only. If `delete-machine` is set directly on a Machine without the corresponding Node annotation, the controller will remove it on the next reconciliation cycle. + + --- ## Source: docs/content/how-to/autoscaling.md diff --git a/docs/mkdocs.yml b/docs/mkdocs.yml index 0543cf3f931e..b647226d04b7 100644 --- a/docs/mkdocs.yml +++ b/docs/mkdocs.yml @@ -67,6 +67,7 @@ nav: - how-to/automated-machine-management/node-tuning.md - how-to/automated-machine-management/nodepool-lifecycle.md - how-to/automated-machine-management/scale-to-zero-dataplane.md + - how-to/automated-machine-management/targeted-scale-down.md - how-to/autoscaling.md - 'CI': - 'AI-Assisted CI Jobs': how-to/ci/ai-assisted-ci-jobs.md diff --git a/test/e2e/autoscaling_test.go b/test/e2e/autoscaling_test.go index ce3251e6d46c..795246f09f24 100644 --- a/test/e2e/autoscaling_test.go +++ b/test/e2e/autoscaling_test.go @@ -5,6 +5,7 @@ package e2e import ( "context" "fmt" + "sort" "strings" "testing" "time" @@ -146,7 +147,49 @@ func testAutoscaling(ctx context.Context, mgtClient crclient.Client, hostedClust // Wait for one more node. // TODO (alberto): have ability for NodePool to label Nodes and let workload target specific Nodes. - _ = e2eutil.WaitForNReadyNodes(t, ctx, guestClient, max, hostedCluster.Spec.Platform.Type) + nodes = e2eutil.WaitForNReadyNodes(t, ctx, guestClient, max, hostedCluster.Spec.Platform.Type) + + // --- Targeted scale-down verification (CNTRLPLANE-3318) --- + // Sort nodes oldest-first; annotate the oldest so CAPI prioritizes it for + // deletion even though its default policy (newest-first) would keep it. + sort.Slice(nodes, func(i, j int) bool { + return nodes[i].CreationTimestamp.Before(&nodes[j].CreationTimestamp) + }) + targetNode := &nodes[0] + t.Logf("Target node (oldest) for targeted scale-down: %s (created %s)", targetNode.Name, targetNode.CreationTimestamp) + + machineName := targetNode.Annotations[capiv1.MachineAnnotation] + g.Expect(machineName).NotTo(BeEmpty(), "node %s should have %s annotation", targetNode.Name, capiv1.MachineAnnotation) + machineNamespace := targetNode.Annotations[capiv1.ClusterNamespaceAnnotation] + g.Expect(machineNamespace).NotTo(BeEmpty(), "node %s should have %s annotation", targetNode.Name, capiv1.ClusterNamespaceAnnotation) + + err = e2eutil.UpdateObject(t, ctx, guestClient, targetNode, func(obj *corev1.Node) { + if obj.Annotations == nil { + obj.Annotations = make(map[string]string) + } + obj.Annotations[hyperv1.NodeScaleDownAnnotation] = "true" + }) + g.Expect(err).NotTo(HaveOccurred(), "failed to annotate node for targeted scale-down") + t.Logf("Annotated node %s with %s=true", targetNode.Name, hyperv1.NodeScaleDownAnnotation) + + e2eutil.EventuallyObject(t, ctx, + fmt.Sprintf("Machine %s/%s to get delete-machine annotation", machineNamespace, machineName), + func(ctx context.Context) (*capiv1.Machine, error) { + machine := &capiv1.Machine{} + err := mgtClient.Get(ctx, crclient.ObjectKey{Namespace: machineNamespace, Name: machineName}, machine) + return machine, err + }, + []e2eutil.Predicate[*capiv1.Machine]{ + func(m *capiv1.Machine) (done bool, reasons string, err error) { + _, has := m.Annotations[capiv1.DeleteMachineAnnotation] + return has, fmt.Sprintf("delete-machine annotation present: %v", has), nil + }, + }, + e2eutil.WithTimeout(5*time.Minute), + e2eutil.WithoutConditionDump(), + ) + t.Logf("HCCO sync verified: Machine %s has delete-machine annotation", machineName) + targetNodeName := targetNode.Name // Delete workload. cascadeDelete := metav1.DeletePropagationForeground @@ -157,7 +200,12 @@ func testAutoscaling(ctx context.Context, mgtClient crclient.Client, hostedClust t.Logf("Deleted workload") // Wait for one less node. - _ = e2eutil.WaitForNReadyNodes(t, ctx, guestClient, numNodes, hostedCluster.Spec.Platform.Type) + remainingNodes := e2eutil.WaitForNReadyNodes(t, ctx, guestClient, numNodes, hostedCluster.Spec.Platform.Type) + for _, n := range remainingNodes { + g.Expect(n.Name).NotTo(Equal(targetNodeName), + "annotated node %s should have been deleted during scale-down", targetNodeName) + } + t.Logf("Targeted scale-down verified: annotated node %s was deleted", targetNodeName) } } diff --git a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.go b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.go index dc17f42ee9e6..280f4e1faf03 100644 --- a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.go +++ b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.go @@ -29,6 +29,20 @@ const ( // NodePoolReleaseVersionAnnotation is set on userdata Secrets (Replace strategy) and on Machines (InPlace strategy) // to track the OCP release version associated with each machine. NodePoolReleaseVersionAnnotation = "hypershift.openshift.io/release-version" + + // NodeScaleDownAnnotation is applied to Nodes in the hosted cluster by users. + // When set to exactly "true" (case-sensitive), the HCCO Node Controller sets + // cluster.x-k8s.io/delete-machine=yes on the corresponding Machine, giving it + // top priority for deletion during NodePool scale-down. + // + // This annotation is a deletion-priority hint, not a trigger: the annotated + // Machine is only removed when a scale-down actually occurs (e.g., the NodePool + // replica count is reduced or the cluster autoscaler decides to shrink the pool). + // + // Removing this annotation from the Node causes the controller to remove the + // delete-machine annotation from the Machine, allowing the user to change + // their mind before scale-down occurs. + NodeScaleDownAnnotation = "hypershift.openshift.io/scale-down" ) // ImageType specifies the type of image to use for node instances.