From 9e75e6d55e80224c468a4208d8d2495df27923fa Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Sat, 19 Sep 2026 12:16:48 -0700 Subject: [PATCH 1/2] fix(nvca): keep the model-cache init namespace out of namespace GC and the metadata webhook The agent creates nvca-modelcache-init at startup and labels it workload-instance-type=miniservice so the unbound-DNS policy matches its cache writer jobs (#1116, #1303). Two other components select on that label and neither expects a namespace without a function instance: - internal/gc/namespace treats every labelled namespace without an ICMSRequest of the same name as orphaned and deletes it. The GC runs at startup and hourly, so the init namespace disappeared right after creation; the agent then logged "namespaces nvca-modelcache-init not found" on every permissions pass and Helm functions fell back to per-worker model downloads. - The miniservice mutating webhook's namespaceSelector matches the same label, and its Fail policy denies any pod whose namespace lacks the nvcf-miniservice-metadata ConfigMap. Writer pods in the init namespace have no instance metadata, so cache initialization could not start even when the namespace existed. The GC now skips storage.ModelCacheInitNamespace, and the webhook admits pods in it unchanged. The GC also logged nothing about any of this: controller-runtime starts runnables with its own context, on which core.GetLogger returns a discard logger. The agent now hands the GC a logging context so orphan detection and deletions are visible. Tests: the namespace cleaner test includes a labelled init namespace with no ICMSRequest and asserts it is not collected; the webhook test admits a writer pod in the init namespace without a metadata ConfigMap. Closes #1991 Co-Authored-By: Balaji Ganesan --- .../nvca/internal/gc/gc.go | 13 ++++++++++++ .../nvca/internal/gc/namespace/BUILD.bazel | 2 ++ .../nvca/internal/gc/namespace/cleaner.go | 8 ++++++++ .../internal/gc/namespace/cleaner_test.go | 7 ++++++- .../nvca/pkg/nvca/agent_manager.go | 1 + .../webhook/miniservice_mutating_webhook.go | 8 ++++++++ .../miniservice_mutating_webhook_test.go | 20 +++++++++++++++++++ 7 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/compute-plane-services/nvca/internal/gc/gc.go b/src/compute-plane-services/nvca/internal/gc/gc.go index 0521ace4a7..ac8545ff89 100644 --- a/src/compute-plane-services/nvca/internal/gc/gc.go +++ b/src/compute-plane-services/nvca/internal/gc/gc.go @@ -59,6 +59,16 @@ type Cleaner interface { type Runnable struct { cleaners []Cleaner interval time.Duration + // logCtx carries the agent's logger. controller-runtime starts runnables + // with its own context, on which core.GetLogger returns a discard logger, + // so without this every GC log line, including deletions, is lost. + logCtx context.Context +} + +// SetLogContext records a context whose logger the GC uses for its own log +// lines. Call it before the runnable is started. +func (gc *Runnable) SetLogContext(ctx context.Context) { + gc.logCtx = ctx } // NewRunnable creates a new garbage collection controller with the specified interval @@ -85,6 +95,9 @@ func NewRunnable(clients *kubeclients.KubeClients, m *metrics.Metrics, interval // Start begins the GC controller, executing cleaners immediately and then at the configured interval // This implements the controller-runtime Runnable interface func (gc *Runnable) Start(ctx context.Context) error { + if gc.logCtx != nil { + ctx = core.WithLogger(ctx, core.GetLogger(gc.logCtx)) + } log := core.GetLogger(ctx) // Run cleaners immediately on startup diff --git a/src/compute-plane-services/nvca/internal/gc/namespace/BUILD.bazel b/src/compute-plane-services/nvca/internal/gc/namespace/BUILD.bazel index 3a22d44038..3a03b81ff9 100644 --- a/src/compute-plane-services/nvca/internal/gc/namespace/BUILD.bazel +++ b/src/compute-plane-services/nvca/internal/gc/namespace/BUILD.bazel @@ -16,6 +16,7 @@ go_library( "//src/compute-plane-services/nvca/internal/metrics/gctypes", "//src/compute-plane-services/nvca/pkg/apis/nvca/v2beta1", "//src/compute-plane-services/nvca/pkg/client/clientset/versioned", + "//src/compute-plane-services/nvca/pkg/storage", "//src/compute-plane-services/nvca/pkg/types", "//src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/core", "//src/compute-plane-services/nvca/vendor/github.com/sourcegraph/conc/pool", @@ -40,6 +41,7 @@ go_test( deps = [ "//src/compute-plane-services/nvca/pkg/apis/nvca/v2beta1", "//src/compute-plane-services/nvca/pkg/client/clientset/versioned/fake", + "//src/compute-plane-services/nvca/pkg/storage", "//src/compute-plane-services/nvca/pkg/types", "//src/compute-plane-services/nvca/vendor/github.com/stretchr/testify/assert", "//src/compute-plane-services/nvca/vendor/github.com/stretchr/testify/require", diff --git a/src/compute-plane-services/nvca/internal/gc/namespace/cleaner.go b/src/compute-plane-services/nvca/internal/gc/namespace/cleaner.go index 86af690ebc..cc26b08718 100644 --- a/src/compute-plane-services/nvca/internal/gc/namespace/cleaner.go +++ b/src/compute-plane-services/nvca/internal/gc/namespace/cleaner.go @@ -33,6 +33,7 @@ import ( "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/internal/metrics" metricsgctypes "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/internal/metrics/gctypes" bartclient "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/pkg/client/clientset/versioned" + "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/pkg/storage" "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/pkg/types" ) @@ -112,6 +113,13 @@ func (c *Cleaner) collectOrphanedNamespaces(ctx context.Context) ([]corev1.Names var toCleanup []corev1.Namespace for _, ns := range nsList.Items { + // The shared model-cache init namespace carries the miniservice + // instance-type label so the unbound-DNS policy matches its writer + // jobs, but it never has an ICMSRequest. It is owned by the agent + // for the cluster's lifetime, not by any function instance. + if ns.Name == storage.ModelCacheInitNamespace { + continue + } if c.isOrphaned(ctx, &ns) { toCleanup = append(toCleanup, ns) } else if ns.DeletionTimestamp != nil { diff --git a/src/compute-plane-services/nvca/internal/gc/namespace/cleaner_test.go b/src/compute-plane-services/nvca/internal/gc/namespace/cleaner_test.go index 0b654b4937..91ba86cc5a 100644 --- a/src/compute-plane-services/nvca/internal/gc/namespace/cleaner_test.go +++ b/src/compute-plane-services/nvca/internal/gc/namespace/cleaner_test.go @@ -36,6 +36,7 @@ import ( nvcav2beta1 "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/pkg/apis/nvca/v2beta1" bartfake "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/pkg/client/clientset/versioned/fake" + "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/pkg/storage" "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/pkg/types" ) @@ -132,9 +133,13 @@ func TestCleaner_collectOrphanedNamespaces(t *testing.T) { nsOrphan := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns-orphan", Labels: map[string]string{"nvca.nvcf.nvidia.io/workload-instance-type": "miniservice"}}} nsValid := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns-valid", Labels: map[string]string{"nvca.nvcf.nvidia.io/workload-instance-type": "miniservice"}}} + // The model-cache init namespace carries the same label and has no + // ICMSRequest, but must never be collected. + nsInit := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: storage.ModelCacheInitNamespace, Labels: map[string]string{"nvca.nvcf.nvidia.io/workload-instance-type": "miniservice"}}} + // StorageRequest inside nsValid (to be cleaned later) // Create fake clients - k8sClient := fake.NewSimpleClientset(nsOrphan, nsValid) + k8sClient := fake.NewSimpleClientset(nsOrphan, nsValid, nsInit) nvcaClient := bartfake.NewSimpleClientset() icmsGetter := &mockICMSRequestGetter{requests: map[string]*nvcav2beta1.ICMSRequest{"ns-valid": {ObjectMeta: metav1.ObjectMeta{Name: "ns-valid", Namespace: types.DefaultICMSRequestNamespace}}}} diff --git a/src/compute-plane-services/nvca/pkg/nvca/agent_manager.go b/src/compute-plane-services/nvca/pkg/nvca/agent_manager.go index 477bbc9c97..be56878234 100644 --- a/src/compute-plane-services/nvca/pkg/nvca/agent_manager.go +++ b/src/compute-plane-services/nvca/pkg/nvca/agent_manager.go @@ -225,6 +225,7 @@ func startControllerManagerForAgent( // Add GC cleaners to the manager gcRunnable := gc.NewRunnable(clients, metrics, gc.DefaultInterval, a.RequestsNamespace) + gcRunnable.SetLogContext(ctx) if err := mgr.Add(gcRunnable); err != nil { log.WithError(err).Error("Failed to add GC controller to controller manager") return fmt.Errorf("add GC controller to controller manager: %v", err) diff --git a/src/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook.go b/src/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook.go index d32642e08b..aab4dc06a3 100644 --- a/src/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook.go +++ b/src/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook.go @@ -49,6 +49,7 @@ import ( "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/internal/util/k8sutil" nvcfdra "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/pkg/dra" "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/pkg/featureflag" + "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/pkg/storage" nvcatypes "github.com/NVIDIA/nvcf/src/compute-plane-services/nvca/pkg/types" ) @@ -219,6 +220,13 @@ func (w *miniserviceMutatingWebhook) Handle(ctx context.Context, req admission.R // MutatingWebhookConfiguration should prevent this case in practice. return admission.Allowed("cluster-scoped object: skipping miniservice mutating webhook") } + if namespace == storage.ModelCacheInitNamespace { + // The shared model-cache init namespace matches the webhook's namespace + // selector (it carries the miniservice instance-type label for DNS + // injection) but hosts cache writer jobs, not function instances: there + // is no per-instance metadata ConfigMap to inject. + return admission.Allowed("model-cache init namespace: no NVCF metadata to inject") + } gvk := schema.GroupVersionKind(req.Kind) diff --git a/src/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook_test.go b/src/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook_test.go index 49a8f49d03..9c20a70270 100644 --- a/src/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook_test.go +++ b/src/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook_test.go @@ -183,6 +183,26 @@ func TestMiniserviceOperatorWebhook_ConfigMapMissing(t *testing.T) { assert.Equal(t, http.StatusForbidden, int(resp.Result.Code), "Denied should use 403, not 500 (Errored)") } +func TestMiniserviceOperatorWebhook_ModelCacheInitNamespace_Allowed(t *testing.T) { + wh := makeWebhook(t) // no ConfigMap, as in the real init namespace + ctx := core.WithDefaultLogger(context.Background()) + + pod := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Pod"}, + ObjectMeta: metav1.ObjectMeta{Name: "writer-job-abc-xyz", Namespace: cmnnvcastorage.ModelCacheInitNamespace}, + } + raw, _ := json.Marshal(pod) + req := admission.Request{} + req.Namespace = cmnnvcastorage.ModelCacheInitNamespace + req.Kind = metav1.GroupVersionKind{Version: "v1", Kind: "Pod"} + req.Object = runtime.RawExtension{Raw: raw} + req.Operation = admissionv1.Create + + resp := wh.Handle(ctx, req) + assert.True(t, resp.Allowed, "cache writer pods carry no instance metadata and must not be denied") + assert.Empty(t, resp.Patches) +} + func TestMiniserviceOperatorWebhook_ClusterScopedObject(t *testing.T) { wh := makeWebhook(t) // no ConfigMap needed ctx := core.WithDefaultLogger(context.Background()) From b74d8d15e9e4978b6f1bcb60bb3040cc3911e813 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Fri, 18 Sep 2026 14:07:31 -0700 Subject: [PATCH 2/2] fix(ci): push each image of a multi-image subtree to its own dev repository image-push mapped every target named `image` to the service repository. NVCA has four such targets, cmd/nvca, cmd/nvca-operator, cmd/cluster-validator and cmd/tools, so a deploy-to-stg build pushed all four to /nvca: and the last one won. When a subtree has more than one plain `image` target, the package leaf now names the repository, with the leaf equal to the service keeping the service name. Subtrees with a single `image` target keep mapping to the service wherever that target lives. Co-Authored-By: Balaji Ganesan (cherry picked from commit 94b4dd04f7aa7c0593c5f7d0641043fc5d97ee7f) --- .github/workflows/image-push-manual.yml | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/.github/workflows/image-push-manual.yml b/.github/workflows/image-push-manual.yml index dda5a035ae..2fd8351498 100644 --- a/.github/workflows/image-push-manual.yml +++ b/.github/workflows/image-push-manual.yml @@ -328,6 +328,18 @@ jobs: exit 1 fi echo "discovered: ${indexes[*]}" + # A subtree with several targets all named `image` (nvca: cmd/nvca, + # cmd/nvca-operator, cmd/cluster-validator, cmd/tools) would map every + # one of them to the service repo and the last push would win. When + # that is the case, the package leaf names the repo instead, with the + # leaf that equals the service keeping the plain service name. A + # subtree with a single `image` target keeps mapping to the service + # wherever that target lives (cmd/, crates/server/, ...). + plain_image_count=0 + for tgt in "${indexes[@]}"; do + n="${tgt##*:}"; n="${n%_index}" + [ "$n" = "image" ] && plain_image_count=$((plain_image_count + 1)) + done for tgt in "${indexes[@]}"; do name="${tgt##*:}"; name="${name%_index}" # Two naming conventions exist in the tree and they mean different @@ -342,7 +354,13 @@ jobs: # Previously the hyphenated form fell through to the default and # produced names like byoo-otel-collector-byoo-otel-collector-image. case "$name" in - image) repo="${svc}" ;; + image) + if [ "$plain_image_count" -gt 1 ]; then + pkg="${tgt%%:*}"; leaf="${pkg##*/}" + if [ "$leaf" = "$svc" ]; then repo="${svc}"; else repo="${leaf}"; fi + else + repo="${svc}" + fi ;; *_image) sub="$(printf '%s' "${name%_image}" | tr '_' '-')"; repo="${svc}-${sub}" ;; *-image) repo="${name%-image}" ;; *) sub="$(printf '%s' "$name" | tr '_' '-')"; repo="${svc}-${sub}" ;;