[kubevirt-csi] Update Role of CSI controller #1165
Conversation
|
""" WalkthroughThe changes update the Kubernetes application's Helm chart version to 0.25.1, expand the permissions in the CSI service account Role to cover additional resources and actions, update the version mapping to reflect the new chart version and its commit reference, and make the CSI driver container image configurable via Helm values. Additionally, the Makefile target for building the CSI driver image now updates the Helm values file with the generated image tag. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant Helm
participant Kubernetes API
participant CSI Controller
User->>Makefile: Build CSI driver image
Makefile->>Helm values.yaml: Update csiDriver.image with generated image tag
User->>Helm: Deploy/Upgrade chart (v0.25.1) with configurable CSI image
Helm->>Kubernetes API: Apply updated Role and DaemonSet with templated image
Kubernetes API->>CSI Controller: Grant expanded permissions
CSI Controller->>Kubernetes API: Perform actions (get/list VMs, manage snapshots, patch PVCs)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/apps/kubernetes/templates/csi/infra-cluster-service-account.yaml (1)
21-26: Consider addingupdateverb for PVCsThe controller patches PVC annotations, but future releases may perform full updates. Granting
updatealongsidepatchis harmless and avoids a potential RBAC regression.- verbs: ["get", "patch"] + verbs: ["get", "patch", "update"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/apps/kubernetes/Chart.yaml(1 hunks)packages/apps/kubernetes/templates/csi/infra-cluster-service-account.yaml(1 hunks)packages/apps/versions_map(1 hunks)
⏰ 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)
19-19: Confirm accompanying docs & artifacts for the version bump
versionwas incremented to0.25.1. Please make sure:
• CHANGELOG / release-notes mention the change,
• anyChart.lockor OCI images are rebuilt,
• down-stream manifests that pin the chart version are updated (e.g. GitOps repos).packages/apps/kubernetes/templates/csi/infra-cluster-service-account.yaml (2)
16-18: LGTM – broader VM resource coverageAdding
virtualmachineinstances/virtualmachineswithlist,getcorrectly reflects the controller’s discovery needs.
19-20: Accurate subresource verbsUsing
updateforaddvolume/removevolumealigns with KubeVirt’s API (these endpoints expect a PUT). No concerns here.packages/apps/versions_map (1)
57-58: Version map consistency checkThe old
0.25.0now points to a fixed commit while0.25.1points toHEAD. Double-check that the referenced commit (70f82667) actually contains the chart at version0.25.0; mismatched pointers break reproducible builds.
Following a [recent update](kubevirt/csi-driver@0171916), the KubeVirt CSI controller now needs new permissions to manage volumes for tenant k8s clusters. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
6d04efb to
8d3324f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/system/kubevirt-csi-node/templates/deploy.yaml (1)
166-166: Parameterised image is good; consider tightening the pull policy
imagePullPolicy: Alwaysdefeats the immutability benefit of a digest-pinned image and increases node-startup latency. Unless you rely on rapid image mutation,IfNotPresent(or making the policy configurable) is typically preferable.- imagePullPolicy: Always + # Consider making this configurable and defaulting to IfNotPresent + imagePullPolicy: IfNotPresent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/apps/kubernetes/Chart.yaml(1 hunks)packages/apps/kubernetes/templates/csi/infra-cluster-service-account.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/system/kubevirt-csi-node/templates/deploy.yaml(1 hunks)packages/system/kubevirt-csi-node/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/apps/kubernetes/Chart.yaml
- packages/apps/versions_map
- packages/apps/kubernetes/templates/csi/infra-cluster-service-account.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 (1)
packages/system/kubevirt-csi-node/values.yaml (1)
2-3: Confirm image reference and automation version syncThe chart’s
version: 0.0.0in Chart.yaml is just a placeholder and will be set by your build/release pipeline, so you don’t need to manually bump it here. However, please:
- Verify the correct GHCR path—should it be
ghcr.io/cozystack/kubevirt-csi-driver:0.25.1@sha256:<new-digest>
instead of
ghcr.io/cozystack/cozystack/kubevirt-csi-driver:…?- Ensure your release automation updates the
image.tagandimage.digestto the 0.25.1 driver.- (Optional) Split
repository,tag, anddigestinto separate values for easier automated bumps.
| @@ -1 +1,3 @@ | |||
| storageClass: replicated | |||
| csiDriver: | |||
| image: ghcr.io/cozystack/cozystack/kubevirt-csi-driver:0.25.0@sha256:445c2727b04ac68595b43c988ff17b3d69a7b22b0644fde3b10c65b47a7bc036 | |||
There was a problem hiding this comment.
Could you also add auto update for this string via make image target of tenant kubernetes application, please?
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/apps/kubernetes/Makefile (1)
67-68: Guardcatfailure so an absent tag file doesn’t silently wipe the Helm value
IMAGE=$$(cat images/kubevirt-csi-driver.tag) \ yq …swallows a failingcatexit-code because the status that reachesmakeisyq’s.
If the tag file is missing,IMAGEbecomes empty andyqwill overwrite.csiDriver.imagewith an empty string, yet the recipe still succeeds.- IMAGE=$$(cat images/kubevirt-csi-driver.tag) \ - yq -i '.csiDriver.image = strenv(IMAGE)' ../../system/kubevirt-csi-node/values.yaml + IMAGE=$$(cat images/kubevirt-csi-driver.tag) && \ + yq -i '.csiDriver.image = strenv(IMAGE)' ../../system/kubevirt-csi-node/values.yamlUsing
&&(orset -eat recipe start) ensures the build aborts if the tag file is absent or unreadable, preventing accidental blanking of the Helm value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/apps/kubernetes/Makefile(1 hunks)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: pre-commit
What this PR does
Following a recent update, the KubeVirt CSI controller now needs new permissions to manage volumes for tenant k8s clusters. This patch updates the role granted to the kcsi-controller deployment of each tenant k8s cluster.
Release note
Summary by CodeRabbit
New Features
Chores