feat: generalize condition check, add tests

Author: mchmarny

janitor/Makefile

@@ -60,6 +60,8 @@ generate: ## Generate CRDs and move them to Helm chart directory
 	@# Install controller-gen if not present
 	@which controller-gen > /dev/null || (echo "Installing controller-gen..." && go install sigs.k8s.io/controller-tools/cmd/controller-gen@latest)
 	@# Generate CRDs into api/v1alpha1 directory
+	@# Note: Generated CRD YAML files do not include license headers (this is expected)
+	@# YAML files are excluded from license header checks via main Makefile: -ignore '**/*.yaml'
 	@controller-gen crd paths=./$(API_DIR) output:crd:dir=./$(API_DIR)
 	@# Move generated CRDs to Helm chart crds directory
 	@echo "Moving generated CRDs to $(CRD_OUTPUT_DIR)..."

janitor/api/v1alpha1/rebootnode_types.go

@@ -192,6 +192,33 @@ func (r *RebootNode) SetCompletionTime() {
 	}
 }
 
+// Interface implementation for generic status update handling
+
+// GetRetryCount returns the retry count
+func (s *RebootNodeStatus) GetRetryCount() int32 {
+	return s.RetryCount
+}
+
+// GetConsecutiveFailures returns the consecutive failures count
+func (s *RebootNodeStatus) GetConsecutiveFailures() int32 {
+	return s.ConsecutiveFailures
+}
+
+// GetStartTime returns the start time
+func (s *RebootNodeStatus) GetStartTime() *metav1.Time {
+	return s.StartTime
+}
+
+// GetCompletionTime returns the completion time
+func (s *RebootNodeStatus) GetCompletionTime() *metav1.Time {
+	return s.CompletionTime
+}
+
+// GetConditions returns the conditions
+func (s *RebootNodeStatus) GetConditions() []metav1.Condition {
+	return s.Conditions
+}
+
 func init() {
 	SchemeBuilder.Register(&RebootNode{}, &RebootNodeList{})
 }

janitor/api/v1alpha1/terminatenode_types.go

@@ -182,6 +182,33 @@ func (t *TerminateNode) SetCompletionTime() {
 	}
 }
 
+// Interface implementation for generic status update handling
+
+// GetRetryCount returns the retry count
+func (s *TerminateNodeStatus) GetRetryCount() int32 {
+	return s.RetryCount
+}
+
+// GetConsecutiveFailures returns the consecutive failures count
+func (s *TerminateNodeStatus) GetConsecutiveFailures() int32 {
+	return s.ConsecutiveFailures
+}
+
+// GetStartTime returns the start time
+func (s *TerminateNodeStatus) GetStartTime() *metav1.Time {
+	return s.StartTime
+}
+
+// GetCompletionTime returns the completion time
+func (s *TerminateNodeStatus) GetCompletionTime() *metav1.Time {
+	return s.CompletionTime
+}
+
+// GetConditions returns the conditions
+func (s *TerminateNodeStatus) GetConditions() []metav1.Condition {
+	return s.Conditions
+}
+
 func init() {
 	SchemeBuilder.Register(&TerminateNode{}, &TerminateNodeList{})
 }

janitor/pkg/controller/backoff.go

@@ -0,0 +1,41 @@
+// Copyright (c) 2025, NVIDIA CORPORATION.  All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package controller
+
+import "time"
+
+// getNextRequeueDelay calculates per-resource exponential backoff delay based on consecutive failures.
+// This is used with ctrl.Result{RequeueAfter: delay} rather than the controller's built-in rate limiter
+// because we need independent backoff per node based on each node's failure history, not global controller
+// rate limiting. Each resource (RebootNode/TerminateNode) tracks its own ConsecutiveFailures counter
+// and gets its own backoff schedule.
+//
+// Backoff schedule: 30s, 1m, 2m, 5m (capped at max after 3+ failures)
+func getNextRequeueDelay(consecutiveFailures int32) time.Duration {
+	delays := []time.Duration{
+		30 * time.Second, // First retry after initial failure
+		1 * time.Minute,  // Second retry
+		2 * time.Minute,  // Third retry
+		5 * time.Minute,  // Fourth+ retry (capped)
+	}
+
+	// Safely convert int32 to int for array indexing
+	idx := int(consecutiveFailures)
+	if idx >= len(delays) {
+		return delays[len(delays)-1] // Cap at maximum
+	}
+
+	return delays[idx]
+}

janitor/pkg/controller/backoff_test.go

