From eb7efe730d7f8ef412d64b0289a2009364fbba70 Mon Sep 17 00:00:00 2001 From: Noah Kontur Date: Thu, 15 Oct 2026 22:55:35 -0400 Subject: [PATCH 1/4] fix(cli): clear Status.ConfigUpdates on nodewright reset A reset removes every node's state but left the per-package config update keys behind. With them present HasInterrupt ignores a package's own interrupt, so the restarted run could finish without running it. A full reset clears status.configUpdates; reset --package clears only that package's keys. Both run after every node reset succeeded, and not on --dry-run. A missing NodeWright is ignored, other patch failures warn. Fixes #776 Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Noah Kontur --- docs/user-guide/cli.md | 5 + k8s-tests/chainsaw/cli/reset/README.md | 2 + .../chainsaw/cli/reset/chainsaw-test.yaml | 23 ++- operator/cmd/cli/app/reset.go | 36 ++++- operator/cmd/cli/app/reset_test.go | 141 ++++++++++++++++++ 5 files changed, 205 insertions(+), 2 deletions(-) diff --git a/docs/user-guide/cli.md b/docs/user-guide/cli.md index d3878e3cc..e5fa1098b 100644 --- a/docs/user-guide/cli.md +++ b/docs/user-guide/cli.md @@ -231,6 +231,8 @@ kubectl nodewright reset gpu-init --package pkg1:1.0 --confirm --skip-batch-rese > **Note:** By default, `reset` also resets the deployment policy batch state so the next rollout starts from batch 1, and clears node ordering state (`NodeOrderOffset` and `NodePriority`) so `SKYHOOK_NODE_ORDER` restarts from `0`. Use `--skip-batch-reset` to preserve the existing batch and ordering state. > +> **Note:** A non-dry-run reset also clears the NodeWright's recorded config changes (`Status.ConfigUpdates`), so the restarted run is not affected by keys left by earlier ConfigMap edits; with `--package` only that package's keys are cleared. This happens regardless of `--skip-batch-reset`, and only after every node reset succeeded. A failed status patch prints a warning and the reset continues; a NodeWright that no longer exists is ignored. +> > **Note:** `reset` and `node reset` never uncordon a node. On nodes the NodeWright still selects they also leave its `nodewright.nvidia.com/cordon_` annotation in place, so the re-run releases the cordon when it completes; the operator does not release a cordon it holds no annotation for (see [Cordons NodeWright Did Not Create](../architecture/interrupt-flow.md#cordons-nodewright-did-not-create)). A full reset removes the annotation only where no re-run will come, because the NodeWright no longer exists or no longer selects the node; after that, uncordon the node yourself, only once no other `nodewright.nvidia.com/cordon_*` or `runtimeRequiredCordon` annotation remains and nothing else placed the cordon (see [Orphaned Cordon Recovery](../architecture/interrupt-flow.md#orphaned-cordon-recovery)). `reset --package` only edits the package's `nodeState` entry and never touches the annotation; it requires the NodeWright to exist. Against an operator after v0.19.1, CLI v0.4.0 and earlier remove the annotation unconditionally, so after a reset with one of them, `kubectl uncordon` the node once the re-run completes, subject to the same check. #### `--package [:]` @@ -243,6 +245,9 @@ each node's `nodeState` annotation instead of removing the whole annotation: are left untouched and reported in the command output. - If the package was the last entry in the annotation, the entire annotation is removed (matching the behavior of a full reset). +- The package's recorded config changes (`Status.ConfigUpdates[]`) are + cleared once every node was updated, so the re-run does not skip the + package's own interrupt. Other packages' keys are left alone. - Batch state is **deliberately not reset** on this path regardless of `--skip-batch-reset`: restarting the full rollout from batch 1 to recover a single package would be disproportionate. Pair with diff --git a/k8s-tests/chainsaw/cli/reset/README.md b/k8s-tests/chainsaw/cli/reset/README.md index 3103f67f0..77ce49837 100644 --- a/k8s-tests/chainsaw/cli/reset/README.md +++ b/k8s-tests/chainsaw/cli/reset/README.md @@ -12,12 +12,14 @@ Validates the nodewright reset command for resetting all nodes in a NodeWright. - Run `nodewright reset ` - Assert all nodes are reset to initial state 4. Verify node annotations are cleared +5. Seed `status.configUpdates` before the reset, and verify the reset clears it ## Key Features Tested - `nodewright reset` - Resets all nodes for a NodeWright - Node state cleanup - Annotation removal +- Recorded config changes (`status.configUpdates`) cleared ## Files diff --git a/k8s-tests/chainsaw/cli/reset/chainsaw-test.yaml b/k8s-tests/chainsaw/cli/reset/chainsaw-test.yaml index dcc47318e..3c8235146 100644 --- a/k8s-tests/chainsaw/cli/reset/chainsaw-test.yaml +++ b/k8s-tests/chainsaw/cli/reset/chainsaw-test.yaml @@ -51,7 +51,18 @@ spec: - assert: file: assert-nodewright-disabled.yaml - # Step 3: Test reset command + # Step 3: Record config changes the way an earlier ConfigMap edit would have, so reset has + # something to clear (they are per package and survive a node-state reset) + - name: seed-config-updates + try: + - script: + timeout: 30s + content: | + kubectl patch nodewright cli-reset-test --subresource=status --type=merge \ + -p '{"status":{"configUpdates":{"cli-reset-pkg":["game.properties"]}}}' + test -n "$(kubectl get nodewright cli-reset-test -o jsonpath='{.status.configUpdates}')" + + # Step 4: Test reset command - name: test-reset try: - script: @@ -60,3 +71,13 @@ spec: ../nodewright-cli reset cli-reset-test --confirm - assert: file: assert-nodes-reset.yaml + + # Step 5: Reset clears the recorded config changes, or the restarted run would skip a package's + # own interrupt + - name: config-updates-cleared + try: + - script: + timeout: 30s + content: | + out="$(kubectl get nodewright cli-reset-test -o jsonpath='{.status.configUpdates}')" + test -z "$out" || { echo "configUpdates not cleared: $out"; exit 1; } diff --git a/operator/cmd/cli/app/reset.go b/operator/cmd/cli/app/reset.go index 55a734da4..f5cdb4545 100644 --- a/operator/cmd/cli/app/reset.go +++ b/operator/cmd/cli/app/reset.go @@ -26,6 +26,7 @@ import ( "strings" "github.com/spf13/cobra" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/NVIDIA/nodewright/operator/api/nodewright/v1alpha1" @@ -108,7 +109,11 @@ that have state for the specified NodeWright. By default, this command also resets the deployment policy batch state, allowing the rollout to start fresh from batch 1. Use --skip-batch-reset to preserve the -existing batch state.`, +existing batch state. + +It also clears the config changes recorded in the NodeWright's status (only the +package's with --package), so keys from earlier ConfigMap edits do not carry into +the restarted run.`, Example: ` # Reset all nodes for gpu-init NodeWright kubectl nodewright reset gpu-init --confirm @@ -248,6 +253,13 @@ func runReset(ctx context.Context, cmd *cobra.Command, kubeClient *client.Client _, _ = fmt.Fprintf(cmd.OutOrStdout(), "\nSuccessfully reset %d node(s) for NodeWright %q\n", successCount, skyhookName) } + // Every node restarts from apply, so keys recorded by earlier config changes must not + // outlive the reset: they make the next full cycle skip the package's own interrupt. + // Skipped on partial failure, where some nodes still carry state that relies on them. + if successCount > 0 && len(updateErrors) == 0 { + resetConfigUpdatesForReset(ctx, cmd, kubeClient, skyhookName, "") + } + // Reset batch state unless --skip-batch-reset is set if !opts.skipBatchReset { resetBatchStateForReset(ctx, cmd, kubeClient, skyhookName) @@ -306,6 +318,24 @@ func resetNodeAnnotations( return successCount, updateErrors } +// resetConfigUpdatesForReset clears Status.ConfigUpdates, or only pkg's keys when pkg is set, +// if a dynamic client is available +func resetConfigUpdatesForReset(ctx context.Context, cmd *cobra.Command, kubeClient *client.Client, skyhookName, pkg string) { + if kubeClient.Dynamic() == nil { + return + } + + // Raw merge patch: null deletes the omitempty map or the package's entry + patch := []byte(`{"status":{"configUpdates":null}}`) + if pkg != "" { + patch, _ = json.Marshal(map[string]any{"status": map[string]any{"configUpdates": map[string]any{pkg: nil}}}) + } + if err := utils.PatchSkyhookStatusRaw(ctx, kubeClient.Dynamic(), skyhookName, patch); err != nil && !apierrors.IsNotFound(err) { + // NotFound: the NodeWright is gone (orphaned-cordon cleanup), so there is nothing to clear + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to clear config updates: %v\n", err) + } +} + // resetBatchStateForReset resets the batch state for a NodeWright if dynamic client is available func resetBatchStateForReset(ctx context.Context, cmd *cobra.Command, kubeClient *client.Client, skyhookName string) { if kubeClient.Dynamic() == nil { @@ -480,5 +510,9 @@ func runPackageReset( } _, _ = fmt.Fprintf(cmd.OutOrStdout(), "\nSuccessfully updated %d node(s)\n", success) + // Same reasoning as a full reset, scoped to this package's keys + if firstErr == nil { + resetConfigUpdatesForReset(ctx, cmd, kubeClient, skyhookName, name) + } return firstErr } diff --git a/operator/cmd/cli/app/reset_test.go b/operator/cmd/cli/app/reset_test.go index 3c729cbe5..3a23ca7c8 100644 --- a/operator/cmd/cli/app/reset_test.go +++ b/operator/cmd/cli/app/reset_test.go @@ -447,6 +447,8 @@ var _ = Describe("Reset Command", func() { mockDynamic.On("Resource", schema.GroupVersionResource{ Group: "nodewright.nvidia.com", Version: "v1alpha1", Resource: "nodewrights", }).Return(mockNSRes) + mockNSRes.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(&unstructured.Unstructured{}, nil).Maybe() kubeClient = client.NewWithClientsAndConfig(fakeKube, mockDynamic, nil) cliCtx = context.NewCLIContext(nil) out = &bytes.Buffer{} @@ -527,6 +529,143 @@ var _ = Describe("Reset Command", func() { }) }) + // Keys recorded by earlier config changes make the next full cycle skip the package's + // own interrupt, so a full reset has to clear them (issue #776). + Describe("config updates", func() { + var ( + fakeKube *fake.Clientset + mockNSRes *mockdynamic.NamespaceableResourceInterface + kubeClient *client.Client + cliCtx *context.CLIContext + cmd *cobra.Command + ) + const name = "demo" + const clearPatch = `{"status":{"configUpdates":null}}` + + BeforeEach(func() { + fakeKube = fake.NewClientset() + mockDynamic := &mockdynamic.Interface{} + mockNSRes = &mockdynamic.NamespaceableResourceInterface{} + mockDynamic.On("Resource", schema.GroupVersionResource{ + Group: "nodewright.nvidia.com", Version: "v1alpha1", Resource: "nodewrights", + }).Return(mockNSRes) + u := &unstructured.Unstructured{} + u.SetName(name) + u.SetAnnotations(map[string]string{"nodewright.nvidia.com/version": "v0.15.0"}) + mockNSRes.On("Get", mock.Anything, name, mock.Anything, mock.Anything).Return(u, nil) + mockNSRes.On("Patch", mock.Anything, name, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(&unstructured.Unstructured{}, nil).Maybe() + kubeClient = client.NewWithClientsAndConfig(fakeKube, mockDynamic, nil) + cliCtx = context.NewCLIContext(nil) + cmd = &cobra.Command{} + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err := fakeKube.CoreV1().Nodes().Create(gocontext.Background(), &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "worker-1", Annotations: map[string]string{nodeStateAnnotationPrefix + name: "{}"}}, + }, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + }) + + patchedWith := func(body string) bool { + for _, call := range mockNSRes.Calls { + if call.Method == "Patch" && string(call.Arguments.Get(3).([]byte)) == body { + return true + } + } + return false + } + + It("clears Status.ConfigUpdates on a full reset", func() { + opts := &resetOptions{confirm: true} + Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, cliCtx)).To(Succeed()) + + Expect(patchedWith(clearPatch)).To(BeTrue()) + }) + + It("clears it even when batch state is preserved", func() { + opts := &resetOptions{confirm: true, skipBatchReset: true} + Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, cliCtx)).To(Succeed()) + + Expect(patchedWith(clearPatch)).To(BeTrue()) + }) + + It("leaves it alone when resetting a node fails", func() { + fakeKube.PrependReactor("patch", "nodes", func(ktesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("apiserver unreachable") + }) + opts := &resetOptions{confirm: true} + Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, cliCtx)).To(Succeed()) + + Expect(patchedWith(clearPatch)).To(BeFalse()) + }) + + Describe("with --package", func() { + const pkgClearPatch = `{"status":{"configUpdates":{"pkg1":null}}}` + + BeforeEach(func() { + _, err := fakeKube.CoreV1().Nodes().Update(gocontext.Background(), &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "worker-1", Annotations: map[string]string{ + nodeStateAnnotationPrefix + name: `{"pkg1|1.0":{"name":"pkg1","version":"1.0","stage":"config","state":"complete"}}`, + }}, + }, metav1.UpdateOptions{}) + Expect(err).NotTo(HaveOccurred()) + }) + + It("clears only that package's keys", func() { + opts := &resetOptions{confirm: true, pkg: "pkg1"} + Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, cliCtx)).To(Succeed()) + + Expect(patchedWith(pkgClearPatch)).To(BeTrue()) + Expect(patchedWith(clearPatch)).To(BeFalse()) + }) + + It("leaves them alone when resetting a node fails", func() { + fakeKube.PrependReactor("patch", "nodes", func(ktesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("apiserver unreachable") + }) + opts := &resetOptions{confirm: true, pkg: "pkg1"} + Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, cliCtx)).NotTo(Succeed()) + + Expect(patchedWith(pkgClearPatch)).To(BeFalse()) + }) + + It("leaves them alone when no node holds the package", func() { + opts := &resetOptions{confirm: true, pkg: "other"} + Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, cliCtx)).To(Succeed()) + + Expect(patchedWith(`{"status":{"configUpdates":{"other":null}}}`)).To(BeFalse()) + }) + }) + + It("does not warn when the NodeWright is already gone", func() { + mockNSRes.ExpectedCalls = nil + mockNSRes.On("Get", mock.Anything, name, mock.Anything, mock.Anything).Return( + (*unstructured.Unstructured)(nil), + apierrors.NewNotFound(schema.GroupResource{Group: "nodewright.nvidia.com", Resource: "nodewrights"}, name)) + mockNSRes.On("Patch", mock.Anything, name, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return((*unstructured.Unstructured)(nil), + apierrors.NewNotFound(schema.GroupResource{Group: "nodewright.nvidia.com", Resource: "nodewrights"}, name)) + stderr := &bytes.Buffer{} + cmd.SetErr(stderr) + + opts := &resetOptions{confirm: true, skipBatchReset: true} + Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, cliCtx)).To(Succeed()) + + Expect(patchedWith(clearPatch)).To(BeTrue()) + Expect(stderr.String()).NotTo(ContainSubstring("clear config updates")) + }) + + It("leaves it alone on a dry run", func() { + dryRunCtx := context.NewCLIContext(context.NewCLIConfig()) + dryRunCtx.GlobalFlags.DryRun = true + opts := &resetOptions{confirm: true} + Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, dryRunCtx)).To(Succeed()) + + Expect(patchedWith(clearPatch)).To(BeFalse()) + }) + }) + Describe("--package", func() { var ( fakeKube *fake.Clientset @@ -569,6 +708,8 @@ var _ = Describe("Reset Command", func() { mockNSRes := &mockdynamic.NamespaceableResourceInterface{} mockDynamic.On("Resource", skyhookGVR).Return(mockNSRes) mockNSRes.On("Get", mock.Anything, name, mock.Anything, mock.Anything).Return(u, nil) + mockNSRes.On("Patch", mock.Anything, name, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(&unstructured.Unstructured{}, nil).Maybe() } addNode := func(name string, ns v1alpha1.NodeState) { From ebdd7bfba1f75ef66c5ad934b9d02a3b74f802a7 Mon Sep 17 00:00:00 2001 From: Noah Kontur Date: Fri, 16 Oct 2026 13:40:46 -0400 Subject: [PATCH 2/4] docs(cli): describe when reset clears config updates Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Noah Kontur --- docs/user-guide/cli.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/user-guide/cli.md b/docs/user-guide/cli.md index e5fa1098b..4bcace20a 100644 --- a/docs/user-guide/cli.md +++ b/docs/user-guide/cli.md @@ -231,7 +231,7 @@ kubectl nodewright reset gpu-init --package pkg1:1.0 --confirm --skip-batch-rese > **Note:** By default, `reset` also resets the deployment policy batch state so the next rollout starts from batch 1, and clears node ordering state (`NodeOrderOffset` and `NodePriority`) so `SKYHOOK_NODE_ORDER` restarts from `0`. Use `--skip-batch-reset` to preserve the existing batch and ordering state. > -> **Note:** A non-dry-run reset also clears the NodeWright's recorded config changes (`Status.ConfigUpdates`), so the restarted run is not affected by keys left by earlier ConfigMap edits; with `--package` only that package's keys are cleared. This happens regardless of `--skip-batch-reset`, and only after every node reset succeeded. A failed status patch prints a warning and the reset continues; a NodeWright that no longer exists is ignored. +> **Note:** During a non-dry-run reset, `reset` attempts to clear the NodeWright's recorded config changes (`Status.ConfigUpdates`) after at least one node reset succeeds and no node-state update error occurs, so the restarted run is not affected by keys left by earlier ConfigMap edits. With `--package` only that package's keys are cleared. This is independent of `--skip-batch-reset`. If the status patch fails for an error other than `NotFound`, `reset` prints a warning and continues; `NotFound` (the NodeWright no longer exists) is ignored. > > **Note:** `reset` and `node reset` never uncordon a node. On nodes the NodeWright still selects they also leave its `nodewright.nvidia.com/cordon_` annotation in place, so the re-run releases the cordon when it completes; the operator does not release a cordon it holds no annotation for (see [Cordons NodeWright Did Not Create](../architecture/interrupt-flow.md#cordons-nodewright-did-not-create)). A full reset removes the annotation only where no re-run will come, because the NodeWright no longer exists or no longer selects the node; after that, uncordon the node yourself, only once no other `nodewright.nvidia.com/cordon_*` or `runtimeRequiredCordon` annotation remains and nothing else placed the cordon (see [Orphaned Cordon Recovery](../architecture/interrupt-flow.md#orphaned-cordon-recovery)). `reset --package` only edits the package's `nodeState` entry and never touches the annotation; it requires the NodeWright to exist. Against an operator after v0.19.1, CLI v0.4.0 and earlier remove the annotation unconditionally, so after a reset with one of them, `kubectl uncordon` the node once the re-run completes, subject to the same check. From 68a10c4e41386b883dd08b265a7d673022afcac0 Mon Sep 17 00:00:00 2001 From: Noah Kontur Date: Tue, 20 Oct 2026 12:28:51 -0400 Subject: [PATCH 3/4] fix(cli): keep configUpdates when another version of the package remains reset --package name:version cleared configUpdates[name], which is keyed by name only, while nodes could still hold another version of the package. Skip the clear in that case, and reword the docs and help to match. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Noah Kontur --- docs/user-guide/cli.md | 9 +++++--- operator/cmd/cli/app/reset.go | 28 ++++++++++++++++++++---- operator/cmd/cli/app/reset_test.go | 35 ++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/docs/user-guide/cli.md b/docs/user-guide/cli.md index 4bcace20a..08f23cd6f 100644 --- a/docs/user-guide/cli.md +++ b/docs/user-guide/cli.md @@ -231,9 +231,9 @@ kubectl nodewright reset gpu-init --package pkg1:1.0 --confirm --skip-batch-rese > **Note:** By default, `reset` also resets the deployment policy batch state so the next rollout starts from batch 1, and clears node ordering state (`NodeOrderOffset` and `NodePriority`) so `SKYHOOK_NODE_ORDER` restarts from `0`. Use `--skip-batch-reset` to preserve the existing batch and ordering state. > -> **Note:** During a non-dry-run reset, `reset` attempts to clear the NodeWright's recorded config changes (`Status.ConfigUpdates`) after at least one node reset succeeds and no node-state update error occurs, so the restarted run is not affected by keys left by earlier ConfigMap edits. With `--package` only that package's keys are cleared. This is independent of `--skip-batch-reset`. If the status patch fails for an error other than `NotFound`, `reset` prints a warning and continues; `NotFound` (the NodeWright no longer exists) is ignored. +> **Note:** During a non-dry-run reset, `reset` attempts to clear the NodeWright's recorded config changes (`Status.ConfigUpdates`) after at least one node reset succeeds and no node-state update error occurs, so the restarted run is not affected by keys left by earlier ConfigMap edits. With `--package` only that package's keys are cleared, and only if no node still holds another version of it. This is independent of `--skip-batch-reset`. If the status patch fails for an error other than `NotFound`, `reset` prints a warning and continues; `NotFound` (the NodeWright no longer exists) is ignored. > -> **Note:** `reset` and `node reset` never uncordon a node. On nodes the NodeWright still selects they also leave its `nodewright.nvidia.com/cordon_` annotation in place, so the re-run releases the cordon when it completes; the operator does not release a cordon it holds no annotation for (see [Cordons NodeWright Did Not Create](../architecture/interrupt-flow.md#cordons-nodewright-did-not-create)). A full reset removes the annotation only where no re-run will come, because the NodeWright no longer exists or no longer selects the node; after that, uncordon the node yourself, only once no other `nodewright.nvidia.com/cordon_*` or `runtimeRequiredCordon` annotation remains and nothing else placed the cordon (see [Orphaned Cordon Recovery](../architecture/interrupt-flow.md#orphaned-cordon-recovery)). `reset --package` only edits the package's `nodeState` entry and never touches the annotation; it requires the NodeWright to exist. Against an operator after v0.19.1, CLI v0.4.0 and earlier remove the annotation unconditionally, so after a reset with one of them, `kubectl uncordon` the node once the re-run completes, subject to the same check. +> **Note:** `reset` and `node reset` never uncordon a node. On nodes the NodeWright still selects they also leave its `nodewright.nvidia.com/cordon_` annotation in place, so the re-run releases the cordon when it completes; the operator does not release a cordon it holds no annotation for (see [Cordons NodeWright Did Not Create](../architecture/interrupt-flow.md#cordons-nodewright-did-not-create)). A full reset removes the annotation only where no re-run will come, because the NodeWright no longer exists or no longer selects the node; after that, uncordon the node yourself, only once no other `nodewright.nvidia.com/cordon_*` or `runtimeRequiredCordon` annotation remains and nothing else placed the cordon (see [Orphaned Cordon Recovery](../architecture/interrupt-flow.md#orphaned-cordon-recovery)). `reset --package` edits only the package's `nodeState` entry and its `configUpdates` keys, and never touches the annotation; it requires the NodeWright to exist. Against an operator after v0.19.1, CLI v0.4.0 and earlier remove the annotation unconditionally, so after a reset with one of them, `kubectl uncordon` the node once the re-run completes, subject to the same check. #### `--package [:]` @@ -247,7 +247,10 @@ each node's `nodeState` annotation instead of removing the whole annotation: is removed (matching the behavior of a full reset). - The package's recorded config changes (`Status.ConfigUpdates[]`) are cleared once every node was updated, so the re-run does not skip the - package's own interrupt. Other packages' keys are left alone. + package's own interrupt. They are keyed by name only, so they are kept while + any node still holds another version of the package (for example after + `pkg1:2.0` when nodes also hold `pkg1:1.0`). Other packages' keys are left + alone. - Batch state is **deliberately not reset** on this path regardless of `--skip-batch-reset`: restarting the full rollout from batch 1 to recover a single package would be disproportionate. Pair with diff --git a/operator/cmd/cli/app/reset.go b/operator/cmd/cli/app/reset.go index f5cdb4545..f97c2eb97 100644 --- a/operator/cmd/cli/app/reset.go +++ b/operator/cmd/cli/app/reset.go @@ -112,8 +112,8 @@ the rollout to start fresh from batch 1. Use --skip-batch-reset to preserve the existing batch state. It also clears the config changes recorded in the NodeWright's status (only the -package's with --package), so keys from earlier ConfigMap edits do not carry into -the restarted run.`, +package's with --package, and only if no node keeps another version of it), so keys +from earlier ConfigMap edits do not carry into the restarted run.`, Example: ` # Reset all nodes for gpu-init NodeWright kubectl nodewright reset gpu-init --confirm @@ -480,6 +480,25 @@ func runPackageReset( } } + // After the reset, does any node still hold an entry for this package (another version)? + stillHeld := func() bool { + after := make(map[string]v1alpha1.NodeState, len(nodeStates)) + for n, ns := range nodeStates { + after[n] = ns + } + for _, t := range targets { + after[t.nodeName] = t.remaining + } + for _, ns := range after { + for _, status := range ns { + if status.Name == name { + return true + } + } + } + return false + } + var firstErr error success := 0 for _, t := range targets { @@ -510,8 +529,9 @@ func runPackageReset( } _, _ = fmt.Fprintf(cmd.OutOrStdout(), "\nSuccessfully updated %d node(s)\n", success) - // Same reasoning as a full reset, scoped to this package's keys - if firstErr == nil { + // Same reasoning as a full reset, scoped to this package's keys. The keys are per name, so + // keep them while any node still holds another version of the package. + if firstErr == nil && !stillHeld() { resetConfigUpdatesForReset(ctx, cmd, kubeClient, skyhookName, name) } return firstErr diff --git a/operator/cmd/cli/app/reset_test.go b/operator/cmd/cli/app/reset_test.go index 3a23ca7c8..56756c3d9 100644 --- a/operator/cmd/cli/app/reset_test.go +++ b/operator/cmd/cli/app/reset_test.go @@ -630,6 +630,41 @@ var _ = Describe("Reset Command", func() { Expect(patchedWith(pkgClearPatch)).To(BeFalse()) }) + It("keeps them when a node still holds another version of the package", func() { + _, err := fakeKube.CoreV1().Nodes().Update(gocontext.Background(), &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "worker-1", Annotations: map[string]string{ + nodeStateAnnotationPrefix + name: `{"pkg1|1.0":{"name":"pkg1","version":"1.0","stage":"config","state":"complete"},"pkg1|2.0":{"name":"pkg1","version":"2.0","stage":"config","state":"complete"}}`, + }}, + }, metav1.UpdateOptions{}) + Expect(err).NotTo(HaveOccurred()) + + opts := &resetOptions{confirm: true, pkg: "pkg1:2.0"} + Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, cliCtx)).To(Succeed()) + + Expect(patchedWith(pkgClearPatch)).To(BeFalse()) + }) + + It("keeps them when an untargeted node holds another version", func() { + _, err := fakeKube.CoreV1().Nodes().Create(gocontext.Background(), &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "worker-2", Annotations: map[string]string{ + nodeStateAnnotationPrefix + name: `{"pkg1|2.0":{"name":"pkg1","version":"2.0","stage":"config","state":"complete"}}`, + }}, + }, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + + opts := &resetOptions{confirm: true, pkg: "pkg1:1.0"} + Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, cliCtx)).To(Succeed()) + + Expect(patchedWith(pkgClearPatch)).To(BeFalse()) + }) + + It("clears them when a version-filtered reset leaves no other version", func() { + opts := &resetOptions{confirm: true, pkg: "pkg1:1.0"} + Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, cliCtx)).To(Succeed()) + + Expect(patchedWith(pkgClearPatch)).To(BeTrue()) + }) + It("leaves them alone when no node holds the package", func() { opts := &resetOptions{confirm: true, pkg: "other"} Expect(runReset(gocontext.Background(), cmd, kubeClient, name, opts, cliCtx)).To(Succeed()) From 10adc771adbad50bd1b937dd80cc4cbf91e2132f Mon Sep 17 00:00:00 2001 From: Noah Kontur Date: Tue, 20 Oct 2026 15:18:11 -0400 Subject: [PATCH 4/4] fix(cli): satisfy gocyclo and lll in the package reset Move the still-held check out of runPackageReset and wrap a long test line. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Noah Kontur --- operator/cmd/cli/app/reset.go | 57 ++++++++++++++++-------------- operator/cmd/cli/app/reset_test.go | 3 +- 2 files changed, 32 insertions(+), 28 deletions(-) diff --git a/operator/cmd/cli/app/reset.go b/operator/cmd/cli/app/reset.go index f97c2eb97..c592fa01e 100644 --- a/operator/cmd/cli/app/reset.go +++ b/operator/cmd/cli/app/reset.go @@ -390,6 +390,33 @@ func splitPackageRef(s string) (name, version string, hasVersion bool, err error // state (which restarts the entire rollout from batch 1) would be wildly // disproportionate to a one-package recovery, so this path deliberately // skips it regardless of --skip-batch-reset. +// packageResetTarget is one node a package reset edits, with the state it keeps. +type packageResetTarget struct { + nodeName string + remaining v1alpha1.NodeState + emptied bool +} + +// packageStillHeld reports whether any node still holds an entry for the package once the +// targets have their remaining state, that is, another version of it. +func packageStillHeld(nodeStates map[string]v1alpha1.NodeState, targets []packageResetTarget, name string) bool { + after := make(map[string]v1alpha1.NodeState, len(nodeStates)) + for n, ns := range nodeStates { + after[n] = ns + } + for _, t := range targets { + after[t.nodeName] = t.remaining + } + for _, ns := range after { + for _, status := range ns { + if status.Name == name { + return true + } + } + } + return false +} + func runPackageReset( ctx context.Context, cmd *cobra.Command, @@ -414,12 +441,7 @@ func runPackageReset( annotationKey := nodeStateAnnotationPrefix + skyhookName - type target struct { - nodeName string - remaining v1alpha1.NodeState - emptied bool - } - var targets []target + var targets []packageResetTarget nodeNames := make([]string, 0, len(nodeStates)) for n := range nodeStates { nodeNames = append(nodeNames, n) @@ -444,7 +466,7 @@ func runPackageReset( if !matched { continue } - targets = append(targets, target{ + targets = append(targets, packageResetTarget{ nodeName: nodeName, remaining: next, emptied: len(next) == 0, @@ -480,25 +502,6 @@ func runPackageReset( } } - // After the reset, does any node still hold an entry for this package (another version)? - stillHeld := func() bool { - after := make(map[string]v1alpha1.NodeState, len(nodeStates)) - for n, ns := range nodeStates { - after[n] = ns - } - for _, t := range targets { - after[t.nodeName] = t.remaining - } - for _, ns := range after { - for _, status := range ns { - if status.Name == name { - return true - } - } - } - return false - } - var firstErr error success := 0 for _, t := range targets { @@ -531,7 +534,7 @@ func runPackageReset( _, _ = fmt.Fprintf(cmd.OutOrStdout(), "\nSuccessfully updated %d node(s)\n", success) // Same reasoning as a full reset, scoped to this package's keys. The keys are per name, so // keep them while any node still holds another version of the package. - if firstErr == nil && !stillHeld() { + if firstErr == nil && !packageStillHeld(nodeStates, targets, name) { resetConfigUpdatesForReset(ctx, cmd, kubeClient, skyhookName, name) } return firstErr diff --git a/operator/cmd/cli/app/reset_test.go b/operator/cmd/cli/app/reset_test.go index 56756c3d9..daab745a4 100644 --- a/operator/cmd/cli/app/reset_test.go +++ b/operator/cmd/cli/app/reset_test.go @@ -633,7 +633,8 @@ var _ = Describe("Reset Command", func() { It("keeps them when a node still holds another version of the package", func() { _, err := fakeKube.CoreV1().Nodes().Update(gocontext.Background(), &corev1.Node{ ObjectMeta: metav1.ObjectMeta{Name: "worker-1", Annotations: map[string]string{ - nodeStateAnnotationPrefix + name: `{"pkg1|1.0":{"name":"pkg1","version":"1.0","stage":"config","state":"complete"},"pkg1|2.0":{"name":"pkg1","version":"2.0","stage":"config","state":"complete"}}`, + nodeStateAnnotationPrefix + name: `{"pkg1|1.0":{"name":"pkg1","version":"1.0","stage":"config","state":"complete"},` + + `"pkg1|2.0":{"name":"pkg1","version":"2.0","stage":"config","state":"complete"}}`, }}, }, metav1.UpdateOptions{}) Expect(err).NotTo(HaveOccurred())