Add hooks to update instanceType, instanceProfile, and storage - #590
Conversation
WalkthroughThis pull request focuses on updating the versioning and configuration for the Changes
Sequence DiagramsequenceDiagram
participant Helm
participant UpdateHook
participant VirtualMachine
participant PersistentVolumeClaim
Helm->>UpdateHook: Trigger pre-install/pre-upgrade hook
UpdateHook->>VirtualMachine: Check current configuration
UpdateHook->>PersistentVolumeClaim: Check current configuration
alt Configuration needs update
UpdateHook->>VirtualMachine: Patch instance type/profile
UpdateHook->>PersistentVolumeClaim: Resize storage
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/apps/vm-instance/templates/vm-update-hook.yaml (2)
49-51: Consider using server-side apply instead of patch.Server-side apply provides better conflict resolution and is the recommended approach for Kubernetes resource updates.
- kubectl patch virtualmachine {{ $vmName }} -n {{ $namespace }} \ - --type merge \ - -p '{"spec":{"instancetype":{"name": "{{ $instanceType }}", "revisionName": null}}}' + cat <<EOF | kubectl apply -f - --server-side + apiVersion: kubevirt.io/v1 + kind: VirtualMachine + metadata: + name: {{ $vmName }} + namespace: {{ $namespace }} + spec: + instancetype: + name: "{{ $instanceType }}" + revisionName: null + EOFAlso applies to: 56-58
53-53: Remove trailing whitespace.There's a trailing whitespace that should be removed for consistency.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 53-53: trailing spaces
(trailing-spaces)
packages/apps/virtual-machine/templates/vm-update-hook.yaml (1)
63-63: Remove trailing whitespace.There is a trailing whitespace on this line.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 63-63: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/apps/versions_map(1 hunks)packages/apps/virtual-machine/Chart.yaml(1 hunks)packages/apps/virtual-machine/templates/vm-update-hook.yaml(1 hunks)packages/apps/vm-instance/Chart.yaml(1 hunks)packages/apps/vm-instance/templates/vm-update-hook.yaml(1 hunks)packages/apps/vm-instance/templates/vm.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/apps/vm-instance/Chart.yaml
🧰 Additional context used
🪛 yamllint (1.35.1)
packages/apps/virtual-machine/templates/vm-update-hook.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 17-17: wrong indentation: expected 0 but found 4
(indentation)
[warning] 18-18: wrong indentation: expected 0 but found 2
(indentation)
[warning] 22-22: wrong indentation: expected 0 but found 2
(indentation)
[warning] 23-23: wrong indentation: expected 0 but found 4
(indentation)
[warning] 24-24: wrong indentation: expected 0 but found 2
(indentation)
[warning] 28-28: wrong indentation: expected 0 but found 2
(indentation)
[warning] 29-29: wrong indentation: expected 0 but found 2
(indentation)
[warning] 30-30: wrong indentation: expected 0 but found 4
(indentation)
[warning] 31-31: wrong indentation: expected 0 but found 2
(indentation)
[error] 63-63: trailing spaces
(trailing-spaces)
packages/apps/vm-instance/templates/vm-update-hook.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 14-14: wrong indentation: expected 0 but found 4
(indentation)
[warning] 15-15: wrong indentation: expected 0 but found 2
(indentation)
[warning] 19-19: wrong indentation: expected 0 but found 2
(indentation)
[warning] 20-20: wrong indentation: expected 0 but found 4
(indentation)
[warning] 21-21: wrong indentation: expected 0 but found 2
(indentation)
[error] 53-53: trailing spaces
(trailing-spaces)
🔇 Additional comments (6)
packages/apps/versions_map (1)
109-110: Version updates look good!The version updates for both packages follow semantic versioning and include commit hashes for better traceability. The new versions (0.7.0 for virtual-machine and 0.4.0 for vm-instance) align with the feature additions in this PR.
Also applies to: 114-115
packages/apps/virtual-machine/Chart.yaml (1)
20-20: Version updates are consistent!The version and appVersion updates to 0.7.0 align with the changes in versions_map and follow proper semantic versioning practices.
Also applies to: 26-26
packages/apps/vm-instance/templates/vm.yaml (1)
20-20: Template structure improvements look good!The indentation adjustments for instancetype and preference blocks improve readability while maintaining the validation checks for resource existence.
Also applies to: 25-25
packages/apps/virtual-machine/templates/vm-update-hook.yaml (3)
1-10: LGTM! Well-structured variable declarations.The variable declarations and resource lookups are implemented correctly, with proper default value handling.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
78-118: LGTM! Well-structured RBAC configuration.The RBAC resources follow security best practices with:
- Principle of least privilege
- Appropriate hook annotations
- Proper cleanup policies
1-118: Verify hook conflicts.Let's verify there are no conflicting hooks that might interfere with VM or PVC updates.
✅ Verification successful
No conflicting hooks found
The hook is safe to use as it's the only pre-hook managing VM and PVC resources in the codebase. The hook's execution order and cleanup are properly configured through weights and delete policies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other hooks that might modify the same resources echo "Searching for other hooks that modify VMs or PVCs..." rg -l 'kind:\s+Job.*helm.sh/hook:\s*pre-' --type yaml | while read -r file; do if grep -q 'virtualmachines\|persistentvolumeclaims' "$file"; then echo "Found potential conflicting hook in: $file" cat "$file" fi doneLength of output: 185
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 17-17: wrong indentation: expected 0 but found 4
(indentation)
[warning] 18-18: wrong indentation: expected 0 but found 2
(indentation)
[warning] 22-22: wrong indentation: expected 0 but found 2
(indentation)
[warning] 23-23: wrong indentation: expected 0 but found 4
(indentation)
[warning] 24-24: wrong indentation: expected 0 but found 2
(indentation)
[warning] 28-28: wrong indentation: expected 0 but found 2
(indentation)
[warning] 29-29: wrong indentation: expected 0 but found 2
(indentation)
[warning] 30-30: wrong indentation: expected 0 but found 4
(indentation)
[warning] 31-31: wrong indentation: expected 0 but found 2
(indentation)
[error] 63-63: trailing spaces
(trailing-spaces)
| spec: | ||
| template: | ||
| metadata: | ||
| labels: | ||
| app: "{{ $.Release.Name }}-update-hook" | ||
| spec: | ||
| serviceAccountName: {{ $.Release.Name }}-update-hook | ||
| restartPolicy: Never |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add job timeout configuration.
The job should have a timeout to prevent indefinite hanging in case of issues.
spec:
+ activeDeadlineSeconds: 300
template:
metadata:
labels:
app: "{{ $.Release.Name }}-update-hook"
spec:
serviceAccountName: {{ $.Release.Name }}-update-hook
restartPolicy: Never📝 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.
| spec: | |
| template: | |
| metadata: | |
| labels: | |
| app: "{{ $.Release.Name }}-update-hook" | |
| spec: | |
| serviceAccountName: {{ $.Release.Name }}-update-hook | |
| restartPolicy: Never | |
| spec: | |
| activeDeadlineSeconds: 300 | |
| template: | |
| metadata: | |
| labels: | |
| app: "{{ $.Release.Name }}-update-hook" | |
| spec: | |
| serviceAccountName: {{ $.Release.Name }}-update-hook | |
| restartPolicy: Never |
| restartPolicy: Never | ||
| containers: | ||
| - name: update-resources | ||
| image: bitnami/kubectl:latest |
There was a problem hiding this comment.
Avoid using 'latest' tag for kubectl image.
Using the 'latest' tag can lead to unpredictable behavior and potential security risks. Specify a fixed version instead.
- image: bitnami/kubectl:latest
+ image: bitnami/kubectl:1.28.4📝 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.
| image: bitnami/kubectl:latest | |
| image: bitnami/kubectl:1.28.4 |
| rules: | ||
| - apiGroups: ["kubevirt.io"] | ||
| resources: ["virtualmachines"] | ||
| verbs: ["patch", "get", "list", "watch"] |
There was a problem hiding this comment.
💡 Codebase verification
Remove unnecessary RBAC permissions
The VM update hook only requires patch and get permissions on virtualmachines. The list and watch permissions can be safely removed as the hook only operates on specific VMs using lookup/get operations.
rules:
- apiGroups: ["kubevirt.io"]
resources: ["virtualmachines"]
verbs: ["patch", "get"]🔗 Analysis chain
Review RBAC permissions scope.
The Role permissions look appropriate for the update hook's requirements. However, verify if 'list' and 'watch' permissions are actually needed since the hook only patches specific VMs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if list/watch operations are used in the codebase
rg -l 'kubectl.*\s(get|list|watch).*virtualmachine'
Length of output: 53
Script:
#!/bin/bash
# Search for Kubernetes client imports and VirtualMachine related code
rg -l "import.*k8s.io/client-go|import.*sigs.k8s.io/controller-runtime"
# Look for list/watch operations and VirtualMachine references
rg -A 3 "List|Watch.*VirtualMachine|virtualmachine"
# Search for any VirtualMachine related code to understand the context
ast-grep --pattern 'VirtualMachine'
Length of output: 105430
Script:
#!/bin/bash
# Search for controller/reconciler patterns and VirtualMachine usage
rg -A 5 "type.*Controller struct|Reconcile|NewInformer.*virtualmachine|Watch.*virtualmachine"
# Look for imports and VirtualMachine type usage
rg "kubevirt.io.*VirtualMachine|client-go/tools/cache|sigs.k8s.io/controller-runtime"
# Search for any code that lists or watches VirtualMachines
ast-grep --pattern 'List($$$VirtualMachine$$$)|Watch($$$VirtualMachine$$$)'
Length of output: 37664
| spec: | ||
| template: | ||
| metadata: | ||
| labels: | ||
| app: "{{ $.Release.Name }}-update-hook" | ||
| spec: | ||
| serviceAccountName: {{ $.Release.Name }}-update-hook | ||
| restartPolicy: Never | ||
| containers: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add job configuration for better reliability.
The job specification is missing important configuration parameters for production readiness.
spec:
+ backoffLimit: 3
+ activeDeadlineSeconds: 300
template:
metadata:
labels:
app: "{{ $.Release.Name }}-update-hook"
spec:
serviceAccountName: {{ $.Release.Name }}-update-hook
+ securityContext:
+ runAsNonRoot: true
+ seccompProfile:
+ type: RuntimeDefault
restartPolicy: Never📝 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.
| spec: | |
| template: | |
| metadata: | |
| labels: | |
| app: "{{ $.Release.Name }}-update-hook" | |
| spec: | |
| serviceAccountName: {{ $.Release.Name }}-update-hook | |
| restartPolicy: Never | |
| containers: | |
| spec: | |
| backoffLimit: 3 | |
| activeDeadlineSeconds: 300 | |
| template: | |
| metadata: | |
| labels: | |
| app: "{{ $.Release.Name }}-update-hook" | |
| spec: | |
| serviceAccountName: {{ $.Release.Name }}-update-hook | |
| securityContext: | |
| runAsNonRoot: true | |
| seccompProfile: | |
| type: RuntimeDefault | |
| restartPolicy: Never | |
| containers: |
| restartPolicy: Never | ||
| containers: | ||
| - name: update-resources | ||
| image: bitnami/kubectl:latest |
There was a problem hiding this comment.
Avoid using 'latest' tag for kubectl image.
Using the 'latest' tag can lead to inconsistent behavior across different environments and times. Specify a fixed version instead.
- image: bitnami/kubectl:latest
+ image: bitnami/kubectl:1.28.4📝 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.
| image: bitnami/kubectl:latest | |
| image: bitnami/kubectl:1.28.4 |
| {{- if and $existingPVC $desiredStorage -}} | ||
| {{- $currentStorage := $existingPVC.spec.resources.requests.storage | toString -}} | ||
| {{- if not (eq $currentStorage $desiredStorage) -}} | ||
| {{- $needResizePVC = true -}} | ||
| {{- end -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve storage size comparison logic.
The current storage comparison might be problematic due to potential format inconsistencies (e.g., "1Gi" vs "1024Mi"). Consider normalizing the storage values before comparison.
{{- if and $existingPVC $desiredStorage -}}
- {{- $currentStorage := $existingPVC.spec.resources.requests.storage | toString -}}
+ {{- $currentBytes := include "virtual-machine.parseStorage" $existingPVC.spec.resources.requests.storage -}}
+ {{- $desiredBytes := include "virtual-machine.parseStorage" $desiredStorage -}}
- {{- if not (eq $currentStorage $desiredStorage) -}}
+ {{- if not (eq $currentBytes $desiredBytes) -}}
{{- $needResizePVC = true -}}
{{- end -}}
{{- end -}}You'll need to implement the virtual-machine.parseStorage helper function to convert storage strings to bytes for accurate comparison.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 28-28: wrong indentation: expected 0 but found 2
(indentation)
[warning] 29-29: wrong indentation: expected 0 but found 2
(indentation)
[warning] 30-30: wrong indentation: expected 0 but found 4
(indentation)
[warning] 31-31: wrong indentation: expected 0 but found 2
(indentation)
Summary by CodeRabbit
New Features
Version Updates
Improvements