@@ -0,0 +1,89 @@
+// Copyright (c) 2025, NVIDIA CORPORATION.  All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package controller
+
+import (
+	"testing"
+	"time"
+)
+
+func TestGetNextRequeueDelay(t *testing.T) {
+	tests := []struct {
+		name                string
+		consecutiveFailures int32
+		expectedDelay       time.Duration
+	}{
+		{
+			name:                "first retry after initial failure",
+			consecutiveFailures: 0,
+			expectedDelay:       30 * time.Second,
+		},
+		{
+			name:                "second retry",
+			consecutiveFailures: 1,
+			expectedDelay:       1 * time.Minute,
+		},
+		{
+			name:                "third retry",
+			consecutiveFailures: 2,
+			expectedDelay:       2 * time.Minute,
+		},
+		{
+			name:                "fourth retry",
+			consecutiveFailures: 3,
+			expectedDelay:       5 * time.Minute,
+		},
+		{
+			name:                "fifth retry - capped at max",
+			consecutiveFailures: 4,
+			expectedDelay:       5 * time.Minute,
+		},
+		{
+			name:                "many retries - still capped at max",
+			consecutiveFailures: 10,
+			expectedDelay:       5 * time.Minute,
+		},
+		{
+			name:                "very large failure count - still capped",
+			consecutiveFailures: 100,
+			expectedDelay:       5 * time.Minute,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := getNextRequeueDelay(tt.consecutiveFailures)
+			if got != tt.expectedDelay {
+				t.Errorf("getNextRequeueDelay(%d) = %v, want %v",
+					tt.consecutiveFailures, got, tt.expectedDelay)
+			}
+		})
+	}
+}
+
+func TestGetNextRequeueDelay_BackoffProgression(t *testing.T) {
+	// Test that delays increase monotonically until capped
+	var prevDelay time.Duration
+	for i := int32(0); i < 10; i++ {
+		delay := getNextRequeueDelay(i)
+
+		if i > 0 && delay < prevDelay {
+			t.Errorf("Backoff delay decreased at failure count %d: prev=%v, current=%v",
+				i, prevDelay, delay)
+		}
+
+		prevDelay = delay
+	}
+}

janitor/pkg/controller/rebootnode_controller.go

@@ -22,7 +22,6 @@ import (
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -48,28 +47,6 @@ const (
 	MaxRebootRetries = 20 // 10 minutes at 30s base intervals
 )
 
-// getNextRequeueDelay calculates per-resource exponential backoff delay based on consecutive failures.
-// This is used with ctrl.Result{RequeueAfter: delay} rather than the controller's built-in rate limiter
-// because we need independent backoff per node based on each node's failure history, not global controller
-// rate limiting. Each RebootNode tracks its own ConsecutiveFailures counter and gets its own backoff schedule.
-// Returns: 30s, 1m, 2m, 5m (capped at max after 3+ failures)
-func getNextRequeueDelay(consecutiveFailures int32) time.Duration {
-	delays := []time.Duration{
-		30 * time.Second, // First retry after initial failure
-		1 * time.Minute,  // Second retry
-		2 * time.Minute,  // Third retry
-		5 * time.Minute,  // Fourth+ retry (capped)
-	}
-
-	// Safely convert int32 to int for array indexing
-	idx := int(consecutiveFailures)
-	if idx >= len(delays) {
-		return delays[len(delays)-1] // Cap at maximum
-	}
-
-	return delays[idx]
-}
-
 // conditionsChanged compares two sets of conditions and returns true if they differ.
 // It checks the type, status, reason, and message of each condition.
 func conditionsChanged(original, updated []metav1.Condition) bool {
@@ -102,47 +79,25 @@ func conditionsChanged(original, updated []metav1.Condition) bool {
 }
 
 // updateRebootNodeStatus is a helper function that handles status updates with proper error handling.
-// It centralizes the status update logic to avoid code duplication and provides consistent handling
-// of status updates across different code paths in the reconciliation loop.
-//
-//nolint:dupl // Similar to updateTerminateNodeStatus but operates on RebootNode type
+// It delegates to the generic updateNodeActionStatus function.
 func (r *RebootNodeReconciler) updateRebootNodeStatus(
 	ctx context.Context,
 	req ctrl.Request,
 	original *janitordgxcnvidiacomv1alpha1.RebootNode,
 	updated *janitordgxcnvidiacomv1alpha1.RebootNode,
 	result ctrl.Result,
 ) (ctrl.Result, error) {
-	logger := log.FromContext(ctx)
-
-	// Check if status changed by comparing individual fields.
-	// This status update is safe because controller-runtime uses leader election
-	// to ensure only one controller instance is active at a time, even with multiple replicas.
-	// The active controller has exclusive write access to RebootNode status.
-	statusChanged := original.Status.RetryCount != updated.Status.RetryCount ||
-		original.Status.ConsecutiveFailures != updated.Status.ConsecutiveFailures ||
-		(original.Status.StartTime == nil) != (updated.Status.StartTime == nil) ||
-		(original.Status.CompletionTime == nil) != (updated.Status.CompletionTime == nil) ||
-		conditionsChanged(original.Status.Conditions, updated.Status.Conditions)
-
-	if statusChanged {
-		if err := r.Status().Update(ctx, updated); err != nil {
-			if apierrors.IsNotFound(err) {
-				logger.V(0).Info("post-reconciliation status update: object not found, assumed deleted",
-					"name", updated.Name)
-				return ctrl.Result{}, nil
-			}
-			logger.Error(err, "failed to update rebootnode status",
-				"node", updated.Spec.NodeName)
-			return ctrl.Result{}, err
-		}
-		logger.Info("rebootnode status updated",
-			"node", updated.Spec.NodeName,
-			"retryCount", int(updated.Status.RetryCount),
-			"consecutiveFailures", int(updated.Status.ConsecutiveFailures))
-	}
-
-	return result, nil
+	return updateNodeActionStatus(
+		ctx,
+		r.Status(),
+		original,
+		updated,
+		&original.Status,
+		&updated.Status,
+		updated.Spec.NodeName,
+		"rebootnode",
+		result,
+	)
 }
 
 // RebootNodeReconciler reconciles a RebootNode object