Unverified Commit c8770464 authored by hhzhang16's avatar hhzhang16 Committed by GitHub
Browse files

feat: normalize dynamo namespace computation (#5231)


Signed-off-by: default avatarHannah Zhang <hannahz@nvidia.com>
parent abd4b5d9
...@@ -93,7 +93,6 @@ class DgdPlannerServiceConfig(BaseModel): ...@@ -93,7 +93,6 @@ class DgdPlannerServiceConfig(BaseModel):
automatically created and mounted by the profiler; no PVC dependencies automatically created and mounted by the profiler; no PVC dependencies
""" """
dynamoNamespace: str = "dynamo" # placeholder
componentType: str = "planner" componentType: str = "planner"
replicas: int = 1 replicas: int = 1
extraPodSpec: PodSpec = PodSpec( extraPodSpec: PodSpec = PodSpec(
......
...@@ -261,7 +261,6 @@ def generate_dgd_config_with_planner( ...@@ -261,7 +261,6 @@ def generate_dgd_config_with_planner(
# add the planner service # add the planner service
planner_config = DgdPlannerServiceConfig() planner_config = DgdPlannerServiceConfig()
frontend_service = config.spec.services["Frontend"] frontend_service = config.spec.services["Frontend"]
planner_config.dynamoNamespace = getattr(frontend_service, "dynamoNamespace", "dynamo") # type: ignore[attr-defined]
frontend_image: Optional[str] = None frontend_image: Optional[str] = None
if frontend_service.extraPodSpec and frontend_service.extraPodSpec.mainContainer: if frontend_service.extraPodSpec and frontend_service.extraPodSpec.mainContainer:
frontend_image = frontend_service.extraPodSpec.mainContainer.image frontend_image = frontend_service.extraPodSpec.mainContainer.image
...@@ -275,8 +274,6 @@ def generate_dgd_config_with_planner( ...@@ -275,8 +274,6 @@ def generate_dgd_config_with_planner(
# Override profiling-specific arguments with results from profiling # Override profiling-specific arguments with results from profiling
# Remove and re-add to ensure correct values from profiling context # Remove and re-add to ensure correct values from profiling context
# Note: --namespace is NOT added here; planner gets it from DYN_NAMESPACE env var
# which is automatically injected by the operator based on dynamoNamespace
planner_args = [ planner_args = [
arg arg
for arg in planner_args for arg in planner_args
...@@ -511,17 +508,6 @@ def _generate_mocker_config_with_planner( ...@@ -511,17 +508,6 @@ def _generate_mocker_config_with_planner(
# Add planner service (reuse the same planner config but with mocker backend) # Add planner service (reuse the same planner config but with mocker backend)
mocker_planner_dict = copy.deepcopy(planner_dict) mocker_planner_dict = copy.deepcopy(planner_dict)
# Get the mocker's dynamoNamespace from Frontend service
mocker_namespace = (
mocker_config.get("spec", {})
.get("services", {})
.get("Frontend", {})
.get("dynamoNamespace", "mocker-disagg")
)
# Update planner's dynamoNamespace to match mocker's namespace
mocker_planner_dict["dynamoNamespace"] = mocker_namespace
# Planner args use --key=value format, so we need to find and replace # Planner args use --key=value format, so we need to find and replace
planner_main_container = mocker_planner_dict.get("extraPodSpec", {}).get( planner_main_container = mocker_planner_dict.get("extraPodSpec", {}).get(
"mainContainer", {} "mainContainer", {}
......
...@@ -14,14 +14,12 @@ spec: ...@@ -14,14 +14,12 @@ spec:
value: "debug" value: "debug"
services: services:
Frontend: Frontend:
dynamoNamespace: dynamo
componentType: frontend componentType: frontend
replicas: 1 replicas: 1
extraPodSpec: extraPodSpec:
mainContainer: mainContainer:
image: ${IMAGE} image: ${IMAGE}
VllmDecodeWorker: VllmDecodeWorker:
dynamoNamespace: vllm-disagg
componentType: decode componentType: decode
replicas: 1 replicas: 1
resources: resources:
...@@ -40,7 +38,6 @@ spec: ...@@ -40,7 +38,6 @@ spec:
- --model - --model
- Qwen/Qwen3-0.6B - Qwen/Qwen3-0.6B
VllmPrefillWorker: VllmPrefillWorker:
dynamoNamespace: vllm-disagg
componentType: prefill componentType: prefill
replicas: 1 replicas: 1
resources: resources:
......
...@@ -20,6 +20,8 @@ ...@@ -20,6 +20,8 @@
package v1alpha1 package v1alpha1
import ( import (
"fmt"
commonconsts "github.com/ai-dynamo/dynamo/deploy/operator/internal/consts" commonconsts "github.com/ai-dynamo/dynamo/deploy/operator/internal/consts"
corev1 "k8s.io/api/core/v1" corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
...@@ -309,6 +311,21 @@ func (s *DynamoComponentDeployment) GetParentGraphDeploymentNamespace() string { ...@@ -309,6 +311,21 @@ func (s *DynamoComponentDeployment) GetParentGraphDeploymentNamespace() string {
return s.GetNamespace() return s.GetNamespace()
} }
// GetDynamoNamespace returns the Dynamo namespace for this component.
func (s *DynamoComponentDeployment) GetDynamoNamespace() string {
return ComputeDynamoNamespace(s.Spec.GlobalDynamoNamespace, s.GetNamespace(), s.GetParentGraphDeploymentName())
}
// ComputeDynamoNamespace is the single source of truth for computing the Dynamo namespace.
// If globalDynamoNamespace is true, returns "dynamo" (global constant).
// Otherwise, returns {k8sNamespace}-{dgdName}.
func ComputeDynamoNamespace(globalDynamoNamespace bool, k8sNamespace, dgdName string) string {
if globalDynamoNamespace {
return commonconsts.GlobalDynamoNamespace
}
return fmt.Sprintf("%s-%s", k8sNamespace, dgdName)
}
// ModelReference identifies a model served by this component // ModelReference identifies a model served by this component
type ModelReference struct { type ModelReference struct {
// Name is the base model identifier (e.g., "llama-3-70b-instruct-v1") // Name is the base model identifier (e.g., "llama-3-70b-instruct-v1")
......
...@@ -233,3 +233,8 @@ func (s *DynamoGraphDeployment) HasAnyMultinodeService() bool { ...@@ -233,3 +233,8 @@ func (s *DynamoGraphDeployment) HasAnyMultinodeService() bool {
} }
return false return false
} }
// GetDynamoNamespaceForService returns the Dynamo namespace for a given service.
func (s *DynamoGraphDeployment) GetDynamoNamespaceForService(service *DynamoComponentDeploymentSharedSpec) string {
return ComputeDynamoNamespace(service.GlobalDynamoNamespace, s.GetNamespace(), s.GetName())
}
...@@ -851,7 +851,7 @@ func TestDynamoComponentDeploymentReconciler_generateLeaderWorkerSet(t *testing. ...@@ -851,7 +851,7 @@ func TestDynamoComponentDeploymentReconciler_generateLeaderWorkerSet(t *testing.
{Name: commonconsts.DynamoComponentEnvVar, Value: commonconsts.ComponentTypeWorker}, {Name: commonconsts.DynamoComponentEnvVar, Value: commonconsts.ComponentTypeWorker},
{Name: commonconsts.DynamoDiscoveryBackendEnvVar, Value: "kubernetes"}, {Name: commonconsts.DynamoDiscoveryBackendEnvVar, Value: "kubernetes"},
{Name: "DYN_HEALTH_CHECK_ENABLED", Value: "false"}, {Name: "DYN_HEALTH_CHECK_ENABLED", Value: "false"},
{Name: commonconsts.DynamoNamespaceEnvVar, Value: "default"}, {Name: commonconsts.DynamoNamespaceEnvVar, Value: "default-test-lws-deploy"},
{Name: "DYN_PARENT_DGD_K8S_NAME", Value: "test-lws-deploy"}, {Name: "DYN_PARENT_DGD_K8S_NAME", Value: "test-lws-deploy"},
{Name: "DYN_PARENT_DGD_K8S_NAMESPACE", Value: "default"}, {Name: "DYN_PARENT_DGD_K8S_NAMESPACE", Value: "default"},
{Name: "DYN_SYSTEM_ENABLED", Value: "true"}, {Name: "DYN_SYSTEM_ENABLED", Value: "true"},
...@@ -986,7 +986,7 @@ func TestDynamoComponentDeploymentReconciler_generateLeaderWorkerSet(t *testing. ...@@ -986,7 +986,7 @@ func TestDynamoComponentDeploymentReconciler_generateLeaderWorkerSet(t *testing.
{Name: commonconsts.DynamoComponentEnvVar, Value: commonconsts.ComponentTypeWorker}, {Name: commonconsts.DynamoComponentEnvVar, Value: commonconsts.ComponentTypeWorker},
{Name: commonconsts.DynamoDiscoveryBackendEnvVar, Value: "kubernetes"}, {Name: commonconsts.DynamoDiscoveryBackendEnvVar, Value: "kubernetes"},
{Name: "DYN_HEALTH_CHECK_ENABLED", Value: "false"}, {Name: "DYN_HEALTH_CHECK_ENABLED", Value: "false"},
{Name: commonconsts.DynamoNamespaceEnvVar, Value: "default"}, {Name: commonconsts.DynamoNamespaceEnvVar, Value: "default-test-lws-deploy"},
{Name: "DYN_PARENT_DGD_K8S_NAME", Value: "test-lws-deploy"}, {Name: "DYN_PARENT_DGD_K8S_NAME", Value: "test-lws-deploy"},
{Name: "DYN_PARENT_DGD_K8S_NAMESPACE", Value: "default"}, {Name: "DYN_PARENT_DGD_K8S_NAMESPACE", Value: "default"},
{Name: "DYN_SYSTEM_ENABLED", Value: "true"}, {Name: "DYN_SYSTEM_ENABLED", Value: "true"},
......
...@@ -154,6 +154,17 @@ func (r *DynamoGraphDeploymentReconciler) Reconcile(ctx context.Context, req ctr ...@@ -154,6 +154,17 @@ func (r *DynamoGraphDeploymentReconciler) Reconcile(ctx context.Context, req ctr
logger.Info("Reconciliation done") logger.Info("Reconciliation done")
}() }()
// Handle finalizer
deleted, err := commoncontroller.HandleFinalizer(ctx, dynamoDeployment, r.Client, r)
if err != nil {
logger.Error(err, "failed to handle the finalizer")
reason = "failed_to_handle_the_finalizer"
return ctrl.Result{}, err
}
if deleted {
return ctrl.Result{}, nil
}
// Validate the DynamoGraphDeployment spec (defense in depth - only when webhooks are disabled) // Validate the DynamoGraphDeployment spec (defense in depth - only when webhooks are disabled)
if !r.Config.WebhooksEnabled { if !r.Config.WebhooksEnabled {
validator := webhookvalidation.NewDynamoGraphDeploymentValidator(dynamoDeployment) validator := webhookvalidation.NewDynamoGraphDeploymentValidator(dynamoDeployment)
...@@ -176,15 +187,6 @@ func (r *DynamoGraphDeploymentReconciler) Reconcile(ctx context.Context, req ctr ...@@ -176,15 +187,6 @@ func (r *DynamoGraphDeploymentReconciler) Reconcile(ctx context.Context, req ctr
} }
} }
deleted, err := commoncontroller.HandleFinalizer(ctx, dynamoDeployment, r.Client, r)
if err != nil {
logger.Error(err, "failed to handle the finalizer")
reason = "failed_to_handle_the_finalizer"
return ctrl.Result{}, err
}
if deleted {
return ctrl.Result{}, nil
}
reconcileResult, err := r.reconcileResources(ctx, dynamoDeployment) reconcileResult, err := r.reconcileResources(ctx, dynamoDeployment)
state = reconcileResult.State state = reconcileResult.State
......
...@@ -337,10 +337,7 @@ func GenerateDynamoComponentsDeployments(ctx context.Context, parentDynamoGraphD ...@@ -337,10 +337,7 @@ func GenerateDynamoComponentsDeployments(ctx context.Context, parentDynamoGraphD
} }
func getDynamoNamespace(object metav1.Object, service *v1alpha1.DynamoComponentDeploymentSharedSpec) string { func getDynamoNamespace(object metav1.Object, service *v1alpha1.DynamoComponentDeploymentSharedSpec) string {
if service.GlobalDynamoNamespace { return v1alpha1.ComputeDynamoNamespace(service.GlobalDynamoNamespace, object.GetNamespace(), object.GetName())
return commonconsts.GlobalDynamoNamespace
}
return fmt.Sprintf("%s-%s", object.GetNamespace(), object.GetName())
} }
// updateDynDeploymentConfig updates the runtime config object for the given dynamoDeploymentComponent // updateDynDeploymentConfig updates the runtime config object for the given dynamoDeploymentComponent
...@@ -1071,15 +1068,15 @@ func setMetricsLabels(labels map[string]string, dynamoGraphDeployment *v1alpha1. ...@@ -1071,15 +1068,15 @@ func setMetricsLabels(labels map[string]string, dynamoGraphDeployment *v1alpha1.
} }
func generateComponentContext(component *v1alpha1.DynamoComponentDeploymentSharedSpec, parentGraphDeploymentName string, namespace string, numberOfNodes int32, discoveryBackend string) ComponentContext { func generateComponentContext(component *v1alpha1.DynamoComponentDeploymentSharedSpec, parentGraphDeploymentName string, namespace string, numberOfNodes int32, discoveryBackend string) ComponentContext {
dynamoNamespace := v1alpha1.ComputeDynamoNamespace(component.GlobalDynamoNamespace, namespace, parentGraphDeploymentName)
componentContext := ComponentContext{ componentContext := ComponentContext{
numberOfNodes: numberOfNodes, numberOfNodes: numberOfNodes,
ComponentType: component.ComponentType, ComponentType: component.ComponentType,
ParentGraphDeploymentName: parentGraphDeploymentName, ParentGraphDeploymentName: parentGraphDeploymentName,
ParentGraphDeploymentNamespace: namespace, ParentGraphDeploymentNamespace: namespace,
DiscoveryBackend: discoveryBackend, DiscoveryBackend: discoveryBackend,
} DynamoNamespace: dynamoNamespace,
if component.DynamoNamespace != nil {
componentContext.DynamoNamespace = *component.DynamoNamespace
} }
return componentContext return componentContext
} }
......
...@@ -780,6 +780,123 @@ func Test_GetDynamoComponentDeploymentsGlobalNamespace(t *testing.T) { ...@@ -780,6 +780,123 @@ func Test_GetDynamoComponentDeploymentsGlobalNamespace(t *testing.T) {
} }
} }
// TestGenerateComponentContext tests the generateComponentContext function
// to ensure it correctly computes the DynamoNamespace from authoritative sources
// (k8s namespace + DGD name), ignoring any deprecated dynamoNamespace field.
func TestGenerateComponentContext(t *testing.T) {
tests := []struct {
name string
component *v1alpha1.DynamoComponentDeploymentSharedSpec
parentGraphDeploymentName string
namespace string
numberOfNodes int32
discoveryBackend string
expectedDynamoNamespace string
expectedComponentType string
expectedParentDGDName string
expectedParentDGDNamespace string
}{
{
name: "namespace-scoped operator: computes correct dynamo namespace",
component: &v1alpha1.DynamoComponentDeploymentSharedSpec{
ComponentType: commonconsts.ComponentTypePlanner,
// Deprecated field set to incorrect value - should be ignored
DynamoNamespace: ptr.To("old-incorrect-value"),
},
parentGraphDeploymentName: "my-deployment",
namespace: "my-namespace",
numberOfNodes: 1,
discoveryBackend: "kubernetes",
expectedDynamoNamespace: "my-namespace-my-deployment",
expectedComponentType: commonconsts.ComponentTypePlanner,
expectedParentDGDName: "my-deployment",
expectedParentDGDNamespace: "my-namespace",
},
{
name: "deprecated dynamoNamespace field is ignored",
component: &v1alpha1.DynamoComponentDeploymentSharedSpec{
ComponentType: commonconsts.ComponentTypeFrontend,
// This is the bug case: profiler sets dynamoNamespace to just DGD name
DynamoNamespace: ptr.To("vllm-disagg"),
},
parentGraphDeploymentName: "vllm-disagg",
namespace: "djangoz",
numberOfNodes: 1,
discoveryBackend: "kubernetes",
expectedDynamoNamespace: "djangoz-vllm-disagg", // Should be k8s-namespace + DGD name
expectedComponentType: commonconsts.ComponentTypeFrontend,
expectedParentDGDName: "vllm-disagg",
expectedParentDGDNamespace: "djangoz",
},
{
name: "GlobalDynamoNamespace takes precedence",
component: &v1alpha1.DynamoComponentDeploymentSharedSpec{
ComponentType: commonconsts.ComponentTypeWorker,
GlobalDynamoNamespace: true,
// Even with deprecated field set, GlobalDynamoNamespace should win
DynamoNamespace: ptr.To("should-be-ignored"),
},
parentGraphDeploymentName: "shared-frontend",
namespace: "production",
numberOfNodes: 2,
discoveryBackend: "etcd",
expectedDynamoNamespace: commonconsts.GlobalDynamoNamespace, // "dynamo"
expectedComponentType: commonconsts.ComponentTypeWorker,
expectedParentDGDName: "shared-frontend",
expectedParentDGDNamespace: "production",
},
{
name: "nil dynamoNamespace field still computes correctly",
component: &v1alpha1.DynamoComponentDeploymentSharedSpec{
ComponentType: commonconsts.ComponentTypePlanner,
DynamoNamespace: nil, // Not set at all
},
parentGraphDeploymentName: "test-dgd",
namespace: "default",
numberOfNodes: 1,
discoveryBackend: "kubernetes",
expectedDynamoNamespace: "default-test-dgd",
expectedComponentType: commonconsts.ComponentTypePlanner,
expectedParentDGDName: "test-dgd",
expectedParentDGDNamespace: "default",
},
{
name: "different namespace and DGD name combinations",
component: &v1alpha1.DynamoComponentDeploymentSharedSpec{
ComponentType: commonconsts.ComponentTypeFrontend,
},
parentGraphDeploymentName: "llama-70b-prod",
namespace: "ml-inference",
numberOfNodes: 4,
discoveryBackend: "nats",
expectedDynamoNamespace: "ml-inference-llama-70b-prod",
expectedComponentType: commonconsts.ComponentTypeFrontend,
expectedParentDGDName: "llama-70b-prod",
expectedParentDGDNamespace: "ml-inference",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx := generateComponentContext(
tt.component,
tt.parentGraphDeploymentName,
tt.namespace,
tt.numberOfNodes,
tt.discoveryBackend,
)
assert.Equal(t, tt.expectedDynamoNamespace, ctx.DynamoNamespace,
"DynamoNamespace should be computed from k8s namespace + DGD name")
assert.Equal(t, tt.expectedComponentType, ctx.ComponentType)
assert.Equal(t, tt.expectedParentDGDName, ctx.ParentGraphDeploymentName)
assert.Equal(t, tt.expectedParentDGDNamespace, ctx.ParentGraphDeploymentNamespace)
assert.Equal(t, tt.numberOfNodes, ctx.numberOfNodes)
assert.Equal(t, tt.discoveryBackend, ctx.DiscoveryBackend)
})
}
}
func Test_updateDynDeploymentConfig(t *testing.T) { func Test_updateDynDeploymentConfig(t *testing.T) {
type args struct { type args struct {
dynamoDeploymentComponent *v1alpha1.DynamoComponentDeployment dynamoDeploymentComponent *v1alpha1.DynamoComponentDeployment
...@@ -5139,7 +5256,7 @@ func TestGenerateBasePodSpec_Worker(t *testing.T) { ...@@ -5139,7 +5256,7 @@ func TestGenerateBasePodSpec_Worker(t *testing.T) {
{Name: commonconsts.DynamoComponentEnvVar, Value: "worker"}, {Name: commonconsts.DynamoComponentEnvVar, Value: "worker"},
{Name: commonconsts.DynamoDiscoveryBackendEnvVar, Value: "kubernetes"}, {Name: commonconsts.DynamoDiscoveryBackendEnvVar, Value: "kubernetes"},
{Name: "DYN_HEALTH_CHECK_ENABLED", Value: "false"}, {Name: "DYN_HEALTH_CHECK_ENABLED", Value: "false"},
{Name: commonconsts.DynamoNamespaceEnvVar, Value: ""}, {Name: commonconsts.DynamoNamespaceEnvVar, Value: "default-test-deployment"},
{Name: "DYN_PARENT_DGD_K8S_NAME", Value: "test-deployment"}, {Name: "DYN_PARENT_DGD_K8S_NAME", Value: "test-deployment"},
{Name: "DYN_PARENT_DGD_K8S_NAMESPACE", Value: "default"}, {Name: "DYN_PARENT_DGD_K8S_NAMESPACE", Value: "default"},
{Name: "DYN_SYSTEM_ENABLED", Value: "true"}, {Name: "DYN_SYSTEM_ENABLED", Value: "true"},
......
...@@ -41,7 +41,8 @@ func NewDynamoComponentDeploymentValidator(deployment *nvidiacomv1alpha1.DynamoC ...@@ -41,7 +41,8 @@ func NewDynamoComponentDeploymentValidator(deployment *nvidiacomv1alpha1.DynamoC
// Returns warnings and error. // Returns warnings and error.
func (v *DynamoComponentDeploymentValidator) Validate() (admission.Warnings, error) { func (v *DynamoComponentDeploymentValidator) Validate() (admission.Warnings, error) {
// Validate shared spec fields using SharedSpecValidator // Validate shared spec fields using SharedSpecValidator
sharedValidator := NewSharedSpecValidator(&v.deployment.Spec.DynamoComponentDeploymentSharedSpec, "spec") calculatedNamespace := v.deployment.GetDynamoNamespace()
sharedValidator := NewSharedSpecValidator(&v.deployment.Spec.DynamoComponentDeploymentSharedSpec, "spec", calculatedNamespace)
// DCD-specific validation would go here (currently none) // DCD-specific validation would go here (currently none)
......
...@@ -226,7 +226,8 @@ func (v *DynamoGraphDeploymentValidator) validateReplicasChanges(old *nvidiacomv ...@@ -226,7 +226,8 @@ func (v *DynamoGraphDeploymentValidator) validateReplicasChanges(old *nvidiacomv
func (v *DynamoGraphDeploymentValidator) validateService(serviceName string, service *nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec) (admission.Warnings, error) { func (v *DynamoGraphDeploymentValidator) validateService(serviceName string, service *nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec) (admission.Warnings, error) {
// Use SharedSpecValidator to validate service spec (which is a DynamoComponentDeploymentSharedSpec) // Use SharedSpecValidator to validate service spec (which is a DynamoComponentDeploymentSharedSpec)
fieldPath := fmt.Sprintf("spec.services[%s]", serviceName) fieldPath := fmt.Sprintf("spec.services[%s]", serviceName)
sharedValidator := NewSharedSpecValidator(service, fieldPath) calculatedNamespace := v.deployment.GetDynamoNamespaceForService(service)
sharedValidator := NewSharedSpecValidator(service, fieldPath, calculatedNamespace)
return sharedValidator.Validate() return sharedValidator.Validate()
} }
......
...@@ -30,20 +30,36 @@ import ( ...@@ -30,20 +30,36 @@ import (
type SharedSpecValidator struct { type SharedSpecValidator struct {
spec *nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec spec *nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec
fieldPath string // e.g., "spec" for DCD, "spec.services[foo]" for DGD fieldPath string // e.g., "spec" for DCD, "spec.services[foo]" for DGD
calculatedNamespace string // The namespace that will be used: {k8s_namespace}-{dgd_name}
} }
// NewSharedSpecValidator creates a new validator for DynamoComponentDeploymentSharedSpec. // NewSharedSpecValidator creates a new validator for DynamoComponentDeploymentSharedSpec.
// fieldPath is used to provide context in error messages (e.g., "spec" or "spec.services[main]"). // fieldPath is used to provide context in error messages (e.g., "spec" or "spec.services[main]").
func NewSharedSpecValidator(spec *nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec, fieldPath string) *SharedSpecValidator { // calculatedNamespace is the namespace the operator will use:
// - If GlobalDynamoNamespace is true: "dynamo" (global constant)
// - Otherwise: {k8s_namespace}-{dgd_name}
func NewSharedSpecValidator(spec *nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec, fieldPath string, calculatedNamespace string) *SharedSpecValidator {
return &SharedSpecValidator{ return &SharedSpecValidator{
spec: spec, spec: spec,
fieldPath: fieldPath, fieldPath: fieldPath,
calculatedNamespace: calculatedNamespace,
} }
} }
// Validate performs validation on the shared spec fields. // Validate performs validation on the shared spec fields.
// Returns warnings (e.g., deprecation notices) and error if validation fails. // Returns warnings (e.g., deprecation notices) and error if validation fails.
func (v *SharedSpecValidator) Validate() (admission.Warnings, error) { func (v *SharedSpecValidator) Validate() (admission.Warnings, error) {
// Collect warnings (e.g., deprecation notices)
var warnings admission.Warnings
// Warn about deprecated dynamoNamespace field
if v.spec.DynamoNamespace != nil && *v.spec.DynamoNamespace != "" {
warnings = append(warnings, fmt.Sprintf(
"%s.dynamoNamespace is deprecated and ignored. Value '%s' will be replaced with '%s'. "+
"Remove this field from your configuration",
v.fieldPath, *v.spec.DynamoNamespace, v.calculatedNamespace))
}
// Validate replicas if specified // Validate replicas if specified
if v.spec.Replicas != nil && *v.spec.Replicas < 0 { if v.spec.Replicas != nil && *v.spec.Replicas < 0 {
return nil, fmt.Errorf("%s.replicas must be non-negative", v.fieldPath) return nil, fmt.Errorf("%s.replicas must be non-negative", v.fieldPath)
...@@ -68,9 +84,6 @@ func (v *SharedSpecValidator) Validate() (admission.Warnings, error) { ...@@ -68,9 +84,6 @@ func (v *SharedSpecValidator) Validate() (admission.Warnings, error) {
} }
} }
// Collect warnings (e.g., deprecation notices)
var warnings admission.Warnings
// Check for deprecated autoscaling field // Check for deprecated autoscaling field
//nolint:staticcheck // SA1019: Intentionally checking deprecated field to warn users //nolint:staticcheck // SA1019: Intentionally checking deprecated field to warn users
if v.spec.Autoscaling != nil { if v.spec.Autoscaling != nil {
......
...@@ -24,6 +24,11 @@ import ( ...@@ -24,6 +24,11 @@ import (
"k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/api/resource"
) )
// ptr is a helper function to create a pointer to a string
func ptr(s string) *string {
return &s
}
func TestSharedSpecValidator_Validate(t *testing.T) { func TestSharedSpecValidator_Validate(t *testing.T) {
var ( var (
negativeReplicas = int32(-1) negativeReplicas = int32(-1)
...@@ -34,6 +39,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -34,6 +39,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
name string name string
spec *nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec spec *nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec
fieldPath string fieldPath string
calculatedNamespace string
wantErr bool wantErr bool
errMsg string errMsg string
}{ }{
...@@ -61,6 +67,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -61,6 +67,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
}, },
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: false, wantErr: false,
}, },
{ {
...@@ -69,9 +76,28 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -69,9 +76,28 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
Replicas: &negativeReplicas, Replicas: &negativeReplicas,
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: true, wantErr: true,
errMsg: "spec.replicas must be non-negative", errMsg: "spec.replicas must be non-negative",
}, },
{
name: "nil dynamoNamespace is allowed",
spec: &nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec{
DynamoNamespace: nil,
},
fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: false,
},
{
name: "empty string dynamoNamespace is allowed",
spec: &nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec{
DynamoNamespace: ptr(""),
},
fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: false,
},
{ {
name: "ingress enabled without host", name: "ingress enabled without host",
spec: &nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec{ spec: &nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec{
...@@ -81,6 +107,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -81,6 +107,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
}, },
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: true, wantErr: true,
errMsg: "spec.ingress.host is required when ingress is enabled", errMsg: "spec.ingress.host is required when ingress is enabled",
}, },
...@@ -93,6 +120,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -93,6 +120,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
}, },
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: false, wantErr: false,
}, },
{ {
...@@ -107,6 +135,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -107,6 +135,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
}, },
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: true, wantErr: true,
errMsg: "spec.volumeMounts[0].mountPoint is required when useAsCompilationCache is false", errMsg: "spec.volumeMounts[0].mountPoint is required when useAsCompilationCache is false",
}, },
...@@ -121,6 +150,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -121,6 +150,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
}, },
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: false, wantErr: false,
}, },
{ {
...@@ -134,6 +164,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -134,6 +164,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
}, },
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: false, wantErr: false,
}, },
{ {
...@@ -145,6 +176,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -145,6 +176,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
}, },
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: true, wantErr: true,
errMsg: "spec.sharedMemory.size is required when disabled is false", errMsg: "spec.sharedMemory.size is required when disabled is false",
}, },
...@@ -157,6 +189,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -157,6 +189,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
}, },
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: false, wantErr: false,
}, },
{ {
...@@ -168,6 +201,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -168,6 +201,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
}, },
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantErr: false, wantErr: false,
}, },
{ {
...@@ -176,6 +210,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -176,6 +210,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
Replicas: &negativeReplicas, Replicas: &negativeReplicas,
}, },
fieldPath: "spec.services[main]", fieldPath: "spec.services[main]",
calculatedNamespace: "default-my-dgd",
wantErr: true, wantErr: true,
errMsg: "spec.services[main].replicas must be non-negative", errMsg: "spec.services[main].replicas must be non-negative",
}, },
...@@ -183,7 +218,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) { ...@@ -183,7 +218,7 @@ func TestSharedSpecValidator_Validate(t *testing.T) {
for _, tt := range tests { for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) { t.Run(tt.name, func(t *testing.T) {
validator := NewSharedSpecValidator(tt.spec, tt.fieldPath) validator := NewSharedSpecValidator(tt.spec, tt.fieldPath, tt.calculatedNamespace)
_, err := validator.Validate() _, err := validator.Validate()
if (err != nil) != tt.wantErr { if (err != nil) != tt.wantErr {
...@@ -205,7 +240,9 @@ func TestSharedSpecValidator_Validate_Warnings(t *testing.T) { ...@@ -205,7 +240,9 @@ func TestSharedSpecValidator_Validate_Warnings(t *testing.T) {
name string name string
spec *nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec spec *nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec
fieldPath string fieldPath string
calculatedNamespace string
wantWarnings int wantWarnings int
wantWarningContains string // optional substring to check in warning
}{ }{
{ {
name: "no warnings for spec without autoscaling", name: "no warnings for spec without autoscaling",
...@@ -213,6 +250,7 @@ func TestSharedSpecValidator_Validate_Warnings(t *testing.T) { ...@@ -213,6 +250,7 @@ func TestSharedSpecValidator_Validate_Warnings(t *testing.T) {
Replicas: &validReplicas, Replicas: &validReplicas,
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantWarnings: 0, wantWarnings: 0,
}, },
{ {
...@@ -227,13 +265,25 @@ func TestSharedSpecValidator_Validate_Warnings(t *testing.T) { ...@@ -227,13 +265,25 @@ func TestSharedSpecValidator_Validate_Warnings(t *testing.T) {
}, },
}, },
fieldPath: "spec", fieldPath: "spec",
calculatedNamespace: "default-my-dgd",
wantWarnings: 1, wantWarnings: 1,
}, },
{
name: "warning for deprecated dynamoNamespace field shows calculated namespace",
spec: &nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec{
Replicas: &validReplicas,
DynamoNamespace: ptr("my-custom-namespace"),
},
fieldPath: "spec.services[Frontend]",
calculatedNamespace: "hannahz-trtllm-disagg",
wantWarnings: 1,
wantWarningContains: "Value 'my-custom-namespace' will be replaced with 'hannahz-trtllm-disagg'",
},
} }
for _, tt := range tests { for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) { t.Run(tt.name, func(t *testing.T) {
validator := NewSharedSpecValidator(tt.spec, tt.fieldPath) validator := NewSharedSpecValidator(tt.spec, tt.fieldPath, tt.calculatedNamespace)
warnings, err := validator.Validate() warnings, err := validator.Validate()
if err != nil { if err != nil {
...@@ -244,6 +294,34 @@ func TestSharedSpecValidator_Validate_Warnings(t *testing.T) { ...@@ -244,6 +294,34 @@ func TestSharedSpecValidator_Validate_Warnings(t *testing.T) {
if len(warnings) != tt.wantWarnings { if len(warnings) != tt.wantWarnings {
t.Errorf("SharedSpecValidator.Validate() warnings count = %d, want %d", len(warnings), tt.wantWarnings) t.Errorf("SharedSpecValidator.Validate() warnings count = %d, want %d", len(warnings), tt.wantWarnings)
} }
if tt.wantWarningContains != "" && len(warnings) > 0 {
found := false
for _, w := range warnings {
if contains(w, tt.wantWarningContains) {
found = true
break
}
}
if !found {
t.Errorf("SharedSpecValidator.Validate() warnings = %v, want warning containing %q", warnings, tt.wantWarningContains)
}
}
}) })
} }
} }
// contains checks if s contains substr
func contains(s, substr string) bool {
return len(s) >= len(substr) && (s == substr || len(substr) == 0 ||
(len(s) > 0 && len(substr) > 0 && findSubstring(s, substr)))
}
func findSubstring(s, substr string) bool {
for i := 0; i <= len(s)-len(substr); i++ {
if s[i:i+len(substr)] == substr {
return true
}
}
return false
}
...@@ -8,7 +8,6 @@ metadata: ...@@ -8,7 +8,6 @@ metadata:
spec: spec:
services: services:
Frontend: Frontend:
dynamoNamespace: mocker-agg
componentType: frontend componentType: frontend
replicas: 1 replicas: 1
extraPodSpec: extraPodSpec:
...@@ -16,7 +15,6 @@ spec: ...@@ -16,7 +15,6 @@ spec:
image: my-registry/mocker-runtime:my-tag image: my-registry/mocker-runtime:my-tag
decode: decode:
envFromSecret: hf-token-secret envFromSecret: hf-token-secret
dynamoNamespace: mocker-agg
componentType: worker componentType: worker
subComponentType: decode subComponentType: decode
replicas: 1 replicas: 1
......
...@@ -8,7 +8,6 @@ metadata: ...@@ -8,7 +8,6 @@ metadata:
spec: spec:
services: services:
Frontend: Frontend:
dynamoNamespace: mocker-disagg
componentType: frontend componentType: frontend
replicas: 1 replicas: 1
extraPodSpec: extraPodSpec:
...@@ -16,7 +15,6 @@ spec: ...@@ -16,7 +15,6 @@ spec:
image: my-registry/mocker-runtime:my-tag image: my-registry/mocker-runtime:my-tag
prefill: prefill:
envFromSecret: hf-token-secret envFromSecret: hf-token-secret
dynamoNamespace: mocker-disagg
componentType: worker componentType: worker
subComponentType: prefill subComponentType: prefill
replicas: 1 replicas: 1
...@@ -40,7 +38,6 @@ spec: ...@@ -40,7 +38,6 @@ spec:
- --is-prefill-worker - --is-prefill-worker
decode: decode:
envFromSecret: hf-token-secret envFromSecret: hf-token-secret
dynamoNamespace: mocker-disagg
componentType: worker componentType: worker
subComponentType: decode subComponentType: decode
replicas: 1 replicas: 1
......
...@@ -8,7 +8,6 @@ metadata: ...@@ -8,7 +8,6 @@ metadata:
spec: spec:
services: services:
Frontend: Frontend:
dynamoNamespace: sglang-agg
componentType: frontend componentType: frontend
replicas: 1 replicas: 1
extraPodSpec: extraPodSpec:
...@@ -16,7 +15,6 @@ spec: ...@@ -16,7 +15,6 @@ spec:
image: my-registry/sglang-runtime:my-tag image: my-registry/sglang-runtime:my-tag
decode: decode:
envFromSecret: hf-token-secret envFromSecret: hf-token-secret
dynamoNamespace: sglang-agg
componentType: worker componentType: worker
replicas: 1 replicas: 1
resources: resources:
......
...@@ -11,7 +11,6 @@ spec: ...@@ -11,7 +11,6 @@ spec:
value: "1" value: "1"
services: services:
Frontend: Frontend:
dynamoNamespace: sglang-agg
componentType: frontend componentType: frontend
replicas: 1 replicas: 1
extraPodSpec: extraPodSpec:
...@@ -19,7 +18,6 @@ spec: ...@@ -19,7 +18,6 @@ spec:
image: my-registry/sglang-runtime:my-tag image: my-registry/sglang-runtime:my-tag
decode: decode:
envFromSecret: hf-token-secret envFromSecret: hf-token-secret
dynamoNamespace: sglang-agg
componentType: worker componentType: worker
replicas: 1 replicas: 1
resources: resources:
......
...@@ -8,7 +8,6 @@ metadata: ...@@ -8,7 +8,6 @@ metadata:
spec: spec:
services: services:
Frontend: Frontend:
dynamoNamespace: sglang-agg-router
componentType: frontend componentType: frontend
replicas: 1 replicas: 1
extraPodSpec: extraPodSpec:
...@@ -19,7 +18,6 @@ spec: ...@@ -19,7 +18,6 @@ spec:
value: kv value: kv
decode: decode:
envFromSecret: hf-token-secret envFromSecret: hf-token-secret
dynamoNamespace: sglang-agg-router
componentType: worker componentType: worker
replicas: 1 replicas: 1
resources: resources:
......
...@@ -17,7 +17,6 @@ spec: ...@@ -17,7 +17,6 @@ spec:
backendFramework: sglang backendFramework: sglang
services: services:
Frontend: Frontend:
dynamoNamespace: sglang-disagg-multinode
componentType: frontend componentType: frontend
replicas: 1 replicas: 1
extraPodSpec: extraPodSpec:
...@@ -27,7 +26,6 @@ spec: ...@@ -27,7 +26,6 @@ spec:
multinode: multinode:
nodeCount: 2 nodeCount: 2
envFromSecret: hf-token-secret envFromSecret: hf-token-secret
dynamoNamespace: sglang-disagg-multinode
componentType: worker componentType: worker
replicas: 1 replicas: 1
resources: resources:
...@@ -64,7 +62,6 @@ spec: ...@@ -64,7 +62,6 @@ spec:
multinode: multinode:
nodeCount: 2 nodeCount: 2
envFromSecret: hf-token-secret envFromSecret: hf-token-secret
dynamoNamespace: sglang-disagg-multinode
componentType: worker componentType: worker
replicas: 1 replicas: 1
resources: resources:
......
...@@ -8,7 +8,6 @@ metadata: ...@@ -8,7 +8,6 @@ metadata:
spec: spec:
services: services:
Frontend: Frontend:
dynamoNamespace: sglang-disagg
componentType: frontend componentType: frontend
replicas: 1 replicas: 1
extraPodSpec: extraPodSpec:
...@@ -16,7 +15,6 @@ spec: ...@@ -16,7 +15,6 @@ spec:
image: my-registry/sglang-runtime:my-tag image: my-registry/sglang-runtime:my-tag
decode: decode:
envFromSecret: hf-token-secret envFromSecret: hf-token-secret
dynamoNamespace: sglang-disagg
componentType: worker componentType: worker
subComponentType: decode subComponentType: decode
replicas: 1 replicas: 1
...@@ -52,7 +50,6 @@ spec: ...@@ -52,7 +50,6 @@ spec:
- "0.0.0.0" - "0.0.0.0"
prefill: prefill:
envFromSecret: hf-token-secret envFromSecret: hf-token-secret
dynamoNamespace: sglang-disagg
componentType: worker componentType: worker
subComponentType: prefill subComponentType: prefill
replicas: 1 replicas: 1
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment