feat: add SKYHOOK_NODE_ORDER env var for monotonic node ordering

lockwobr · lockwobr · commit 9f439c69c71d · 2026-03-17T14:38:06.000-07:00
Packages need to know their position for kubeadm upgrades (first node
runs "kubeadm upgrade apply", subsequent nodes run "kubeadm upgrade node").

- Add NodeOrderOffset to SkyhookStatus, incremented via RemoveNodePriority
- Add RemoveNodePriority/NodeOrder methods on *Skyhook wrapper
- Replace 3 raw delete(NodePriority) sites with RemoveNodePriority
- Add SKYHOOK_NODE_ORDER to getAgentConfigEnvVars for all pods
- Add name tiebreaker to node sort for deterministic ordering
- Reset NodeOrderOffset and NodePriority on CLI reset
- Remove omitempty from NodePriority/NodeOrderOffset so nil/0 serialize in merge patches
diff --git a/chart/templates/skyhook-crd.yaml b/chart/templates/skyhook-crd.yaml
@@ -712,6 +712,11 @@ spec:
                 description: NodeBootIds tracks the boot ids of nodes for triggering
                   on reboot
                 type: object
+              nodeOrderOffset:
+                description: |-
+                  NodeOrderOffset tracks the cumulative count of nodes removed from NodePriority.
+                  Used with NodePriority to compute monotonic SKYHOOK_NODE_ORDER across batches.
+                type: integer
               nodePriority:
                 additionalProperties:
                   format: date-time
diff --git a/k8s-tests/chainsaw/cli/reset/assert-nodes-reset.yaml b/k8s-tests/chainsaw/cli/reset/assert-nodes-reset.yaml
@@ -1,4 +1,4 @@
-# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
 # SPDX-License-Identifier: Apache-2.0
 #
 #
@@ -29,4 +29,11 @@ metadata:
     (contains(keys(@), "skyhook.nvidia.com/cordon_cli-reset-test")): false
     skyhook.nvidia.com/status_cli-reset-test: disabled ## disabled after clearing
     # skyhook.nvidia.com/version_cli-reset-test: .* ## also rest after clearing
-
+---
+# Verify node ordering is reset after CLI reset
+apiVersion: skyhook.nvidia.com/v1alpha1
+kind: Skyhook
+metadata:
+  name: cli-reset-test
+status:
+  nodeOrderOffset: 0
diff --git a/k8s-tests/chainsaw/skyhook/strict-order/assert-both-nodes-complete.yaml b/k8s-tests/chainsaw/skyhook/strict-order/assert-both-nodes-complete.yaml
@@ -29,6 +29,16 @@ metadata:
     skyhook.nvidia.com/status_strict-order-skyhook-c: complete
     skyhook.nvidia.com/status_strict-order-skyhook-d: disabled
 ---
+# Verify nodeOrderOffset incremented as nodes completed and were pruned from NodePriority
+# With 2 nodes, each skyhook should have offset=2 after both complete
+apiVersion: skyhook.nvidia.com/v1alpha1
+kind: Skyhook
+metadata:
+  name: strict-order-skyhook-zzz
+status:
+  nodeOrderOffset: 2
+  (length(nodePriority || `{}`)): 0
+---
 # Worker2 node complete (d still disabled)
 apiVersion: v1
 kind: Node
diff --git a/k8s-tests/chainsaw/skyhook/strict-order/chainsaw-test.yaml b/k8s-tests/chainsaw/skyhook/strict-order/chainsaw-test.yaml
@@ -77,6 +77,39 @@ spec:
     try:
     - sleep:
         duration: 5s
