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 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/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 ca913fc86..d3b0aa69c 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" @@ -56,6 +57,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. @@ -319,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 { @@ -332,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 @@ -342,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 @@ -357,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) @@ -373,6 +398,12 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, Affected: []KilledRequest{}, } + timeout := opts.Timeout + if timeout == 0 { + timeout = DefaultKillTimeout + } + + var failures []error for i := range items { fid, vid := functionIdentity(items[i].Object) if !match(fid, vid) { @@ -385,10 +416,19 @@ 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 { + 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. + // 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", @@ -397,23 +437,93 @@ 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. 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. +// 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 { + 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) { + return false, nil + } + if ctx.Err() != nil { + // The caller's own context ended, not our synthetic deadline. + return false, ctx.Err() + } + 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. 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 + } + + 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(wait): + } } - return nil } // stripFinalizers clears the finalizers on an ICMSRequest, mirroring the @@ -441,11 +551,19 @@ func (m *k8sMaintainer) stripFinalizers(ctx context.Context, namespace, name str }) } -func aggregateKillError(result *KillResult) error { - if result.FailedCount == 0 { +// 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): %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: 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..55ad09533 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" @@ -667,6 +668,240 @@ 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) + } +} + +// 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 + }) + + 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: timeout, + }) + 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 >= 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) + } +} + +// 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 !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) + } +} + 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..bf2a91a8f 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,23 @@ 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 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 +// 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 +140,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"` }