vms add sockets to resources - #1131
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 5 minutes and 24 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 (11)
""" WalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HelmChart
participant VMTemplate
User->>HelmChart: Provide values.yaml (including resources.cpu and resources.sockets)
HelmChart->>VMTemplate: Render vm.yaml with provided values
VMTemplate-->>HelmChart: If resources.cpu and resources.sockets are set, include both in CPU config
HelmChart-->>User: Deploys VM with specified CPU cores and sockets
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:
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 (
|
45c7584 to
f71dee9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/apps/vm-instance/values.yaml (1)
33-40: Same integer-type consideration applies here
resources.socketswould benefit from being an integer rather than an empty string.packages/apps/virtual-machine/values.schema.json (1)
188-192: Mirror the integer change for consistencyFor parity with the previous comment, define
socketsas an integer in this schema too.
🧹 Nitpick comments (4)
packages/apps/vm-instance/README.md (1)
39-55: Minor wording fixes in the parameters table
- “passthrough” → “pass through” (verb form).
- Sentence “Can be a single key or a list of keys.” lacks a subject.
-| `externalMethod` | specify method to passthrough the traffic to the virtual machine. Allowed values: `WholeIP` and `PortList` | `PortList` | +| `externalMethod` | Specify the method to pass through traffic to the virtual machine. Allowed values: `WholeIP` and `PortList` | `PortList` | ... -| `sshKeys` | List of SSH public keys for authentication. Can be a single key or a list of keys. | `[]` | +| `sshKeys` | List of SSH public keys for authentication. It can be a single key or a list of keys. | `[]` |packages/apps/virtual-machine/README.md (1)
42-44: Same grammar nit: “pass through” is a verb-| `externalMethod` | specify method to passthrough the traffic to the virtual machine. Allowed values: `WholeIP` and `PortList` | `PortList` | +| `externalMethod` | Specify the method to pass through traffic to the virtual machine. Allowed values: `WholeIP` and `PortList` | `PortList` |packages/apps/virtual-machine/values.yaml (1)
35-42: Prefer integer type forresources.socketsUnlike
cpu/memory,socketsis a plain count. Defining it as an int (0default) tightens validation and avoids string-to-int coercion later.packages/apps/vm-instance/values.schema.json (1)
168-173: Schema could tighten validation by switchingsocketstointegerCurrent definition (
type: "string", default: "") allows non-numeric input. Usingintegerwithminimum: 1would catch user errors early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/apps/versions_map(2 hunks)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)
🧰 Additional context used
🪛 LanguageTool
packages/apps/vm-instance/README.md
[grammar] ~42-~42: The word “passthrough” is a noun. The verb is spelled with a white space.
Context: ...externalMethod | specify method to passthrough the traffic to the virtual machine. All...
(NOUN_VERB_CONFUSION)
[style] ~52-~52: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ... of SSH public keys for authentication. Can be a single key or a list of keys. ...
(MISSING_IT_THERE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (3)
packages/apps/virtual-machine/Chart.yaml (1)
20-20: Chart version bumped, but appVersion left unchanged – double-check intentIf the underlying VM application code (not just the chart templates) changed to support
resources.sockets, consider incrementingappVersionas well.
LeavingappVersion: 0.11.0is perfectly fine when only chart logic / manifests change, but please verify this is deliberate.packages/apps/vm-instance/Chart.yaml (1)
20-20: Same note on appVersion consistency
version→ 0.9.0 is correct.
Ensure the runtime application itself is still 0.8.0; otherwise bumpappVersiontoo for clarity.packages/apps/virtual-machine/README.md (1)
53-54: Great to seeresources.socketsdocumentedDoc addition syncs with the chart change. ✅
| {{- if and .Values.resources .Values.resources.cpu .Values.resources.sockets }} | ||
| cpu: | ||
| cores: {{ .Values.resources.cpu }} | ||
| sockets: {{ .Values.resources.sockets }} | ||
| {{- end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Breaking: CPU block now suppressed unless both cpu and sockets are set
Requiring .Values.resources.sockets as part of the and guard means existing charts that only specify resources.cpu will lose the entire cpu stanza, silently reverting to KubeVirt defaults.
-{{- if and .Values.resources .Values.resources.cpu .Values.resources.sockets }}
-cpu:
- cores: {{ .Values.resources.cpu }}
- sockets: {{ .Values.resources.sockets }}
-{{- end }}
+{{- if or .Values.resources.cpu .Values.resources.sockets }}
+cpu:
+ {{- if .Values.resources.cpu }}
+ cores: {{ .Values.resources.cpu }}
+ {{- end }}
+ {{- if .Values.resources.sockets }}
+ sockets: {{ .Values.resources.sockets }}
+ {{- end }}
+{{- end }}Restores backward-compatibility while still enabling socket topology.
📝 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.resources .Values.resources.cpu .Values.resources.sockets }} | |
| cpu: | |
| cores: {{ .Values.resources.cpu }} | |
| sockets: {{ .Values.resources.sockets }} | |
| {{- end }} | |
| {{- if or .Values.resources.cpu .Values.resources.sockets }} | |
| cpu: | |
| {{- if .Values.resources.cpu }} | |
| cores: {{ .Values.resources.cpu }} | |
| {{- end }} | |
| {{- if .Values.resources.sockets }} | |
| sockets: {{ .Values.resources.sockets }} | |
| {{- end }} | |
| {{- end }} |
🤖 Prompt for AI Agents
In packages/apps/vm-instance/templates/vm.yaml around lines 34 to 38, the
current condition requires both .Values.resources.cpu and
.Values.resources.sockets to render the cpu block, which breaks backward
compatibility for charts specifying only cpu. Modify the condition to render the
cpu block if .Values.resources.cpu is set, and include sockets only if
.Values.resources.sockets is also set, so the cpu stanza is not suppressed when
sockets is missing.
| {{- if and .Values.resources .Values.resources.cpu .Values.resources.sockets }} | ||
| cpu: | ||
| cores: {{ .Values.resources.cpu }} | ||
| sockets: {{ .Values.resources.sockets }} | ||
| {{- end }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
cpu block now hard-requires sockets → breaks backward compatibility
The new guard
{{- if and .Values.resources .Values.resources.cpu .Values.resources.sockets }}means that existing values files that specify only:
resources:
cpu: 2will no longer render a cpu: stanza at all. That is a silent breaking change for anyone upgrading the chart without adding the new key, and it also prevents the perfectly valid KubeVirt default (sockets defaults to 1 when omitted).
Recommend loosening the condition and making sockets optional:
-{{- if and .Values.resources .Values.resources.cpu .Values.resources.sockets }}
+{{- if and .Values.resources .Values.resources.cpu }}
cpu:
cores: {{ .Values.resources.cpu }}
- sockets: {{ .Values.resources.sockets }}
+ {{- if .Values.resources.sockets }}
+ sockets: {{ .Values.resources.sockets }}
+ {{- end }}
{{- end }}This preserves previous behaviour while allowing the new topology knob.
Please double-check that the default values.yaml actually sets resources.sockets; otherwise charts that relied only on cpu will break after upgrade. If you’d like, I can open a follow-up PR with the above patch.
🏁 Script executed:
#!/bin/bash
# Check default CPU and sockets settings in the chart's values.yaml
grep -R "resources:" -n packages/apps/virtual-machine/values.yaml -A5
grep -R "sockets" -n packages/apps/virtual-machine/values.yamlLength of output: 446
Make sockets optional when rendering the cpu block to preserve backward compatibility
We’ve confirmed that packages/apps/virtual-machine/values.yaml defines:
resources:
cpu: ""
memory: ""
sockets: ""
so users who set only resources.cpu (and rely on the default socket count) will currently see no cpu: stanza due to the extra .Values.resources.sockets guard. To restore the previous behavior (render cpu whenever cpu is set) while still allowing an explicit sockets override, update templates/vm.yaml as follows:
--- a/packages/apps/virtual-machine/templates/vm.yaml
+++ b/packages/apps/virtual-machine/templates/vm.yaml
@@ -63,7 +63,11 @@
-{{- if and .Values.resources .Values.resources.cpu .Values.resources.sockets }}
+{{- if and .Values.resources .Values.resources.cpu }}
cpu:
cores: {{ .Values.resources.cpu }}
- sockets: {{ .Values.resources.sockets }}
+ {{- if .Values.resources.sockets }}
+ sockets: {{ .Values.resources.sockets }}
+ {{- end }}
{{- end }}Locations needing this patch:
- packages/apps/virtual-machine/templates/vm.yaml (around line 63)
This ensures:
- Charts that specify only
resources.cpustill render thecpu:block. socketsis added only when explicitly provided (otherwise KubeVirt’s default of 1 socket applies).
📝 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.resources .Values.resources.cpu .Values.resources.sockets }} | |
| cpu: | |
| cores: {{ .Values.resources.cpu }} | |
| sockets: {{ .Values.resources.sockets }} | |
| {{- end }} | |
| {{- if and .Values.resources .Values.resources.cpu }} | |
| cpu: | |
| cores: {{ .Values.resources.cpu }} | |
| {{- if .Values.resources.sockets }} | |
| sockets: {{ .Values.resources.sockets }} | |
| {{- end }} | |
| {{- end }} |
🤖 Prompt for AI Agents
In packages/apps/virtual-machine/templates/vm.yaml around lines 63 to 67, the
current condition requires both cpu and sockets to render the cpu block, which
breaks backward compatibility. Modify the condition to render the cpu block
whenever cpu is set, and include sockets only if it is explicitly provided. This
means checking for cpu alone to render the cpu block, and inside it,
conditionally add the sockets line only if sockets is set.
032060f to
d089d95
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/apps/vm-instance/README.md (3)
42-42: “pass through” is a verb phrase – fix wording for clarityUse the verb phrase “pass through” (two words) instead of the noun “passthrough”.
-| `externalMethod` | specify method to passthrough the traffic to the virtual machine. Allowed values: `WholeIP` and `PortList` | `PortList` | +| `externalMethod` | Specify how to pass traffic through to the virtual machine. Allowed values: `WholeIP` and `PortList` | `PortList` |
49-51: Document the coupling betweenresources.cpuand newresources.socketsThe chart template only renders the CPU topology when both
resources.cpuandresources.socketsare set. Without mentioning this here, users may provide only one of the two values and wonder why nothing happens.Consider amending the descriptions to make the dependency explicit, e.g.:
-| `resources.cpu` | The number of CPU cores allocated to the virtual machine | `""` | -| `resources.sockets` | The number of CPU sockets allocated to the virtual machine (used to define vCPU topology) | `""` | +| `resources.cpu` | The number of CPU cores allocated to the virtual machine (must be set together with `resources.sockets`) | `""` | +| `resources.sockets` | The number of CPU sockets allocated to the virtual machine (defines vCPU topology; requires `resources.cpu`) | `""` |
52-52: Complete the sentence forsshKeysrowThe description lacks a subject. Small tweak improves readability.
-| `sshKeys` | List of SSH public keys for authentication. Can be a single key or a list of keys. | `[]` | +| `sshKeys` | List of SSH public keys for authentication. This can be a single key or a list of keys. | `[]` |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/apps/versions_map(2 hunks)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 (10)
- packages/apps/vm-instance/Chart.yaml
- packages/apps/vm-instance/templates/vm.yaml
- packages/apps/virtual-machine/Chart.yaml
- packages/apps/versions_map
- packages/apps/vm-instance/values.yaml
- packages/apps/virtual-machine/values.yaml
- packages/apps/virtual-machine/README.md
- packages/apps/virtual-machine/values.schema.json
- packages/apps/vm-instance/values.schema.json
- packages/apps/virtual-machine/templates/vm.yaml
🧰 Additional context used
🪛 LanguageTool
packages/apps/vm-instance/README.md
[grammar] ~42-~42: The word “passthrough” is a noun. The verb is spelled with a white space.
Context: ...externalMethod | specify method to passthrough the traffic to the virtual machine. All...
(NOUN_VERB_CONFUSION)
[style] ~52-~52: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ... of SSH public keys for authentication. Can be a single key or a list of keys. ...
(MISSING_IT_THERE)
Signed-off-by: kklinch0 <[email protected]>
d089d95 to
70c7978
Compare
Updating changes from - cozystack/cozystack#1131 - cozystack/cozystack#1137 Signed-off-by: Nick Volynkin <[email protected]>
Updating changes from - cozystack/cozystack#1131 - cozystack/cozystack#1137 Signed-off-by: Nick Volynkin <[email protected]>
3 participants
What this PR does
Release note
Summary by CodeRabbit
New Features
Documentation
Chores