[Fix] CloudInit for VM Instance - #1020
Conversation
WalkthroughThe condition for including the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/apps/vm-instance/templates/vm.yaml (1)
103-124: Refined three-case volume logic for cloudinitdiskThe volume definitions correctly handle all three scenarios (SSH keys +
cloudInit, SSH keys only, neither).Consider changing the
{{-at the start of your conditionals to{{so that the generated- name: cloudinitdisklines retain their leading indentation and avoid potential YAML misalignment.packages/apps/virtual-machine/templates/vm.yaml (1)
85-89: Standardize property order for cloudinit diskFor better readability and consistency with KubeVirt examples, move the
name:before thedisk:block:- - disk: - bus: virtio - name: cloudinitdisk + - name: cloudinitdisk + disk: + bus: virtio
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/apps/virtual-machine/templates/vm.yaml(4 hunks)packages/apps/vm-instance/templates/vm.yaml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (4)
packages/apps/vm-instance/templates/vm.yaml (2)
71-75: Simplified conditional for cloudinit disk deviceThe disk device is now included whenever SSH keys are provided, regardless of whether a
cloudInitvalue is set. This aligns with the intended three-case logic for cloud-init configuration.
83-92: Conditional accessCredentials on SSH keys onlyGood enhancement:
accessCredentialsis now conditioned solely on.Values.sshKeys, removing the previouswithblock and ensuring credentials are only injected when SSH keys exist.packages/apps/virtual-machine/templates/vm.yaml (2)
98-107: SimplifiedaccessCredentialsconditionalGood update:
accessCredentialsnow only depends on.Values.sshKeys, matching the new logic in the VM instance template.
116-137:Details
✅ Verification successful
Confirm removal of
final_messageThe previous fallback userData contained a
final_message, which has now been removed. Please verify that no downstream templates or scripts rely on this message. You can search the repo with:
🏁 Script executed:
#!/bin/bash # Search for any references to 'final_message' rg -n "final_message" -C 2Length of output: 26
Removal of
final_messageVerifiedA repository-wide search for “final_message” returned no matches, confirming that no downstream templates or scripts reference it. Removal is safe.
| @@ -13,6 +13,7 @@ metadata: | |||
| {{- include "virtual-machine.labels" . | nindent 4 }} | |||
| spec: | |||
| running: {{ .Values.running | default "true" }} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Running default should be a boolean, not a string
Your current line uses:
running: {{ .Values.running | default "true" }}This will render "true" (a string) instead of a boolean true. KubeVirt expects a boolean. Apply this diff:
- running: {{ .Values.running | default "true" }}
+ running: {{ .Values.running | default true }}🤖 Prompt for AI Agents
In packages/apps/virtual-machine/templates/vm.yaml at line 15, the default value
for "running" is set as the string "true" instead of a boolean. Change the
default to a boolean by removing the quotes around true so that it renders as a
boolean true, ensuring compatibility with KubeVirt's expectations.
cf192cb to
f8b148c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/apps/vm-instance/templates/vm.yaml (1)
83-92: Ensure correct templating context forincludelookupIn the
accessCredentialsblock you invoke the fullname helper with$:secretName: {{ include "virtual-machine.fullname" $ }}-ssh-keysI don’t see an assignment for
$in this template, which may lead to a render error. If there isn’t a root‐context alias set, switch to using.:- secretName: {{ include "virtual-machine.fullname" $ }}-ssh-keys + secretName: {{ include "virtual-machine.fullname" . }}-ssh-keys
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/apps/virtual-machine/templates/vm.yaml(4 hunks)packages/apps/vm-instance/templates/vm.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/apps/virtual-machine/templates/vm.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (1)
packages/apps/vm-instance/templates/vm.yaml (1)
103-125: Review cloud-init conditional branches and secretRef usageYou’ve split the
cloudinitdiskvolume into three cases (secretRef only when both SSH keys and CloudInit; inline only for SSH keys; minimal otherwise). This means a standalone.Values.cloudInitno longer yields asecretRefto user-provided data.
- If you intend to support pure CloudInit via secret (without SSH keys), you’ll need to add an explicit branch for
.Values.cloudInit.- Otherwise, please document that
.Values.cloudInitby itself produces only a header (#cloud-config) and will not reference a secret.Can you confirm this matches the desired behavior?
d32bec7 to
6f2b484
Compare
|
Timofei Larkin (@lllamnyp) klinch0 all the tests are passed successfully, ready for approval / merge |
6f2b484 to
bdde913
Compare
f8b148c to
adf2a4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/apps/vm-instance/templates/vm.yaml (1)
82-89: Unexpected device condition may omit cloud-init disk for cloudInit-only use cases.The disk device is only attached when
.Values.sshKeysis set, but you still render acloudinitdiskvolume for cloudInit-only scenarios, causing an unattached volume. Update the condition:- {{- if .Values.sshKeys }} + {{- if or .Values.sshKeys .Values.cloudInit }}
🧹 Nitpick comments (2)
packages/apps/vm-instance/templates/vm.yaml (2)
1-3: Improve error message grammar for instancetype validation.The phrase "not exists" is ungrammatical; consider changing to "does not exist". For example:
- fail (printf "Specified instancetype not exists in cluster: %s" .Values.instanceType) + fail (printf "Specified instancetype does not exist in cluster: %s" .Values.instanceType)🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
4-6: Improve error message grammar for profile validation.Similarly, update "not exists" to "does not exist" to maintain consistency:
- fail (printf "Specified profile not exists in cluster: %s" .Values.instanceProfile) + fail (printf "Specified profile does not exist in cluster: %s" .Values.instanceProfile)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/apps/versions_map(2 hunks)packages/apps/virtual-machine/Chart.yaml(1 hunks)packages/apps/vm-instance/Chart.yaml(1 hunks)packages/apps/vm-instance/templates/vm.yaml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/apps/virtual-machine/Chart.yaml
- packages/apps/vm-instance/Chart.yaml
- packages/apps/versions_map
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (5)
packages/apps/vm-instance/templates/vm.yaml (5)
17-21: Nothing to comment here.
22-26: Nothing to comment here.
98-107: Nothing to comment here, SSH key injection is correct.
112-115: Nothing to comment here.
139-142: Nothing to comment here.
| running: {{ .Values.running | default "true" }} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a boolean default for running instead of a string.
Templating running: {{ .Values.running | default "true" }} injects a string, but KubeVirt expects a boolean. Update to:
- running: {{ .Values.running | default "true" }}
+ running: {{ .Values.running | default true }}📝 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.
| running: {{ .Values.running | default "true" }} | |
| running: {{ .Values.running | default true }} |
🤖 Prompt for AI Agents
In packages/apps/vm-instance/templates/vm.yaml at lines 15 to 16, the default
value for the running field is set as a string "true" which should be a boolean.
Change the default value from the string "true" to the boolean true by removing
the quotes around true in the default function to ensure KubeVirt receives a
boolean value.
| dataVolumeTemplates: | ||
| - metadata: | ||
| name: {{ include "virtual-machine.fullname" . }} | ||
| spec: | ||
| storage: | ||
| resources: | ||
| requests: | ||
| storage: {{ .Values.systemDisk.storage | quote }} | ||
| {{- with .Values.systemDisk.storageClass }} | ||
| storageClassName: {{ . }} | ||
| {{- end }} | ||
| source: | ||
| http: | ||
| {{- if eq .Values.systemDisk.image "cirros" }} | ||
| url: https://download.cirros-cloud.net/0.6.2/cirros-0.6.2-x86_64-disk.img | ||
| {{- else if eq .Values.systemDisk.image "ubuntu" }} | ||
| url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img | ||
| {{- else if eq .Values.systemDisk.image "fedora" }} | ||
| url: https://download.fedoraproject.org/pub/fedora/linux/releases/40/Cloud/x86_64/images/Fedora-Cloud-Base-Generic.x86_64-40-1.14.qcow2 | ||
| {{- else if eq .Values.systemDisk.image "alpine" }} | ||
| url: https://dl-cdn.alpinelinux.org/alpine/v3.20/releases/cloud/nocloud_alpine-3.20.2-x86_64-bios-tiny-r0.qcow2 | ||
| {{- else if eq .Values.systemDisk.image "talos" }} | ||
| url: https://github.com/siderolabs/talos/releases/download/v1.7.6/nocloud-amd64.raw.xz | ||
| {{- end }} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle unknown systemDisk.image values and DRY up mapping.
The current if/else chain covers only five images and silently omits a URL for any other value, leading to an invalid spec. Consider adding a default else that fails:
{{- else }}
- url:
+ fail (printf "Unsupported systemDisk.image: %s" .Values.systemDisk.image)Alternatively, move the image-to-URL mapping into values.yaml (as a map) to simplify the template and avoid long conditionals.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/apps/vm-instance/templates/vm.yaml around lines 28 to 52, the
current if/else chain for setting the systemDisk image URL only handles five
specific images and does not handle unknown values, which can result in an
invalid spec. To fix this, add a default else clause that either sets a clear
error or fallback URL to prevent silent failures. Alternatively, refactor by
moving the image-to-URL mapping into values.yaml as a map and update the
template to look up the URL from this map, eliminating the long conditional
chain and improving maintainability.
| {{- if and .Values.sshKeys .Values.cloudInit }} | ||
| - name: cloudinitdisk | ||
| cloudInitNoCloud: | ||
| secretRef: | ||
| name: {{ include "virtual-machine.fullname" . }}-cloud-init | ||
| {{- else if .Values.sshKeys }} | ||
| - name: cloudinitdisk | ||
| cloudInitNoCloud: | ||
| userData: | | ||
| {{ printf "%s" "#cloud-config" }} | ||
| ssh_authorized_keys: | ||
| {{- range .Values.sshKeys }} | ||
| - {{ . }} | ||
| {{- end }} | ||
| chpasswd: | ||
| expire: false | ||
| {{- else }} | ||
| userData: | | ||
| #cloud-config | ||
| final_message: Cloud-init user-data was left blank intentionally. | ||
| - name: cloudinitdisk | ||
| cloudInitNoCloud: | ||
| userData: | | ||
| {{ printf "%s" "#cloud-config" }} | ||
| {{- end }} |
There was a problem hiding this comment.
Fix cloudInit-only volume case.
When only .Values.cloudInit is true (and no SSH keys), the template falls back to an empty #cloud-config, discarding user-provided data. Introduce a branch to mount secretRef for pure cloudInit:
- {{- else }}
- - name: cloudinitdisk
- cloudInitNoCloud:
- userData: |
- {{ printf "%s" "#cloud-config" }}
+ {{- else if .Values.cloudInit }}
+ - name: cloudinitdisk
+ cloudInitNoCloud:
+ secretRef:
+ name: {{ include "virtual-machine.fullname" . }}-cloud-init
+ {{- else }}
+ - name: cloudinitdisk
+ cloudInitNoCloud:
+ userData: |
+ {{ printf "%s" "#cloud-config" }}This ensures provided cloudInit data is used even without SSH keys.
📝 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.
| {{- if and .Values.sshKeys .Values.cloudInit }} | |
| - name: cloudinitdisk | |
| cloudInitNoCloud: | |
| secretRef: | |
| name: {{ include "virtual-machine.fullname" . }}-cloud-init | |
| {{- else if .Values.sshKeys }} | |
| - name: cloudinitdisk | |
| cloudInitNoCloud: | |
| userData: | | |
| {{ printf "%s" "#cloud-config" }} | |
| ssh_authorized_keys: | |
| {{- range .Values.sshKeys }} | |
| - {{ . }} | |
| {{- end }} | |
| chpasswd: | |
| expire: false | |
| {{- else }} | |
| userData: | | |
| #cloud-config | |
| final_message: Cloud-init user-data was left blank intentionally. | |
| - name: cloudinitdisk | |
| cloudInitNoCloud: | |
| userData: | | |
| {{ printf "%s" "#cloud-config" }} | |
| {{- end }} | |
| {{- if and .Values.sshKeys .Values.cloudInit }} | |
| - name: cloudinitdisk | |
| cloudInitNoCloud: | |
| secretRef: | |
| name: {{ include "virtual-machine.fullname" . }}-cloud-init | |
| {{- else if .Values.sshKeys }} | |
| - name: cloudinitdisk | |
| cloudInitNoCloud: | |
| userData: | | |
| {{ printf "%s" "#cloud-config" }} | |
| ssh_authorized_keys: | |
| {{- range .Values.sshKeys }} | |
| - {{ . }} | |
| {{- end }} | |
| chpasswd: | |
| expire: false | |
| {{- else if .Values.cloudInit }} | |
| - name: cloudinitdisk | |
| cloudInitNoCloud: | |
| secretRef: | |
| name: {{ include "virtual-machine.fullname" . }}-cloud-init | |
| {{- else }} | |
| - name: cloudinitdisk | |
| cloudInitNoCloud: | |
| userData: | | |
| {{ printf "%s" "#cloud-config" }} | |
| {{- end }} |
🤖 Prompt for AI Agents
In packages/apps/vm-instance/templates/vm.yaml around lines 116 to 137, the
template does not handle the case when only .Values.cloudInit is true without
sshKeys, causing it to use an empty #cloud-config instead of mounting the
secretRef. Add a new conditional branch that checks if .Values.cloudInit is true
and .Values.sshKeys is false or empty, and in that case, mount the cloudinitdisk
volume using the secretRef with the name derived from "virtual-machine.fullname"
plus "-cloud-init". This will ensure the user-provided cloudInit data is
correctly used even when no sshKeys are provided.
34a0cf2 to
ab67603
Compare
made same change as in [PR](#1019) Signed-off-by: Ahmad Murzahmatov <gwynbleidd2106@yandex.com>
ab67603 to
bbee810
Compare
Made same changes as in PR
Summary by CodeRabbit