[virtual-machines] Introduce golden disks functionality - #1112
Conversation
WalkthroughA new script is introduced to automate golden image DataVolume creation for Kubernetes CDI. Helm templates for VM and disk resources are updated to prefer golden image PVCs over HTTP sources. The CDI configuration is enhanced with clone strategy and RBAC permissions. Documentation and versioning for the vm-disk package are updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script
participant Kubernetes API
User->>Script: Run cdi_golden_image_create.sh <name> <url>
Script->>Kubernetes API: Create DataVolume vm-image-<name> (HTTP source: <url>)
Kubernetes API-->>User: DataVolume created in cozy-public namespace
sequenceDiagram
participant VM_Template
participant K8s_API
VM_Template->>K8s_API: Lookup DataVolume vm-image-<imageName> in cozy-public
alt DataVolume exists
VM_Template->>K8s_API: Use PVC source from DataVolume
else
VM_Template->>K8s_API: Use HTTP source for image import
end
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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. 🪧 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
CodeRabbit Configuration File (
|
40df3b1 to
a0d2f94
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/apps/virtual-machine/templates/vm.yaml (1)
42-61: Missing guards whensystemDisk.imageis empty can produce invalid manifestsIf
systemDisk.imageis omitted or an unsupported value is supplied:
•$goldenImageNameevaluates to empty →lookupreturnsnil, theelsebranch is chosen, but the HTTPurlstanza is also empty, leavingsource:without children – the VM object will be rejected by the API server.Add a sanity check or fallback:
-{{- $goldenImageName := .Values.systemDisk.image }} +{{- $goldenImageName := required ".Values.systemDisk.image must be set" .Values.systemDisk.image }}and optionally fail when the value is not one of the recognised images:
{{- fail (printf "Unsupported systemDisk.image: %s" $goldenImageName) }}
🧹 Nitpick comments (3)
hack/cdi_golden_image_create.sh (3)
3-3: Harden script flagsAdd
set -u -o pipefailto catch unset vars and pipeline errors.-set -e +set -euo pipefail
8-12: Improve usage message & exit code semanticsReturn exit status
2for bad usage (conventional) and print the script name automatically:echo "Usage: $(basename "$0") <name> <url>" >&2 exit 2
14-32:kubectl createfails on reruns – preferapplyand pre-check namespaceRunning the script twice currently errors with “already exists”. Using
kubectl applymakes it idempotent, and ensuring the namespace exists avoids a race.-kubectl create -f - <<EOF +kubectl apply -f - <<EOF ... - namespace: cozy-public + namespace: cozy-public ... EOF # ensure namespace beforehand kubectl get ns cozy-public >/dev/null 2>&1 || kubectl create ns cozy-public
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
hack/cdi_golden_image_create.sh(1 hunks)packages/apps/virtual-machine/templates/vm.yaml(2 hunks)packages/apps/vm-disk/templates/dv.yaml(1 hunks)packages/apps/vm-disk/values.yaml(1 hunks)packages/system/kubevirt-cdi/templates/cdi-cr.yaml(2 hunks)
🔇 Additional comments (2)
packages/system/kubevirt-cdi/templates/cdi-cr.yaml (1)
6-6: Global clone‐strategy change may have unintended cluster-wide impactSetting
cloneStrategyOverride: copyaffects all DataVolume clones handled by CDI.
• “copy” is slower and heavier on storage than “snapshot”/“csivolume” on back-ends that support them.
• There is no per-namespace escape hatch once the override is in place.Double-check that every storage-class in every environment is comfortable with the forced copy strategy, or consider omitting the override and letting CDI auto-select the optimal clone strategy.
packages/apps/vm-disk/values.yaml (1)
3-7: Docs update looks goodInline examples now clearly spell out that
source.imageexpects a golden-image name – reduces ambiguity for chart users.
| {{- $goldenImageName := .Values.source.image }} | ||
| {{- if $goldenImageName }} | ||
| {{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }} | ||
| pvc: | ||
| name: goldenimage-{{ $goldenImageName }} | ||
| namespace: cozy-public | ||
| {{- else if hasKey .Values.source "http" }} |
There was a problem hiding this comment.
PVC branch should verify that the golden image DataVolume actually exists
Unlike the VM template, this chart always emits a PVC reference even when the lookup returns nil, causing the DV creation to hang forever.
Wrap the PVC block with the same existence check or fail early:
-{{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }}
-pvc:
- name: goldenimage-{{ $goldenImageName }}
- namespace: cozy-public
+{{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }}
+{{- if $dv }}
+pvc:
+ name: goldenimage-{{ $goldenImageName }}
+ namespace: cozy-public
+{{- else }}
+{{- fail (printf "Golden image %s not found in cozy-public namespace" $goldenImageName) }}
+{{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- $goldenImageName := .Values.source.image }} | |
| {{- if $goldenImageName }} | |
| {{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }} | |
| pvc: | |
| name: goldenimage-{{ $goldenImageName }} | |
| namespace: cozy-public | |
| {{- else if hasKey .Values.source "http" }} | |
| {{- $goldenImageName := .Values.source.image }} | |
| {{- if $goldenImageName }} | |
| {{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" "cozy-public" (printf "goldenimage-%s" $goldenImageName) }} | |
| {{- if $dv }} | |
| pvc: | |
| name: goldenimage-{{ $goldenImageName }} | |
| namespace: cozy-public | |
| {{- else }} | |
| {{- fail (printf "Golden image %s not found in cozy-public namespace" $goldenImageName) }} | |
| {{- end }} | |
| {{- else if hasKey .Values.source "http" }} |
🤖 Prompt for AI Agents
In packages/apps/vm-disk/templates/dv.yaml around lines 23 to 29, the PVC block
is emitted even when the lookup for the golden image DataVolume returns nil,
causing hangs. Fix this by wrapping the PVC block inside a conditional that
checks if the DataVolume exists (i.e., the lookup result is not nil), so the PVC
is only referenced when the DataVolume is present, or alternatively add an early
failure if it does not exist.
ecbf8b2 to
88cc13a
Compare
88cc13a to
a562b82
Compare
Use Golden Images to speed up VM / VMI deploy Signed-off-by: gwynbleidd <gwynbleidd2106@yandex.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
a562b82 to
986de71
Compare
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
Just refactored a bit, renamed goldenimage- to vm-image-.
added check for image name input for vm-instance application
other things are LGTM
Use Golden Images to speed up VM / VMI deploy
Summary by CodeRabbit
New Features
Improvements
Versioning