[virtual-machine] Add GPU support - #766
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request updates chart versioning for both virtual-machine and vm-instance applications and introduces GPU configuration. In the virtual-machine package, the chart’s version and appVersion have been bumped, and a conditional GPU block is added to the VM template. A new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Config
participant TE as Template Engine
participant VM as Virtual Machine Spec
U->>TE: Provide configuration values (including 'gpus')
alt GPUs defined
TE->>VM: Insert GPU devices block from .Values.gpus
else
TE->>VM: Render configuration without GPU details
end
VM-->>U: Return rendered VM manifest
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
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 (
|
Nick Volynkin (NickVolynkin)
left a comment
There was a problem hiding this comment.
a bit of nitpicking :)
| | `instanceType` | Virtual Machine instance type | `u1.medium` | | ||
| | `instanceProfile` | Virtual Machine prefferences profile | `ubuntu` | | ||
| | `disks` | List of disks to attach | `[]` | | ||
| | `gpus` | List of gpus to attach | `[]` | |
There was a problem hiding this comment.
| | `gpus` | List of gpus to attach | `[]` | | |
| | `gpus` | List of GPUs to attach | `[]` | |
There was a problem hiding this comment.
thanks fixed
| }, | ||
| "gpus": { | ||
| "type": "array", | ||
| "description": "List of gpus to attach", |
There was a problem hiding this comment.
| "description": "List of gpus to attach", | |
| "description": "List of GPUs to attach", |
There was a problem hiding this comment.
thanks fixed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/apps/vm-instance/templates/vm.yaml (1)
52-53: Avoid Variable Shadowing in Disk LookupWithin the disks loop, the lookup function reassigns the loop variable
$disk—this can lead to shadowing and potential confusion. It is recommended to use a new variable name (e.g.$foundDisk) for the result of the lookup. For example:- {{- $disk := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" $.Release.Namespace (printf "vm-disk-%s" $disk.name) }} - {{- if $disk }} - {{- if and (hasKey $disk.metadata.annotations "vm-disk.cozystack.io/optical") (eq (index $disk.metadata.annotations "vm-disk.cozystack.io/optical") "true") }} - cdrom: {} - {{- else }} - disk: {} - {{- end }} - {{- if eq $i 0 }} - bootOrder: 1 - {{- end }} - {{- else }} - {{- fail (printf "Specified disk not exists in cluster: %s" .name) }} - {{- end }} + {{- $foundDisk := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" $.Release.Namespace (printf "vm-disk-%s" $disk.name) }} + {{- if $foundDisk }} + {{- if and (hasKey $foundDisk.metadata.annotations "vm-disk.cozystack.io/optical") (eq (index $foundDisk.metadata.annotations "vm-disk.cozystack.io/optical") "true") }} + cdrom: {} + {{- else }} + disk: {} + {{- end }} + {{- if eq $i 0 }} + bootOrder: 1 + {{- end }} + {{- else }} + {{- fail (printf "Specified disk not exists in cluster: %s" $disk.name) }} + {{- end }}This change will avoid unintended shadowing and make the template clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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/README.md(1 hunks)packages/apps/vm-instance/templates/vm.yaml(1 hunks)packages/apps/vm-instance/values.schema.json(1 hunks)packages/apps/vm-instance/values.yaml(1 hunks)
🔇 Additional comments (10)
packages/apps/vm-instance/values.schema.json (1)
148-155: Critical: Inconsistent GPU Definition in JSON Schema
The new"gpus"property is defined as an array of strings with a default value set as"[]". However, elsewhere the GPU configuration is expected to be provided as an object (with at least anameproperty), as shown by the example in the values file and its usage in the template (i.e. using{{ $gpu.name }}). This discrepancy could lead to validation or runtime errors. Consider updating the schema so that each item is defined as an object and the default value is an actual empty array rather than a string.Example diff:
-"gpus": { - "type": "array", - "description": "List of gpus to attach", - "default": "[]", - "items": { - "type": "string" - } -}, +"gpus": { + "type": "array", + "description": "List of gpus to attach", + "default": [], + "items": { + "type": "object", + "properties": { + "name": { "type": "string" } + }, + "required": ["name"] + } +},[flag_critical_issue][suggest_essential_refactor]
packages/apps/virtual-machine/values.yaml (1)
29-33: Approval: New GPU Parameter in Virtual Machine Values
The new GPU parameter is clearly documented with an example and is appropriately initialized as an empty array. Please ensure that GPU entries are provided as objects (with anamefield) so that they align with how they are referenced in the template.
[approve_code_changes]packages/apps/vm-instance/README.md (1)
47-49: Approval: Updated Documentation with GPU Parameter
The addition of thegpusparameter in the parameters table is clear and helpful. The documentation now properly reflects the new configuration option.
[approve_code_changes]packages/apps/virtual-machine/templates/vm.yaml (1)
72-77: Critical: GPU Device Entry Structure Mismatch
The conditional block for GPUs correctly checks for.Values.gpusand iterates over it; however, it assumes that each GPU entry is an object with anameproperty ({{ $gpu.name }}). Please verify that the provided GPU configurations (and the JSON schema definition) match this structure. Adjustments in the schema or values may be needed if GPU entries are meant to be objects.
[flag_critical_issue][request_verification]packages/apps/vm-instance/values.yaml (1)
27-32: Approval: Clear GPU Parameter Configuration
The GPU parameter is added with a clear description and an example entry. The default of an empty array is appropriate. Just ensure that all parts of the system expect each GPU entry to be an object with anameproperty.
[approve_code_changes]packages/apps/virtual-machine/Chart.yaml (2)
20-20: Chart Version Update VerificationThe chart version has been updated from the previous version (0.8.2) to 0.9.0, which is consistent with the intended version bump.
26-26: App Version Type ConsistencyThe appVersion is now specified as an unquoted integer (0.9.0) rather than a string. Please verify that this change in data type is intentional and compatible with any downstream consumers of this chart.
packages/apps/vm-instance/Chart.yaml (2)
20-20: Chart Version BumpThe chart version has been correctly updated from 0.5.1 to 0.6.0. This version bump aligns with the new GPU support and other feature changes.
26-26: App Version Update and Data Type ReviewSimilar to the virtual-machine chart, the appVersion is now an integer (0.6.0) rather than a quoted string. Confirm that this change does not adversely affect components that expect a string value.
packages/apps/vm-instance/templates/vm.yaml (1)
44-49: Addition of GPU Configuration BlockA new GPU configuration block has been added inside the devices section. The conditional check (
if .Values.gpus) and the iteration over each GPU to generate a device entry (- deviceName: {{ $gpu.name }}) look correct. Please verify that the resulting YAML indentation and formatting are as expected when rendered.
xy2
left a comment
There was a problem hiding this comment.
I double the proper capitalization by Nick Volynkin (@NickVolynkin) .
Also, the $disk change is minor, but is not related to the GPU. Should be submitted and tested separately.
0302307 to
4c5bdd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/apps/virtual-machine/README.md (1)
50-50: New 'gpus' Parameter DocumentationThe new
gpusparameter is added to the parameters table with a default value of[]and a clear description. Consider adding an example usage or a brief note on how GPU identifiers should be specified—this could help users unfamiliar with GPU configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/apps/virtual-machine/Chart.yaml(1 hunks)packages/apps/virtual-machine/README.md(1 hunks)packages/apps/virtual-machine/templates/vm.yaml(1 hunks)packages/apps/virtual-machine/values.schema.json(1 hunks)packages/apps/virtual-machine/values.yaml(1 hunks)packages/apps/vm-instance/Chart.yaml(1 hunks)packages/apps/vm-instance/README.md(1 hunks)packages/apps/vm-instance/templates/vm.yaml(1 hunks)packages/apps/vm-instance/values.schema.json(1 hunks)packages/apps/vm-instance/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/apps/virtual-machine/values.yaml
- packages/apps/virtual-machine/templates/vm.yaml
- packages/apps/vm-instance/values.schema.json
- packages/apps/vm-instance/README.md
- packages/apps/vm-instance/values.yaml
- packages/apps/virtual-machine/Chart.yaml
- packages/apps/vm-instance/templates/vm.yaml
- packages/apps/vm-instance/Chart.yaml
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
4c5bdd0 to
9ad6b0d
Compare
Related issues to GPU feature:
Summary by CodeRabbit
Summary by CodeRabbit