Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions api/hypershift/v1beta1/nodepool_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines +50 to +58

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Propagate unexpected machine lookup failures.

This branch only surfaces StatusErrors. If r.client.Get fails with a transport/cache/context error, it gets treated as “annotations may not be set yet” and reconciliation exits cleanly, which can silently stop both label sync and delete-machine sync until another Node event arrives.

Suggested direction
 	machine, err := r.getMachineForNode(ctx, node)
 	if err != nil {
-		var apiErr *apierrors.StatusError
-		if errors.As(err, &apiErr) && !apierrors.IsNotFound(err) {
-			return ctrl.Result{}, err
-		}
-		log.Error(err, "failed to get Machine for Node, CAPI annotations may not be set yet")
-		return ctrl.Result{}, nil
+		switch {
+		case apierrors.IsNotFound(err), errors.Is(err, errMissingMachineReference):
+			log.Info("Machine reference for Node is not available yet", "node", node.Name)
+			return ctrl.Result{}, nil
+		default:
+			return ctrl.Result{}, err
+		}
 	}

getMachineForNode would need to return a sentinel like errMissingMachineReference for the missing-annotation cases.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@control-plane-operator/hostedclusterconfigoperator/controllers/node/node.go`
around lines 50 - 58, The current reconciliation swallows
transport/cache/context errors from r.getMachineForNode; update
getMachineForNode to return a sentinel error (e.g., errMissingMachineReference)
when CAPI annotations are missing, and change the caller logic in Node
reconciliation to treat only that sentinel and apierrors.IsNotFound as expected
(returning ctrl.Result{}, nil) while propagating any other errors (including
transport/context/cache errors from r.client.Get) by returning ctrl.Result{},
err. Reference getMachineForNode, errMissingMachineReference, and r.client.Get
when making these changes.


// 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]
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Loading