-
Notifications
You must be signed in to change notification settings - Fork 738
fix: Hard code image in dynamo-platform #4386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nancy Agarwal <[email protected]>
f14ea6e to
8aad191
Compare
WalkthroughThis pull request replaces hard-coded container image references in a Helm template with configurable Helm values and adds imagePullPolicy fields. Corresponding image configuration blocks are added to the Helm values file for SSH key generation and kubectl containers in the MPI SSH key generation job. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
deploy/cloud/helm/platform/components/operator/templates/mpi-run-ssh-keygen-job.yaml (1)
51-52: Consider pinning the keygenImage tag for consistency and production safety.The alpine/k8s image is documented as supporting latest minor versions of kubectl, so pinning
keygenImage.tagto a specific version (e.g.,2.4.0or similar) would align with the version-pinnedkubectlImage.tag: 1.34.1and reduce the risk of unexpected image mutations in production. Thelatesttag may pull different implementations across deployments.Apply this diff to the values file to pin the keygen image version:
- keygenImage: - repository: bitnamisecure/git - tag: latest - pullPolicy: IfNotPresent + keygenImage: + repository: bitnamisecure/git + tag: "2.4.0" # Pin to a specific version for reproducibility + pullPolicy: IfNotPresent(You may need to verify the correct pinned version available on bitnamisecure/git.)
deploy/cloud/helm/platform/components/operator/values.yaml (1)
125-129: Image choice is reasonable, but consider pinning the version.The
bitnamisecure/gitimage is a valid choice for SSH key generation, as it includes the requiredssh-keygenbinary. However, usingtag: latestcreates a reproducibility and stability risk; different deployments could pull different image versions if the upstream image is updated.For consistency with
kubectlImage.tag: 1.34.1below, pin the keygen image to a specific version:- keygenImage: - repository: bitnamisecure/git - tag: latest - pullPolicy: IfNotPresent + keygenImage: + repository: bitnamisecure/git + tag: "2024.12.1" # Pin to a stable release + pullPolicy: IfNotPresent(Adjust the version string to a valid bitnamisecure/git release tag.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deploy/cloud/helm/platform/components/operator/templates/mpi-run-ssh-keygen-job.yaml(2 hunks)deploy/cloud/helm/platform/components/operator/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: julienmancuso
Repo: ai-dynamo/dynamo PR: 3100
File: deploy/cloud/operator/cmd/main.go:186-190
Timestamp: 2025-09-17T22:35:40.674Z
Learning: The mpiRunSecretName validation in deploy/cloud/operator/cmd/main.go is safe for Helm-based upgrades because the chart automatically provides default values (secretName: "mpi-run-ssh-secret", sshKeygen.enabled: true) and the deployment template conditionally injects the --mpi-run-ssh-secret-name flag, ensuring existing installations get the required configuration during upgrades.
Learnt from: julienmancuso
Repo: ai-dynamo/dynamo PR: 1337
File: deploy/cloud/helm/platform/components/operator/templates/image-builer-serviceaccount.yaml:0-0
Timestamp: 2025-06-03T15:26:55.732Z
Learning: The image-builder ServiceAccount in deploy/cloud/helm/platform/components/operator/templates/image-builer-serviceaccount.yaml does not need imagePullSecrets, unlike the component ServiceAccount.
📚 Learning: 2025-09-17T22:35:40.674Z
Learnt from: julienmancuso
Repo: ai-dynamo/dynamo PR: 3100
File: deploy/cloud/operator/cmd/main.go:186-190
Timestamp: 2025-09-17T22:35:40.674Z
Learning: The mpiRunSecretName validation in deploy/cloud/operator/cmd/main.go is safe for Helm-based upgrades because the chart automatically provides default values (secretName: "mpi-run-ssh-secret", sshKeygen.enabled: true) and the deployment template conditionally injects the --mpi-run-ssh-secret-name flag, ensuring existing installations get the required configuration during upgrades.
Applied to files:
deploy/cloud/helm/platform/components/operator/values.yamldeploy/cloud/helm/platform/components/operator/templates/mpi-run-ssh-keygen-job.yaml
📚 Learning: 2025-09-17T22:35:40.674Z
Learnt from: julienmancuso
Repo: ai-dynamo/dynamo PR: 3100
File: deploy/cloud/operator/cmd/main.go:186-190
Timestamp: 2025-09-17T22:35:40.674Z
Learning: The mpiRunSecretName validation in deploy/cloud/operator/cmd/main.go is safe for upgrades because the Helm chart automatically populates dynamo-operator.dynamo.mpiRun.secretName with a default value of "mpi-run-ssh-secret" and includes SSH key generation functionality via sshKeygen.enabled: true.
Applied to files:
deploy/cloud/helm/platform/components/operator/values.yamldeploy/cloud/helm/platform/components/operator/templates/mpi-run-ssh-keygen-job.yaml
📚 Learning: 2025-06-03T15:26:55.732Z
Learnt from: julienmancuso
Repo: ai-dynamo/dynamo PR: 1337
File: deploy/cloud/helm/platform/components/operator/templates/image-builer-serviceaccount.yaml:0-0
Timestamp: 2025-06-03T15:26:55.732Z
Learning: The image-builder ServiceAccount in deploy/cloud/helm/platform/components/operator/templates/image-builer-serviceaccount.yaml does not need imagePullSecrets, unlike the component ServiceAccount.
Applied to files:
deploy/cloud/helm/platform/components/operator/values.yamldeploy/cloud/helm/platform/components/operator/templates/mpi-run-ssh-keygen-job.yaml
📚 Learning: 2025-06-11T21:18:00.425Z
Learnt from: julienmancuso
Repo: ai-dynamo/dynamo PR: 1474
File: deploy/cloud/operator/internal/controller/dynamocomponent_controller.go:1302-1306
Timestamp: 2025-06-11T21:18:00.425Z
Learning: In the Dynamo operator, the project’s preferred security posture is to set a Pod-level `PodSecurityContext` with `runAsUser`, `runAsGroup`, and `fsGroup` all set to `1000`, and then selectively override the user at the individual container level (e.g., `RunAsUser: 0` for Kaniko) when root is required.
Applied to files:
deploy/cloud/helm/platform/components/operator/templates/mpi-run-ssh-keygen-job.yaml
⏰ 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). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
deploy/cloud/helm/platform/components/operator/templates/mpi-run-ssh-keygen-job.yaml (1)
71-72: Image configuration looks good.The kubectl image is properly version-pinned to
1.34.1and templating is correct. The alpine/k8s image includes kubectl and related Kubernetes tools used during CI/CD and deployment automation.deploy/cloud/helm/platform/components/operator/values.yaml (1)
131-134: Image configuration is well-configured.The
alpine/k8s:1.34.1image is version-pinned and appropriate for kubectl operations in a pre-install hook. Alpine's minimal footprint is suitable for this lightweight Job.
|
Hi Team. Can you please include this fix in 0.7 release. This is blocking us. |
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit