Skip to content

fix(operator): stage DGD admission path migration - #11117

Merged
sttts merged 2 commits into
ai-dynamo:mainfrom
sttts:sttts-admission-graceful-path-change
Jul 1, 2026
Merged

fix(operator): stage DGD admission path migration#11117
sttts merged 2 commits into
ai-dynamo:mainfrom
sttts:sttts-admission-graceful-path-change

Conversation

@sttts

@sttts sttts commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Serve both legacy v1alpha1 and v1beta1 DGD admission endpoints.
  • Convert v1alpha1 admission objects to the v1beta1 hub before validation and defaulting.
  • Keep Helm registered on the v1alpha1 routes through 1.3, so the v1beta1 route change can safely happen in 1.4.

Validation

  • GOCACHE=/private/tmp/dynamo-go-cache go test ./internal/webhook/...
  • helm lint .
  • helm template dynamo-operator . --namespace dynamo-system --set discoveryBackend=kubernetes

Open in Devin Review

Summary by CodeRabbit

  • New Features

    • Added compatibility support for DynamoGraphDeployment admission webhooks across both the older and newer API versions.
    • Enabled defaulting and validation to work through either endpoint during the transition period.
  • Bug Fixes

    • Ensured objects continue to receive the expected default replicas and origin/version metadata.
    • Preserved validation behavior for invalid replica values when using the older API version.

Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
@sttts
sttts requested a review from a team as a code owner July 1, 2026 12:25
@copy-pr-bot

copy-pr-bot Bot commented Jul 1, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@sttts
sttts temporarily deployed to external_collaborator July 1, 2026 12:25 — with GitHub Actions Inactive
@github-actions github-actions Bot added the fix label Jul 1, 2026
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

👋 Hi sttts! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions Bot added external-contribution Pull request is from an external contributor deployment::k8s Relates to dynamo deployment in kubernetes labels Jul 1, 2026
@datadog-official

This comment has been minimized.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Adds v1alpha1 compatibility endpoints for DynamoGraphDeployment admission webhooks (defaulting and validation) alongside existing v1beta1 endpoints, introducing conversion helpers between the two API versions, dual webhook registration, updated Helm webhook configuration paths/apiVersions, and corresponding tests.

Changes

v1alpha1 webhook dual-registration

Layer / File(s) Summary
Conversion helpers
deploy/operator/internal/webhook/dynamographdeployment_conversion.go
New file adds ConvertDynamoGraphDeploymentToV1Beta1 and ConvertDynamoGraphDeploymentToV1Alpha1 functions translating DynamoGraphDeployment between v1alpha1 and v1beta1.
Defaulting webhook dual endpoint
deploy/operator/internal/webhook/defaulting/dynamographdeployment_handler.go, ...handler_test.go
Refactors DGDDefaulter.Default into defaultV1Beta1, adds dgdV1Alpha1Defaulter that converts, applies defaulting, converts back, and registers both v1alpha1 and v1beta1 mutating webhook endpoints; adds a test for the new alpha defaulter.
Validation webhook dual endpoint
deploy/operator/internal/webhook/validation/dynamographdeployment_handler.go, ...handler_test.go
Generalizes ValidateCreate/Update/Delete to accept an expectedGVK, adds dynamoGraphDeploymentV1Alpha1Handler, expands casting/metadata helpers to accept either API version, and registers both webhook routes via a shared helper; adds a test validating negative replicas via the alpha handler.
Helm webhook configuration update
deploy/helm/charts/platform/components/operator/templates/webhook-configuration.yaml
Updates ValidatingWebhookConfiguration and MutatingWebhookConfiguration entries for DynamoGraphDeployment to use v1alpha1 paths and apiVersions with explanatory comments.

Estimated code review effort: 4 (Complex) | ~55 minutes

Related Issues: None referenced.

Related PRs: None referenced.

Suggested labels: operator, webhook, api-versioning

Suggested reviewers: None specified.

Poem

A rabbit hops through webhook lanes,
alpha and beta, twin refrains,
converting shapes with careful paws,
defaulting, validating, no pause,
until 1.4, both paths remain.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description covers summary and validation, but it omits required template sections like Overview, Details, reviewer start, and related issues. Add the required sections from the template: Overview, Details, Where should reviewer start?, and Related Issues with an issue link or no-issue confirmation.
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: staging the DGD admission path migration.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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.

Inline comments:
In
`@deploy/operator/internal/webhook/validation/dynamographdeployment_handler.go`:
- Around line 236-242: Map validation errors back to v1alpha1 field names in the
legacy webhook handler. The ValidateCreate and ValidateUpdate paths in the
dynamoGraphDeploymentV1Alpha1Handler currently pass through validator errors
from the hub conversion unchanged, so legacy clients see v1beta1-style paths
such as spec.components instead of the submitted v1alpha1 paths like
spec.services. Update the v1alpha1 wrapper to translate the returned validation
error field paths/messages back to the v1alpha1 schema before returning, using
the handler methods that invoke validateCreate/validateUpdate and any existing
error-mapping helpers in this package.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: daa77a04-bc90-44f2-a200-8ab32d9d4bd5

📥 Commits

Reviewing files that changed from the base of the PR and between b4999c4 and 7d1b90c.

📒 Files selected for processing (6)
  • deploy/helm/charts/platform/components/operator/templates/webhook-configuration.yaml
  • deploy/operator/internal/webhook/defaulting/dynamographdeployment_handler.go
  • deploy/operator/internal/webhook/defaulting/dynamographdeployment_handler_test.go
  • deploy/operator/internal/webhook/dynamographdeployment_conversion.go
  • deploy/operator/internal/webhook/validation/dynamographdeployment_handler.go
  • deploy/operator/internal/webhook/validation/dynamographdeployment_handler_test.go

Signed-off-by: Dr. Stefan Schimanski <sschimanski@nvidia.com>
@sttts
sttts temporarily deployed to external_collaborator July 1, 2026 14:36 — with GitHub Actions Inactive
@sttts
sttts enabled auto-merge (squash) July 1, 2026 14:37
@sttts

sttts commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 8c1dc85

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment::k8s Relates to dynamo deployment in kubernetes external-contribution Pull request is from an external contributor fix size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants