diff --git a/docs/user-guide/cli.md b/docs/user-guide/cli.md index d3878e3cc..08f23cd6f 100644 --- a/docs/user-guide/cli.md +++ b/docs/user-guide/cli.md @@ -231,7 +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:** `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:** 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` 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 [:]` @@ -243,6 +245,12 @@ 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. 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/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..c592fa01e 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, 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 @@ -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 { @@ -360,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, @@ -384,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) @@ -414,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,5 +532,10 @@ 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 && !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 3c729cbe5..daab745a4 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,179 @@ 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("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()) + + 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 +744,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) {