[virtual-machine] Fix: Add GPU names to virtual machines spec - #862
Conversation
|
Warning Rate limit exceededAndrei Kvapil (@kvaps) has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughThis update modifies both the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HelmChart
participant VMTemplate
User->>HelmChart: Deploy/Upgrade Chart
HelmChart->>VMTemplate: Render vm.yaml with values (including gpus)
VMTemplate->>VMTemplate: For each GPU, assign unique name (gpu1, gpu2, ...)
VMTemplate->>HelmChart: Return rendered manifest with named GPU devices
HelmChart->>User: Deployment includes GPUs with unique identifiers
Possibly related PRs
Suggested labels
Suggested reviewers
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:
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/apps/vm-instance/templates/vm.yaml (1)
49-50: GPUnamefield addition for vm-instance template
Great improvement adding a uniquenamefield for each GPU device. To keep the chart self‑documented, consider updatingpackages/apps/vm-instance/values.yaml(or the README) to include an example and schema description for the newnameproperty under thegpusarray.packages/apps/virtual-machine/values.yaml (1)
34-34: Consider quoting the GPU name
For clarity and to avoid any YAML parsing edge cases, quote the GPU name string:- - name: nvidia.com/GA102GL_A10 + - name: "nvidia.com/GA102GL_A10"packages/apps/virtual-machine/templates/vm.yaml (1)
77-78: GPU naming: Ensure unique identifiers and correct indentationThe added fields correctly introduce a unique
namefor each GPU (gpu1,gpu2, etc.) alongside the existingdeviceName. Please confirm that:
- Your
values.yamldefines.Values.gpusas a list of objects with anamekey (e.g.,- name: "nvidia.com/GA102GL_A10") so$gpu.nameresolves correctly.- The indentation of the
- name:anddeviceName:lines aligns with the surrounding YAML (consider using Helm’snindentfor consistent spacing).If you’d like to support both string and object definitions in
.Values.gpus, consider this refactor:{{- range $i, $gpu := .Values.gpus }} - name: gpu{{ add $i 1 }} deviceName: {{ if kindIs "string" $gpu }}{{ $gpu }}{{ else }}{{ $gpu.name }}{{ end }} {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/apps/versions_map(2 hunks)packages/apps/virtual-machine/Chart.yaml(1 hunks)packages/apps/virtual-machine/templates/vm.yaml(1 hunks)packages/apps/virtual-machine/values.yaml(1 hunks)packages/apps/vm-instance/Chart.yaml(1 hunks)packages/apps/vm-instance/templates/vm.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (5)
packages/apps/vm-instance/Chart.yaml (1)
20-20:Details
✅ Verification successful
Chart version bump to 0.6.1 in vm-instance
Bumping the chart version to0.6.1correctly reflects the new GPU name feature while theappVersionremains unchanged. Please verify thatpackages/apps/versions_mapincludes a matchingvm-instance 0.6.1 HEADentry.
🏁 Script executed:
#!/bin/bash # Verify versions_map entry exists for vm-instance 0.6.1 HEAD set -e rg -n '^vm-instance 0.6.1 HEAD$' packages/apps/versions_mapLength of output: 97
Chart version bump verified
The chart inpackages/apps/vm-instance/Chart.yamlhas been updated to0.6.1, and a matching entry (vm-instance 0.6.1 HEAD) exists inpackages/apps/versions_map(line 158). Approving this change.packages/apps/virtual-machine/Chart.yaml (1)
20-20:Details
✅ Verification successful
Chart version bump to 0.9.1 in virtual‑machine
The chart version is correctly incremented to0.9.1per semantic versioning, withappVersionunchanged at0.9.0. Please confirm thatpackages/apps/versions_maphas an entry forvirtual-machine 0.9.1 HEAD.
🏁 Script executed:
#!/bin/bash # Verify versions_map entry exists for virtual-machine 0.9.1 HEAD set -e rg -n '^virtual-machine 0.9.1 HEAD$' packages/apps/versions_mapLength of output: 105
Chart version bump to 0.9.1 in virtual‑machine
Version updated to0.9.1inChart.yaml(line 20);appVersionremains at0.9.0. Confirmedpackages/apps/versions_mapcontainsvirtual-machine 0.9.1 HEAD. LGTM.packages/apps/versions_map (2)
146-147: Update version mapping for virtual‑machine
The mapping pins0.9.0to commit721c12a7and marks0.9.1asHEAD, matching the chart bump.
157-158: Update version mapping for vm‑instance
The mapping pins0.6.0to commit721c12a7and marks0.6.1asHEAD, aligning with the updated chart version.packages/apps/virtual-machine/values.yaml (1)
33-34: Review default GPU change in values.yaml
Introducing a default GPU entry means VMs will now attachnvidia.com/GA102GL_A10unless overridden. Ensure this behavioural change is intended for all users and document it in the chart’s README or migration guide for transparency.
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
7f673fc to
8547dc3
Compare
|
Successfully created backport PR for |
Signed-off-by: Andrei Kvapil kvapss@gmail.com
Summary by CodeRabbit
New Features
Configuration
Version Updates