fix demo - use arm compatible mongodb image from bitnami#673
fix demo - use arm compatible mongodb image from bitnami#673thapakazi wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughReplaces an embedded NVSentinel/MongoDB YAML in the demo setup script with an external values file, adds architecture-specific overrides for ARM64, switches NVSentinel install to use the external values file (OCI-based), and updates script flow to conditionally include per-arch values and apply Helm with --values. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
demos/local-fault-injection-demo/scripts/00-setup.sh (1)
222-233: Update bitnamilegacy images for arm64 compatibility.Both referenced images lack arm64 support and will fail on arm64 systems:
- Line 224:
bitnamilegacy/nginx:1.27.2-debian-12-r2— amd64-only- Line 232:
bitnamilegacy/mongodb-exporter:0.41.2-debian-12-r1— amd64-onlyThese images need to be replaced with versions that have multi-arch manifests including arm64.
|
Hey, Thank you for your PR! For ARM we actually make use the following image in CI: https://github.com/NVIDIA/NVSentinel/blob/main/distros/kubernetes/nvsentinel/values-tilt-arm64.yaml#L19. Can we use the same? It might be better to update the demo script to detect the architecture and update the values file at runtime |
- Move inline Helm configurations from 00-setup.sh to standalone YAML files. - Add 00-demo-values-arm.yaml to support ARM64 overrides(MongoDB images) - Refactor setup script to use the new values files and improve path handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @demos/local-fault-injection-demo/scripts/00-demo-values.yaml:
- Around line 69-73: The ARM override file is missing ARM-compatible image
overrides for nginx and mongodb-exporter; update 00-demo-values-arm.yaml to
mirror the pattern used for mongodb by replacing the bitnamilegacy
image.repository/tag for the nginx and mongodb-exporter entries (the
image.repository and image.tag keys under the nginx and mongodb-exporter
sections) with ARM-compatible registry/repository/tag values (e.g., an ARM build
equivalent) and ensure image.pullPolicy matches the main arm override entries so
both services use ARM-capable images.
🧹 Nitpick comments (1)
demos/local-fault-injection-demo/scripts/00-demo-values.yaml (1)
46-46: Consider removing fragile line number reference.The comment referencing "lines 73-184" from
values-tilt.yamlwill become stale as that file evolves. Consider removing the line numbers or referencing a section name instead.Suggested change
-# MongoDB Store configuration - copied from values-tilt.yaml (lines 73-184) +# MongoDB Store configuration - based on values-tilt.yaml
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
demos/local-fault-injection-demo/scripts/00-demo-values-arm.yamldemos/local-fault-injection-demo/scripts/00-demo-values.yamldemos/local-fault-injection-demo/scripts/00-setup.sh
✅ Files skipped from review due to trivial changes (1)
- demos/local-fault-injection-demo/scripts/00-demo-values-arm.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- demos/local-fault-injection-demo/scripts/00-setup.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA/NVSentinel PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T22:20:48.152Z
Learning: Applies to **/values.yaml : Include examples for non-obvious configurations in Helm chart documentation
📚 Learning: 2025-11-24T22:20:48.152Z
Learnt from: CR
Repo: NVIDIA/NVSentinel PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T22:20:48.152Z
Learning: Applies to **/values.yaml : Include examples for non-obvious configurations in Helm chart documentation
Applied to files:
demos/local-fault-injection-demo/scripts/00-demo-values.yaml
📚 Learning: 2025-11-26T13:54:26.427Z
Learnt from: rupalis-nv
Repo: NVIDIA/NVSentinel PR: 361
File: distros/kubernetes/nvsentinel/values-tilt.yaml:215-223
Timestamp: 2025-11-26T13:54:26.427Z
Learning: For values-tilt.yaml files: Keep documentation minimal and implementation-focused. Unlike production values.yaml files which need comprehensive inline comments, values-tilt.yaml is a Tilt-specific testing/development override file that doesn't require extensive documentation.
Applied to files:
demos/local-fault-injection-demo/scripts/00-demo-values.yaml
📚 Learning: 2025-11-24T22:20:48.152Z
Learnt from: CR
Repo: NVIDIA/NVSentinel PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T22:20:48.152Z
Learning: Health monitors should include Helm chart in `distros/kubernetes/nvsentinel/charts/`
Applied to files:
demos/local-fault-injection-demo/scripts/00-demo-values.yaml
🔇 Additional comments (4)
demos/local-fault-injection-demo/scripts/00-demo-values.yaml (4)
1-14: LGTM - Global settings are well-organized.The configuration clearly enables only the GPU health monitor while explicitly disabling others, which is appropriate for a focused demo. The inline comments improve readability.
15-44: LGTM - Component configuration is minimal and demo-appropriate.Good practice to explicitly disable unused components. The comment on line 34 explaining why labeler is disabled is helpful.
94-98: Hardcoded password is acceptable for local demo context.The
rootPassword: "demo-password"is fine for a local demo setup with persistence disabled. Just ensure this file isn't accidentally used in any non-demo environments.
100-130: LGTM - Resource configurations are reasonable for demo workloads.The resource requests and limits are appropriately sized for a local demo environment.
| image: | ||
| registry: "docker.io" | ||
| repository: "bitnamilegacy/mongodb" | ||
| tag: "8.0.3-debian-12-r1" | ||
| pullPolicy: "IfNotPresent" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the ARM override values file
find . -name "00-demo-values-arm.yaml" -o -name "*values*arm*.yaml" | head -20Repository: NVIDIA/NVSentinel
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Get a better understanding of the directory structure
ls -la demos/local-fault-injection-demo/scripts/ 2>/dev/null | head -20Repository: NVIDIA/NVSentinel
Length of output: 643
🏁 Script executed:
#!/bin/bash
# Read the ARM override file
echo "=== ARM Override File ==="
cat demos/local-fault-injection-demo/scripts/00-demo-values-arm.yaml
echo -e "\n=== Base Values File ==="
cat demos/local-fault-injection-demo/scripts/00-demo-values.yamlRepository: NVIDIA/NVSentinel
Length of output: 2925
🏁 Script executed:
#!/bin/bash
# Read the CI ARM values file for comparison
echo "=== CI ARM Values File ==="
cat distros/kubernetes/nvsentinel/values-tilt-arm64.yaml | head -150Repository: NVIDIA/NVSentinel
Length of output: 854
ARM override file is incomplete; nginx and mongodb-exporter images still use bitnamilegacy variants on ARM.
The 00-demo-values-arm.yaml file only provides an ARM-compatible override for mongodb (dlavrenuek/bitnami-mongodb-arm), but fails to override the following bitnamilegacy images that also likely lack ARM manifests:
- nginx (lines 82-83):
bitnamilegacy/nginx:1.27.2-debian-12-r2 - mongodb-exporter (lines 90-91):
bitnamilegacy/mongodb-exporter:0.41.2-debian-12-r1
Add ARM-compatible image overrides for both nginx and mongodb-exporter to ensure the demo runs on ARM systems.
🤖 Prompt for AI Agents
In @demos/local-fault-injection-demo/scripts/00-demo-values.yaml around lines 69
- 73, The ARM override file is missing ARM-compatible image overrides for nginx
and mongodb-exporter; update 00-demo-values-arm.yaml to mirror the pattern used
for mongodb by replacing the bitnamilegacy image.repository/tag for the nginx
and mongodb-exporter entries (the image.repository and image.tag keys under the
nginx and mongodb-exporter sections) with ARM-compatible registry/repository/tag
values (e.g., an ARM build equivalent) and ensure image.pullPolicy matches the
main arm override entries so both services use ARM-capable images.
|
|
||
| # pass architecture specific overrides | ||
| local arch_overrides="" | ||
| case "$(uname -m)" in | ||
| arm64|aarch64) | ||
| arch_overrides="--values $BASE_DIR/00-demo-values-arm.yaml" | ||
| ;; | ||
| esac | ||
|
|
||
| helm upgrade --install nvsentinel oci://ghcr.io/nvidia/nvsentinel \ | ||
| --version "$nvsentinel_version" \ | ||
| --namespace "$NAMESPACE" \ | ||
| --values /tmp/nvsentinel-demo-values.yaml \ | ||
| --values $BASE_DIR/00-demo-values.yaml \ | ||
| $arch_overrides \ |
There was a problem hiding this comment.
this is the main change, and values(from files) specific to arm is passed if arch == arm
2 participants
Summary
while trying to run the demo on mac(arm64), mongo image is missing for arm64,
so using arm64 available image from bitnami: https://hub.docker.com/r/bitnamisecure/mongodb
Type of Change
Component(s) Affected
Testing
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.