From a03d0f8e2f91dd552131e0ea0b4bb385420b4d8d Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Tue, 25 Aug 2026 15:46:10 -0700 Subject: [PATCH 1/4] fix(validator): reject incompatible StorageClass before model-cache PVC create Some GPU node families can only attach a subset of a CSI provisioner's disk types (e.g. GKE's a4x-highgpu-4g nodes reject Persistent Disk entirely, including pd-balanced, and need Hyperdisk instead). Without a pre-flight check, the inference-perf model-cache PVC binds to an incompatible StorageClass and the workload just sits Pending until the populate-Job timeout, with no indication of the real cause. Add a rule-table (storageCompatibilityRules) keyed by provisioner and machine family, and check the cache PVC's resolved StorageClass (explicit override or cluster default) against the chosen node's instance-type family before creating it. An incompatible combination now fails immediately with the concrete remediation instead of a slow, opaque timeout. The table is provisioner/family-driven so a future incompatibility on another cloud can be added without touching the check logic itself. Also select the effective default StorageClass by CreationTimestamp when more than one is annotated default, matching the cluster's own DefaultStorageClass admission controller tie-break, and accept parameters.type=dynamic for GKE's pd.csi.storage.gke.io driver on a4x nodes alongside the hyperdisk- prefix, since dynamic always resolves to Hyperdisk on a node family that can't attach Persistent Disk. Signed-off-by: Mike Cook --- .../performance/inference_perf_constraint.go | 2 + validators/performance/model_cache.go | 123 ++++++-- validators/performance/model_cache_test.go | 270 ++++++++++++++---- 3 files changed, 318 insertions(+), 77 deletions(-) diff --git a/validators/performance/inference_perf_constraint.go b/validators/performance/inference_perf_constraint.go index d4fa5f883..c41a748d0 100644 --- a/validators/performance/inference_perf_constraint.go +++ b/validators/performance/inference_perf_constraint.go @@ -405,6 +405,7 @@ type inferenceWorkloadConfig struct { deployedByUs bool // true if we (or a prior run we own) created the workload modelCacheSize string // PVC size (e.g. "100Gi") enabling the model-weights cache; empty = disabled modelCacheStorageClass string // StorageClass for the cache PVC; empty = cluster default + gpuNodeInstanceType string // chosen node's node.kubernetes.io/instance-type; empty if unlabeled routingMode inferenceRoutingMode routerMode string // Dynamo frontend DYN_ROUTER_MODE (dynamo-router path only); env > default (see resolveRouterMode) @@ -770,6 +771,7 @@ func buildInferenceConfig(ctx *validators.Context, mode *allocmode.Mode) (*infer model: model, modelCacheSize: cacheSize, modelCacheStorageClass: strings.TrimSpace(os.Getenv(envModelCacheStorageClass)), + gpuNodeInstanceType: chosen.Labels[instanceTypeLabel], routingMode: routingMode, routerMode: routerMode, gpuAllocMode: mode, diff --git a/validators/performance/model_cache.go b/validators/performance/model_cache.go index 229b46987..cb4235884 100644 --- a/validators/performance/model_cache.go +++ b/validators/performance/model_cache.go @@ -28,6 +28,7 @@ import ( "github.com/NVIDIA/aicr/validators" batchv1 "k8s.io/api/batch/v1" v1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -125,6 +126,36 @@ const ( cacheWorkerFSGroup = int64(1000) ) +// storageCompatibilityRule declares that on a given CSI provisioner, the +// listed machine families can only attach a StorageClass whose +// parameters.type either carries compatibleTypePrefix or exactly matches +// autoSelectType (a driver-specific value that resolves to a compatible disk +// per-node rather than naming one directly, e.g. GKE's "dynamic"; leave empty +// for a provisioner with no such value). Anything else provisioned by that +// driver is rejected at attach time. Add a rule here for any other +// cloud/provisioner with the same shape of restriction; nothing else in this +// file needs to change. +type storageCompatibilityRule struct { + provisioner string + families map[string]bool // node.kubernetes.io/instance-type family segment, e.g. "a4x" for "a4x-highgpu-4g" + compatibleTypePrefix string + autoSelectType string + docsRef string +} + +var storageCompatibilityRules = []storageCompatibilityRule{ + { + provisioner: "pd.csi.storage.gke.io", // GKE Persistent Disk CSI driver (also provisions Hyperdisk) + families: map[string]bool{"a4x": true}, + compatibleTypePrefix: "hyperdisk-", + // "dynamic" auto-selects Hyperdisk vs Persistent Disk per the node's + // machine type (GKE 1.35.3-gke.1290000+); a4x can't attach Persistent + // Disk at all, so on a4x nodes it always resolves to Hyperdisk. + autoSelectType: "dynamic", + docsRef: "docs/integrator/gke-gb200-networking.md#storage-prerequisites", + }, +} + // modelCacheEnabled reports whether the PVC cache is active for this run. // config.modelCacheSize is "" only when the operator explicitly disabled it // (see parseModelCacheSize); the unset case has already been defaulted on. @@ -162,23 +193,65 @@ func parseModelCacheSize(raw string) (string, bool, error) { return raw, true, nil } -// clusterHasDefaultStorageClass reports whether any StorageClass on the cluster -// is annotated as the default. Used to fail fast before provisioning a cache -// PVC with no StorageClass on a cluster that has no default. -func clusterHasDefaultStorageClass(ctx *validators.Context) (bool, error) { +// defaultStorageClass returns the cluster's effective default StorageClass, +// or nil if none is annotated default. Kubernetes tolerates more than one +// StorageClass annotated default; its own DefaultStorageClass admission +// controller resolves the ambiguity by picking the most recently created one +// (https://kubernetes.io/docs/concepts/storage/storage-classes/#default-storageclass). +// Matching that here means this pre-flight checks the same StorageClass a PVC +// with no storageClassName would actually bind to. +func defaultStorageClass(ctx *validators.Context) (*storagev1.StorageClass, error) { listCtx, cancel := context.WithTimeout(ctx.Ctx, defaults.DiagnosticTimeout) defer cancel() scs, err := ctx.Clientset.StorageV1().StorageClasses().List(listCtx, metav1.ListOptions{}) if err != nil { - return false, errors.Wrap(errors.ErrCodeInternal, "failed to list StorageClasses for cache pre-flight", err) + return nil, errors.Wrap(errors.ErrCodeInternal, "failed to list StorageClasses for cache pre-flight", err) } + var best *storagev1.StorageClass for i := range scs.Items { - ann := scs.Items[i].Annotations - if ann[defaultStorageClassAnnotation] == defaultStorageClassAnnotationValue || ann[defaultStorageClassAnnotationBeta] == defaultStorageClassAnnotationValue { - return true, nil + sc := &scs.Items[i] + ann := sc.Annotations + if ann[defaultStorageClassAnnotation] != defaultStorageClassAnnotationValue && ann[defaultStorageClassAnnotationBeta] != defaultStorageClassAnnotationValue { + continue + } + if best == nil || sc.CreationTimestamp.After(best.CreationTimestamp.Time) { + best = sc + } + } + return best, nil //nolint:nilnil // nil, nil means no default StorageClass is set, not an error +} + +// machineFamily returns the leading segment of a node.kubernetes.io/instance-type +// value, e.g. "a4x" for "a4x-highgpu-4g". Empty for an empty or family-less input. +func machineFamily(instanceType string) string { + family, _, _ := strings.Cut(instanceType, "-") + return family +} + +// checkStorageClassNodeCompatibility reports an error when sc's disk type +// can't attach to the worker node's machine family, per +// storageCompatibilityRules. A nil sc (not found, e.g. a typo in the +// explicit override) is not an error here; that surfaces via the normal +// PVC-create path instead. +func checkStorageClassNodeCompatibility(instanceType string, sc *storagev1.StorageClass) error { + if sc == nil { + return nil + } + family := machineFamily(instanceType) + for _, rule := range storageCompatibilityRules { + if sc.Provisioner != rule.provisioner || !rule.families[family] { + continue } + typ := sc.Parameters["type"] + if strings.HasPrefix(typ, rule.compatibleTypePrefix) || (rule.autoSelectType != "" && typ == rule.autoSelectType) { + continue + } + return errors.New(errors.ErrCodeInvalidRequest, fmt.Sprintf( + "model-weights cache PVC would bind to StorageClass %q (provisioner %s), which node machine family %q can't attach; "+ + "set %s to a StorageClass whose parameters.type starts with %q, or disable the cache with %s=off; see %s", + sc.Name, sc.Provisioner, family, envModelCacheStorageClass, rule.compatibleTypePrefix, envModelCacheSize, rule.docsRef)) } - return false, nil + return nil } // ensureModelCache provisions the model-weights cache when enabled: an RWO PVC @@ -207,21 +280,39 @@ func ensureModelCache(ctx *validators.Context, config *inferenceWorkloadConfig) fmt.Sprintf("invalid %s=%q: must be a Kubernetes quantity (e.g. 100Gi)", envModelCacheSize, config.modelCacheSize), err) } - // Fail fast when there is no StorageClass to bind the cache PVC to: with no - // explicit MODEL_CACHE_STORAGE_CLASS, the PVC relies on a cluster default, - // and without one it sits Pending until the populate Job times out (minutes). - // Surface an actionable error immediately instead. - if strings.TrimSpace(config.modelCacheStorageClass) == "" { - hasDefault, derr := clusterHasDefaultStorageClass(ctx) + // Resolve the StorageClass the cache PVC will bind to (explicit name, or + // the cluster default) for the checks below. + explicitSC := strings.TrimSpace(config.modelCacheStorageClass) + var resolvedSC *storagev1.StorageClass + if explicitSC == "" { + sc, derr := defaultStorageClass(ctx) if derr != nil { return derr } - if !hasDefault { + if sc == nil { return errors.New(errors.ErrCodeInvalidRequest, fmt.Sprintf("model-weights cache is enabled but the cluster has no default StorageClass and %s is unset; "+ "set %s= (e.g. gp2/gp3 on EKS, standard-rwo on GKE) or disable the cache with %s=off", envModelCacheStorageClass, envModelCacheStorageClass, envModelCacheSize)) } + resolvedSC = sc + } else { + getCtx, getCancel := context.WithTimeout(ctx.Ctx, defaults.DiagnosticTimeout) + sc, gerr := ctx.Clientset.StorageV1().StorageClasses().Get(getCtx, explicitSC, metav1.GetOptions{}) + getCancel() + switch { + case gerr == nil: + resolvedSC = sc + case apierrors.IsNotFound(gerr): + // Leave resolvedSC nil: fall through to the existing PVC-create + // path, which surfaces a nonexistent StorageClass the same way + // it always has. + default: + return errors.Wrap(errors.ErrCodeInternal, "failed to get StorageClass for cache pre-flight", gerr) + } + } + if cerr := checkStorageClassNodeCompatibility(config.gpuNodeInstanceType, resolvedSC); cerr != nil { + return cerr } // Bound the create calls so a slow/wedged apiserver can't burn the check diff --git a/validators/performance/model_cache_test.go b/validators/performance/model_cache_test.go index ce3fd6618..0af37145f 100644 --- a/validators/performance/model_cache_test.go +++ b/validators/performance/model_cache_test.go @@ -29,6 +29,7 @@ import ( storagev1 "k8s.io/api/storage/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/fake" ) @@ -310,22 +311,6 @@ func TestWrapPopulateJobError(t *testing.T) { } } -// TestEnsureModelCache_DisabledNoop verifies that with the cache disabled no PVC -// or Job is created — the default behavior is unchanged. - -func TestEnsureModelCache_DisabledNoop(t *testing.T) { - client := fake.NewClientset() - ctx := &validators.Context{Ctx: context.Background(), Clientset: client} - cfg := &inferenceWorkloadConfig{namespace: "ns", modelCacheSize: ""} - if err := ensureModelCache(ctx, cfg); err != nil { - t.Fatalf("unexpected error: %v", err) - } - pvcs, _ := client.CoreV1().PersistentVolumeClaims("ns").List(context.Background(), metav1.ListOptions{}) - if len(pvcs.Items) != 0 { - t.Errorf("no PVC should be created when cache disabled, got %d", len(pvcs.Items)) - } -} - // TestParseModelCacheSize verifies the on-by-default policy: unset → default // size (enabled), the disable sentinels → disabled, an explicit quantity passes // through, and garbage fails closed. @@ -368,55 +353,218 @@ func TestParseModelCacheSize(t *testing.T) { } } -// TestClusterHasDefaultStorageClass verifies detection of a default-annotated -// StorageClass (the cache pre-flight's signal). -func TestClusterHasDefaultStorageClass(t *testing.T) { - def := &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ - Name: "gp3", Annotations: map[string]string{defaultStorageClassAnnotation: "true"}}} - nondef := &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{Name: "gp2"}} +// TestDefaultStorageClass verifies detection of a default-annotated +// StorageClass (the cache pre-flight's signal). When more than one +// StorageClass is annotated default, Kubernetes' own admission controller +// picks the most recently created one — the two "multiple defaults" cases +// below list the same pair in both orders to prove selection follows +// CreationTimestamp, not list order. +func TestDefaultStorageClass(t *testing.T) { + older := metav1.NewTime(time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)) + newer := metav1.NewTime(time.Date(2026, 6, 1, 0, 0, 0, 0, time.UTC)) - t.Run("has default", func(t *testing.T) { - ctx := &validators.Context{Ctx: context.Background(), Clientset: fake.NewClientset(def, nondef)} - got, err := clusterHasDefaultStorageClass(ctx) - if err != nil || !got { - t.Errorf("got (%v,%v), want (true,nil)", got, err) - } - }) - t.Run("no default", func(t *testing.T) { - ctx := &validators.Context{Ctx: context.Background(), Clientset: fake.NewClientset(nondef)} - got, err := clusterHasDefaultStorageClass(ctx) - if err != nil || got { - t.Errorf("got (%v,%v), want (false,nil)", got, err) - } - }) - t.Run("legacy beta annotation counts as default", func(t *testing.T) { - beta := &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ - Name: "gp2", Annotations: map[string]string{defaultStorageClassAnnotationBeta: "true"}}} - ctx := &validators.Context{Ctx: context.Background(), Clientset: fake.NewClientset(beta)} - got, err := clusterHasDefaultStorageClass(ctx) - if err != nil || !got { - t.Errorf("got (%v,%v), want (true,nil) for beta is-default-class annotation", got, err) + tests := []struct { + name string + classes []runtime.Object + wantName string // "" means want nil (no default) + }{ + { + name: "has default", + classes: []runtime.Object{ + &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ + Name: "gp3", Annotations: map[string]string{defaultStorageClassAnnotation: "true"}}}, + &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{Name: "gp2"}}, + }, + wantName: "gp3", + }, + { + name: "no default", + classes: []runtime.Object{ + &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{Name: "gp2"}}, + }, + wantName: "", + }, + { + name: "legacy beta annotation counts as default", + classes: []runtime.Object{ + &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ + Name: "gp2", Annotations: map[string]string{defaultStorageClassAnnotationBeta: "true"}}}, + }, + wantName: "gp2", + }, + { + name: "multiple defaults, newer listed first: newer still wins", + classes: []runtime.Object{ + &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ + Name: "newer", CreationTimestamp: newer, Annotations: map[string]string{defaultStorageClassAnnotation: "true"}}}, + &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ + Name: "older", CreationTimestamp: older, Annotations: map[string]string{defaultStorageClassAnnotation: "true"}}}, + }, + wantName: "newer", + }, + { + name: "multiple defaults, older listed first: newer still wins", + classes: []runtime.Object{ + &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ + Name: "older", CreationTimestamp: older, Annotations: map[string]string{defaultStorageClassAnnotation: "true"}}}, + &storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ + Name: "newer", CreationTimestamp: newer, Annotations: map[string]string{defaultStorageClassAnnotation: "true"}}}, + }, + wantName: "newer", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := &validators.Context{Ctx: context.Background(), Clientset: fake.NewClientset(tt.classes...)} + got, err := defaultStorageClass(ctx) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + gotName := "" + if got != nil { + gotName = got.Name + } + if gotName != tt.wantName { + t.Errorf("got %q, want %q", gotName, tt.wantName) + } + }) + } +} + +// TestMachineFamily verifies the family segment extracted from a +// node.kubernetes.io/instance-type value. +func TestMachineFamily(t *testing.T) { + tests := []struct { + instanceType string + want string + }{ + {"a4x-highgpu-4g", "a4x"}, + {"n2-standard-4", "n2"}, + {"", ""}, + } + for _, tt := range tests { + if got := machineFamily(tt.instanceType); got != tt.want { + t.Errorf("machineFamily(%q) = %q, want %q", tt.instanceType, got, tt.want) } - }) + } +} + +// TestCheckStorageClassNodeCompatibility verifies the rule-table lookup: a +// machine family listed under a rule can only attach a StorageClass whose +// parameters.type carries that rule's compatibleTypePrefix; every other +// family/provisioner/type combination passes. +func TestCheckStorageClassNodeCompatibility(t *testing.T) { + pdBalanced := &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{Name: "standard-rwo"}, + Provisioner: "pd.csi.storage.gke.io", + Parameters: map[string]string{"type": "pd-balanced"}, + } + hyperdiskBalanced := &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{Name: "hyperdisk-balanced"}, + Provisioner: "pd.csi.storage.gke.io", + Parameters: map[string]string{"type": "hyperdisk-balanced"}, + } + dynamicSelect := &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{Name: "dynamic-volume"}, + Provisioner: "pd.csi.storage.gke.io", + Parameters: map[string]string{"type": "dynamic", "pd-type": "pd-balanced", "hyperdisk-type": "hyperdisk-balanced"}, + } + otherProvisioner := &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{Name: "gp3"}, + Provisioner: "ebs.csi.aws.com", + Parameters: map[string]string{"type": "gp3"}, + } + + tests := []struct { + name string + instanceType string + sc *storagev1.StorageClass + wantErr bool + }{ + {"a4x with Persistent Disk is rejected", "a4x-highgpu-4g", pdBalanced, true}, + {"a4x with explicit Hyperdisk selection is fine", "a4x-highgpu-4g", hyperdiskBalanced, false}, + {"a4x with dynamic disk-type selection is fine", "a4x-highgpu-4g", dynamicSelect, false}, + {"non-a4x family with Persistent Disk is fine", "n2-standard-4", pdBalanced, false}, + {"non-a4x family with dynamic disk-type selection is fine", "n2-standard-4", dynamicSelect, false}, + {"a4x with an unrelated provisioner is fine", "a4x-highgpu-4g", otherProvisioner, false}, + {"nil StorageClass is fine (not this function's concern)", "a4x-highgpu-4g", nil, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := checkStorageClassNodeCompatibility(tt.instanceType, tt.sc) + if (err != nil) != tt.wantErr { + t.Fatalf("err = %v, wantErr %v", err, tt.wantErr) + } + if tt.wantErr && !stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) { + t.Errorf("error code = %v, want ErrCodeInvalidRequest", err) + } + }) + } } -// TestEnsureModelCache_NoDefaultStorageClassFailsFast verifies that with the -// cache enabled, no explicit StorageClass, and no cluster default, the validator -// fails fast (ErrCodeInvalidRequest) without creating the PVC — rather than -// leaving it Pending until the populate-Job timeout. -func TestEnsureModelCache_NoDefaultStorageClassFailsFast(t *testing.T) { - ctx := &validators.Context{Ctx: context.Background(), Clientset: fake.NewClientset()} - cfg := &inferenceWorkloadConfig{namespace: "ns", model: "Qwen/Qwen3-8B", modelCacheSize: defaultModelCacheSize} - err := ensureModelCache(ctx, cfg) - if err == nil { - t.Fatal("expected fast-fail error when cache enabled with no default StorageClass") - } - if !stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) { - t.Errorf("error code = %v, want ErrCodeInvalidRequest", err) - } - pvcs, _ := ctx.Clientset.CoreV1().PersistentVolumeClaims("ns").List(context.Background(), metav1.ListOptions{}) - if len(pvcs.Items) != 0 { - t.Errorf("no PVC should be created on fast-fail, got %d", len(pvcs.Items)) +// TestEnsureModelCache covers the pre-flight's error paths: disabled is a +// no-op, an enabled cache with no resolvable StorageClass errors, and an +// enabled cache whose resolved StorageClass (cluster-default or explicit +// override) is incompatible with the node's machine family errors too, +// in every case before a PVC is created, rather than that surfacing later +// as a Pending claim or FailedAttachVolume. +func TestEnsureModelCache(t *testing.T) { + pdBalanced := &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{Name: "standard-rwo", Annotations: map[string]string{defaultStorageClassAnnotation: "true"}}, + Provisioner: "pd.csi.storage.gke.io", + Parameters: map[string]string{"type": "pd-balanced"}, + } + + tests := []struct { + name string + classes []runtime.Object + cfg *inferenceWorkloadConfig + wantErr bool + }{ + { + name: "disabled is a no-op", + cfg: &inferenceWorkloadConfig{namespace: "ns", modelCacheSize: ""}, + }, + { + name: "no default StorageClass errors", + cfg: &inferenceWorkloadConfig{namespace: "ns", model: "Qwen/Qwen3-8B", modelCacheSize: defaultModelCacheSize}, + wantErr: true, + }, + { + name: "incompatible cluster default is rejected", + classes: []runtime.Object{pdBalanced}, + cfg: &inferenceWorkloadConfig{ + namespace: "ns", model: "Qwen/Qwen3-8B", modelCacheSize: defaultModelCacheSize, + gpuNodeInstanceType: "a4x-highgpu-4g", + }, + wantErr: true, + }, + { + name: "incompatible explicit override is rejected", + classes: []runtime.Object{pdBalanced}, + cfg: &inferenceWorkloadConfig{ + namespace: "ns", model: "Qwen/Qwen3-8B", modelCacheSize: defaultModelCacheSize, + gpuNodeInstanceType: "a4x-highgpu-4g", modelCacheStorageClass: "standard-rwo", + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := fake.NewClientset(tt.classes...) + ctx := &validators.Context{Ctx: context.Background(), Clientset: client} + err := ensureModelCache(ctx, tt.cfg) + if (err != nil) != tt.wantErr { + t.Fatalf("err = %v, wantErr %v", err, tt.wantErr) + } + if tt.wantErr && !stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) { + t.Errorf("error code = %v, want ErrCodeInvalidRequest", err) + } + pvcs, _ := client.CoreV1().PersistentVolumeClaims(tt.cfg.namespace).List(context.Background(), metav1.ListOptions{}) + if len(pvcs.Items) != 0 { + t.Errorf("no PVC should be created, got %d", len(pvcs.Items)) + } + }) } } From 0dcbb5955ce1fe686694911090d4b3f36fce2bfb Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 14:13:54 -0700 Subject: [PATCH 2/4] fix(validator): pin resolved default StorageClass on cache PVC The pre-flight check validated the cluster-default StorageClass resolved at list time, but PVC creation still left StorageClassName nil when there was no explicit override, letting Kubernetes re-resolve the default at admission. If the cluster default changed between the check and admission, an unvalidated (possibly incompatible) StorageClass could bind, reintroducing the Pending / attach failure this preflight exists to prevent. Pin the PVC to resolvedSC.Name when the StorageClass was implicit; explicit overrides are unaffected. Signed-off-by: Mike Cook --- validators/performance/model_cache.go | 11 ++++++- validators/performance/model_cache_test.go | 37 ++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/validators/performance/model_cache.go b/validators/performance/model_cache.go index cb4235884..bfebabfc3 100644 --- a/validators/performance/model_cache.go +++ b/validators/performance/model_cache.go @@ -315,9 +315,18 @@ func ensureModelCache(ctx *validators.Context, config *inferenceWorkloadConfig) return cerr } + // Pin the StorageClass we just validated onto the PVC: if we resolved an + // implicit cluster default above, use its name explicitly rather than + // leaving StorageClassName nil, so a default that changes between this + // check and PVC admission can't silently bind an unvalidated StorageClass. + pvcStorageClass := explicitSC + if pvcStorageClass == "" && resolvedSC != nil { + pvcStorageClass = resolvedSC.Name + } + // Bound the create calls so a slow/wedged apiserver can't burn the check // budget before the (separately bounded) populate-Job wait even starts. - pvc := buildModelCachePVC(config.namespace, qty, strings.TrimSpace(config.modelCacheStorageClass)) + pvc := buildModelCachePVC(config.namespace, qty, pvcStorageClass) pvcCtx, pvcCancel := context.WithTimeout(ctx.Ctx, defaults.DiagnosticTimeout) _, err = ctx.Clientset.CoreV1().PersistentVolumeClaims(config.namespace).Create(pvcCtx, pvc, metav1.CreateOptions{}) pvcCancel() diff --git a/validators/performance/model_cache_test.go b/validators/performance/model_cache_test.go index 0af37145f..dc8873116 100644 --- a/validators/performance/model_cache_test.go +++ b/validators/performance/model_cache_test.go @@ -568,6 +568,43 @@ func TestEnsureModelCache(t *testing.T) { } } +// TestEnsureModelCachePinsImplicitDefaultStorageClass verifies the PVC created +// for an implicit (cluster-default) StorageClass is pinned to the exact +// default resolved and validated above, rather than left nil. A nil +// StorageClassName lets the apiserver re-resolve the default at admission +// time, which could pick a different (possibly incompatible) default if it +// changed between this pre-flight check and PVC creation. +func TestEnsureModelCachePinsImplicitDefaultStorageClass(t *testing.T) { + def := &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{Name: "standard-rwo", Annotations: map[string]string{defaultStorageClassAnnotation: "true"}}, + Provisioner: "pd.csi.storage.gke.io", + Parameters: map[string]string{"type": "pd-balanced"}, + } + // Non-a4x family: compatible with pd-balanced, so the pre-flight check + // passes and execution reaches PVC creation. + cfg := &inferenceWorkloadConfig{ + namespace: "ns", model: "Qwen/Qwen3-8B", modelCacheSize: defaultModelCacheSize, + gpuNodeInstanceType: "n2-standard-4", runID: "run1", + } + // Force the populate-Job wait to fail fast instead of hanging on the fake + // clientset's Job status, which never progresses to Complete. + t.Setenv(envModelCachePopulateTimeout, "1ms") + + client := fake.NewClientset(def) + ctx := &validators.Context{Ctx: context.Background(), Clientset: client} + if err := ensureModelCache(ctx, cfg); err == nil { + t.Fatal("expected populate-Job wait to time out on the fake clientset") + } + + pvc, err := client.CoreV1().PersistentVolumeClaims(cfg.namespace).Get(context.Background(), modelCachePVCName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("expected PVC to be created before the populate-Job wait, got: %v", err) + } + if pvc.Spec.StorageClassName == nil || *pvc.Spec.StorageClassName != def.Name { + t.Errorf("PVC StorageClassName = %v, want pinned to resolved default %q", pvc.Spec.StorageClassName, def.Name) + } +} + // TestCacheWorkerImageMatchesTemplate guards the cacheWorkerImage constant // against drifting from the worker image in the Dynamo deploy template. The // populate Job must download with the same vLLM runtime the workers use: the From b40327f027c419f542740cfc3e83ba79fb68655b Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 15:09:22 -0700 Subject: [PATCH 3/4] style(validator): move nolint:nilnil directive onto its own line Directive-shaped comments must be self-contained: put the rationale in a normal comment before the directive, not trailing it on the same line as the guarded return. No functional change. Signed-off-by: Mike Cook --- validators/performance/model_cache.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/validators/performance/model_cache.go b/validators/performance/model_cache.go index bfebabfc3..a50756fa4 100644 --- a/validators/performance/model_cache.go +++ b/validators/performance/model_cache.go @@ -218,7 +218,9 @@ func defaultStorageClass(ctx *validators.Context) (*storagev1.StorageClass, erro best = sc } } - return best, nil //nolint:nilnil // nil, nil means no default StorageClass is set, not an error + // nil, nil means no default StorageClass is set, not an error. + //nolint:nilnil + return best, nil } // machineFamily returns the leading segment of a node.kubernetes.io/instance-type From 6ae021803d97961f742314e68d03e253c1a16914 Mon Sep 17 00:00:00 2001 From: Mike Cook Date: Fri, 28 Aug 2026 15:10:43 -0700 Subject: [PATCH 4/4] fix(validator): mention autoSelectType in cache StorageClass remediation The compatibility-check error only told operators to pick a parameters.type starting with compatibleTypePrefix, omitting that autoSelectType (e.g. GKE's "dynamic") is also accepted. Build the type guidance from the rule so it stays correct as new provisioner/family rules are added to the table. Signed-off-by: Mike Cook --- validators/performance/model_cache.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/validators/performance/model_cache.go b/validators/performance/model_cache.go index a50756fa4..d059890e7 100644 --- a/validators/performance/model_cache.go +++ b/validators/performance/model_cache.go @@ -248,10 +248,14 @@ func checkStorageClassNodeCompatibility(instanceType string, sc *storagev1.Stora if strings.HasPrefix(typ, rule.compatibleTypePrefix) || (rule.autoSelectType != "" && typ == rule.autoSelectType) { continue } + typeGuidance := fmt.Sprintf("parameters.type starts with %q", rule.compatibleTypePrefix) + if rule.autoSelectType != "" { + typeGuidance += fmt.Sprintf(" (or is %q)", rule.autoSelectType) + } return errors.New(errors.ErrCodeInvalidRequest, fmt.Sprintf( "model-weights cache PVC would bind to StorageClass %q (provisioner %s), which node machine family %q can't attach; "+ - "set %s to a StorageClass whose parameters.type starts with %q, or disable the cache with %s=off; see %s", - sc.Name, sc.Provisioner, family, envModelCacheStorageClass, rule.compatibleTypePrefix, envModelCacheSize, rule.docsRef)) + "set %s to a StorageClass whose %s, or disable the cache with %s=off; see %s", + sc.Name, sc.Provisioner, family, envModelCacheStorageClass, typeGuidance, envModelCacheSize, rule.docsRef)) } return nil }