feat: flatten out dynamo cloud helm chart #1258
Conversation
julienmancuso
requested review from
biswapanda,
hhzhang16,
hutm,
ishandhanani,
mohammedabdulwahhab and
nnshah1
as code owners
WalkthroughThe changes update Helm charts by adding annotations to CRD manifests to keep them on uninstall, introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeployScript
participant Helm
participant Kubernetes
User->>DeployScript: Run deploy.sh [--crds]
DeployScript->>Helm: helm dep build (platform)
alt --crds flag is set
DeployScript->>Helm: helm upgrade/install crds chart
Helm->>Kubernetes: Apply CRDs (with keep policy)
end
DeployScript->>Helm: helm upgrade/install platform chart
Helm->>Kubernetes: Deploy platform (with PostgreSQL/MinIO if enabled)
Poem
🪧 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: 3
🧹 Nitpick comments (7)
deploy/cloud/helm/platform/components/operator/Chart.yaml (1)
35-35: Add newline at end of file: YAMLlint indicates a missing newline at EOF. Please add a trailing newline to conform to POSIX standards.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 35-35: no new line character at the end of file
(new-line-at-end-of-file)
deploy/cloud/helm/platform/components/operator/values.yaml (1)
121-121: Add newline at end of file: YAMLlint reports missing newline at EOF. Please add a trailing newline.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 121-121: no new line character at the end of file
(new-line-at-end-of-file)
deploy/cloud/helm/platform/components/api-store/Chart.yaml (1)
21-21: Add newline at EOF
The file is missing a trailing newline, which triggers a YAMLlint warning. Please add an empty line at the end to satisfy the linter.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 21-21: no new line character at the end of file
(new-line-at-end-of-file)
docs/guides/dynamo_deploy/dynamo_cloud.md (1)
151-160: Clarify CRD installation step and standardize capitalization
- Capitalize “Helm” for consistency: e.g., “Deploy the Helm charts…”
- Explicitly state that the
--crdsflag installs the CRDs chart before the main platform charts.
Example revision:- 3. Deploy the helm charts using the deploy script: + 3. Deploy the Helm charts (install CRDs first, then platform) using the deployment script:🧰 Tools
🪛 LanguageTool
[grammar] ~151-~151: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...`` 3. Deploy the helm charts using the deploy script:bash ./deploy.sh --crds...(PREPOSITION_VERB)
[grammar] ~157-~157: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...the deployment process, you can run the deploy script with the--interactiveflag: ...(PREPOSITION_VERB)
deploy/cloud/helm/platform/Chart.yaml (1)
49-49: Remove trailing spaces and ensure newline at EOF.
Static analysis tools and CI pre-commit hooks flag the trailing whitespace and missing newline at the end of the file. Please remove any trailing spaces on line 49 and ensure there is a newline character at the end of the file to satisfy YAML lint rules.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
[error] 49-49: trailing spaces
(trailing-spaces)
deploy/cloud/helm/deploy.sh (1)
157-162: Consider using--waitand--atomicfor CRD installs.
Adding--wait(to wait for CRDs to be established) and--atomic(to rollback on failure) to thehelm upgrade --install dynamo-crdscommand can help ensure reliability and clear failure semantics.deploy/cloud/helm/platform/values.yaml (1)
92-142: MinIO defaults are insecure and may not match Bitnami expectationsThe new
minioblock brings a full in-cluster deployment, which is great for GitOps workflows. A few concerns:
- The default
rootUser: minioadmin/rootPassword: minioadminare well-known and must be overridden for production. Consider introducing anexistingSecretfield (as with PostgreSQL) or removing these defaults altogether to force an override.- The
service.ports.api/service.ports.consolekeys deviate from Bitnami’s typicalportName/consolePortNameschema—please verify against the Bitnamivalues.yamlfor v13.3.1 to ensure compatibility.To enforce secret usage and align with the Bitnami chart, you might apply:
minio: enabled: false mode: standalone - auth: - rootUser: minioadmin - rootPassword: minioadmin + auth: + # If set, will reuse an existing Kubernetes Secret instead of generating new creds + existingSecret: "" + # rootUser/rootPassword are ignored when existingSecret is provided + rootUser: "" + rootPassword: ""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
deploy/cloud/helm/crds/Chart.yaml(1 hunks)deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml(1 hunks)deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponents.yaml(1 hunks)deploy/cloud/helm/crds/templates/nvidia.com_dynamographdeployments.yaml(1 hunks)deploy/cloud/helm/deploy.sh(4 hunks)deploy/cloud/helm/dynamo-platform-values.yaml(1 hunks)deploy/cloud/helm/platform/Chart.yaml(1 hunks)deploy/cloud/helm/platform/components/api-store/Chart.yaml(1 hunks)deploy/cloud/helm/platform/components/api-store/templates/deployment.yaml(3 hunks)deploy/cloud/helm/platform/components/api-store/values.yaml(1 hunks)deploy/cloud/helm/platform/components/operator/Chart.yaml(1 hunks)deploy/cloud/helm/platform/components/operator/values.yaml(1 hunks)deploy/cloud/helm/platform/values.yaml(1 hunks)docs/guides/dynamo_deploy/dynamo_cloud.md(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
deploy/cloud/helm/platform/components/api-store/Chart.yaml
[error] 21-21: no new line character at the end of file
(new-line-at-end-of-file)
deploy/cloud/helm/platform/components/operator/Chart.yaml
[error] 35-35: no new line character at the end of file
(new-line-at-end-of-file)
deploy/cloud/helm/platform/components/operator/values.yaml
[error] 121-121: no new line character at the end of file
(new-line-at-end-of-file)
deploy/cloud/helm/platform/Chart.yaml
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
[error] 49-49: trailing spaces
(trailing-spaces)
🪛 LanguageTool
docs/guides/dynamo_deploy/dynamo_cloud.md
[grammar] ~151-~151: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...`` 3. Deploy the helm charts using the deploy script: bash ./deploy.sh --crds ...
(PREPOSITION_VERB)
[grammar] ~157-~157: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...the deployment process, you can run the deploy script with the --interactive flag: ...
(PREPOSITION_VERB)
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/1258/merge) by julienmancuso.
deploy/cloud/helm/platform/Chart.yaml
[error] 46-46: Trailing whitespace detected and fixed by pre-commit hook 'trailing-whitespace'. File was modified to remove trailing whitespace and add newline at end of file.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (16)
deploy/cloud/helm/crds/Chart.yaml (1)
17-17: Approve description update: The chart description has been refined to clarify its purpose.deploy/cloud/helm/platform/components/operator/Chart.yaml (1)
35-35: Approve appVersion quoting: Consistent use of quotes forappVersionaligns with Helm best practices and enhances YAML parsing reliability.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 35-35: no new line character at the end of file
(new-line-at-end-of-file)
deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponents.yaml (1)
22-23: Approve Helm keep annotation: Addinghelm.sh/resource-policy: keepensures CRDs persist after Helm release deletion, which is critical for resource stability.deploy/cloud/helm/platform/components/operator/values.yaml (2)
118-119: Specify metricsService type: Explicitly settingmetricsService.type: ClusterIPclarifies service exposure and aligns with Kubernetes best practices.
120-121: Add messaging and etcd address configs: The newnatsAddrandetcdAddrvalues provide flexibility for operator connectivity. Please ensure these values are referenced in the chart templates.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 121-121: no new line character at the end of file
(new-line-at-end-of-file)
deploy/cloud/helm/dynamo-platform-values.yaml (2)
69-74: Enable PostgreSQL and MinIO by default: Defaultingpostgresql.enabledandminio.enabledtotruealigns with the updated platform dependencies.
69-74:✅ Verification successful
Verify platform Chart dependencies: Ensure
postgresqlandminioare declared indeploy/cloud/helm/platform/Chart.yaml. You can run:
🏁 Script executed:
#!/bin/bash rg -n "postgresql" deploy/cloud/helm/platform/Chart.yaml rg -n "minio" deploy/cloud/helm/platform/Chart.yamlLength of output: 220
Chart dependencies verified
Bothpostgresqlandminioare declared indeploy/cloud/helm/platform/Chart.yaml(lines 41–48). No further changes needed.deploy/cloud/helm/platform/components/api-store/Chart.yaml (1)
21-21: Empty dependency list is intentional
Settingdependencies: []correctly removes subchart dependencies here, delegating PostgreSQL and MinIO to the parent chart. Ensure that the parentplatform/Chart.yamldeclares these dependencies with the appropriate conditional flags, and that subchart values reference the new flattened structure.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 21-21: no new line character at the end of file
(new-line-at-end-of-file)
deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml (1)
22-23: Approve Helm resource-policy annotation
Addinghelm.sh/resource-policy: keepensures thedynamocomponentdeploymentsCRD is not removed onhelm delete, which aligns with best practices for managing cluster-scoped resources.deploy/cloud/helm/crds/templates/nvidia.com_dynamographdeployments.yaml (1)
21-23: Ensure CRDs persist on uninstall.
The addition ofhelm.sh/resource-policy: keepannotation will prevent Helm from deleting this CRD on release deletion, which is essential for resources managed by other releases or controllers.deploy/cloud/helm/platform/Chart.yaml (1)
41-48: Add PostgreSQL and MinIO as conditional dependencies.
Introducing Bitnami Helm charts for PostgreSQL (v16.6.2) and MinIO (v16.0.2) with conditional flags aligns well with centralizing service management at the platform level. Ensure corresponding values blocks invalues.yamlenable and configure these dependencies properly.🧰 Tools
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/1258/merge) by julienmancuso.
[error] 46-46: Trailing whitespace detected and fixed by pre-commit hook 'trailing-whitespace'. File was modified to remove trailing whitespace and add newline at end of file.
deploy/cloud/helm/deploy.sh (2)
44-56: Properly initialize and parse the--crdsflag.
The newINSTALL_CRDSboolean and the associated--crdscase branch correctly allow conditional CRD installation. This pattern is consistent with other flags.
142-142: Log the CRDs flag state for troubleshooting.
PrintingINSTALL_CRDSalongside other parameters helps debug whether CRD installation will be invoked.deploy/cloud/helm/platform/components/api-store/templates/deployment.yaml (3)
41-41: Approve dynamic PostgreSQL readiness check.
Switching to.Values.postgresql.hostand.Values.postgresql.portwith sensible defaults ensures the initContainer waits correctly for the live database endpoint.
63-70: Parameterize PostgreSQL environment variables.
Referencing.Values.postgresql.host,.user,.database, and.portvalues (with defaults) in the DB env block makes the chart more flexible for different environments.
72-76: Use a configurable secret name for DB password.
The template now references.Values.postgresql.passwordSecret, which maps to the secret storing the database password. This correctly externalizes sensitive data.
deploy/cloud/helm/platform/components/api-store/templates/deployment.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deploy/cloud/helm/platform/components/api-store/values.yaml (1)
102-106:⚠️ Potential issueExpose
accessKeySecretfor MinIO credentials
The currentminioblock only lets users override the password secret. You should also allow overriding the access key via a dedicatedaccessKeySecretto fully externalize both root‐user credentials.Suggested patch:
minio: host: "" port: 9000 - # allow to override the secret name containing the minio username and password - passwordSecret: "" + # allow to override the secret name containing the MinIO root user's access key + accessKeySecret: "" + # allow to override the secret name containing the MinIO root user's secret key + passwordSecret: ""
🧹 Nitpick comments (2)
docs/guides/dynamo_deploy/dynamo_cloud.md (1)
151-151: Clarify CRDs chart location
Consider adding a reference to the dedicated CRDs chart directory (deploy/cloud/helm/crds/) so users know exactly where the CRDs manifests reside.+ # (Optional) You can also install CRDs directly: + # helm upgrade --install dynamo-crds deploy/cloud/helm/crds \ + # --namespace $NAMESPACE --create-namespacedeploy/cloud/helm/platform/components/api-store/values.yaml (1)
94-100: Consider adding optional Postgres username override
The simplifiedpostgresqlblock currently only lets users override the password. To support custom users (instead of relying on a hardcoded default), you may want to expose both ausernameand ausernameSecretfield.Proposed diff:
postgresql: host: "" port: 5432 + # optional: override the Postgres user + username: "" + # optional: override the secret name containing the Postgres username + usernameSecret: "" # allow to override the secret name containing the postgres password passwordSecret: "" database: "dynamo"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
deploy/cloud/helm/deploy.sh(4 hunks)deploy/cloud/helm/platform/Chart.yaml(1 hunks)deploy/cloud/helm/platform/components/api-store/templates/deployment.yaml(3 hunks)deploy/cloud/helm/platform/components/api-store/values.yaml(1 hunks)deploy/cloud/helm/platform/components/operator/Chart.yaml(0 hunks)deploy/cloud/helm/platform/components/operator/values.yaml(1 hunks)deploy/cloud/helm/platform/values.yaml(1 hunks)docs/guides/dynamo_deploy/dynamo_cloud.md(1 hunks)
💤 Files with no reviewable changes (1)
- deploy/cloud/helm/platform/components/operator/Chart.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- deploy/cloud/helm/platform/components/operator/values.yaml
- deploy/cloud/helm/platform/values.yaml
- deploy/cloud/helm/platform/components/api-store/templates/deployment.yaml
- deploy/cloud/helm/platform/Chart.yaml
- deploy/cloud/helm/deploy.sh
🧰 Additional context used
🪛 LanguageTool
docs/guides/dynamo_deploy/dynamo_cloud.md
[grammar] ~157-~157: The word ‘deploy’ is a verb. Did you mean the noun “deployment” (= release, placement)?
Context: ...the deployment process, you can run the deploy script with the --interactive flag: ...
(PREPOSITION_VERB)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (3)
docs/guides/dynamo_deploy/dynamo_cloud.md (3)
154-154: Approve explicit--crdsflag usage
Nicely updated the example to include--crds, ensuring CRDs are installed or upgraded before the platform charts.
160-160: Approve interactive flag with CRDs
Good addition of--crds --interactiveto guide users through CRD installation in interactive mode.
163-163: Approve CRDs skip note
The note about skipping CRD installation when omitting--crdsis clear and highlights the cluster-scoped nature of CRDs.
Overview:
flatten out dynamo cloud helm chart
Details:
flatten out dynamo cloud helm chart and make CRD a distinct helm chart to make it easier to use the charts in fluxcd/argocd
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
--crdsoption in the deployment script to manage Custom Resource Definitions (CRDs) separately.natsAddrandetcdAddr).Improvements
Chores