+    # Verify SKYHOOK_NODE_ORDER is set on the config pod for zzz on kind-worker (before zzz completes)
+    - assert:
+        resource:
+          apiVersion: v1
+          kind: Pod
+          metadata:
+            namespace: skyhook
+            labels:
+              skyhook.nvidia.com/name: strict-order-skyhook-zzz
+              skyhook.nvidia.com/package: foobar-1.2
+            annotations:
+              ("skyhook.nvidia.com/package" && parse_json("skyhook.nvidia.com/package")):
+                {
+                  "name": "foobar",
+                  "version": "1.2",
+                  "skyhook": "strict-order-skyhook-zzz",
+                  "stage": "config",
+                  "image": "ghcr.io/nvidia/skyhook/agentless"
+                }
+          spec:
+            nodeName: kind-worker
+            initContainers:
+              - name: foobar-init
+              - name: foobar-config
+                env:
+                  ([5]):
+                    name: SKYHOOK_NODE_ORDER
+                    value: "0"
+              - name: foobar-configcheck
+                env:
+                  ([5]):
+                    name: SKYHOOK_NODE_ORDER
+                    value: "0"
     - assert:
         file: assert-node1-priority1-complete-node2-blocked.yaml
     - script:
@@ -86,7 +119,6 @@ spec:
           ../../metrics_test.py skyhook_node_status_count 1 -t skyhook_name=strict-order-skyhook-zzz -t status=blocked
           ../../metrics_test.py skyhook_package_state_count 1 -t package_name=foobar -t package_version=1.2 -t skyhook_name=strict-order-skyhook-zzz -t state=complete
 
-
     # Unblock worker2 only (keep b paused) so it can catch up through zzz and gate
   - name: unblock-worker2
     try:
@@ -96,7 +128,39 @@ spec:
         content: |
           ## Remove blocking taint from worker2 - it will catch up through zzz then gate
           kubectl taint node kind-worker2 test-block- 2>/dev/null || true
-
+    # Verify SKYHOOK_NODE_ORDER is set on the zzz config pod for kind-worker2 (after taint removed)
+    - assert:
+        resource:
+          apiVersion: v1
+          kind: Pod
+          metadata:
+            namespace: skyhook
+            labels:
+              skyhook.nvidia.com/name: strict-order-skyhook-zzz
+              skyhook.nvidia.com/package: foobar-1.2
+            annotations:
+              ("skyhook.nvidia.com/package" && parse_json("skyhook.nvidia.com/package")):
+                {
+                  "name": "foobar",
+                  "version": "1.2",
+                  "skyhook": "strict-order-skyhook-zzz",
+                  "stage": "config",
+                  "image": "ghcr.io/nvidia/skyhook/agentless"
+                }
+          spec:
+            nodeName: kind-worker2
+            initContainers:
+              - name: foobar-init
+              - name: foobar-config
+                env:
+                  ([5]):
+                    name: SKYHOOK_NODE_ORDER
+                    value: "1"
+              - name: foobar-configcheck
+                env:
+                  ([5]):
+                    name: SKYHOOK_NODE_ORDER
+                    value: "1"
 
   # Phase 3b: Assert sequencing: all gate blocks worker from priority 3
   # Worker completes gate (priority 2) but must WAIT for worker2 to also complete gate
