feat(operator): per-component mainContainerName override for DGD components#11421
feat(operator): per-component mainContainerName override for DGD components#11421HanFa wants to merge 1 commit into
Conversation
HanFa
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
HanFa
had a problem deploying
to
external_collaborator
GitHub Actions
Failure
github-actions
Bot
added
feat
deployment::k8s
This comment has been minimized.
This comment has been minimized.
HanFa
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
HanFa
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
github-actions
Bot
added
the
documentation
Local e2e verification (kind, operator built from this branch)Ran a full local e2e on a kind cluster with the operator image built from this branch (helm chart install, CRDs server-side applied). Evidence below; happy to add any of this as an automated e2e spec if maintainers want it.
Observation (pre-existing, not introduced here): the standalone-DCD validating webhook registers Setup notes for anyone reproducing locally: CRDs need 🤖 Verified with Claude Code |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
WalkthroughThis PR introduces a configurable ChangesMain container name override
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
lib/runtime/src/discovery/kube/utils.rs (1)
26-31: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider caching the env var lookup.
main_container_name()re-reads and re-parses the environment variable on everycr_name()call. SinceDYN_MAIN_CONTAINER_NAMEis set once at process start by the operator, caching the resolved value (e.g., withstd::sync::OnceLock) would avoid repeated syscalls on what could be a hot discovery path.♻️ Suggested refactor
+use std::sync::OnceLock; + +static MAIN_CONTAINER_NAME: OnceLock<String> = OnceLock::new(); + fn main_container_name() -> String { - match std::env::var(DYN_MAIN_CONTAINER_NAME) { - Ok(name) if !name.trim().is_empty() => name, - _ => DEFAULT_MAIN_CONTAINER_NAME.to_string(), - } + MAIN_CONTAINER_NAME + .get_or_init(|| match std::env::var(DYN_MAIN_CONTAINER_NAME) { + Ok(name) if !name.trim().is_empty() => name, + _ => DEFAULT_MAIN_CONTAINER_NAME.to_string(), + }) + .clone() }Note: this would need to be reconciled with tests that mutate the env var at runtime (
temp_env::with_var), since a cached value would no longer reflect changes after the first call.Also applies to: 68-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/runtime/src/discovery/kube/utils.rs` around lines 26 - 31, `main_container_name()` currently re-reads `DYN_MAIN_CONTAINER_NAME` on every `cr_name()` call, which is wasteful on a hot path. Cache the resolved container name once using `std::sync::OnceLock` (or equivalent) inside `main_container_name()` and have `cr_name()` reuse that cached value. Be sure to account for the `temp_env::with_var` tests by either adjusting those tests to avoid runtime env mutation or by isolating the cached lookup so tests can still validate both default and overridden behavior.components/src/dynamo/planner/monitoring/dgd_services.py (1)
51-58: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWhitespace handling differs from the Rust counterpart.
_resolve_main_container_nameonly guards against falsy values (None/""), but the Rustmain_container_name()also trims whitespace before treating the value as empty. A whitespace-onlymainContainerName(e.g." ") would be treated as a valid custom name here, but would fall back to"main"in the Rust discovery layer — causing the planner to look for a different container than the Rust runtime treats as "main".🔧 Suggested fix
def _resolve_main_container_name(component: dict) -> str: """Return the component's main container name. v1beta1 components may rename the main container via ``mainContainerName``; fall back to the default ``main`` when the field is unset or empty. """ - return component.get("mainContainerName") or MAIN_CONTAINER_NAME + name = component.get("mainContainerName") + return name.strip() if name and name.strip() else MAIN_CONTAINER_NAME🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/planner/monitoring/dgd_services.py` around lines 51 - 58, Update _resolve_main_container_name in dgd_services.py so it matches the Rust main_container_name() behavior by trimming whitespace before deciding whether mainContainerName is usable. Right now the function only falls back on None or empty strings, so whitespace-only values are treated as valid; normalize the component["mainContainerName"] value with stripping and return MAIN_CONTAINER_NAME when the trimmed result is empty. Keep the change localized to _resolve_main_container_name and preserve the existing fallback behavior for unset or blank names.deploy/operator/api/v1alpha1/shared_spec_conversion.go (1)
1736-1762: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate reorder-with-bailout logic.
reorderVolumeMountsToPreserved(1788-1810) reimplements the same "remove-and-append, bail out on any mismatch" algorithm already used inrestoreSharedPodTemplateContainerOrder(1736-1762), differing only in the match key (NamevsName+MountPath). Consider extracting a small generic helper parameterized by a key-extraction function to avoid maintaining two copies of this matching logic.♻️ Example generic helper
func reorderToPreserved[T any, K comparable](current, preserved []T, key func(T) K) []T { if len(current) != len(preserved) { return current } remaining := slices.Clone(current) out := make([]T, 0, len(current)) for _, p := range preserved { matched := false for i := range remaining { if key(remaining[i]) != key(p) { continue } out = append(out, remaining[i]) remaining = slices.Delete(remaining, i, i+1) matched = true break } if !matched { return current } } return out }Container-order and volume-mount-order call sites then supply their own key function (e.g., composite
Name+MountPathkey for mounts).Also applies to: 1764-1811
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/operator/api/v1alpha1/shared_spec_conversion.go` around lines 1736 - 1762, The container-order restore logic in restoreSharedPodTemplateContainerOrder is duplicated by reorderVolumeMountsToPreserved with only the matching key changed, so extract the shared remove-and-append, bail-out-on-mismatch algorithm into a small generic helper. Make the helper parameterized by a key-extraction function, then have restoreSharedPodTemplateContainerOrder and reorderVolumeMountsToPreserved call it with their respective keys (Name for containers, composite Name+MountPath for mounts) to keep the behavior identical while removing the duplicate matching logic.deploy/operator/internal/dynamo/component_common.go (1)
159-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist repeated
context.mainContainerName()call.Called twice back-to-back (Lines 162, 169) for the same value; hoisting into a local avoids the redundant method call and slightly improves readability.
♻️ Proposed refactor
if context.Discovery.Mode == configv1alpha1.KubeDiscoveryModeContainer { + mainContainerName := context.mainContainerName() container.Env = append(container.Env, corev1.EnvVar{ Name: "CONTAINER_NAME", - Value: context.mainContainerName(), + Value: mainContainerName, }) // Tell the runtime which container name carries main-container // (pod-level) identity, so a renamed main container keeps the same // discovery identity as the default "main". container.Env = append(container.Env, corev1.EnvVar{ Name: commonconsts.DynamoMainContainerEnvVar, - Value: context.mainContainerName(), + Value: mainContainerName, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/operator/internal/dynamo/component_common.go` around lines 159 - 175, Hoist the repeated context.mainContainerName() call in the KubeDiscoveryModeContainer branch of the container env setup to a local variable, then reuse it for both the CONTAINER_NAME and commonconsts.DynamoMainContainerEnvVar env vars. Keep the logic in the same block inside the function that builds the container env so the value is computed once and the intent is clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@components/src/dynamo/planner/monitoring/dgd_services.py`:
- Around line 51-58: Update _resolve_main_container_name in dgd_services.py so
it matches the Rust main_container_name() behavior by trimming whitespace before
deciding whether mainContainerName is usable. Right now the function only falls
back on None or empty strings, so whitespace-only values are treated as valid;
normalize the component["mainContainerName"] value with stripping and return
MAIN_CONTAINER_NAME when the trimmed result is empty. Keep the change localized
to _resolve_main_container_name and preserve the existing fallback behavior for
unset or blank names.
In `@deploy/operator/api/v1alpha1/shared_spec_conversion.go`:
- Around line 1736-1762: The container-order restore logic in
restoreSharedPodTemplateContainerOrder is duplicated by
reorderVolumeMountsToPreserved with only the matching key changed, so extract
the shared remove-and-append, bail-out-on-mismatch algorithm into a small
generic helper. Make the helper parameterized by a key-extraction function, then
have restoreSharedPodTemplateContainerOrder and reorderVolumeMountsToPreserved
call it with their respective keys (Name for containers, composite
Name+MountPath for mounts) to keep the behavior identical while removing the
duplicate matching logic.
In `@deploy/operator/internal/dynamo/component_common.go`:
- Around line 159-175: Hoist the repeated context.mainContainerName() call in
the KubeDiscoveryModeContainer branch of the container env setup to a local
variable, then reuse it for both the CONTAINER_NAME and
commonconsts.DynamoMainContainerEnvVar env vars. Keep the logic in the same
block inside the function that builds the container env so the value is computed
once and the intent is clearer.
In `@lib/runtime/src/discovery/kube/utils.rs`:
- Around line 26-31: `main_container_name()` currently re-reads
`DYN_MAIN_CONTAINER_NAME` on every `cr_name()` call, which is wasteful on a hot
path. Cache the resolved container name once using `std::sync::OnceLock` (or
equivalent) inside `main_container_name()` and have `cr_name()` reuse that
cached value. Be sure to account for the `temp_env::with_var` tests by either
adjusting those tests to avoid runtime env mutation or by isolating the cached
lookup so tests can still validate both default and overridden behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2c53c531-9f99-4a48-acea-5f3ba31ed005
📒 Files selected for processing (28)
components/src/dynamo/planner/monitoring/dgd_services.pycomponents/src/dynamo/planner/tests/unit/test_kubernetes_connector.pydeploy/helm/charts/platform/components/operator/crds/nvidia.com_dynamocomponentdeployments.yamldeploy/helm/charts/platform/components/operator/crds/nvidia.com_dynamographdeployments.yamldeploy/operator/api/v1alpha1/conversion_field_coverage_test.godeploy/operator/api/v1alpha1/dynamocomponentdeployment_conversion_test.godeploy/operator/api/v1alpha1/shared_spec_conversion.godeploy/operator/api/v1alpha1/shared_spec_conversion_bugs_test.godeploy/operator/api/v1beta1/dynamocomponentdeployment_types.godeploy/operator/config/crd/bases/nvidia.com_dynamocomponentdeployments.yamldeploy/operator/config/crd/bases/nvidia.com_dynamographdeployments.yamldeploy/operator/internal/checkpoint/gms_pod_validation.godeploy/operator/internal/checkpoint/podspec.godeploy/operator/internal/checkpoint/resource.godeploy/operator/internal/consts/consts.godeploy/operator/internal/controller/checkpoint_job.godeploy/operator/internal/controller/dynamocomponentdeployment_controller.godeploy/operator/internal/controller/dynamocomponentdeployment_controller_test.godeploy/operator/internal/controller/dynamographdeployment_controller.godeploy/operator/internal/controller/dynamographdeployment_controller_test.godeploy/operator/internal/dynamo/component_common.godeploy/operator/internal/dynamo/graph.godeploy/operator/internal/dynamo/graph_main_container_name_test.godeploy/operator/internal/dynamo/v1beta1_helpers.godeploy/operator/internal/webhook/validation/shared_test.godeploy/operator/internal/webhook/validation/shared_v1beta1.godocs/kubernetes/api-reference.mdlib/runtime/src/discovery/kube/utils.rs
HanFa
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
julienmancuso
left a comment
julienmancuso
left a comment
There was a problem hiding this comment.
I found two P1 correctness issues and three P2 completeness/backward-compatibility issues in the current mainContainerName design. The inline comments below focus on cases not covered by the current tests: mixed-name discovery, generated frontend sidecars, snapshot restore, intra-pod failover, and updates of existing resources.
hhzhang16
left a comment
hhzhang16
left a comment
There was a problem hiding this comment.
mainContainerName is documented and exposed on standalone DCDs, but the standalone DCD validating webhook still appears to be v1alpha1-only, so the new v1beta1 shared-spec checks do not run for direct v1beta1 DCD creates/updates. That means a direct DCD can accept combinations the DGD path rejects, such as mainContainerName: engine plus a literal main container.
Can we either make the v1beta1 DCD path enforce the same validation, or narrow the PR contract/docs to say this is only validated on the DGD path and track DCD parity separately? My default would be to enforce it here if standalone DCD is intended to support this field.
HanFa
force-pushed
the
fangh/dgd-main-container-name
branch
from
8e4d033 to
3cc4f48
Compare
HanFa
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
HanFa
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
|
Thanks for the thorough reviews — all findings are addressed. I force-pushed with the history reorganized into one feature-base commit (
Also in this push (hardening found while addressing the above):
Re-ran the local kind e2e on the pushed head: a custom-name DCD renders with the renamed main at index 0 and the discovery annotation; ambiguity, |
|
Why The operator's rule: self.name == "main" || (has(self.image) && self.image != "")
message: sidecar containers must specify a non-empty imageWith Adapting it isn't practical at the schema level:
So the rule is removed rather than rewritten. Consequences:
If earlier feedback is preferred, the same check can move into the validation webhook (which has full-spec context) — happy to add that here or as a follow-up. |
|
@hhzhang16 Good catch — enforced in The DCD webhook now mirrors the DGD dual-version pattern:
Verified on a kind cluster with the operator built from this branch: your exact example — Two behavior notes from the parity: shared-spec violations on standalone DCDs now report structural Edit (post-rebase): while this PR was in review, #11479 (@sttts) landed this same dual-version DCD validation architecture on |
HanFa
force-pushed
the
fangh/dgd-main-container-name
branch
from
6c81904 to
209b815
Compare
HanFa
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
|
Rebased onto current Notable interaction with upstream: #11479 (@sttts) landed the dual-version structural DCD validation on Commit links in the earlier comments have been updated to the rebased SHAs. Post-rebase verification: build + |
HanFa
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
julienmancuso
left a comment
julienmancuso
left a comment
There was a problem hiding this comment.
Re-reviewed the updated head. The original five findings are materially addressed; four new findings remain on this revision: two P1 correctness/backward-compatibility issues and two P2 validation/API-contract issues. Details are attached inline.
| // reassertOperatorIdentityEnv restores operator-owned identity env vars, | ||
| // overwriting any user-supplied values of the same name. | ||
| func reassertOperatorIdentityEnv(container *corev1.Container, identity []corev1.EnvVar) { | ||
| for _, id := range identity { |
There was a problem hiding this comment.
[P1] Clear reserved identity variables even when the operator's expected value is absence. For a default-named component, the base intentionally omits DYN_MAIN_CONTAINER_NAME, so operatorIdentityEnv does not capture it and this loop never removes a user-supplied value added by the podTemplate merge. The local runtime can then treat another container as main while the unstamped pod tells remote watchers to fall back to main; a default-name frontend sidecar can similarly claim pod identity. Filter all reserved identity keys from the merged env first, then append only the operator's expected values. Please add default-name cases for both the main and frontend-sidecar paths.
| // +kubebuilder:validation:XValidation:rule="!has(self.eppConfig) || (has(self.type) && self.type == 'epp')",message="eppConfig may only be set when type is epp" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.minAvailable) || (has(self.replicas) && self.replicas == 0) || self.minAvailable <= (has(self.replicas) ? self.replicas : 1)",message="minAvailable must be less than or equal to replicas unless replicas is 0" | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.minAvailable) || (has(self.minAvailable) && self.minAvailable == oldSelf.minAvailable)",message="minAvailable is immutable after creation" | ||
| // +kubebuilder:validation:XValidation:rule="has(oldSelf.mainContainerName) == has(self.mainContainerName) && (!has(self.mainContainerName) || self.mainContainerName == oldSelf.mainContainerName)",message="mainContainerName is immutable after creation" |
There was a problem hiding this comment.
[P1] Enforce the effective-name immutability invariant in the shared webhook update validator as well as CEL. This transition rule only applies to the v1beta1 schema. A served v1alpha1 full replacement can drop the sparse hub-preservation annotation; old conversion still produces the custom name, new conversion falls back to main, and validateDynamoComponentDeploymentSharedSpecUpdate currently does not compare the names, so the live semantic rename is admitted. Compare old.GetMainContainerName() with new.GetMainContainerName() in the version-agnostic update path and cover v1alpha1 DCD and DGD updates that lose the preservation annotation.
| # intentionally NOT generated: with spec.mainContainerName the semantic main | ||
| # container is no longer identifiable at container-item scope, and a | ||
| # spec-level comprehension over the unbounded containers list would exceed | ||
| # the CEL cost budget. Imageless sidecars are rejected by the workload API |
There was a problem hiding this comment.
[P2] Moving this failure from admission to reconciliation is a validation regression: the CR is accepted even though it can never produce a valid workload, leaving the controller to fail repeatedly. Removing the hard-coded item CEL is reasonable, but the check should move into the shared v1beta1 webhook, which can iterate podTemplate.spec.containers, skip spec.GetMainContainerName(), and require a non-empty image for every sidecar. The DCD/DGD admission-chain tests should continue asserting rejection.
| // `podTemplate` must not also contain a container literally named | ||
| // `"main"` (which would be ambiguous). | ||
| // | ||
| // The field is immutable after creation (including adding or removing |
There was a problem hiding this comment.
[P2] Immutability should compare the effective name, not field presence. Omitted and explicit main render identically, but the current CEL rejects both omitted → main and main → omitted. Please make the rule compare (has(field) ? field : 'main') on old and new values (and use the same semantic comparison in webhook validation). Otherwise this behaves like a presence-sensitive override while being named and documented as the effective main-container name.
| // otherwise. Rendering, validation, and any consumer that needs to identify | ||
| // the main container must use this resolver instead of the MainContainerName | ||
| // constant so per-component overrides are honored. | ||
| func (s *DynamoComponentDeploymentSharedSpec) GetMainContainerName() string { |
There was a problem hiding this comment.
return ptr.Deref(s.MainContainerName, MainContainerName)
| }) | ||
| } | ||
|
|
||
| func TestValidateDynamoComponentDeploymentSharedSpecMainContainerName(t *testing.T) { |
There was a problem hiding this comment.
no new tests please. Put it into the main test which is universal (likely you have to rebase as the refactor for that just landed).
| // mainContainerName overrides the name of this component's main container | ||
| // (the container the operator injects its defaults into). When omitted, | ||
| // the well-known default `"main"` is used, preserving the existing | ||
| // contract. Setting it lets integrations align the primary container name |
There was a problem hiding this comment.
what is "existing contract"? Don't talk about implementation changes in API docs. These must use "absolute" terms to describe the semantics. There is not old, new, existing.
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=63 | ||
| // +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$` | ||
| MainContainerName string `json:"mainContainerName,omitempty"` |
There was a problem hiding this comment.
I still think it is misnamed. If it is not defaulted, than this is mainContainerNameOverride.
HanFa
force-pushed
the
fangh/dgd-main-container-name
branch
from
10854ce to
d27fb9b
Compare
HanFa
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
HanFa
force-pushed
the
fangh/dgd-main-container-name
branch
from
d27fb9b to
066d5be
Compare
HanFa
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
…omponents Add an optional, backward-compatible `spec.mainContainerNameOverride` field (default "main") to DynamoComponentDeploymentSharedSpec, letting a DGD/DCD component rename its primary workload container while keeping every main-container semantic: default injection, first position in `containers`, and runtime discovery identity. When the field is omitted, rendered objects are byte-identical to before. The field is presence- sensitive and immutable after creation (add, remove, or change all rejected) -- an override, per its name. Rendering, LWS checkMainContainer, checkpoint target defaulting, the sidecar split, and DGD-level env defaults all resolve the per-component name via GetMainContainerName(). In container-scoped kubernetes discovery mode the operator injects DYN_MAIN_CONTAINER_NAME (only for non-default names) and stamps the pod annotation nvidia.com/dynamo-main-container-name (including the Grove path); the Rust runtime resolves each remote pod's main container from that pod's own annotation, so mixed-name fleets stay mutually discoverable. The SLA planner resolves the name from the component spec. The override is preserved across v1alpha1 conversion via the sparse hub payload. Review hardening folded in: - operator-owned identity envs (CONTAINER_NAME, DYN_MAIN_CONTAINER_NAME, DYN_KUBE_DISCOVERY_MODE) are the operator's sole authority: a user podTemplate value is replaced, and a reserved key the operator did not set (e.g. the main-name env on a default-named component) is stripped - the frontend sidecar gets its own container identity; snapshot restore refreshes DYN_MAIN_CONTAINER_NAME; failover engines strip it and name collisions are rejected - immutability is enforced both by a v1beta1 CEL transition rule and by the version-agnostic webhook update path, so a v1alpha1 replacement that drops the preservation annotation cannot silently rename the main container - imageless sidecars are rejected at admission by the shared webhook (resolving the effective main and frontend-sidecar names), not deferred to reconcile - standalone v1beta1 DCDs run the shared-spec validation via the dual-version webhook - conversion keys the semantic main on the effective name; also fixes a pre-existing volume-mount-order round-trip conversion bug Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Fang Han <fangh@nvidia.com>
HanFa
force-pushed
the
fangh/dgd-main-container-name
branch
from
066d5be to
7d43d0f
Compare
HanFa
temporarily deployed
to
external_collaborator
GitHub Actions
Inactive
4 participants
Overview:
Adds an optional, backward-compatible
spec.mainContainerNamefield (default"main") toDynamoComponentDeploymentSharedSpec, letting a DGD/DCD component rename its primary workload container while keeping every main-container semantic (default injection, first position incontainers, runtime discovery identity).Motivation: platform integrations often have fleet-wide conventions keyed on the primary container name — log routing/filtering, terminal attach, per-container CPU/memory metrics. Today
mainis a hard operator/runtime contract: submitting a differently-named container demotes it to a sidecar and auto-generates a separatemain, so integrators must special-case Dynamo pods across their observability stack. An optional per-component override removes those integration workarounds without breaking any existing deployment: when the field is omitted, rendered objects are byte-identical to before (one scoped exception listed under Behavior changes below).Details:
mainContainerNameonDynamoComponentDeploymentSharedSpecwith DNS-1123 label validation, plus aGetMainContainerName()resolver; the field is immutable after creation (CEL transition rule). Field docs codify the ambiguity rules and discovery behavior.checkMainContainer, and checkpoint target-container defaults all resolve the per-component name. The renamed main container keeps index 0, so positionalContainers[0]consumers (failover, DRA, GMS sidecar injection, backends) are unaffected. Operator-owned identity envs are re-asserted after user env merges.DYN_MAIN_CONTAINER_NAMEalongsideCONTAINER_NAME(only when the name differs from"main", keeping default renders byte-identical), and stampsnvidia.com/dynamo-main-container-nameon rendered pods (including the Grove path). The Rust runtime resolves each remote pod's main container from that pod's own annotation, so mixed-name fleets and rolling renames stay mutually visible; a watcher's own env only determines its own identity.DYN_MAIN_CONTAINER_NAMEparticipates in checkpoint/restore env refresh.get_main_containerresolves the name from the component spec instead of the literal"main".mainContainerNamecombined with a podTemplate container literally named"main"(ambiguous — it would silently become a sidecar); rejectsfrontendSidecarequal to the main-container name whenmainContainerNameis explicitly set; rejects amainContainerNamethat collides with generated intra-pod-failover engine names. Standalone DCDs are validated at v1beta1 via the dual-version webhook architecture that landed upstream in refactor(validation): make DCD validation structural #11479; this PR migrates the helm webhook entry to the v1beta1 path and adds admission-chain coverage.api/CONVERSION.md, and conversion keys the semantic main container on the effective name. The field-coverage tripwire and round-trip fuzz tests pass. This work also surfaced and fixes a pre-existing round-trip bug: the hub main container's volume-mount order was lost when a compilation-cache mount was flattened (extracted repro fails on HEAD without the fix — seeTestBugDCD_HubMainContainerVolumeMountOrderRoundTrips). Happy to split that fix into its own PR if preferred.make manifests.Behavior changes (review hardening — full commit-per-finding map in the comments)
CONTAINER_NAME=main; it now gets its own container name (and, for custom-named components, the parent'sDYN_MAIN_CONTAINER_NAME). This changes rendered output for existingfrontendSidecarusers in container discovery mode — the old value was incorrect (it made the sidecar classify as the pod-identity holder).self.name == "main" || has(self.image)rule oncontainershard-coded the main name and cannot be expressed correctly at item scope; imageless sidecars now fail at reconcile time instead of admission. Rationale comment on the PR.frontendSidecar: main: still admitted for existing specs (nomainContainerName); rejected only whenmainContainerNameis explicitly set.mainContainerNameis immutable after creation (CEL transition rule; add/change/remove all rejected).mainContainerNamechecks on standalone DCDs. Upstream deliberately skips the EPP InferencePool availability check for standalone DCDs; that decision is kept.Validation
go build ./...,go vet ./..., gofmt clean; golangci-lint (CI-pinned v1.64.8) cleango test ./api/... ./internal/...all green, including the envtest controller suite and conversion round-trip fuzz (10k iterations, plus re-runs with fixed seeds after the conversion fixes)cargo test -p dynamo-runtime --lib discovery::kube::utils→ 9 passed (mixed-name remote-pod cases included)frontendSidecarcollision, and both immutability directions rejected at admission; v1alpha1 legacy webhook path admits with deprecation warning; previously-admitted ambiguous standalone v1beta1 DCD now rejected (failover-name collision is covered by webhook table tests)Where should the reviewer start?
deploy/operator/api/v1beta1/dynamocomponentdeployment_types.go— the field contract, resolver, and immutability ruledeploy/operator/internal/dynamo/graph.go+v1beta1_helpers.go— render-path threading and annotation stampingdeploy/operator/internal/webhook/validation/— shared v1beta1 checks and the DCD dual-version handlerdeploy/operator/api/v1alpha1/shared_spec_conversion.go— sparse-payload preservation + semantic-main threadinglib/runtime/src/discovery/kube/utils.rs— per-pod remote identity resolutionRelated Issues
🚫 This PR is NOT linked to an issue:
🤖 Implemented with Claude Code
Summary by CodeRabbit
New Features
"main".Bug Fixes
Tests