Merge pull request #142 from atlassian-labs/mwhittington/cyclops-node-cleanup-controller

Fix various situations where cluster autoscaler scale-down-disabled annotation was removed early, or left on nodes forever

Author: mwhittington21

Makefile

@@ -1,4 +1,4 @@
-VERSION = 1.10.4
+VERSION = 1.10.5
 # IMPORTANT! Update api version if a new release affects cnr
 API_VERSION = 1.0.0
 IMAGE = cyclops:$(VERSION)

cmd/manager/main.go

@@ -13,6 +13,7 @@ import (
 	cnrTransitioner "github.com/atlassian-labs/cyclops/pkg/controller/cyclenoderequest/transitioner"
 	"github.com/atlassian-labs/cyclops/pkg/controller/cyclenodestatus"
 	cnsTransitioner "github.com/atlassian-labs/cyclops/pkg/controller/cyclenodestatus/transitioner"
+	nodecontroller "github.com/atlassian-labs/cyclops/pkg/controller/node"
 	"github.com/atlassian-labs/cyclops/pkg/metrics"
 	"github.com/atlassian-labs/cyclops/pkg/notifications"
 	"github.com/atlassian-labs/cyclops/pkg/notifications/notifierbuilder"
@@ -47,6 +48,9 @@ var (
 	deleteCNRRequeue                 = app.Flag("delete-cnr-requeue", "How often to check if a CNR can be deleted").Default("24h").Duration()
 	defaultCNScyclingExpiry          = app.Flag("default-cns-cycling-expiry", "Fail the CNS if it has been cycling for this long").Default("3h").Duration()
 	unhealthyPodTerminationThreshold = app.Flag("unhealthy-pod-termination-after", "How long to tolerate an un-evictable yet unhealthy pod before forcefully removing it").Default("5m").Duration()
+
+	nodeControllerReconcileConcurrency = app.Flag("node-controller-reconcile-concurrency", "Maximum number of concurrent node controller reconciles").Default("1").Int()
+	nodeControllerRequeueAfter         = app.Flag("node-controller-requeue-after", "How often the node controller rechecks annotated nodes that are still covered by an active CNR").Default("5m").Duration()
 )
 
 var log = logf.Log.WithName("cmd")
@@ -128,6 +132,12 @@ func main() {
 		UnhealthyPodTerminationThreshold: *unhealthyPodTerminationThreshold,
 	}
 
+	// Configure the node controller options
+	nodeOptions := nodecontroller.Options{
+		ReconcileConcurrency: *nodeControllerReconcileConcurrency,
+		RequeueAfter:         *nodeControllerRequeueAfter,
+	}
+
 	// Set up and register the controllers that will share resources between them
 	_, err = cyclenoderequest.NewReconciler(mgr, cloudProvider, notifier, *namespace, cnrOptions)
 	if err != nil {
@@ -139,6 +149,11 @@ func main() {
 		log.Error(err, "Unable to add cycleNodeStatus controller")
 		os.Exit(1)
 	}
+	_, err = nodecontroller.NewReconciler(mgr, *namespace, nodeOptions)
+	if err != nil {
+		log.Error(err, "Unable to add node controller")
+		os.Exit(1)
+	}
 
 	log.Info("Starting the Cmd.")
 

deploy/crds/atlassian.com_cyclenoderequests_crd.yaml

@@ -365,6 +365,15 @@ spec:
                   when it last checked for progress in the cycle operation.
                 format: int64
                 type: integer
+              annotatedNodes:
+                description: |-
+                  AnnotatedNodes tracks the names of nodes that Cyclops added the
+                  scale-down-disabled annotation to during cycling. Used for deterministic
+                  cleanup such that only these nodes have their annotations removed during the
+                  Successful or Healing phase. Cleared after cleanup completes.
+                items:
+                  type: string
+                type: array
               currentNodes:
                 description: |-
                   CurrentNodes stores the current nodes that are being "worked on". Used to batch operations

docs/node-cleanup-reconciler.md

@@ -0,0 +1,46 @@
+# Node Controller
+
+The node controller is a controller-runtime reconciler for Kubernetes `Node` objects. Its current responsibility is to remove stale Cyclops-managed Cluster Autoscaler annotations that can be left behind when a `CycleNodeRequest` is deleted before normal CNR cleanup runs.
+
+## Why it exists
+
+Cyclops adds these annotations to replacement nodes during cycling:
+
+- `cluster-autoscaler.kubernetes.io/scale-down-disabled=true`
+- `cyclops.atlassian.com/annotation-managed=true`
+
+The first annotation prevents Cluster Autoscaler from removing replacement nodes before the old nodes have drained and terminated. The second annotation records that Cyclops added the protection, so Cyclops does not remove annotations that were pre-existing on a node.
+
+If a CNR disappears mid-cycle, the normal CNR transitioner cleanup may never run. The node controller acts as an eventual-consistency backstop so those nodes are not protected from scale-down forever.
+
+## How it works
+
+The controller watches `Node` objects using the manager's shared cache. A predicate only enqueues nodes with the Cyclops marker annotation, so ordinary node changes do not enter the reconcile loop.
+
+For each matching node, the controller:
+
+1. Confirms both the Cyclops marker and scale-down-disabled annotations are still present.
+2. Confirms the node is selected by at least one `NodeGroup`.
+3. Checks whether any non-terminal CNR in the configured namespace still selects the node.
+4. Requeues after the configured interval if an active CNR still covers the node.
+5. Removes both annotations if no active CNR covers the node.
+
+`Successful` and `Failed` CNRs are terminal. Other phases, including `Healing`, are considered active.
+
+## Configuration
+
+The controller defaults are intentionally conservative:
+
+- `--node-controller-reconcile-concurrency=1`
+- `--node-controller-requeue-after=5m`
+
+The controller is a safety net rather than a high-throughput reconciler, so one worker is normally enough. The requeue interval controls how often annotated nodes are rechecked while they are still covered by an active CNR.
+
+## Observability
+
+The node controller emits cleanup-specific metrics so we can tell when the backstop is doing work:
+
+- `cyclops_node_cleanup_annotations_removed_total`
+- `cyclops_node_cleanup_reconciles_total{result=...}`
+
+A non-zero removal rate means stale annotations reached the node controller instead of being cleaned up by the normal CNR flow.

pkg/apis/atlassian/v1/cyclenoderequest.go

@@ -36,3 +36,8 @@ func (in *CycleNodeRequest) IsFromSameNodeGroup(cnr CycleNodeRequest) bool {
 		cnr.GetNodeGroupNames(),
 	)
 }
+
+// IsTerminal returns true when the CycleNodeRequest lifecycle has ended.
+func (in *CycleNodeRequest) IsTerminal() bool {
+	return in.Status.Phase == CycleNodeRequestSuccessful || in.Status.Phase == CycleNodeRequestFailed
+}

pkg/apis/atlassian/v1/cyclenoderequest_types.go

@@ -93,6 +93,12 @@ type CycleNodeRequestStatus struct {
 
 	// PreTerminationChecks keeps track of the instance pre termination check information
 	PreTerminationChecks map[string]PreTerminationCheckStatusList `json:"preTerminationChecks,omitempty"`
+
+	// AnnotatedNodes tracks the names of nodes that Cyclops added the
+	// scale-down-disabled annotation to during cycling. Used for deterministic
+	// cleanup such that only these nodes have their annotations removed during the
+	// Successful or Healing phase. Cleared after cleanup completes.
+	AnnotatedNodes []string `json:"annotatedNodes,omitempty"`
 }
 
 // CycleNodeRequestNode stores a current node that is being worked on

pkg/apis/atlassian/v1/zz_generated.deepcopy.go

@@ -6,6 +6,7 @@ import (
 
 	v1 "github.com/atlassian-labs/cyclops/pkg/apis/atlassian/v1"
 	"github.com/atlassian-labs/cyclops/pkg/controller"
+	"github.com/atlassian-labs/cyclops/pkg/k8s"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
@@ -17,18 +18,14 @@ type transitionFunc func() (reconcile.Result, error)
 // different request types.
 const cycleNodeLabel = "cyclops.atlassian.com/terminate"
 
-// cyclopsManagedAnnotation marks nodes where Cyclops added the scale-down-disabled annotation.
-// Used to track which annotations we added vs pre-existing ones (for safe cleanup).
-// Only remove the cluster-autoscaler annotation if this marker annotation also exists.
-const cyclopsManagedAnnotation = "cyclops.atlassian.com/annotation-managed"
-
-// clusterAutoscalerScaleDownDisabledAnnotation is the annotation key used to prevent
-// Cluster Autoscaler from scaling down a node. This is used to protect new nodes
-// during the cycling process from being removed by Cluster Autoscaler before
-// the corresponding old nodes are fully terminated.
-// See: https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/FAQ.md#how-can-i-prevent-cluster-autoscaler-from-scaling-down-a-particular-node
-const clusterAutoscalerScaleDownDisabledAnnotation = "cluster-autoscaler.kubernetes.io/scale-down-disabled"
-const clusterAutoscalerScaleDownDisabledValue = "true"
+const (
+	// cyclopsManagedAnnotation marks nodes where Cyclops added the scale-down-disabled annotation.
+	cyclopsManagedAnnotation = k8s.CyclopsManagedAnnotation
+
+	// clusterAutoscalerScaleDownDisabledAnnotation protects new nodes during cycling.
+	clusterAutoscalerScaleDownDisabledAnnotation = k8s.ClusterAutoscalerScaleDownDisabledAnnotation
+	clusterAutoscalerScaleDownDisabledValue      = "true"
+)
 
 // nodeGroupAnnotationKey is the annotation key on NodeGroup resources that controls whether
 // Cluster Autoscaler annotation management is enabled or disabled.

pkg/controller/cyclenoderequest/transitioner/transitioner.go

@@ -532,6 +532,20 @@ func (t *CycleNodeRequestTransitioner) transitionWaitingTermination() (reconcile
 		return t.transitionToHealing(err)
 	}
 
+	// When a batch completes and we're looping back to Initialised, clean up
+	// the annotations from this batch's replacement nodes. Their old counterparts
+	// have been terminated, so the protection is no longer needed. This avoids
+	// accumulating protected nodes across the entire cycle duration.
+	//
+	// Trade-off: cluster-autoscaler could theoretically scale down these freshly
+	// drained-onto nodes before the next batch's replacements come up. We accept
+	// this because (a) CA uses the eviction API so pods are rescheduled gracefully,
+	// and (b) keeping protection until the entire CNR completes would leave a
+	// growing set of un-scalable nodes for long-running multi-batch cycles.
+	if desiredPhase == v1.CycleNodeRequestInitialised && t.shouldManageAnnotations() {
+		t.cleanupScaleDownDisabledAnnotations()
+	}
+
 	if err := t.rm.UpdateObject(t.cycleNodeRequest); err != nil {
 		return t.transitionToHealing(err)
 	}
@@ -593,14 +607,6 @@ func (t *CycleNodeRequestTransitioner) transitionHealing() (reconcile.Result, er
 		}
 	}
 
-	// Cleanup scale-down-disabled annotations if enabled
-	if t.shouldManageAnnotations() {
-		t.cleanupScaleDownDisabledAnnotations()
-	} else {
-		t.rm.Logger.Info("Skipping annotation cleanup (disabled)",
-			"cnr", t.cycleNodeRequest.Name)
-	}
-
 	return t.transitionToFailed(nil)
 }
 
@@ -628,14 +634,6 @@ func (t *CycleNodeRequestTransitioner) transitionSuccessful() (reconcile.Result,
 		return reconcile.Result{Requeue: true, RequeueAfter: transitionDuration}, nil
 	}
 
-	// Cleanup scale-down-disabled annotations if enabled
-	if t.shouldManageAnnotations() {
-		t.cleanupScaleDownDisabledAnnotations()
-	} else {
-		t.rm.Logger.Info("Skipping annotation cleanup (disabled)",
-			"cnr", t.cycleNodeRequest.Name)
-	}
-
 	// Delete failed sibling CNRs regardless of whether the CNR for the
 	// transitioner should be deleted. If failed CNRs pile up that will prevent
 	// Cyclops observer from auto-generating new CNRs for a nodegroup.