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}" ;; 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())