feat: add possibility to use grove in dynamo graph helm chart #1954
Conversation
julienmancuso
requested review from
biswapanda,
hhzhang16,
hutm,
ishandhanani,
mohammedabdulwahhab and
nnshah1
as code owners
WalkthroughThe changes introduce support for deploying using Grove's PodGangSet custom resource in addition to the standard Kubernetes Deployment. Documentation is updated to describe the new option, and Helm templates are modified or added to conditionally render either Deployment or PodGangSet resources based on the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant Kubernetes API
participant Grove Controller
User->>Helm: Install chart with deploymentType = "grove"
Helm->>Kubernetes API: Apply PodGangSet manifest
Kubernetes API->>Grove Controller: Notify of new PodGangSet
Grove Controller->>Kubernetes API: Create/Manage pods as per PodGangSet spec
sequenceDiagram
participant User
participant Helm
participant Kubernetes API
User->>Helm: Install chart with deploymentType = "basic" or unset
Helm->>Kubernetes API: Apply Deployment manifest(s)
Kubernetes API->>Kubernetes API: Schedule and run pods as per Deployment spec
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
deploy/helm/README.md (3)
29-30: Spell-out Grove’s optional nature more explicitlyConsider tightening the wording so that the line reads naturally and does not imply Grove is always required:
- - Grove v0.1.0+ (optional if deploying using Grove) + - Grove v0.1.0+ (only needed when `deploymentType=grove`)
39-45: Quote the value passed to--setfor shell-safetyShells treat
=as a word-breaking operator when it appears inside double-quoted arguments preceded by--set. Wrapping the whole assignment avoids edge-cases whendeploymentTypeis later templated or set via env-vars:-helm upgrade --install dynamo-graph ./deploy/helm/chart -n dynamo-cloud -f ./examples/vllm_v1/deploy/agg.yaml --set deploymentType=grove +helm upgrade --install dynamo-graph ./deploy/helm/chart -n dynamo-cloud \ + -f ./examples/vllm_v1/deploy/agg.yaml \ + --set "deploymentType=grove"
66-66: Grammar tweak for the description columnMinor language polish to make the sentence complete:
-| `deploymentType` | Type of deployment to use. Can be `basic` or `grove`. If not specified, `basic` is used. | `deploymentType=grove` | +| `deploymentType` | Deployment strategy: `basic` (default) or `grove`. | `deploymentType=grove` |deploy/helm/chart/templates/deployment.yaml (2)
15-16: Fix misleading comment & keep terminology consistentThe in-file comment still refers to “dynamo” instead of “basic”, which can be confusing now that two deployment modes exist.
-# if deploymentType is empty, or explicitly set to dynamo, use dynamo as default +# If `.Values.deploymentType` is unset or set to `basic`, render the standard Deployment manifests
116-116: Add a trailing newlineSome linters (including
yamllint) complain when the file lacks a final newline. Adding one avoids noisy CI warnings.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deploy/helm/README.md(2 hunks)deploy/helm/chart/templates/deployment.yaml(2 hunks)deploy/helm/chart/templates/grove-podgangset.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#1474
File: deploy/cloud/operator/internal/controller/dynamocomponent_controller.go:1308-1312
Timestamp: 2025-06-11T21:29:28.650Z
Learning: User julienmancuso expects replies in English; avoid switching languages unless explicitly requested.
deploy/helm/README.md (1)
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#1365
File: deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go:171-178
Timestamp: 2025-06-04T13:09:53.416Z
Learning: The `DYN_DEPLOYMENT_CONFIG` environment variable (commonconsts.DynamoDeploymentConfigEnvVar) in the Dynamo operator will never be set via ValueFrom (secrets/config maps), only via direct Value assignment. The GetDynamoDeploymentConfig method correctly only checks env.Value for this specific environment variable.
deploy/helm/chart/templates/deployment.yaml (1)
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#1365
File: deploy/cloud/operator/api/v1alpha1/dynamocomponentdeployment_types.go:171-178
Timestamp: 2025-06-04T13:09:53.416Z
Learning: The `DYN_DEPLOYMENT_CONFIG` environment variable (commonconsts.DynamoDeploymentConfigEnvVar) in the Dynamo operator will never be set via ValueFrom (secrets/config maps), only via direct Value assignment. The GetDynamoDeploymentConfig method correctly only checks env.Value for this specific environment variable.
🪛 LanguageTool
deploy/helm/README.md
[style] ~66-~66: To form a complete sentence, be sure to include a subject.
Context: ...ymentType| Type of deployment to use. Can bebasicorgrove`. If not specified...
(MISSING_IT_THERE)
🪛 YAMLlint (1.37.1)
deploy/helm/chart/templates/deployment.yaml
[error] 16-16: syntax error: expected the node content, but found '-'
(syntax)
[error] 116-116: no new line character at the end of file
(new-line-at-end-of-file)
deploy/helm/chart/templates/grove-podgangset.yaml
[error] 15-15: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: Build and Test - vllm
🔇 Additional comments (1)
deploy/helm/chart/templates/grove-podgangset.yaml (1)
15-110: Double-check PodGangSet schema & indentationGrove’s CRD is still evolving and the current manifest assumes the following structure:
spec: replicas: 1 template: cliques: - name: … spec: roleName: … replicas: … podSpec: …Please verify against the installed CRD:
- Some versions expect
template.cliques[].podTemplate, notpodSpec.replicasat the root ofspecmay be deprecated in favour ofminReplicas/maxReplicas.- Indentation under
args:relies onnindent 14; ifargsis empty, Helm will emit an invalidargs:key with no value. A guard such as{{- if $serviceSpec.extraPodSpec.mainContainer.args }}may be safer.If any of these discrepancies exist, deployment will fail at install time.
|
@dmitry-tokarev-nv to review for OSRB implications to |
Overview:
add possibility to use grove in dynamo graph helm chart
Summary by CodeRabbit
New Features
Documentation