[virtual-machine] Fix vm update hook #1376
Conversation
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
WalkthroughUpdated kubectl patch targets in two vm-update-hook.yaml templates to use the fully qualified KubeVirt resource name virtualmachines.kubevirt.io for instancetype and preference patches. Patch payloads, conditions, PVC resize logic, and overall hook structure remain unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @kvaps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request resolves an issue in the VirtualMachine update hooks by correcting the kubectl patch commands. The change ensures that when a VirtualMachine's instancetype or preference needs to be updated, the patch operation correctly targets the Kubernetes API resource, leading to reliable application of these configuration changes.
Highlights
- Kubectl Patch Command Fix: The
kubectl patchcommands within thevm-update-hook.yamltemplates have been updated to correctly reference VirtualMachine resources using their fully qualified name,virtualmachines.kubevirt.io, instead of the shorthandvirtualmachine. - VirtualMachine Instancetype and Preference Updates: This fix ensures that updates to a VirtualMachine's instancetype and preference are applied successfully by addressing an issue where the
kubectl patchcommand might not have correctly identified the resource.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request correctly fixes the VM update hook by using the fully qualified resource name virtualmachines.kubevirt.io instead of a short name. This improves the robustness of the hook. I've identified a separate potential issue where template variables in the patch payload are not safely escaped, which could cause the hook to fail with certain input values. My review includes suggestions to use the toJson function to address this.
| kubectl patch virtualmachines.kubevirt.io {{ $vmName }} -n {{ $namespace }} \ | ||
| --type merge \ | ||
| -p '{"spec":{"instancetype":{"name": "{{ $instanceType }}", "revisionName": null}}}' |
There was a problem hiding this comment.
While using the fully qualified resource name is a good fix, the patch payload itself is not robust. The $instanceType variable is directly injected into the JSON string. If it contains characters that need JSON escaping (e.g., a double quote), this will produce invalid JSON and cause the patch to fail. To prevent this, you should use Helm's toJson function, which will correctly format the value as a JSON string.
kubectl patch virtualmachines.kubevirt.io {{ $vmName }} -n {{ $namespace }} \
--type merge \
-p '{"spec":{"instancetype":{"name": {{ $instanceType | toJson }}, "revisionName": null}}}'| kubectl patch virtualmachines.kubevirt.io {{ $vmName }} -n {{ $namespace }} \ | ||
| --type merge \ | ||
| -p '{"spec":{"preference":{"name": "{{ $instanceProfile }}", "revisionName": null}}}' |
There was a problem hiding this comment.
Similar to the instancetype patch, the $instanceProfile variable is directly injected into the JSON string. This can lead to invalid JSON if the value contains special characters. Please use the toJson function to ensure the value is correctly JSON-encoded.
kubectl patch virtualmachines.kubevirt.io {{ $vmName }} -n {{ $namespace }} \
--type merge \
-p '{"spec":{"preference":{"name": {{ $instanceProfile | toJson }}, "revisionName": null}}}'| kubectl patch virtualmachines.kubevirt.io {{ $vmName }} -n {{ $namespace }} \ | ||
| --type merge \ | ||
| -p '{"spec":{"instancetype":{"name": "{{ $instanceType }}", "revisionName": null}}}' |
There was a problem hiding this comment.
While using the fully qualified resource name is a good fix, the patch payload itself is not robust. The $instanceType variable is directly injected into the JSON string. If it contains characters that need JSON escaping (e.g., a double quote), this will produce invalid JSON and cause the patch to fail. To prevent this, you should use Helm's toJson function, which will correctly format the value as a JSON string.
kubectl patch virtualmachines.kubevirt.io {{ $vmName }} -n {{ $namespace }} \
--type merge \
-p '{"spec":{"instancetype":{"name": {{ $instanceType | toJson }}, "revisionName": null}}}'| kubectl patch virtualmachines.kubevirt.io {{ $vmName }} -n {{ $namespace }} \ | ||
| --type merge \ | ||
| -p '{"spec":{"preference":{"name": "{{ $instanceProfile }}", "revisionName": null}}}' |
There was a problem hiding this comment.
Similar to the instancetype patch, the $instanceProfile variable is directly injected into the JSON string. This can lead to invalid JSON if the value contains special characters. Please use the toJson function to ensure the value is correctly JSON-encoded.
kubectl patch virtualmachines.kubevirt.io {{ $vmName }} -n {{ $namespace }} \
--type merge \
-p '{"spec":{"preference":{"name": {{ $instanceProfile | toJson }}, "revisionName": null}}}'There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/apps/vm-instance/templates/vm-update-hook.yaml (1)
61-61: Remove trailing whitespace (yamllint).Line 61 contains trailing spaces flagged by YAMLlint. Strip them to satisfy linters.
- +packages/apps/virtual-machine/templates/vm-update-hook.yaml (1)
71-71: Remove trailing whitespace (yamllint).Line 71 has trailing spaces; clean up to appease YAMLlint.
- +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/apps/virtual-machine/templates/vm-update-hook.yaml(1 hunks)packages/apps/vm-instance/templates/vm-update-hook.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
packages/apps/virtual-machine/templates/vm-update-hook.yaml
[error] 71-71: trailing spaces
(trailing-spaces)
packages/apps/vm-instance/templates/vm-update-hook.yaml
[error] 61-61: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (4)
packages/apps/vm-instance/templates/vm-update-hook.yaml (2)
57-60: Good fix: use fully-qualified KubeVirt resource.Switching to virtualmachines.kubevirt.io avoids ambiguity and aligns with RBAC rules (resources: virtualmachines). Looks correct.
64-67: Good fix mirrored for preference patch.Consistent with the instancetype change; targeting virtualmachines.kubevirt.io is correct.
packages/apps/virtual-machine/templates/vm-update-hook.yaml (2)
67-70: Good fix: patch against virtualmachines.kubevirt.io.Fully-qualified resource is correct for KubeVirt CRDs and matches Role rules.
74-77: Consistent target for preference patch.Using virtualmachines.kubevirt.io here as well is correct and consistent.
|
Successfully created backport PR for |
# Description Backport of #1376 to `release-0.35`.
Signed-off-by: Andrei Kvapil kvapss@gmail.com
What this PR does
Fix regression introduced by #1169, now we have correct singular names for virtualmachines which are conflictiing with KubeVirt ones.
Solution: explicitly specify apiversion
Release note
Summary by CodeRabbit
Bug Fixes
Chores