[k8s] add expanding persistent volumes in tenant clusters - #1438
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIncrements Kubernetes app chart to 0.29.2 with appVersion 1.32.6, updates kubevirt-csi-driver image, adds csi-resizer sidecar to the controller, introduces cluster-scoped RBAC for PV access, enables StorageClass volume expansion, adjusts CoreDNS values (removes image repository), and updates versions_map entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin
participant KAPI as Kubernetes API Server
participant Resizer as csi-resizer (sidecar)
participant Ctrl as kcsi-controller (CSI)
participant CSI as KubeVirt CSI Driver
Admin->>KAPI: Patch PVC with larger size
KAPI-->>Resizer: PVC/PV size change event (watch)
Resizer->>Ctrl: gRPC over /csi/csi.sock: ControllerExpandVolume
Ctrl->>CSI: Perform volume expansion
CSI-->>Ctrl: Expansion result (success/failure)
Ctrl-->>Resizer: gRPC response
Resizer->>KAPI: Update PV/PVC status and capacity
note over Resizer,KAPI: Requires ClusterRole get on persistentvolumes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
Summary of ChangesHello klinch0, 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 significantly enhances the storage capabilities within tenant Kubernetes clusters by enabling persistent volume expansion. It achieves this by integrating the Kubernetes CSI resizer component into the KubeVirt CSI driver deployment, updating the underlying CSI driver image, and configuring the StorageClass to support volume resizing. These changes provide greater flexibility for users to adjust their storage resources dynamically without manual intervention. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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 (@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
|
There was a problem hiding this comment.
Code Review
This pull request enables persistent volume expansion in tenant clusters by adding the csi-resizer sidecar and updating the StorageClass. It also includes updates to the Helm chart version, documentation, and values.schema.json. While the core changes are a good step, I have identified several issues. There is a critical issue with an invalid image version for the csi-resizer, which will prevent it from starting. The RBAC permissions for the CSI driver may also be insufficient. Furthermore, a container image is being pulled from a personal repository, which poses a security and maintenance risk. I've also noted several errors in the documentation and JSON schema files, including incorrect default values and typos.
| @@ -1 +1 @@ | |||
| ghcr.io/cozystack/cozystack/kubevirt-csi-driver:0.29.1@sha256:cae43eae09fc39e5f2140d30ef55253f871cc565b8b7a564a54077b7cbd92212 | |||
| kklinch0/kubevirt-csi-driver:0.37.0@sha256:d334ba727f0974b085f6e44ee52a61fa0c507063875b52006665dbacfa332cbd | |||
There was a problem hiding this comment.
The CSI driver image is being pulled from a personal Docker Hub repository (kklinch0/...). For production-grade software, images should be sourced from an official or organization-controlled registry to ensure security, availability, and maintainability. Using personal repositories introduces a risk and should be avoided.
| "md0": { | ||
| "ephemeralStorage": "20Gi", | ||
| "gpus": [], | ||
| "gpus": {}, |
There was a problem hiding this comment.
The default value for gpus is set to {}, but the schema defines it as an array. The correct default for an empty array is []. This mismatch can cause issues with schema validation and tooling.
| "gpus": {}, | |
| "gpus": [], |
| storageClass: replicated | ||
| csiDriver: | ||
| image: ghcr.io/cozystack/cozystack/kubevirt-csi-driver:0.29.1@sha256:cae43eae09fc39e5f2140d30ef55253f871cc565b8b7a564a54077b7cbd92212 | ||
| image: kklinch0/kubevirt-csi-driver:0.37.0@sha256:d334ba727f0974b085f6e44ee52a61fa0c507063875b52006665dbacfa332cbd |
There was a problem hiding this comment.
The CSI driver image is being pulled from a personal Docker Hub repository (kklinch0/...). For production-grade software, images should be sourced from an official or organization-controlled registry to ensure security, availability, and maintainability. Using personal repositories introduces a risk and should be avoided.
| | `nodeGroups[name].minReplicas` | Minimum amount of replicas | `int` | `{}` | | ||
| | `nodeGroups[name].maxReplicas` | Maximum amount of replicas | `int` | `{}` | |
There was a problem hiding this comment.
The default values for nodeGroups[name].minReplicas and nodeGroups[name].maxReplicas have been changed to {}, but their type is int. This is incorrect and will be confusing for users reading the documentation. Please provide a valid integer default (like 0 and 10 before) or use n/a if there is no default.
| | `nodeGroups[name].resources` | } Resources available to each worker node | `object` | `{}` | | ||
| | `nodeGroups[name].resources.cpu` | CPU available to each worker node | `*quantity` | `null` | | ||
| | `nodeGroups[name].resources.memory` | Memory (RAM) available to each worker node | `*quantity` | `null` | | ||
| | `nodeGroups[name].gpus` | List of GPUs to attach (WARN: NVIDIA driver requires at least 4 GiB of RAM) | `[]object` | `{}` | | ||
| | `nodeGroups[name].gpus[i].name` | Name of GPU, such as "nvidia.com/AD102GL_L40S" | `string` | `""` | | ||
| | `nodeGroups[name].gpus` | } List of GPUs to attach (WARN: NVIDIA driver requires at least 4 GiB of RAM) | `[]object` | `[]` | | ||
| | `nodeGroups[name].gpus.name` | Name of GPU, such as "nvidia.com/AD102GL_L40S" | `string` | `""` | |
There was a problem hiding this comment.
There are a few issues in this part of the documentation:
- Lines 104 and 107: The descriptions for
nodeGroups[name].resourcesandnodeGroups[name].gpusstart with a}character, which seems to be a typo. - Line 108: The parameter
nodeGroups[name].gpus[i].namehas been changed tonodeGroups[name].gpus.name. Sincegpusis an array of objects, the previous notation was clearer. The new notation is confusing as it impliesgpusis an object. Consider using a notation likegpus[].nameto indicate it's a property of an object within an array.
| }, | ||
| "gpus": { | ||
| "description": "List of GPUs to attach (WARN: NVIDIA driver requires at least 4 GiB of RAM)", | ||
| "description": "} List of GPUs to attach (WARN: NVIDIA driver requires at least 4 GiB of RAM)", |
There was a problem hiding this comment.
The description for gpus starts with a } character, which appears to be a typo.
| "description": "} List of GPUs to attach (WARN: NVIDIA driver requires at least 4 GiB of RAM)", | |
| "description": "List of GPUs to attach (WARN: NVIDIA driver requires at least 4 GiB of RAM)", |
| }, | ||
| "resources": { | ||
| "description": "Resources available to each worker node", | ||
| "description": "} Resources available to each worker node", |
There was a problem hiding this comment.
The description for resources starts with a } character, which appears to be a typo.
| "description": "} Resources available to each worker node", | |
| "description": "Resources available to each worker node", |
Signed-off-by: kklinch0 <kklinch0@gmail.com>
966d873 to
ca19529
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/system/kubevirt-csi-node/values.yaml (1)
3-3: Do not use a personal Docker Hub image for the CSI driver.Use an organization-controlled registry (e.g., ghcr.io/cozystack/...) and pin a known-good digest. Personal repos are a supply‑chain and availability risk.
Verify all references to this image across the repo:
#!/bin/bash rg -n 'kklinch0/kubevirt-csi-driver|kubevirt-csi-driver:' -C2packages/apps/kubernetes/images/kubevirt-csi-driver.tag (1)
1-1: Replace personal image with org-controlled image.Same concern as values.yaml. Promote/publish 0.37.0 to your official registry and reference that digest here.
#!/bin/bash # Ensure only a single source of truth for the driver image tag rg -nP 'kubevirt-csi-driver:(\d+\.\d+\.\d+)|kklinch0/kubevirt-csi-driver' -C2
🧹 Nitpick comments (2)
packages/apps/kubernetes/Chart.yaml (1)
19-19: Version bump looks good. Consider minor bump for a new feature.Enabling volume expansion and adding a sidecar is a feature, not a fix; 0.30.0 may better reflect semver for the chart.
packages/apps/kubernetes/templates/csi/deploy.yaml (1)
208-231: Enable leader election; csi-resizer:v1.13.1 verifiedpackages/apps/kubernetes/templates/csi/deploy.yaml (lines 208–231): image exists and requires Kubernetes ≥ v1.16 (v1.13.x series documented for recent releases up through v1.32).
- - name: csi-resizer - image: registry.k8s.io/sig-storage/csi-resizer:v1.13.1 + - name: csi-resizer + image: registry.k8s.io/sig-storage/csi-resizer:v1.13.1 args: - "-csi-address=/csi/csi.sock" - "-kubeconfig=/etc/kubernetes/kubeconfig/super-admin.svc" - "-v=5" - "-timeout=3m" - '-handle-volume-inuse-error=false' + - "--leader-election=true" + - "--leader-election-namespace=$(INFRACLUSTER_NAMESPACE)"Also pin other non-resizer sidecars (avoid :latest) for reproducibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/apps/kubernetes/Chart.yaml(1 hunks)packages/apps/kubernetes/images/kubevirt-csi-driver.tag(1 hunks)packages/apps/kubernetes/templates/csi/deploy.yaml(1 hunks)packages/apps/kubernetes/templates/csi/infra-cluster-service-account.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/system/coredns/values.yaml(0 hunks)packages/system/kubevirt-csi-node/templates/deploy.yaml(1 hunks)packages/system/kubevirt-csi-node/values.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- packages/system/coredns/values.yaml
⏰ 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/kubernetes/Chart.yaml (1)
25-25: Confirm appVersion alignment.
appVersion 1.32.6 appears in Chart.yaml and is referenced by Cilium image tags — confirm controller/sidecar images and any user‑visible "app version" semantics in release notes actually target 1.32.6. Refs: packages/apps/kubernetes/Chart.yaml:25; packages/apps/kubernetes/files/versions.yaml:5; packages/system/cilium/charts/cilium/values.yaml:2356; packages/system/cilium/charts/cilium/README.md:381.packages/apps/versions_map (1)
75-76: Pin kubernetes 0.29.2 to a commit SHA (replace HEAD)
packages/apps/versions_map:76 — currentlykubernetes 0.29.2 HEAD; replace HEAD with the exact commit SHA to ensure reproducible chart releases.packages/apps/kubernetes/templates/csi/infra-cluster-service-account.yaml (1)
39-61: RBAC likely too narrow for PVs.At minimum, add list and watch on persistentvolumes; consider patch if annotations/status updates are needed by the driver flow.
Apply:
- verbs: ["get"] + verbs: ["get", "list", "watch"]packages/system/kubevirt-csi-node/templates/deploy.yaml (1)
276-276: StorageClass expansion enabled — verify driver capabilities end-to-end
- Confirmed: allowVolumeExpansion: true at packages/system/kubevirt-csi-node/templates/deploy.yaml:276; csi-resizer sidecar present in controller manifest packages/apps/kubernetes/templates/csi/deploy.yaml:208–209 (image registry.k8s.io/sig-storage/csi-resizer:v1.13.1).
- Action: Verify kubevirt-csi-driver v0.37.0 implements ControllerExpandVolume and NodeExpandVolume (both controller- and node-side expansion behavior).
- Recommendation: Make allowVolumeExpansion configurable via values (default false) if expansion should be opt-in.
What this PR does
Release note
Summary by CodeRabbit
New Features
Chores