diff --git a/operator/api/v1alpha1/skyhook_types.go b/operator/api/v1alpha1/skyhook_types.go
@@ -371,6 +371,10 @@ type SkyhookStatus struct {
 	// NodePriority tracks what nodes we are working on. This is makes the interrupts budgets sticky.
 	NodePriority map[string]metav1.Time `json:"nodePriority,omitempty"`
 
+	// NodeOrderOffset tracks the cumulative count of nodes removed from NodePriority.
+	// Used with NodePriority to compute monotonic SKYHOOK_NODE_ORDER across batches.
+	NodeOrderOffset int `json:"nodeOrderOffset,omitempty"`
+
 	// ConfigUpdates tracks config updates
 	ConfigUpdates map[string][]string `json:"configUpdates,omitempty"`
 
diff --git a/operator/cmd/cli/app/reset.go b/operator/cmd/cli/app/reset.go
@@ -257,14 +257,30 @@ func resetBatchStateForReset(ctx context.Context, cmd *cobra.Command, kubeClient
 		return
 	}
 
+	// Reset node ordering in memory
+	skyhook.Status.NodeOrderOffset = 0
+	skyhook.Status.NodePriority = nil
+
 	if len(skyhook.Status.CompartmentStatuses) == 0 {
+		// No compartments — use raw patch to clear omitempty fields explicitly
+		if err := utils.PatchSkyhookStatusRaw(ctx, kubeClient.Dynamic(), skyhookName,
+			[]byte(`{"status":{"nodeOrderOffset":0,"nodePriority":null}}`)); err != nil {
+			_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to reset node ordering: %v\n", err)
+		}
 		return
 	}
 
+	// With compartments, PatchSkyhookStatus writes the full status.
+	// NodeOrderOffset=0 is dropped by omitempty, so follow up with raw patch.
 	skyhook.ResetCompartmentBatchStates()
 	if err := utils.PatchSkyhookStatus(ctx, kubeClient.Dynamic(), skyhookName, skyhook.Status); err != nil {
 		_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to reset batch state: %v\n", err)
 		return
 	}
+	// Raw patch to clear omitempty fields that PatchSkyhookStatus can't zero
+	if err := utils.PatchSkyhookStatusRaw(ctx, kubeClient.Dynamic(), skyhookName,
+		[]byte(`{"status":{"nodeOrderOffset":0,"nodePriority":null}}`)); err != nil {
+		_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to reset node ordering: %v\n", err)
+	}
 	_, _ = fmt.Fprintf(cmd.OutOrStdout(), "Batch state reset for Skyhook %q\n", skyhookName)
 }
diff --git a/operator/config/crd/bases/skyhook.nvidia.com_skyhooks.yaml b/operator/config/crd/bases/skyhook.nvidia.com_skyhooks.yaml
@@ -719,6 +719,11 @@ spec:
                 description: NodeBootIds tracks the boot ids of nodes for triggering
                   on reboot
                 type: object
+              nodeOrderOffset:
+                description: |-
+                  NodeOrderOffset tracks the cumulative count of nodes removed from NodePriority.
+                  Used with NodePriority to compute monotonic SKYHOOK_NODE_ORDER across batches.
+                type: integer
               nodePriority:
                 additionalProperties:
                   format: date-time
diff --git a/operator/internal/cli/utils/utils.go b/operator/internal/cli/utils/utils.go
@@ -456,6 +456,24 @@ func ExtractImageTag(image string) string {
 	return strings.TrimSpace(parts[len(parts)-1])
 }
 
