From 373c34381a99f1e181bd54c4909f196ca4bd7082 Mon Sep 17 00:00:00 2001 From: rohithb Date: Fri, 21 Aug 2026 03:20:35 +0530 Subject: [PATCH 1/6] fix(cli): verify ICMSRequest deletion before reporting kill-all/kill-function success --- .../nvcf-cli/cmd/cluster_agent_maintenance.go | 19 ++++- .../internal/clusteragent/k8s_maintainer.go | 84 ++++++++++++++++--- .../clusteragent/k8s_maintainer_test.go | 64 ++++++++++++++ .../internal/clusteragent/maintainer.go | 23 ++++- 4 files changed, 171 insertions(+), 19 deletions(-) diff --git a/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go b/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go index 966900c1b..f9e794d73 100644 --- a/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go +++ b/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go @@ -150,6 +150,7 @@ func initClusterAgentMaintenanceCmds() { for _, c := range []*cobra.Command{clusterAgentKillFunctionCmd, clusterAgentKillAllCmd} { c.Flags().String(flagReason, "", "Optional reason recorded in logs for audit") c.Flags().Bool(flagForce, false, "Strip finalizers so requests stuck Terminating are removed") + c.Flags().Duration(flagTimeout, clusteragent.DefaultKillTimeout, "How long to wait for NVCA to finish evicting a terminated request before reporting it as still terminating") } clusterAgentKillAllCmd.Flags().String(flagConfirm, "", "Cluster name confirming kill-all (required with --yes)") } @@ -251,6 +252,7 @@ func runClusterAgentKillFunction(cmd *cobra.Command, args []string) error { force, _ := cmd.Flags().GetBool(flagForce) reason, _ := cmd.Flags().GetString(flagReason) expect, _ := cmd.Flags().GetString(flagExpectClusterID) + timeout, _ := cmd.Flags().GetDuration(flagTimeout) ctx := context.Background() @@ -278,6 +280,7 @@ func runClusterAgentKillFunction(cmd *cobra.Command, args []string) error { Reason: reason, DryRun: dryRun, Force: force, + Timeout: timeout, }) return finishKill(cmd, res, err) } @@ -295,6 +298,7 @@ func runClusterAgentKillAll(cmd *cobra.Command, _ []string) error { reason, _ := cmd.Flags().GetString(flagReason) expect, _ := cmd.Flags().GetString(flagExpectClusterID) confirm, _ := cmd.Flags().GetString(flagConfirm) + timeout, _ := cmd.Flags().GetDuration(flagTimeout) ctx := context.Background() @@ -346,6 +350,7 @@ func runClusterAgentKillAll(cmd *cobra.Command, _ []string) error { Reason: reason, DryRun: dryRun, Force: force, + Timeout: timeout, }) return finishKill(cmd, res, err) } @@ -480,21 +485,27 @@ func printKillResult(cmd *cobra.Command, res *clusteragent.KillResult) { prefix = "[dry-run] " verbed = "would terminate" } - fmt.Fprintf(w, "%s%s %d request(s) in namespace %s\n", prefix, verbed, len(res.Affected)-res.FailedCount, res.RequestsNamespace) + deletedCount := len(res.Affected) - res.FailedCount - res.TerminatingCount + fmt.Fprintf(w, "%s%s %d request(s) in namespace %s\n", prefix, verbed, deletedCount, res.RequestsNamespace) if res.Reason != "" { fmt.Fprintf(w, " reason: %s\n", res.Reason) } for _, r := range res.Affected { status := "deleted" - if res.DryRun { + switch { + case res.DryRun: status = "would delete" - } - if r.Error != "" { + case r.Error != "": status = "FAILED: " + r.Error + case r.Terminating: + status = "terminating: NVCA has not finished evicting the workload yet" } fmt.Fprintf(w, " %s/%s function=%s version=%s [%s]\n", r.Namespace, r.Name, orDash(r.FunctionID), orDash(r.FunctionVersionID), status) } if res.FailedCount > 0 { fmt.Fprintf(w, "%d of %d request(s) failed\n", res.FailedCount, len(res.Affected)) } + if res.TerminatingCount > 0 { + fmt.Fprintf(w, "%d of %d request(s) still terminating; re-check with cluster agent get-function\n", res.TerminatingCount, len(res.Affected)) + } } diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go index ca913fc86..78dfe0af0 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -56,6 +56,11 @@ const ( // is a var so tests can shorten it. var rolloutPollInterval = 2 * time.Second +// killDeletionPollInterval bounds how often deleteICMSRequest polls for the +// ICMSRequest to actually disappear after Delete is called. It is a var so +// tests can shorten it. +var killDeletionPollInterval = 2 * time.Second + // k8sMaintainer mutates NVCA state on a compute-plane cluster. It uses the // dynamic client for the ICMSRequest and NVCFBackend custom resources and the // typed clientset for the agent-config ConfigMap and the NVCA Deployment. @@ -373,6 +378,11 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, Affected: []KilledRequest{}, } + timeout := opts.Timeout + if timeout <= 0 { + timeout = DefaultKillTimeout + } + for i := range items { fid, vid := functionIdentity(items[i].Object) if !match(fid, vid) { @@ -385,10 +395,18 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, FunctionVersionID: vid, } if !opts.DryRun { - if err := m.deleteICMSRequest(ctx, killed.Namespace, killed.Name, opts.Force); err != nil { + terminating, err := m.deleteICMSRequest(ctx, killed.Namespace, killed.Name, opts.Force, timeout) + switch { + case err != nil: killed.Error = err.Error() result.FailedCount++ - } else { + case terminating: + // The delete was accepted (deletionTimestamp set) but NVCA had not + // removed its finalizer and evicted the workload by the deadline. + // This is not a failure to report deletion as complete when it is not. + killed.Terminating = true + result.TerminatingCount++ + default: // Audit line for the termination, including the operator-supplied // reason. Carried in the result too, but this emits it to logs. logging.Info("terminated ICMSRequest %s/%s (function=%s version=%s) reason=%q", @@ -400,20 +418,58 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, return result, nil } -// deleteICMSRequest deletes one ICMSRequest. When force is set, it first strips -// finalizers so a CR stuck Terminating is removed even if NVCA is not running. -// A NotFound on delete is treated as success (the reconciler raced us). -func (m *k8sMaintainer) deleteICMSRequest(ctx context.Context, namespace, name string, force bool) error { +// deleteICMSRequest deletes one ICMSRequest and waits up to timeout for it to +// actually disappear. When force is set, it first strips finalizers so a CR +// stuck Terminating is removed even if NVCA is not running. +// +// Delete() only guarantees the deletion was accepted: when the object carries +// a finalizer (nvca.finalizers.nvidia.io), the API server sets +// deletionTimestamp and returns success while the object, and the pod it +// owns, keep running until the NVCA reconciler removes the finalizer. Callers +// must not treat a nil error from Delete alone as "the resource is gone." +// +// Returns (terminating=true, nil) when the delete was accepted but the object +// still existed when the wait deadline elapsed. A NotFound at any point +// (delete or poll) is treated as success (the reconciler raced us). +func (m *k8sMaintainer) deleteICMSRequest(ctx context.Context, namespace, name string, force bool, timeout time.Duration) (bool, error) { if force { if err := m.stripFinalizers(ctx, namespace, name); err != nil { - return err + return false, err } } err := m.dc.Resource(icmsRequestGVR).Namespace(namespace).Delete(ctx, name, metav1.DeleteOptions{}) - if err != nil && !apierrors.IsNotFound(err) { - return err + if err != nil { + if apierrors.IsNotFound(err) { + return false, nil + } + return false, err + } + return m.waitForICMSRequestGone(ctx, namespace, name, timeout) +} + +// waitForICMSRequestGone polls until the ICMSRequest is gone or timeout +// elapses. It returns (true, nil) rather than an error on timeout: the +// request was validly accepted for deletion, it just has not finished yet. +func (m *k8sMaintainer) waitForICMSRequestGone(ctx context.Context, namespace, name string, timeout time.Duration) (bool, error) { + deadline := time.Now().Add(timeout) + for { + _, err := m.dc.Resource(icmsRequestGVR).Namespace(namespace).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + return false, nil + } + return false, err + } + + if time.Now().After(deadline) { + return true, nil + } + select { + case <-ctx.Done(): + return false, ctx.Err() + case <-time.After(killDeletionPollInterval): + } } - return nil } // stripFinalizers clears the finalizers on an ICMSRequest, mirroring the @@ -442,10 +498,14 @@ func (m *k8sMaintainer) stripFinalizers(ctx context.Context, namespace, name str } func aggregateKillError(result *KillResult) error { - if result.FailedCount == 0 { + switch { + case result.FailedCount > 0: + return fmt.Errorf("failed to terminate %d of %d ICMSRequest(s)", result.FailedCount, len(result.Affected)) + case result.TerminatingCount > 0: + return fmt.Errorf("%d of %d ICMSRequest(s) still terminating: NVCA has not finished evicting the workload; re-check with cluster agent get-function", result.TerminatingCount, len(result.Affected)) + default: return nil } - return fmt.Errorf("failed to terminate %d of %d ICMSRequest(s)", result.FailedCount, len(result.Affected)) } // --- agent-config YAML edits --- diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go index ccbdb0711..21618b3bf 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go @@ -667,6 +667,70 @@ func TestKillForceDeletesFinalizedRequest(t *testing.T) { } } +// TestKillReportsTerminatingWhenFinalizerBlocksDeletion is a regression test +// for the false-positive "[deleted]" report: when Delete is accepted but a +// finalizer keeps the object present (the real-world behavior when NVCA has +// not evicted the workload yet), the fake dynamic client's default tracker +// removes the object immediately regardless of finalizers, so a delete +// reactor is used to simulate the object surviving Delete, mirroring a real +// API server with a finalizer still set. +func TestKillReportsTerminatingWhenFinalizerBlocksDeletion(t *testing.T) { + orig := killDeletionPollInterval + killDeletionPollInterval = time.Millisecond + t.Cleanup(func() { killDeletionPollInterval = orig }) + + cr := icmsRequestWithFinalizers(testRequestsNS, "r1", "fn-1", "v1", "nvca.finalizers.nvidia.io") + m, dc, _ := newFakeMaintainer([]runtime.Object{defaultBackend(), cr}, nil) + dc.PrependReactor("delete", "icmsrequests", func(action k8stesting.Action) (bool, runtime.Object, error) { + // Simulate the real API server: the delete is accepted (no error) + // but the object, carrying a finalizer, is not actually removed. + return true, nil, nil + }) + + res, err := m.KillFunction(context.Background(), "fn-1", "v1", KillOptions{ + BackendNS: testBackendNS, + Timeout: 5 * time.Millisecond, + }) + if err == nil { + t.Fatal("expected an error reporting the request is still terminating") + } + if !strings.Contains(err.Error(), "terminating") { + t.Errorf("error = %q, want it to mention terminating", err.Error()) + } + if res.TerminatingCount != 1 || res.FailedCount != 0 { + t.Fatalf("TerminatingCount/FailedCount = %d/%d, want 1/0", res.TerminatingCount, res.FailedCount) + } + if len(res.Affected) != 1 || !res.Affected[0].Terminating || res.Affected[0].Error != "" { + t.Fatalf("affected = %+v, want a single non-error Terminating entry", res.Affected) + } + if !icmsExists(t, dc, testRequestsNS, "r1") { + t.Error("r1 must still exist: it was never actually removed, only marked for deletion") + } +} + +// TestKillWithinTimeoutReportsDeletedNotTerminating confirms the happy path +// still reports plain "deleted" (not terminating) when the object disappears +// before the deadline: the poll loop must not itself introduce a false +// negative on a normal, fast reconcile. +func TestKillWithinTimeoutReportsDeletedNotTerminating(t *testing.T) { + orig := killDeletionPollInterval + killDeletionPollInterval = time.Millisecond + t.Cleanup(func() { killDeletionPollInterval = orig }) + + m, _, _ := newFakeMaintainer(killSeed(), nil) + + res, err := m.KillFunction(context.Background(), "fn-1", "v2", KillOptions{ + BackendNS: testBackendNS, + Timeout: 50 * time.Millisecond, + }) + if err != nil { + t.Fatalf("KillFunction returned error: %v", err) + } + if res.TerminatingCount != 0 || len(res.Affected) != 1 || res.Affected[0].Terminating { + t.Fatalf("unexpected result: %+v", res) + } +} + func TestResolveClusterAppliesNamespaceDefaults(t *testing.T) { // Backend with no system/requests namespace set. b := backendObj(testBackendNS, testClusterID, testCluster, "", "") diff --git a/src/clis/nvcf-cli/internal/clusteragent/maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/maintainer.go index 8ea88b61a..0405840a6 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/maintainer.go @@ -22,6 +22,10 @@ import ( "time" ) +// DefaultKillTimeout bounds how long KillFunction/KillAll wait for a deleted +// ICMSRequest to actually disappear before reporting it as still Terminating. +const DefaultKillTimeout = 60 * time.Second + // AgentMaintainer performs maintenance mutations against a compute-plane // cluster's NVCA. It is the write-side counterpart to AgentInspector: drain and // undrain toggle CordonAndDrain maintenance on the NVCA agent-config ConfigMap @@ -88,6 +92,11 @@ type KillOptions struct { // Force strips finalizers before deleting, so a CR stuck Terminating is // removed even when NVCA is not running to process its finalizer. Force bool + // Timeout bounds how long to wait, after issuing the delete, for the + // ICMSRequest to actually disappear (NVCA's finalizer removed). A request + // still present when the timeout elapses is reported as Terminating, not + // deleted. Zero uses DefaultKillTimeout. + Timeout time.Duration } // DrainResult is the outcome of a Drain or Undrain. @@ -103,14 +112,21 @@ type DrainResult struct { Message string `json:"message,omitempty"` } -// KilledRequest is one ICMSRequest targeted by a kill operation. Error is set -// when that CR failed to delete; otherwise it was deleted (or would be, in a -// dry run). +// KilledRequest is one ICMSRequest targeted by a kill operation. +// +// - Error set: the delete call itself failed. +// - Terminating true (Error empty): the delete was accepted and +// deletionTimestamp was set, but the object still existed with its +// finalizer when the wait timed out. NVCA has not finished evicting the +// workload; the request is not actually gone yet. +// - Neither set: the object was confirmed gone (or, in a dry run, would be +// deleted). type KilledRequest struct { Namespace string `json:"namespace"` Name string `json:"name"` FunctionID string `json:"functionId,omitempty"` FunctionVersionID string `json:"functionVersionId,omitempty"` + Terminating bool `json:"terminating,omitempty"` Error string `json:"error,omitempty"` } @@ -122,5 +138,6 @@ type KillResult struct { Reason string `json:"reason,omitempty"` Affected []KilledRequest `json:"affected"` FailedCount int `json:"failedCount"` + TerminatingCount int `json:"terminatingCount"` DryRun bool `json:"dryRun"` } From a187bcd7e2a67afbd31b6fb22b1c44ea2bcdcd1f Mon Sep 17 00:00:00 2001 From: rohithb Date: Fri, 21 Aug 2026 13:04:03 +0530 Subject: [PATCH 2/6] fix(cli): address CodeRabbit review findings on kill-all deletion verification --- .../cmd/cluster_agent_maintenance_test.go | 74 +++++++++++++++ .../internal/clusteragent/k8s_maintainer.go | 72 ++++++++++++--- .../clusteragent/k8s_maintainer_test.go | 89 +++++++++++++++++++ .../internal/clusteragent/maintainer.go | 4 +- 4 files changed, 225 insertions(+), 14 deletions(-) diff --git a/src/clis/nvcf-cli/cmd/cluster_agent_maintenance_test.go b/src/clis/nvcf-cli/cmd/cluster_agent_maintenance_test.go index 863809300..bf97ad034 100644 --- a/src/clis/nvcf-cli/cmd/cluster_agent_maintenance_test.go +++ b/src/clis/nvcf-cli/cmd/cluster_agent_maintenance_test.go @@ -25,6 +25,7 @@ import ( "os" "strings" "testing" + "time" "nvcf-cli/internal/clusteragent" @@ -391,6 +392,41 @@ func TestKillFunctionPartialFailureReturnsError(t *testing.T) { } } +func TestKillFunctionTimeoutFlagForwarded(t *testing.T) { + f := &fakeMaintainer{killResult: &clusteragent.KillResult{RequestsNamespace: "nvcf-backend", Affected: []clusteragent.KilledRequest{{Name: "r1", FunctionID: "fn"}}}} + withFakeMaintainer(t, f) + + if _, err := executeMaintenance(t, "", "cluster", "agent", "kill-function", "fn", "--yes", "--timeout", "45s"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if f.lastKillOpts.Timeout != 45*time.Second { + t.Fatalf("Timeout = %s, want 45s", f.lastKillOpts.Timeout) + } +} + +func TestKillFunctionTerminatingOutput(t *testing.T) { + f := &fakeMaintainer{ + killResult: &clusteragent.KillResult{ + RequestsNamespace: "nvcf-backend", + Affected: []clusteragent.KilledRequest{{Name: "r1", FunctionID: "fn", Terminating: true}}, + TerminatingCount: 1, + }, + killErr: errFakeKill, + } + withFakeMaintainer(t, f) + + out, err := executeMaintenance(t, "", "cluster", "agent", "kill-function", "fn", "--yes") + if err == nil { + t.Fatal("expected the aggregate error to propagate") + } + if !strings.Contains(out, "terminating") { + t.Errorf("expected the still-terminating request to be printed, got:\n%s", out) + } + if strings.Contains(out, "[deleted]") { + t.Errorf("a still-terminating request must not be reported as deleted, got:\n%s", out) + } +} + // --- kill-all --- func TestKillAllTypeInInteractive(t *testing.T) { @@ -526,6 +562,21 @@ func TestKillAllDryRun(t *testing.T) { } } +func TestKillAllTimeoutFlagForwarded(t *testing.T) { + f := &fakeMaintainer{ + target: &clusteragent.ClusterTarget{ClusterID: "c1", ClusterName: "edge-1", RequestsNamespace: "nvcf-backend"}, + killResult: &clusteragent.KillResult{RequestsNamespace: "nvcf-backend", Affected: []clusteragent.KilledRequest{{Name: "r1", FunctionID: "fn"}}}, + } + withFakeMaintainer(t, f) + + if _, err := executeMaintenance(t, "", "cluster", "agent", "kill-all", "--yes", "--confirm", "edge-1", "--timeout", "45s"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if f.lastKillOpts.Timeout != 45*time.Second { + t.Fatalf("Timeout = %s, want 45s", f.lastKillOpts.Timeout) + } +} + // --- JSON output --- func TestDrainJSONOutput(t *testing.T) { @@ -543,3 +594,26 @@ func TestDrainJSONOutput(t *testing.T) { t.Errorf("unexpected JSON output:\n%s", out) } } + +func TestKillFunctionJSONOutput(t *testing.T) { + f := &fakeMaintainer{ + killResult: &clusteragent.KillResult{ + RequestsNamespace: "nvcf-backend", + Affected: []clusteragent.KilledRequest{{Name: "r1", FunctionID: "fn", Terminating: true}}, + TerminatingCount: 1, + }, + killErr: errFakeKill, + } + withFakeMaintainer(t, f) + + var err error + out := captureMaintStdout(t, func() { + _, err = executeMaintenance(t, "", "cluster", "agent", "kill-function", "fn", "--yes", "--json") + }) + if err == nil { + t.Fatal("expected the aggregate error to propagate") + } + if !strings.Contains(out, `"terminatingCount": 1`) || !strings.Contains(out, `"terminating": true`) { + t.Errorf("unexpected JSON output:\n%s", out) + } +} diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go index 78dfe0af0..a6d14037b 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -19,6 +19,7 @@ package clusteragent import ( "context" + "errors" "fmt" "strings" "time" @@ -324,8 +325,11 @@ func (m *k8sMaintainer) KillFunction(ctx context.Context, functionID, versionID if err != nil { return nil, err } + if err := validateKillTimeout(opts.Timeout); err != nil { + return nil, err + } - result, err := m.killMatching(ctx, target, opts, func(fid, vid string) bool { + result, failures, err := m.killMatching(ctx, target, opts, func(fid, vid string) bool { return fid == functionID && (versionID == "" || vid == versionID) }) if err != nil { @@ -337,7 +341,7 @@ func (m *k8sMaintainer) KillFunction(ctx context.Context, functionID, versionID } return nil, fmt.Errorf("no scheduled function found for function %s in namespace %s", functionID, target.RequestsNamespace) } - return result, aggregateKillError(result) + return result, aggregateKillError(result, failures) } // KillAll terminates every ICMSRequest on the cluster. An empty cluster returns @@ -347,12 +351,24 @@ func (m *k8sMaintainer) KillAll(ctx context.Context, opts KillOptions) (*KillRes if err != nil { return nil, err } + if err := validateKillTimeout(opts.Timeout); err != nil { + return nil, err + } - result, err := m.killMatching(ctx, target, opts, func(string, string) bool { return true }) + result, failures, err := m.killMatching(ctx, target, opts, func(string, string) bool { return true }) if err != nil { return nil, err } - return result, aggregateKillError(result) + return result, aggregateKillError(result, failures) +} + +// validateKillTimeout rejects a negative --timeout. Zero is valid: it means +// "use DefaultKillTimeout" (handled in killMatching). +func validateKillTimeout(timeout time.Duration) error { + if timeout < 0 { + return fmt.Errorf("--timeout must not be negative, got %s", timeout) + } + return nil } // killMatching lists ICMSRequests in the requests namespace, selects the ones @@ -362,10 +378,14 @@ func (m *k8sMaintainer) KillAll(ctx context.Context, opts KillOptions) (*KillRes // recorded in the NVCFBackend CR's requestsNamespace field. The inspector's // all-namespace scan is a visibility-only read path that tolerates stale state; // kill operations use the authoritative namespace to avoid accidental cross-cluster deletions. -func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, opts KillOptions, match func(functionID, versionID string) bool) (*KillResult, error) { +// The second return value collects the underlying error for each per-item +// delete failure (distinct from the KilledRequest.Error strings, which exist +// for JSON/text output). Callers wrap these into the aggregate error so +// errors.Is/errors.As can still reach the original cause. +func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, opts KillOptions, match func(functionID, versionID string) bool) (*KillResult, []error, error) { items, err := listICMSRequests(ctx, m.dc, target.RequestsNamespace) if err != nil { - return nil, err + return nil, nil, err } sortICMSRequests(items) @@ -379,10 +399,11 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, } timeout := opts.Timeout - if timeout <= 0 { + if timeout == 0 { timeout = DefaultKillTimeout } + var failures []error for i := range items { fid, vid := functionIdentity(items[i].Object) if !match(fid, vid) { @@ -400,6 +421,7 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, case err != nil: killed.Error = err.Error() result.FailedCount++ + failures = append(failures, fmt.Errorf("%s/%s: %w", killed.Namespace, killed.Name, err)) case terminating: // The delete was accepted (deletionTimestamp set) but NVCA had not // removed its finalizer and evicted the workload by the deadline. @@ -415,7 +437,7 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, } result.Affected = append(result.Affected, killed) } - return result, nil + return result, failures, nil } // deleteICMSRequest deletes one ICMSRequest and waits up to timeout for it to @@ -450,24 +472,44 @@ func (m *k8sMaintainer) deleteICMSRequest(ctx context.Context, namespace, name s // waitForICMSRequestGone polls until the ICMSRequest is gone or timeout // elapses. It returns (true, nil) rather than an error on timeout: the // request was validly accepted for deletion, it just has not finished yet. +// Both the Get call and the poll sleep are bounded by the deadline, so a slow +// or blocked API call cannot make the wait overrun the configured timeout, +// and a timeout shorter than killDeletionPollInterval is still honored +// instead of sleeping through the whole poll interval regardless. func (m *k8sMaintainer) waitForICMSRequestGone(ctx context.Context, namespace, name string, timeout time.Duration) (bool, error) { deadline := time.Now().Add(timeout) for { - _, err := m.dc.Resource(icmsRequestGVR).Namespace(namespace).Get(ctx, name, metav1.GetOptions{}) + getCtx, cancel := context.WithDeadline(ctx, deadline) + _, err := m.dc.Resource(icmsRequestGVR).Namespace(namespace).Get(getCtx, name, metav1.GetOptions{}) + cancel() if err != nil { if apierrors.IsNotFound(err) { return false, nil } + if ctx.Err() != nil { + // The caller's own context ended, not our synthetic deadline. + return false, ctx.Err() + } + if errors.Is(err, context.DeadlineExceeded) { + // Our per-Get deadline (== the overall deadline) fired mid-call: + // treat exactly like a timeout that elapsed between polls. + return true, nil + } return false, err } - if time.Now().After(deadline) { + remaining := time.Until(deadline) + if remaining <= 0 { return true, nil } + wait := killDeletionPollInterval + if remaining < wait { + wait = remaining + } select { case <-ctx.Done(): return false, ctx.Err() - case <-time.After(killDeletionPollInterval): + case <-time.After(wait): } } } @@ -497,10 +539,14 @@ func (m *k8sMaintainer) stripFinalizers(ctx context.Context, namespace, name str }) } -func aggregateKillError(result *KillResult) error { +// aggregateKillError summarizes a kill outcome. failures carries the +// underlying per-item errors (wrapped with %w by the caller), so a caller +// inspecting the returned error with errors.Is/errors.As can still reach the +// original cause behind the summary text. +func aggregateKillError(result *KillResult, failures []error) error { switch { case result.FailedCount > 0: - return fmt.Errorf("failed to terminate %d of %d ICMSRequest(s)", result.FailedCount, len(result.Affected)) + return fmt.Errorf("failed to terminate %d of %d ICMSRequest(s): %w", result.FailedCount, len(result.Affected), errors.Join(failures...)) case result.TerminatingCount > 0: return fmt.Errorf("%d of %d ICMSRequest(s) still terminating: NVCA has not finished evicting the workload; re-check with cluster agent get-function", result.TerminatingCount, len(result.Affected)) default: diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go index 21618b3bf..acdd39405 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go @@ -19,6 +19,7 @@ package clusteragent import ( "context" + "errors" "fmt" "strings" "testing" @@ -731,6 +732,94 @@ func TestKillWithinTimeoutReportsDeletedNotTerminating(t *testing.T) { } } +// TestKillNegativeTimeoutRejected is a regression test: --timeout=-1s parses +// to a valid negative time.Duration with no error from the flag layer, so +// negative values must be rejected explicitly rather than silently falling +// back to DefaultKillTimeout like zero does. +func TestKillNegativeTimeoutRejected(t *testing.T) { + m, dc, _ := newFakeMaintainer(killSeed(), nil) + + _, err := m.KillAll(context.Background(), KillOptions{BackendNS: testBackendNS, Timeout: -1 * time.Second}) + if err == nil { + t.Fatal("expected an error for a negative --timeout") + } + if !strings.Contains(err.Error(), "negative") { + t.Errorf("error = %q, want it to mention the timeout must not be negative", err.Error()) + } + if !icmsExists(t, dc, testRequestsNS, "r1") { + t.Error("KillAll must not delete anything when --timeout validation fails") + } +} + +// simulatedDeleteError is a typed error a delete reactor can inject, so tests +// can confirm the aggregate error returned by Kill* still lets a caller reach +// the original cause via errors.As instead of only a flattened string. +type simulatedDeleteError struct{ detail string } + +func (e *simulatedDeleteError) Error() string { return "simulated delete failure: " + e.detail } + +// TestKillAggregateErrorWrapsUnderlyingCause is a regression test: the +// aggregate error from a partial kill failure must still let +// errors.As reach the original per-item error, not just a summary string. +func TestKillAggregateErrorWrapsUnderlyingCause(t *testing.T) { + m, dc, _ := newFakeMaintainer(killSeed(), nil) + want := &simulatedDeleteError{detail: "r2"} + dc.PrependReactor("delete", "icmsrequests", func(action k8stesting.Action) (bool, runtime.Object, error) { + if da, ok := action.(k8stesting.DeleteAction); ok && da.GetName() == "r2" { + return true, nil, want + } + return false, nil, nil + }) + + _, err := m.KillAll(context.Background(), KillOptions{BackendNS: testBackendNS}) + if err == nil { + t.Fatal("expected aggregate error on partial failure") + } + var got *simulatedDeleteError + if !errors.As(err, &got) { + t.Fatalf("errors.As could not find the underlying cause in: %v", err) + } + if got != want { + t.Errorf("recovered cause = %+v, want %+v", got, want) + } +} + +// TestKillTimeoutShorterThanPollIntervalIsHonored is a regression test: the +// deletion wait must not sleep through a poll interval longer than the +// configured --timeout before reporting Terminating. Uses a long poll +// interval and a short timeout, and asserts the call returns well within the +// poll interval. +func TestKillTimeoutShorterThanPollIntervalIsHonored(t *testing.T) { + orig := killDeletionPollInterval + killDeletionPollInterval = time.Minute + t.Cleanup(func() { killDeletionPollInterval = orig }) + + cr := icmsRequestWithFinalizers(testRequestsNS, "r1", "fn-1", "v1", "nvca.finalizers.nvidia.io") + m, dc, _ := newFakeMaintainer([]runtime.Object{defaultBackend(), cr}, nil) + dc.PrependReactor("delete", "icmsrequests", func(action k8stesting.Action) (bool, runtime.Object, error) { + // Delete is accepted but the object is never actually removed, + // simulating a finalizer the fake tracker can't model natively. + return true, nil, nil + }) + + start := time.Now() + res, err := m.KillFunction(context.Background(), "fn-1", "v1", KillOptions{ + BackendNS: testBackendNS, + Timeout: 10 * time.Millisecond, + }) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected an error reporting the request is still terminating") + } + if res.TerminatingCount != 1 { + t.Fatalf("TerminatingCount = %d, want 1", res.TerminatingCount) + } + if elapsed >= killDeletionPollInterval { + t.Errorf("elapsed = %s, want well under the %s poll interval: the wait must be bounded by --timeout, not the poll interval", elapsed, killDeletionPollInterval) + } +} + func TestResolveClusterAppliesNamespaceDefaults(t *testing.T) { // Backend with no system/requests namespace set. b := backendObj(testBackendNS, testClusterID, testCluster, "", "") diff --git a/src/clis/nvcf-cli/internal/clusteragent/maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/maintainer.go index 0405840a6..bf2a91a8f 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/maintainer.go @@ -114,7 +114,9 @@ type DrainResult struct { // KilledRequest is one ICMSRequest targeted by a kill operation. // -// - Error set: the delete call itself failed. +// - Error set: the delete operation failed. This covers the delete call +// itself, the --force finalizer strip that precedes it, and the +// post-delete existence check, not just the Delete API call. // - Terminating true (Error empty): the delete was accepted and // deletionTimestamp was set, but the object still existed with its // finalizer when the wait timed out. NVCA has not finished evicting the From 523dcc3953bced7f3c213dc9a831453d88a97d02 Mon Sep 17 00:00:00 2001 From: rohithb Date: Fri, 21 Aug 2026 13:26:08 +0530 Subject: [PATCH 3/6] fix(cli): classify only the local deletion deadline as terminating --- .../internal/clusteragent/k8s_maintainer.go | 16 ++++-- .../clusteragent/k8s_maintainer_test.go | 49 +++++++++++++++++-- 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go index a6d14037b..2b4d01811 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -481,6 +481,10 @@ func (m *k8sMaintainer) waitForICMSRequestGone(ctx context.Context, namespace, n for { getCtx, cancel := context.WithDeadline(ctx, deadline) _, err := m.dc.Resource(icmsRequestGVR).Namespace(namespace).Get(getCtx, name, metav1.GetOptions{}) + // Read getCtx.Err() before cancel(): cancel() makes every derived + // context report Canceled regardless of why it actually ended, so + // this is the only point where it still reflects the real cause. + localDeadlineExceeded := getCtx.Err() == context.DeadlineExceeded cancel() if err != nil { if apierrors.IsNotFound(err) { @@ -490,9 +494,15 @@ func (m *k8sMaintainer) waitForICMSRequestGone(ctx context.Context, namespace, n // The caller's own context ended, not our synthetic deadline. return false, ctx.Err() } - if errors.Is(err, context.DeadlineExceeded) { - // Our per-Get deadline (== the overall deadline) fired mid-call: - // treat exactly like a timeout that elapsed between polls. + if localDeadlineExceeded { + // Our per-Get deadline (== the overall deadline) is what ended + // the call: treat exactly like a timeout that elapsed between + // polls. Checking getCtx.Err() rather than + // errors.Is(err, context.DeadlineExceeded) matters here: a + // client/transport-level timeout unrelated to getCtx can also + // produce a context.DeadlineExceeded-shaped error before our + // deadline is actually reached, and that must still surface + // as a real error, not a false "still terminating". return true, nil } return false, err diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go index acdd39405..7ea9c02f0 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go @@ -802,10 +802,17 @@ func TestKillTimeoutShorterThanPollIntervalIsHonored(t *testing.T) { return true, nil, nil }) + const timeout = 10 * time.Millisecond + // Generous scheduling tolerance so this doesn't flake under CI load, but + // still tight enough to prove the wait tracks --timeout rather than the + // 1-minute killDeletionPollInterval: prior to the fix this took the full + // poll interval to return. + const tolerance = 2 * time.Second + start := time.Now() res, err := m.KillFunction(context.Background(), "fn-1", "v1", KillOptions{ BackendNS: testBackendNS, - Timeout: 10 * time.Millisecond, + Timeout: timeout, }) elapsed := time.Since(start) @@ -815,8 +822,44 @@ func TestKillTimeoutShorterThanPollIntervalIsHonored(t *testing.T) { if res.TerminatingCount != 1 { t.Fatalf("TerminatingCount = %d, want 1", res.TerminatingCount) } - if elapsed >= killDeletionPollInterval { - t.Errorf("elapsed = %s, want well under the %s poll interval: the wait must be bounded by --timeout, not the poll interval", elapsed, killDeletionPollInterval) + if elapsed >= timeout+tolerance { + t.Errorf("elapsed = %s, want close to the configured --timeout of %s (+%s tolerance): the wait must be bounded by --timeout, not the poll interval", elapsed, timeout, tolerance) + } +} + +// TestKillClassifiesOnlyLocalDeadlineAsTerminating is a regression test: a +// context.DeadlineExceeded-shaped error from the Get call must only be +// treated as "still terminating" when it actually came from +// waitForICMSRequestGone's own synthetic per-Get deadline. An unrelated +// transport/client-level timeout that happens to produce the same error +// shape, well before that deadline, must still surface as a real error +// instead of being silently reported as a successful (if incomplete) +// termination wait. +func TestKillClassifiesOnlyLocalDeadlineAsTerminating(t *testing.T) { + cr := icmsRequestWithFinalizers(testRequestsNS, "r1", "fn-1", "v1", "nvca.finalizers.nvidia.io") + m, dc, _ := newFakeMaintainer([]runtime.Object{defaultBackend(), cr}, nil) + dc.PrependReactor("delete", "icmsrequests", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }) + dc.PrependReactor("get", "icmsrequests", func(action k8stesting.Action) (bool, runtime.Object, error) { + if ga, ok := action.(k8stesting.GetAction); ok && ga.GetName() == "r1" { + // Simulate a spurious client/transport timeout unrelated to our + // own deadline: it arrives immediately, long before the + // generous Timeout below could have elapsed. + return true, nil, context.DeadlineExceeded + } + return false, nil, nil + }) + + _, err := m.KillFunction(context.Background(), "fn-1", "v1", KillOptions{ + BackendNS: testBackendNS, + Timeout: time.Hour, + }) + if err == nil { + t.Fatal("expected the spurious Get error to surface as a real failure") + } + if strings.Contains(err.Error(), "still terminating") { + t.Errorf("a spurious transport timeout must not be misreported as the deletion deadline elapsing, got: %v", err) } } From 2c5b54dcd7b9170c5784919b2e0acfd55dbfaa57 Mon Sep 17 00:00:00 2001 From: rohithb Date: Fri, 21 Aug 2026 13:37:39 +0530 Subject: [PATCH 4/6] fix(cli): require both local deadline and error shape before reporting terminating --- .../internal/clusteragent/k8s_maintainer.go | 16 ++++---- .../clusteragent/k8s_maintainer_test.go | 39 +++++++++++++++++++ 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go index 2b4d01811..d3b0aa69c 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -494,15 +494,17 @@ func (m *k8sMaintainer) waitForICMSRequestGone(ctx context.Context, namespace, n // The caller's own context ended, not our synthetic deadline. return false, ctx.Err() } - if localDeadlineExceeded { + if localDeadlineExceeded && errors.Is(err, context.DeadlineExceeded) { // Our per-Get deadline (== the overall deadline) is what ended // the call: treat exactly like a timeout that elapsed between - // polls. Checking getCtx.Err() rather than - // errors.Is(err, context.DeadlineExceeded) matters here: a - // client/transport-level timeout unrelated to getCtx can also - // produce a context.DeadlineExceeded-shaped error before our - // deadline is actually reached, and that must still surface - // as a real error, not a false "still terminating". + // polls. Both checks matter: getCtx.Err() alone would also + // match an unrelated client/transport-level timeout that + // races with our deadline; errors.Is(err, ...) alone would + // also match a spurious deadline-shaped error the transport + // returns well before our deadline actually elapses. Only + // requiring both guards against silently discarding a real, + // unrelated error (e.g. Forbidden) that happens to land in + // the same instant our deadline fires. return true, nil } return false, err diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go index 7ea9c02f0..03f5b731d 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go @@ -863,6 +863,45 @@ func TestKillClassifiesOnlyLocalDeadlineAsTerminating(t *testing.T) { } } +// TestKillPreservesUnrelatedErrorRacingWithLocalDeadline is a regression +// test for the inverse edge case: even when our own synthetic deadline has +// genuinely elapsed (a vanishingly small Timeout guarantees getCtx.Err() == +// DeadlineExceeded by the time it's checked), an unrelated error returned by +// the same Get call (e.g. Forbidden) must not be discarded and silently +// replaced with a "still terminating" result. Both localDeadlineExceeded and +// errors.Is(err, context.DeadlineExceeded) must hold before that happens. +func TestKillPreservesUnrelatedErrorRacingWithLocalDeadline(t *testing.T) { + cr := icmsRequestWithFinalizers(testRequestsNS, "r1", "fn-1", "v1", "nvca.finalizers.nvidia.io") + m, dc, _ := newFakeMaintainer([]runtime.Object{defaultBackend(), cr}, nil) + dc.PrependReactor("delete", "icmsrequests", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }) + wantErr := errors.New("forbidden") + dc.PrependReactor("get", "icmsrequests", func(action k8stesting.Action) (bool, runtime.Object, error) { + if ga, ok := action.(k8stesting.GetAction); ok && ga.GetName() == "r1" { + return true, nil, wantErr + } + return false, nil, nil + }) + + _, err := m.KillFunction(context.Background(), "fn-1", "v1", KillOptions{ + BackendNS: testBackendNS, + // A vanishingly small timeout: our own getCtx deadline will have + // elapsed by the time we check getCtx.Err(), but the reactor's + // "forbidden" error has nothing to do with that deadline. + Timeout: time.Nanosecond, + }) + if err == nil { + t.Fatal("expected the unrelated Get error to surface") + } + if !strings.Contains(err.Error(), "forbidden") { + t.Errorf("expected the original cause (%v) to be preserved, got: %v", wantErr, err) + } + if strings.Contains(err.Error(), "still terminating") { + t.Errorf("an unrelated error racing with the local deadline must not be misreported as terminating, got: %v", err) + } +} + func TestResolveClusterAppliesNamespaceDefaults(t *testing.T) { // Backend with no system/requests namespace set. b := backendObj(testBackendNS, testClusterID, testCluster, "", "") From 9c7bb8a8778cc8f127894afe1bbe94e45eb2b3a8 Mon Sep 17 00:00:00 2001 From: rohithb Date: Fri, 21 Aug 2026 13:46:45 +0530 Subject: [PATCH 5/6] test(cli): assert error identity with errors.Is instead of string matching --- .../nvcf-cli/internal/clusteragent/k8s_maintainer_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go index 03f5b731d..55ad09533 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go @@ -894,8 +894,8 @@ func TestKillPreservesUnrelatedErrorRacingWithLocalDeadline(t *testing.T) { if err == nil { t.Fatal("expected the unrelated Get error to surface") } - if !strings.Contains(err.Error(), "forbidden") { - t.Errorf("expected the original cause (%v) to be preserved, got: %v", wantErr, err) + if !errors.Is(err, wantErr) { + t.Errorf("expected errors.Is to reach the original cause (%v), got: %v", wantErr, err) } if strings.Contains(err.Error(), "still terminating") { t.Errorf("an unrelated error racing with the local deadline must not be misreported as terminating, got: %v", err) From 7a26531689986ad51a21124a966aaacb8d3ae0c9 Mon Sep 17 00:00:00 2001 From: rohithb Date: Fri, 21 Aug 2026 14:02:27 +0530 Subject: [PATCH 6/6] docs(cli): update kill-function/kill-all docs for deletion-verification behavior --- src/clis/nvcf-cli/README.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/clis/nvcf-cli/README.md b/src/clis/nvcf-cli/README.md index 7d043b4c0..970695429 100644 --- a/src/clis/nvcf-cli/README.md +++ b/src/clis/nvcf-cli/README.md @@ -1742,10 +1742,15 @@ the config change is already persisted and re-running is a no-op. ### How kill works `kill-function` and `kill-all` delete the matching `ICMSRequest` CRs; the NVCA -reconciler detects the deletion and evicts the workloads. Deletion is -asynchronous, so the command returns once the delete is accepted. `--force` -additionally strips finalizers so a request stuck `Terminating` is removed even -when NVCA is not running to process its finalizer. +reconciler detects the deletion and evicts the workloads. Deleting a CR only +accepts the deletion; the object stays `Terminating` behind its finalizer +until NVCA finishes evicting the workload and removes it. The command polls +for the CR to actually disappear before reporting success: a request removed +within `--timeout` (default 60s) is reported `deleted`, and one still present +when the timeout elapses is reported `terminating` instead, with a non-zero +exit code. `--force` additionally strips finalizers so a request stuck +`Terminating` is removed even when NVCA is not running to process its +finalizer. ### Confirmation and safety @@ -1760,7 +1765,9 @@ connected cluster. When the cluster has no name, it falls back to the cluster id All maintenance commands accept `--dry-run` to preview without mutating, and `--expect-cluster-id ` to refuse to act unless the connected cluster's id or name matches (guards against a wrong `--compute-plane-context`). `kill-function` -and `kill-all` accept `--reason` for an audit note, and `--json` for automation. +and `kill-all` accept `--reason` for an audit note, `--timeout` to bound how +long to wait for NVCA to finish evicting a terminated request (default 60s), +and `--json` for automation. These commands need write access to the target cluster: get/update on the `agent-config` ConfigMap and the `nvca` Deployment for drain, and list/delete