From 0aa2ea6cf88d8173cd2a51c0fe2644c69ef6b3b2 Mon Sep 17 00:00:00 2001 From: rohithb Date: Thu, 27 Aug 2026 09:33:20 +0530 Subject: [PATCH 1/4] fix(cli): terminate workload directly so kill-all/kill-function actually completes Signed-off-by: rohithb --- src/clis/nvcf-cli/README.md | 31 ++++--- .../internal/clusteragent/k8s_maintainer.go | 87 +++++++++++++++++++ .../clusteragent/k8s_maintainer_test.go | 42 +++++++++ 3 files changed, 148 insertions(+), 12 deletions(-) diff --git a/src/clis/nvcf-cli/README.md b/src/clis/nvcf-cli/README.md index 96a72c99e..aa9db416a 100644 --- a/src/clis/nvcf-cli/README.md +++ b/src/clis/nvcf-cli/README.md @@ -1756,16 +1756,21 @@ the desired state. ### How kill works -`kill-function` and `kill-all` delete the matching `ICMSRequest` CRs; the NVCA -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. +`kill-function` and `kill-all` terminate the matching `ICMSRequest`'s +Pod-type instances directly, mark them terminated on the CR, then delete the +CR. Deleting the CR alone never evicts the workload: NVCA's reconciler only +clears the CR's finalizer once its own `status.instances` shows every +instance gone and reported terminated, and nothing else in NVCA ever +produces that for a CLI-initiated kill. Performing the eviction and status +update directly satisfies that precondition, so NVCA's own reconcile clears +the finalizer on its next pass. 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. MiniService +(Helm function) instances are not evicted directly; only Pod-type instances +are. ### Confirmation and safety @@ -1786,8 +1791,10 @@ and `--json` for automation. These commands need write access to the target cluster: list/update on the `NVCFBackend` CR for drain (plus read access to the `agent-config` ConfigMap -and the `nvca` Deployment, to wait for the NVCA operator's rollout), and -list/delete (and update, with `--force`) on `ICMSRequest` CRs for kill. +and the `nvca` Deployment, to wait for the NVCA operator's rollout), and for +kill, list/delete (and update, with `--force`) on `ICMSRequest` CRs, update on +the `ICMSRequest` status subresource, and delete on Pods in the requests +namespace. ### Examples diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go index af8bbb962..08a83c61a 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -510,6 +510,15 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, FunctionVersionID: vid, } if !opts.DryRun { + // Best-effort: deleting the ICMSRequest CR alone never evicts the + // workload (see evictInstances doc comment), so drive the real + // eviction ourselves before asking NVCA's reconcile to notice. + // A failure here (e.g. missing RBAC) is not fatal on its own; + // deleteICMSRequest's poll below will honestly report + // "terminating" if the workload is still running as a result. + if err := m.evictInstances(ctx, killed.Namespace, &items[i]); err != nil { + logging.Warning("failed to evict instances for %s/%s: %v", killed.Namespace, killed.Name, err) + } terminating, err := m.deleteICMSRequest(ctx, killed.Namespace, killed.Name, opts.Force, timeout) switch { case err != nil: @@ -534,6 +543,84 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, return result, failures, nil } +// evictInstances directly terminates the Pod-type instances an ICMSRequest +// tracks in status.instances, and marks each one lastReportedStatus: +// "terminated" on that same CR. +// +// Deleting the ICMSRequest CR alone never evicts the workload: the NVCA +// reconciler's deletion-handling branch is a passive gate that only removes +// the finalizer once AllInstancesTerminatedAndReported is true for that CR's +// own status.instances (the pod is gone from Kubernetes AND +// lastReportedStatus == "terminated"). Nothing else in NVCA drives eviction +// or sets that field for a CLI-initiated kill: the only other code path that +// sets it is ApplyTerminationMessage, reachable only via a genuine upstream +// ICMS termination queue message, and it writes to the termination message's +// own CR, never back onto this one. So without this, the CR (and pod) can +// stay stuck behind the finalizer forever, regardless of --timeout. +// +// Performing both steps here satisfies the reconciler's own precondition, so +// its existing, unmodified logic clears the finalizer itself on its next +// pass. MiniService (Helm function) instances are left untouched: the CLI +// has no existing support for evicting those directly. +func (m *k8sMaintainer) evictInstances(ctx context.Context, namespace string, obj *unstructured.Unstructured) error { + instances, found, err := unstructured.NestedMap(obj.Object, "status", "instances") + if err != nil || !found || len(instances) == 0 { + return nil + } + + var errs []error + terminated := map[string]interface{}{} + for id, raw := range instances { + inst, ok := raw.(map[string]interface{}) + if !ok { + continue + } + if instanceType, _, _ := unstructured.NestedString(inst, "instanceType"); instanceType != "" && instanceType != "Pod" { + continue + } + if err := m.cs.CoreV1().Pods(namespace).Delete(ctx, id, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + errs = append(errs, fmt.Errorf("deleting pod %s: %w", id, err)) + continue + } + inst["lastReportedStatus"] = "terminated" + terminated[id] = inst + } + if len(terminated) == 0 { + return errors.Join(errs...) + } + + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + latest, err := m.dc.Resource(icmsRequestGVR).Namespace(namespace).Get(ctx, obj.GetName(), metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return err + } + existing, _, _ := unstructured.NestedMap(latest.Object, "status", "instances") + if existing == nil { + existing = map[string]interface{}{} + } + for id, v := range terminated { + existing[id] = v + } + if err := unstructured.SetNestedMap(latest.Object, existing, "status", "instances"); err != nil { + return err + } + latest.SetGroupVersionKind(schema.GroupVersionKind{ + Group: icmsRequestGVR.Group, + Version: icmsRequestGVR.Version, + Kind: "ICMSRequest", + }) + _, err = m.dc.Resource(icmsRequestGVR).Namespace(namespace).UpdateStatus(ctx, latest, metav1.UpdateOptions{}) + return err + }) + if err != nil { + errs = append(errs, fmt.Errorf("patching instance status: %w", err)) + } + return errors.Join(errs...) +} + // 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. 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 cf6899fbb..fdadbc32e 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go @@ -868,6 +868,48 @@ func TestKillWithinTimeoutReportsDeletedNotTerminating(t *testing.T) { // 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. +// TestKillEvictsPodBackedInstanceAndMarksItTerminated is a regression test +// for the reopened bug: deleting the ICMSRequest CR alone never evicts the +// workload, because NVCA's reconciler only clears the finalizer once +// AllInstancesTerminatedAndReported is true for that CR's own +// status.instances (pod gone from Kubernetes AND lastReportedStatus == +// "terminated"), and nothing else in NVCA ever satisfies that for a +// CLI-initiated kill. Verified live against a real cluster: manually +// deleting the pod and patching lastReportedStatus is what let NVCA's own +// unmodified reconcile actually clear the finalizer. This test asserts +// kill-function performs both steps itself. A delete reactor keeps the CR +// present after Delete (mirroring TestKillReportsTerminatingWhenFinalizerBlocksDeletion), +// so the patched status.instances is still inspectable afterward. +func TestKillEvictsPodBackedInstanceAndMarksItTerminated(t *testing.T) { + cr := icmsRequestWithFinalizers(testRequestsNS, "r1", "fn-1", "v1", "nvca.finalizers.nvidia.io") + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "inst-a", Namespace: testRequestsNS}} + m, dc, cs := newFakeMaintainer([]runtime.Object{defaultBackend(), cr}, []runtime.Object{pod}) + dc.PrependReactor("delete", "icmsrequests", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }) + + _, 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 (the fake finalizer never actually clears)") + } + + if _, err := cs.CoreV1().Pods(testRequestsNS).Get(context.Background(), "inst-a", metav1.GetOptions{}); !apierrors.IsNotFound(err) { + t.Errorf("pod inst-a should have been deleted, got err=%v", err) + } + + obj, err := dc.Resource(icmsRequestGVR).Namespace(testRequestsNS).Get(context.Background(), "r1", metav1.GetOptions{}) + if err != nil { + t.Fatalf("getting r1: %v", err) + } + status, _, _ := unstructured.NestedString(obj.Object, "status", "instances", "inst-a", "lastReportedStatus") + if status != "terminated" { + t.Errorf("status.instances.inst-a.lastReportedStatus = %q, want %q", status, "terminated") + } +} + func TestKillNegativeTimeoutRejected(t *testing.T) { m, dc, _ := newFakeMaintainer(killSeed(), nil) From c0362914b0ee9663f42ed31e6f9f08d5dd9d79f2 Mon Sep 17 00:00:00 2001 From: rohithb Date: Thu, 27 Aug 2026 11:05:33 +0530 Subject: [PATCH 2/4] fix(cli): add MiniService eviction support and fail closed on incomplete termination Signed-off-by: rohithb --- src/clis/nvcf-cli/README.md | 32 +-- .../internal/clusteragent/k8s_inspector.go | 4 + .../internal/clusteragent/k8s_maintainer.go | 83 ++++++-- .../clusteragent/k8s_maintainer_test.go | 199 ++++++++++++++++-- 4 files changed, 269 insertions(+), 49 deletions(-) diff --git a/src/clis/nvcf-cli/README.md b/src/clis/nvcf-cli/README.md index aa9db416a..fbe041c5f 100644 --- a/src/clis/nvcf-cli/README.md +++ b/src/clis/nvcf-cli/README.md @@ -1757,20 +1757,20 @@ the desired state. ### How kill works `kill-function` and `kill-all` terminate the matching `ICMSRequest`'s -Pod-type instances directly, mark them terminated on the CR, then delete the -CR. Deleting the CR alone never evicts the workload: NVCA's reconciler only -clears the CR's finalizer once its own `status.instances` shows every -instance gone and reported terminated, and nothing else in NVCA ever -produces that for a CLI-initiated kill. Performing the eviction and status -update directly satisfies that precondition, so NVCA's own reconcile clears -the finalizer on its next pass. 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. MiniService -(Helm function) instances are not evicted directly; only Pod-type instances -are. +instances directly (deleting the Pod for a container function, or the +`MiniService` object for a Helm function), mark them terminated on the CR, +then delete the CR. Deleting the CR alone never evicts the workload: NVCA's +reconciler only clears the CR's finalizer once its own `status.instances` +shows every instance gone and reported terminated, and nothing else in NVCA +ever produces that for a CLI-initiated kill. Performing the eviction and +status update directly satisfies that precondition, so NVCA's own reconcile +clears the finalizer on its next pass. 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 @@ -1793,8 +1793,8 @@ These commands need write access to the target cluster: list/update on the `NVCFBackend` CR for drain (plus read access to the `agent-config` ConfigMap and the `nvca` Deployment, to wait for the NVCA operator's rollout), and for kill, list/delete (and update, with `--force`) on `ICMSRequest` CRs, update on -the `ICMSRequest` status subresource, and delete on Pods in the requests -namespace. +the `ICMSRequest` status subresource, delete on Pods in the requests +namespace, and delete on `MiniService` CRs (cluster-scoped). ### Examples diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_inspector.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_inspector.go index d511e1ee5..2c5200c30 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_inspector.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_inspector.go @@ -39,6 +39,10 @@ var ( icmsRequestGVR = schema.GroupVersionResource{ Group: "nvca.nvcf.nvidia.io", Version: "v2beta1", Resource: "icmsrequests", } + // miniServiceGVR is cluster-scoped, unlike NVCFBackend/ICMSRequest. + miniServiceGVR = schema.GroupVersionResource{ + Group: "nvca.nvcf.nvidia.io", Version: "v1alpha1", Resource: "miniservices", + } ) // k8sInspector reads NVCA state from a compute-plane cluster's Kubernetes API diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go index 08a83c61a..096da1ca9 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -510,14 +510,20 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, FunctionVersionID: vid, } if !opts.DryRun { - // Best-effort: deleting the ICMSRequest CR alone never evicts the - // workload (see evictInstances doc comment), so drive the real - // eviction ourselves before asking NVCA's reconcile to notice. - // A failure here (e.g. missing RBAC) is not fatal on its own; - // deleteICMSRequest's poll below will honestly report - // "terminating" if the workload is still running as a result. + // Deleting the ICMSRequest CR alone never evicts the workload + // (see evictInstances doc comment), so drive the real eviction + // ourselves first. A failure here must stop this item rather + // than fall through to delete: with --force in particular, + // proceeding would strip the finalizer and report success while + // the workload (pod or MiniService) may still be running. if err := m.evictInstances(ctx, killed.Namespace, &items[i]); err != nil { - logging.Warning("failed to evict instances for %s/%s: %v", killed.Namespace, killed.Name, err) + logging.Warning("failed to evict instances for ICMSRequest %s/%s (function=%s version=%s cluster=%s): %v", + killed.Namespace, killed.Name, killed.FunctionID, killed.FunctionVersionID, clusterLabel(target), err) + killed.Error = fmt.Sprintf("evicting instances: %v", err) + result.FailedCount++ + failures = append(failures, fmt.Errorf("%s/%s: evicting instances: %w", killed.Namespace, killed.Name, err)) + result.Affected = append(result.Affected, killed) + continue } terminating, err := m.deleteICMSRequest(ctx, killed.Namespace, killed.Name, opts.Force, timeout) switch { @@ -543,9 +549,9 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, return result, failures, nil } -// evictInstances directly terminates the Pod-type instances an ICMSRequest -// tracks in status.instances, and marks each one lastReportedStatus: -// "terminated" on that same CR. +// evictInstances directly terminates the instances an ICMSRequest tracks in +// status.instances (Pod or MiniService), and marks each one +// lastReportedStatus: "terminated" on that same CR. // // Deleting the ICMSRequest CR alone never evicts the workload: the NVCA // reconciler's deletion-handling branch is a passive gate that only removes @@ -560,30 +566,56 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, // // Performing both steps here satisfies the reconciler's own precondition, so // its existing, unmodified logic clears the finalizer itself on its next -// pass. MiniService (Helm function) instances are left untouched: the CLI -// has no existing support for evicting those directly. +// pass. +// +// MiniService (Helm function) instances differ from Pod instances: deleting +// the MiniService object is itself sufficient to drive real teardown (its +// own controller, internal/miniservice/reconcile.go, actively deletes the +// rendered chart's objects, namespace, and cache entries on deletion, unlike +// ICMSRequest's passive gate), so no separate resource-deletion step is +// needed beyond the Delete call. The lastReportedStatus patch below is still +// required for both instance types: AllInstancesTerminatedAndReported checks +// it after the pod/MiniService-existence check regardless of type. func (m *k8sMaintainer) evictInstances(ctx context.Context, namespace string, obj *unstructured.Unstructured) error { instances, found, err := unstructured.NestedMap(obj.Object, "status", "instances") - if err != nil || !found || len(instances) == 0 { + if err != nil { + return fmt.Errorf("reading status.instances: %w", err) + } + if !found || len(instances) == 0 { return nil } var errs []error - terminated := map[string]interface{}{} + terminated := map[string]string{} for id, raw := range instances { inst, ok := raw.(map[string]interface{}) if !ok { continue } - if instanceType, _, _ := unstructured.NestedString(inst, "instanceType"); instanceType != "" && instanceType != "Pod" { + // instanceType is the current field; type is a legacy alias some + // older records still carry (see extractInstances in + // k8s_inspector.go, which reads both for the same reason). + instanceType, _, _ := unstructured.NestedString(inst, "instanceType") + if instanceType == "" { + instanceType, _, _ = unstructured.NestedString(inst, "type") + } + var delErr error + switch instanceType { + case "", "Pod": + delErr = m.cs.CoreV1().Pods(namespace).Delete(ctx, id, metav1.DeleteOptions{}) + case "MiniService": + // Cluster-scoped: no .Namespace(...). + delErr = m.dc.Resource(miniServiceGVR).Delete(ctx, id, metav1.DeleteOptions{}) + default: + // Unrecognized instance type: leave it for NVCA's own reconcile + // rather than guessing which resource kind to delete. continue } - if err := m.cs.CoreV1().Pods(namespace).Delete(ctx, id, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { - errs = append(errs, fmt.Errorf("deleting pod %s: %w", id, err)) + if delErr != nil && !apierrors.IsNotFound(delErr) { + errs = append(errs, fmt.Errorf("deleting %s instance %s: %w", firstNonEmpty(instanceType, "Pod"), id, delErr)) continue } - inst["lastReportedStatus"] = "terminated" - terminated[id] = inst + terminated[id] = "terminated" } if len(terminated) == 0 { return errors.Join(errs...) @@ -601,8 +633,17 @@ func (m *k8sMaintainer) evictInstances(ctx context.Context, namespace string, ob if existing == nil { existing = map[string]interface{}{} } - for id, v := range terminated { - existing[id] = v + // Merge lastReportedStatus into whatever is currently on the + // server, rather than overwriting the whole instance record with + // the pre-eviction snapshot: NVCA may have concurrently updated + // other instance fields (attributes, timestamps) since obj was read. + for id, status := range terminated { + cur, ok := existing[id].(map[string]interface{}) + if !ok { + cur = map[string]interface{}{"id": id} + } + cur["lastReportedStatus"] = status + existing[id] = cur } if err := unstructured.SetNestedMap(latest.Object, existing, "status", "instances"); err != nil { return 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 fdadbc32e..71ceb861c 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go @@ -51,6 +51,7 @@ func newFakeMaintainer(dynObjs, k8sObjs []runtime.Object) (*k8sMaintainer, *dyna gvrToListKind := map[schema.GroupVersionResource]string{ nvcfBackendGVR: "NVCFBackendList", icmsRequestGVR: "ICMSRequestList", + miniServiceGVR: "MiniServiceList", } dc := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(scheme, gvrToListKind, dynObjs...) cs := k8sfake.NewSimpleClientset(k8sObjs...) @@ -868,18 +869,11 @@ func TestKillWithinTimeoutReportsDeletedNotTerminating(t *testing.T) { // 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. -// TestKillEvictsPodBackedInstanceAndMarksItTerminated is a regression test -// for the reopened bug: deleting the ICMSRequest CR alone never evicts the -// workload, because NVCA's reconciler only clears the finalizer once -// AllInstancesTerminatedAndReported is true for that CR's own -// status.instances (pod gone from Kubernetes AND lastReportedStatus == -// "terminated"), and nothing else in NVCA ever satisfies that for a -// CLI-initiated kill. Verified live against a real cluster: manually -// deleting the pod and patching lastReportedStatus is what let NVCA's own -// unmodified reconcile actually clear the finalizer. This test asserts -// kill-function performs both steps itself. A delete reactor keeps the CR -// present after Delete (mirroring TestKillReportsTerminatingWhenFinalizerBlocksDeletion), -// so the patched status.instances is still inspectable afterward. +// TestKillEvictsPodBackedInstanceAndMarksItTerminated asserts kill-function +// deletes the backing pod and marks the instance terminated on the CR. A +// delete reactor keeps the CR present after Delete (mirroring +// TestKillReportsTerminatingWhenFinalizerBlocksDeletion), so the patched +// status.instances is still inspectable afterward. func TestKillEvictsPodBackedInstanceAndMarksItTerminated(t *testing.T) { cr := icmsRequestWithFinalizers(testRequestsNS, "r1", "fn-1", "v1", "nvca.finalizers.nvidia.io") pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "inst-a", Namespace: testRequestsNS}} @@ -910,6 +904,187 @@ func TestKillEvictsPodBackedInstanceAndMarksItTerminated(t *testing.T) { } } +// TestKillEvictsMiniServiceBackedInstanceAndMarksItTerminated is the +// MiniService-instance counterpart to TestKillEvictsPodBackedInstanceAndMarksItTerminated. +// Unlike a Pod instance, deleting the MiniService object is itself +// sufficient to drive real teardown (its own controller actively cleans up +// on deletion), so this only needs to verify the MiniService object gets +// deleted and the ICMSRequest's instance status still gets patched +// terminated the same way. +func TestKillEvictsMiniServiceBackedInstanceAndMarksItTerminated(t *testing.T) { + cr := &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "nvca.nvcf.nvidia.io/v2beta1", + "kind": "ICMSRequest", + "metadata": map[string]interface{}{ + "namespace": testRequestsNS, + "name": "r1", + "finalizers": []interface{}{"nvca.finalizers.nvidia.io"}, + }, + "spec": map[string]interface{}{ + "functionDetails": map[string]interface{}{ + "functionId": "fn-1", + "functionVersionId": "v1", + }, + }, + "status": map[string]interface{}{ + "requestStatus": statusCompleted, + "instances": map[string]interface{}{ + "ms-a": map[string]interface{}{ + "id": "ms-a", + "instanceType": "MiniService", + "status": "Running", + }, + }, + }, + }} + ms := &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "nvca.nvcf.nvidia.io/v1alpha1", + "kind": "MiniService", + "metadata": map[string]interface{}{"name": "ms-a"}, + }} + m, dc, _ := newFakeMaintainer([]runtime.Object{defaultBackend(), cr, ms}, nil) + dc.PrependReactor("delete", "icmsrequests", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }) + + _, 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 (the fake finalizer never actually clears)") + } + + if _, err := dc.Resource(miniServiceGVR).Get(context.Background(), "ms-a", metav1.GetOptions{}); !apierrors.IsNotFound(err) { + t.Errorf("MiniService ms-a should have been deleted, got err=%v", err) + } + + obj, err := dc.Resource(icmsRequestGVR).Namespace(testRequestsNS).Get(context.Background(), "r1", metav1.GetOptions{}) + if err != nil { + t.Fatalf("getting r1: %v", err) + } + status, _, _ := unstructured.NestedString(obj.Object, "status", "instances", "ms-a", "lastReportedStatus") + if status != "terminated" { + t.Errorf("status.instances.ms-a.lastReportedStatus = %q, want %q", status, "terminated") + } +} + +// TestKillEvictsLegacyTypeMiniServiceInstance is a regression test: a legacy +// instance record with type: "MiniService" and no instanceType field must +// still be deleted as a MiniService, not defaulted to a Pod delete (which +// would silently mark it terminated without ever touching the real +// MiniService object). +func TestKillEvictsLegacyTypeMiniServiceInstance(t *testing.T) { + cr := &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "nvca.nvcf.nvidia.io/v2beta1", + "kind": "ICMSRequest", + "metadata": map[string]interface{}{ + "namespace": testRequestsNS, + "name": "r1", + "finalizers": []interface{}{"nvca.finalizers.nvidia.io"}, + }, + "spec": map[string]interface{}{ + "functionDetails": map[string]interface{}{ + "functionId": "fn-1", + "functionVersionId": "v1", + }, + }, + "status": map[string]interface{}{ + "requestStatus": statusCompleted, + "instances": map[string]interface{}{ + "ms-a": map[string]interface{}{ + "id": "ms-a", + "type": "MiniService", + "status": "Running", + }, + }, + }, + }} + ms := &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "nvca.nvcf.nvidia.io/v1alpha1", + "kind": "MiniService", + "metadata": map[string]interface{}{"name": "ms-a"}, + }} + m, dc, _ := newFakeMaintainer([]runtime.Object{defaultBackend(), cr, ms}, nil) + dc.PrependReactor("delete", "icmsrequests", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }) + + _, 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 (the fake finalizer never actually clears)") + } + + if _, err := dc.Resource(miniServiceGVR).Get(context.Background(), "ms-a", metav1.GetOptions{}); !apierrors.IsNotFound(err) { + t.Errorf("MiniService ms-a should have been deleted via the legacy type field, got err=%v", err) + } +} + +// TestKillStopsOnEvictionFailureInsteadOfReportingSuccess is a regression +// test: if evicting an instance fails (e.g. RBAC denies the Pod delete), the +// CLI must not proceed to delete the ICMSRequest CR and report success or +// terminating. With --force in particular, proceeding would strip the +// finalizer and remove the CR while the pod keeps running. +func TestKillStopsOnEvictionFailureInsteadOfReportingSuccess(t *testing.T) { + cr := icmsRequestWithFinalizers(testRequestsNS, "r1", "fn-1", "v1", "nvca.finalizers.nvidia.io") + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "inst-a", Namespace: testRequestsNS}} + m, dc, cs := newFakeMaintainer([]runtime.Object{defaultBackend(), cr}, []runtime.Object{pod}) + cs.PrependReactor("delete", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewForbidden(corev1.Resource("pods"), "inst-a", errors.New("denied")) + }) + + res, err := m.KillFunction(context.Background(), "fn-1", "v1", KillOptions{ + BackendNS: testBackendNS, + Force: true, + }) + if err == nil { + t.Fatal("expected an error: eviction failed, so the CR must not be reported as killed") + } + if res.FailedCount != 1 || res.TerminatingCount != 0 { + t.Fatalf("FailedCount/TerminatingCount = %d/%d, want 1/0", res.FailedCount, res.TerminatingCount) + } + if len(res.Affected) != 1 || res.Affected[0].Error == "" || res.Affected[0].Terminating { + t.Fatalf("affected = %+v, want a single non-terminating entry carrying the eviction error", res.Affected) + } + if !icmsExists(t, dc, testRequestsNS, "r1") { + t.Error("r1 must still exist: --force must not strip the finalizer when eviction itself failed") + } + if _, err := cs.CoreV1().Pods(testRequestsNS).Get(context.Background(), "inst-a", metav1.GetOptions{}); err != nil { + t.Errorf("pod inst-a should still exist (delete was denied), got err=%v", err) + } +} + +// TestKillPropagatesMalformedInstanceStatusError is a regression test: a +// status.instances value that is not a map (corrupt/unexpected data) must +// surface as an error from evictInstances, not be silently treated as "no +// instances to evict." +func TestKillPropagatesMalformedInstanceStatusError(t *testing.T) { + cr := icmsRequestWithFinalizers(testRequestsNS, "r1", "fn-1", "v1", "nvca.finalizers.nvidia.io") + // status.instances must be a map[string]InstanceStatus; force it to a + // string to simulate corrupt/unexpected data shape. + if err := unstructured.SetNestedField(cr.Object, "not-a-map", "status", "instances"); err != nil { + t.Fatalf("seeding malformed status.instances: %v", err) + } + m, dc, _ := newFakeMaintainer([]runtime.Object{defaultBackend(), cr}, nil) + + res, err := m.KillFunction(context.Background(), "fn-1", "v1", KillOptions{ + BackendNS: testBackendNS, + Force: true, + }) + if err == nil { + t.Fatal("expected an error: malformed status.instances must not be silently ignored") + } + if res.FailedCount != 1 { + t.Fatalf("FailedCount = %d, want 1", res.FailedCount) + } + if !icmsExists(t, dc, testRequestsNS, "r1") { + t.Error("r1 must still exist: eviction must fail closed on malformed data, not proceed to delete") + } +} + func TestKillNegativeTimeoutRejected(t *testing.T) { m, dc, _ := newFakeMaintainer(killSeed(), nil) From ec82684c438e4e45dd5f368da5f846cfaa99400b Mon Sep 17 00:00:00 2001 From: rohithb Date: Thu, 27 Aug 2026 11:22:48 +0530 Subject: [PATCH 3/4] fix(cli): fail closed on unresolvable instance records instead of skipping Signed-off-by: rohithb --- .../internal/clusteragent/k8s_maintainer.go | 13 +++-- .../clusteragent/k8s_maintainer_test.go | 49 +++++++++++++++++++ 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go index 096da1ca9..e9740c6a8 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -517,11 +517,10 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, // proceeding would strip the finalizer and report success while // the workload (pod or MiniService) may still be running. if err := m.evictInstances(ctx, killed.Namespace, &items[i]); err != nil { - logging.Warning("failed to evict instances for ICMSRequest %s/%s (function=%s version=%s cluster=%s): %v", - killed.Namespace, killed.Name, killed.FunctionID, killed.FunctionVersionID, clusterLabel(target), err) killed.Error = fmt.Sprintf("evicting instances: %v", err) result.FailedCount++ - failures = append(failures, fmt.Errorf("%s/%s: evicting instances: %w", killed.Namespace, killed.Name, err)) + failures = append(failures, fmt.Errorf("%s/%s (function=%s version=%s cluster=%s): evicting instances: %w", + killed.Namespace, killed.Name, killed.FunctionID, killed.FunctionVersionID, clusterLabel(target), err)) result.Affected = append(result.Affected, killed) continue } @@ -590,6 +589,7 @@ func (m *k8sMaintainer) evictInstances(ctx context.Context, namespace string, ob for id, raw := range instances { inst, ok := raw.(map[string]interface{}) if !ok { + errs = append(errs, fmt.Errorf("instance %s: status.instances record is not an object", id)) continue } // instanceType is the current field; type is a legacy alias some @@ -607,8 +607,11 @@ func (m *k8sMaintainer) evictInstances(ctx context.Context, namespace string, ob // Cluster-scoped: no .Namespace(...). delErr = m.dc.Resource(miniServiceGVR).Delete(ctx, id, metav1.DeleteOptions{}) default: - // Unrecognized instance type: leave it for NVCA's own reconcile - // rather than guessing which resource kind to delete. + // Unrecognized instance type: do not guess which resource kind + // to delete. Reported as a failure (rather than silently + // skipped) so the caller does not proceed to delete the CR + // while this instance's workload was never touched. + errs = append(errs, fmt.Errorf("instance %s: unsupported instance type %q", id, instanceType)) continue } if delErr != nil && !apierrors.IsNotFound(delErr) { 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 71ceb861c..a38e5f18f 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go @@ -1085,6 +1085,55 @@ func TestKillPropagatesMalformedInstanceStatusError(t *testing.T) { } } +// TestKillFailsClosedOnUnrecognizedInstanceType is a regression test: an +// instance whose type is neither Pod nor MiniService (nor unset) must not be +// silently skipped. Skipping it without an error would let evictInstances +// report success, and killMatching would proceed to delete the ICMSRequest +// (stripping the finalizer under --force) while that instance's workload +// was never touched. +func TestKillFailsClosedOnUnrecognizedInstanceType(t *testing.T) { + cr := &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "nvca.nvcf.nvidia.io/v2beta1", + "kind": "ICMSRequest", + "metadata": map[string]interface{}{ + "namespace": testRequestsNS, + "name": "r1", + "finalizers": []interface{}{"nvca.finalizers.nvidia.io"}, + }, + "spec": map[string]interface{}{ + "functionDetails": map[string]interface{}{ + "functionId": "fn-1", + "functionVersionId": "v1", + }, + }, + "status": map[string]interface{}{ + "requestStatus": statusCompleted, + "instances": map[string]interface{}{ + "weird-a": map[string]interface{}{ + "id": "weird-a", + "instanceType": "SomeFutureType", + "status": "Running", + }, + }, + }, + }} + m, dc, _ := newFakeMaintainer([]runtime.Object{defaultBackend(), cr}, nil) + + res, err := m.KillFunction(context.Background(), "fn-1", "v1", KillOptions{ + BackendNS: testBackendNS, + Force: true, + }) + if err == nil { + t.Fatal("expected an error: an unrecognized instance type must not be silently skipped") + } + if res.FailedCount != 1 { + t.Fatalf("FailedCount = %d, want 1", res.FailedCount) + } + if !icmsExists(t, dc, testRequestsNS, "r1") { + t.Error("r1 must still exist: eviction must fail closed on an unrecognized instance type, not proceed to delete") + } +} + func TestKillNegativeTimeoutRejected(t *testing.T) { m, dc, _ := newFakeMaintainer(killSeed(), nil) From f04d89bde0fd3dba888ee14917b0b6ed64a1506f Mon Sep 17 00:00:00 2001 From: rohithb Date: Thu, 27 Aug 2026 15:32:31 +0530 Subject: [PATCH 4/4] docs(cli): condense the evictInstances doc comment Signed-off-by: rohithb --- .../internal/clusteragent/k8s_maintainer.go | 34 ++++++------------- 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go index e9740c6a8..5c93af0d0 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -548,33 +548,19 @@ func (m *k8sMaintainer) killMatching(ctx context.Context, target *ClusterTarget, return result, failures, nil } -// evictInstances directly terminates the instances an ICMSRequest tracks in -// status.instances (Pod or MiniService), and marks each one +// evictInstances deletes the Pod or MiniService backing each instance an +// ICMSRequest tracks in status.instances, and marks each one // lastReportedStatus: "terminated" on that same CR. // -// Deleting the ICMSRequest CR alone never evicts the workload: the NVCA -// reconciler's deletion-handling branch is a passive gate that only removes -// the finalizer once AllInstancesTerminatedAndReported is true for that CR's -// own status.instances (the pod is gone from Kubernetes AND -// lastReportedStatus == "terminated"). Nothing else in NVCA drives eviction -// or sets that field for a CLI-initiated kill: the only other code path that -// sets it is ApplyTerminationMessage, reachable only via a genuine upstream -// ICMS termination queue message, and it writes to the termination message's -// own CR, never back onto this one. So without this, the CR (and pod) can -// stay stuck behind the finalizer forever, regardless of --timeout. +// NVCA's reconciler only removes the CR's finalizer once +// AllInstancesTerminatedAndReported is true for that CR's own +// status.instances; nothing else in NVCA ever makes that true for a +// CLI-initiated kill, so deleting the CR alone leaves it (and the workload) +// stuck behind the finalizer forever. Doing both steps here satisfies that +// precondition, so NVCA's existing reconcile clears the finalizer itself. // -// Performing both steps here satisfies the reconciler's own precondition, so -// its existing, unmodified logic clears the finalizer itself on its next -// pass. -// -// MiniService (Helm function) instances differ from Pod instances: deleting -// the MiniService object is itself sufficient to drive real teardown (its -// own controller, internal/miniservice/reconcile.go, actively deletes the -// rendered chart's objects, namespace, and cache entries on deletion, unlike -// ICMSRequest's passive gate), so no separate resource-deletion step is -// needed beyond the Delete call. The lastReportedStatus patch below is still -// required for both instance types: AllInstancesTerminatedAndReported checks -// it after the pod/MiniService-existence check regardless of type. +// MiniService deletion needs no further cleanup step: its own controller +// (internal/miniservice/reconcile.go) tears down the chart on delete. func (m *k8sMaintainer) evictInstances(ctx context.Context, namespace string, obj *unstructured.Unstructured) error { instances, found, err := unstructured.NestedMap(obj.Object, "status", "instances") if err != nil {