+// PatchSkyhookStatusRaw patches the skyhook status with raw JSON bytes.
+// Use this when you need explicit null values to clear omitempty fields.
+func PatchSkyhookStatusRaw(ctx context.Context, dynamicClient dynamic.Interface, skyhookName string, patchBytes []byte) error {
+	gvr := v1alpha1.GroupVersion.WithResource("skyhooks")
+	_, err := dynamicClient.Resource(gvr).Patch(
+		ctx,
+		skyhookName,
+		types.MergePatchType,
+		patchBytes,
+		metav1.PatchOptions{},
+		"status",
+	)
+	if err != nil {
+		return fmt.Errorf("patching skyhook %q status: %w", skyhookName, err)
+	}
+	return nil
+}
+
 // PatchSkyhookStatus patches the status subresource of a Skyhook CR using the dynamic client.
 // This is used to update status fields without triggering a spec update.
 func PatchSkyhookStatus(ctx context.Context, dynamicClient dynamic.Interface, skyhookName string, status v1alpha1.SkyhookStatus) error {
diff --git a/operator/internal/controller/cluster_state_v2.go b/operator/internal/controller/cluster_state_v2.go
@@ -144,7 +144,12 @@ func BuildState(skyhooks *v1alpha1.SkyhookList, nodes *corev1.NodeList, deployme
 
 	for _, skyhook := range ret.skyhooks {
 		sort.Slice(skyhook.GetNodes(), func(i, j int) bool {
-			return skyhook.GetNodes()[i].GetNode().CreationTimestamp.Before(&skyhook.GetNodes()[j].GetNode().CreationTimestamp)
+			ti := skyhook.GetNodes()[i].GetNode().CreationTimestamp
+			tj := skyhook.GetNodes()[j].GetNode().CreationTimestamp
+			if !ti.Equal(&tj) {
+				return ti.Before(&tj)
+			}
+			return skyhook.GetNodes()[i].GetNode().Name < skyhook.GetNodes()[j].GetNode().Name
 		})
 	}
 
@@ -648,8 +653,7 @@ func (s *NodePicker) primeAndPruneNodes(skyhook SkyhookNodes) {
 		// prune
 		// if the node is complete, remove it from the priority list
 		if nodeStatus, _ := skyhook.GetNode(n); nodeStatus == v1alpha1.StatusComplete {
-			delete(skyhook.GetSkyhook().Status.NodePriority, n)
-			skyhook.GetSkyhook().Updated = true
+			skyhook.GetSkyhook().RemoveNodePriority(n)
 		} else {
 			s.priorityNodes[n] = t.Time
 		}
@@ -1458,11 +1462,17 @@ func CleanupRemovedNodes(skyhook SkyhookNodes) {
 
 	status := skyhook.GetSkyhook().Status
 
-	// Check and remove nodes from all status maps
+	// NodePriority needs special handling to track offset
+	for name := range status.NodePriority {
+		if _, ok := currentNodeNames[name]; !ok {
+			skyhook.GetSkyhook().RemoveNodePriority(name)
+		}
+	}
+
+	// Check and remove nodes from remaining status maps
 	change := cleanupNodeMap(status.NodeState, currentNodeNames)
 	change = cleanupNodeMap(status.NodeStatus, currentNodeNames) || change
 	change = cleanupNodeMap(status.NodeBootIds, currentNodeNames) || change
-	change = cleanupNodeMap(status.NodePriority, currentNodeNames) || change
 
 	// Only set Updated flag if there were changes
 	if change {
diff --git a/operator/internal/controller/cluster_state_v2_test.go b/operator/internal/controller/cluster_state_v2_test.go
@@ -901,6 +901,9 @@ var _ = Describe("CleanupRemovedNodes", func() {
 		Expect(mockSkyhook.Status.ConfigUpdates).To(HaveKey("package2"))
 		Expect(mockSkyhook.Status.ConfigUpdates).To(HaveKey("package3"))
 
+		// Verify NodeOrderOffset was incremented for the removed node
+		Expect(mockSkyhook.Status.NodeOrderOffset).To(Equal(1))
+
 		// Verify that Updated flag was set since changes were made
 		Expect(mockSkyhook.Updated).To(BeTrue())
 	})
@@ -963,7 +966,10 @@ var _ = Describe("CleanupRemovedNodes", func() {
 		// ConfigUpdates should remain unchanged since it's keyed by package names, not node names
 		Expect(mockSkyhook.Status.ConfigUpdates).To(HaveKey("package1"))
 
-		// Verify that Updated flag was set since changes were made
+		// Verify NodeOrderOffset was NOT incremented
+		Expect(mockSkyhook.Status.NodeOrderOffset).To(Equal(0))
+
+		// Verify that Updated flag was NOT set since no changes were made
 		Expect(mockSkyhook.Updated).To(BeFalse())
 	})
 
diff --git a/operator/internal/controller/skyhook_controller.go b/operator/internal/controller/skyhook_controller.go
@@ -29,6 +29,7 @@ import (
 	"reflect"
 	"slices"
 	"sort"
+	"strconv"
 	"strings"
 	"time"
 
@@ -684,10 +685,7 @@ func (r *SkyhookReconciler) SaveNodesAndSkyhook(ctx context.Context, clusterStat
 				r.recorder.Eventf(node.GetNode(), EventTypeNormal, EventsReasonSkyhookStateChange, "Skyhook [%s] complete.", skyhook.GetSkyhook().Name)
 
 				// since node is complete remove from priority
-				if _, ok := skyhook.GetSkyhook().Status.NodePriority[node.GetNode().Name]; ok {
-					delete(skyhook.GetSkyhook().Status.NodePriority, node.GetNode().Name)
-					skyhook.GetSkyhook().Updated = true
-				}
+				skyhook.GetSkyhook().RemoveNodePriority(node.GetNode().Name)
 			}
 		}
 
@@ -1594,7 +1592,7 @@ func createInterruptPodForPackage(opts SkyhookOperatorOptions, _interrupt *v1alp
 					Name:  InterruptContainerName,
 					Image: getAgentImage(opts, _package),
 					Args:  []string{"interrupt", "/root", copyDir, argEncode},
-					Env:   getAgentConfigEnvVars(opts, _package.Name, _package.Version, skyhook.ResourceID(), skyhook.Name),
+					Env:   getAgentConfigEnvVars(opts, _package.Name, _package.Version, skyhook.ResourceID(), skyhook.Name, skyhook.NodeOrder(nodeName)),
 					SecurityContext: &corev1.SecurityContext{
 						Privileged: ptr(true),
 					},
@@ -1674,7 +1672,7 @@ func getPackageImage(_package *v1alpha1.Package) string {
 	return fmt.Sprintf("%s:%s", _package.Image, _package.Version)
 }
 
-func getAgentConfigEnvVars(opts SkyhookOperatorOptions, packageName string, packageVersion string, resourceID string, skyhookName string) []corev1.EnvVar {
+func getAgentConfigEnvVars(opts SkyhookOperatorOptions, packageName string, packageVersion string, resourceID string, skyhookName string, nodeOrder int) []corev1.EnvVar {
 	return []corev1.EnvVar{
 		{
 			Name:  "SKYHOOK_LOG_DIR",
@@ -1692,6 +1690,10 @@ func getAgentConfigEnvVars(opts SkyhookOperatorOptions, packageName string, pack
 			Name:  "SKYHOOK_RESOURCE_ID",
 			Value: fmt.Sprintf("%s_%s_%s", resourceID, packageName, packageVersion),
 		},
+		{
+			Name:  "SKYHOOK_NODE_ORDER",
+			Value: strconv.Itoa(nodeOrder),
+		},
 	}
 }
 
@@ -1765,7 +1767,7 @@ func createPodFromPackage(opts SkyhookOperatorOptions, _package *v1alpha1.Packag
 
 	agentEnvs := append(
 		_package.Env,
-		getAgentConfigEnvVars(opts, _package.Name, _package.Version, skyhook.ResourceID(), skyhook.Name)...,
+		getAgentConfigEnvVars(opts, _package.Name, _package.Version, skyhook.ResourceID(), skyhook.Name, skyhook.NodeOrder(nodeName))...,
 	)
 
 	pod := &corev1.Pod{
@@ -1931,7 +1933,7 @@ func podMatchesPackage(opts SkyhookOperatorOptions, _package *v1alpha1.Package,
 		// TODO: This is ignoring all the static env vars that are set by operator config.
 		// It probably should be just SKYHOOK_RESOURCE_ID that is ignored. Otherwise,
 		// a user will have to manually delete the pod to update the package when operator is updated.
-		dummyAgentEnv := getAgentConfigEnvVars(opts, "", "", "", "")
+		dummyAgentEnv := getAgentConfigEnvVars(opts, "", "", "", "", 0)
 		excludedEnvs := make([]string, len(dummyAgentEnv))
 		for i, env := range dummyAgentEnv {
 			excludedEnvs[i] = env.Name
diff --git a/operator/internal/controller/skyhook_controller_test.go b/operator/internal/controller/skyhook_controller_test.go
diff --git a/operator/internal/wrapper/skyhook.go b/operator/internal/wrapper/skyhook.go
diff --git a/operator/internal/wrapper/skyhook_test.go b/operator/internal/wrapper/skyhook_test.go
