Skip to content

feat(kubevirt)!: drop HotplugVolumes for declarative volume hotplug - #4453

Merged
Aleksei Sviridkin (lexfrei) merged 2 commits into
mainfrom
feat/kubevirt-declarative-hotplug
Oct 6, 2026
Merged

Aleksei Sviridkin (lexfrei) merged 2 commits into
mainfrom
feat/kubevirt-declarative-hotplug

Conversation

@lexfrei

@lexfrei Aleksei Sviridkin (lexfrei) commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

What this PR does

This turns off the deprecated KubeVirt HotplugVolumes gate and moves the platform to declarative volume hotplug. It changes how every volume hotplug on the platform works, so the title carries !. Closes #4032.

This PR is stacked on #4451 and has to merge after it. #4451 adds the kubevirt-csi flag this PR flips.

The replacement gate is already on in KubeVirt 1.9, and the old gate is the only thing keeping it off. DeclarativeHotplugVolumes is Beta at v1.9.0, and Beta gates are on by default. virt-controller skips handleDeclarativeVolumeHotplug while HotplugVolumes is on, so dropping the old gate is what switches the new one on.

The kubevirt-csi fallback goes off in the same change. With the old gate off, virt-api rejects the VMI-level removevolume on a VM-owned VMI, so the chart now passes --enable-vmi-hotplug-fallback=false. The comments next to the gate and the flag point at each other.

This needs the rebuilt kubevirt-csi image. A driver built before #4451 does not know the flag and exits on it at startup. The image is stamped at release preparation and ships in the same release as this change, so only a make apply from main before then would crash-loop the CSI controller.

What changes at runtime on an upgrade:

  • The VM-level addvolume and removevolume calls now edit VM.spec directly. virt-controller then makes the VMI hotplug volumes match it, instead of going through VM.status.volumeRequests.
  • Volumes hot-plugged through the VM subresource are in VM.spec, so they stay attached.
  • During a tenant teardown, when the VM goes before its VMI, the driver no longer calls the VMI subresource. It waits for the VMI status to drop the volume, and the garbage collector deletes the VMI with its VM. Upstream waits up to two minutes. If the VMI is still there after that, the unpublish fails and is retried.

A new platform migration keeps old VMI-only volumes attached. On its first pass virt-controller removes every hotpluggable VMI volume that is missing from VM.spec. Until July 2025 kubevirt-csi attached volumes at the VMI level (the Role then granted virtualmachineinstances/addvolume), and live migration carries such a VMI across KubeVirt updates. Without help, a tenant node not recreated since then would lose a mounted disk on upgrade.

Migration 59 copies each such volume and its disk from the VMI into the VM template before anything else changes. Adding a hotplug volume to the VM spec is a valid live update in KubeVirt, so no VM needs a restart. It runs as the platform chart's pre-upgrade hook, before the upgrade re-points cozystack-packages. The KubeVirt CR and the tenant kubernetes charts both render from that source, so the gate and the flag cannot change before the migration has finished. It only touches VMs that need it, logs each one, and a second run changes nothing.

The migration also makes ephemeral hotplugs permanent. A volume a user added at the VMI level, for example with virtctl addvolume without --persist from an older virtctl, has the same shape as kubevirt-csi's old attachments, because virt-api builds both the same way. There is nothing reliable to tell them apart by, so both are copied. Leaving the user's volume out would not keep it ephemeral: the first reconcile would unplug it from the running VM. After the upgrade it is part of VM.spec and attaches again on a restart, and virtctl removevolume drops it.

Ephemeral hotplug stops working on VMs. With the old gate off, virt-api refuses VMI-level addvolume and removevolume on a VMI that a VM owns (vmiVolumePatch in v1.9.0). A volume on such a VMI is added and removed through the VM from then on. A VMI without a VM is not affected.

Only running VMIs are copied. virt-controller reconciles no other VMI, and a stopped VM builds its next VMI from VM.spec, so copying from a stopped one would bring back volumes the restart was going to drop.

Migrations run only on the isp-full, isp-full-generic and isp-hosted variants. KubeVirt comes from the platform only on isp-full and isp-full-generic, so those are covered. On the default variant, where packages are installed by hand, nothing runs the migration.

This optional check lists the VMIs and volumes the migration would copy. It uses the same rule. On the default variant, run it before upgrading and recreate each tenant node it lists:

kubectl get vm,vmi --all-namespaces --output json | jq --raw-output '
  ([.items[] | select(.kind == "VirtualMachine")
    | {key: "\(.metadata.namespace)/\(.metadata.name)", value: [.spec.template.spec.volumes[]?.name]}]
    | from_entries) as $vm
  | .items[] | select(.kind == "VirtualMachineInstance")
  | ([.metadata.ownerReferences[]? | select(.controller == true and .kind == "VirtualMachine" and (.apiVersion | startswith("kubevirt.io/"))) | .name][0]) as $owner
  | "\(.metadata.namespace)/\($owner)" as $key
  | select(.status.phase == "Running" and $owner != null and $vm[$key] != null)
  | .spec.volumes[]? | select(.dataVolume.hotpluggable == true or .persistentVolumeClaim.hotpluggable == true)
  | .name as $n | select($vm[$key] | index($n) | not)
  | "\($key) \($n)"'

Empty output means nothing is affected. I checked the query against the migration's test fixtures only. Neither the query nor the migration has run on a live cluster.

#4287 also adds a migration 59. If it merges first, this one moves to 60: the file, the version it stamps, migrations.targetVersion, the bats file (its run_migration argument and expected STAMP) and hack/testdata/migration-59-vmi-hotplug.

A VM with a pending hotplug request at upgrade time may also get stuck. virt-controller still processes VM.status.volumeRequests before the declarative handler, and it applies them through the VMI subresource, which virt-api rejects once the old gate is off. The sync then fails before the VM spec is updated, so nothing clears the request, and every later sync of that VM fails the same way. A retry through the VM subresource edits the spec and clears it, which covers kubevirt-csi's own requests. A one-shot request stays stuck until the VM restarts. I read this from the v1.9.0 code and haven't reproduced it. Waiting until this lists nothing before upgrading narrows the window, but a request made during the rollout can still hit it:

kubectl get vm --all-namespaces --output json | jq --raw-output '.items[] | select((.status.volumeRequests // []) | length > 0) | .metadata.namespace + "/" + .metadata.name'

The two halves live in different HelmReleases, so there is a short window during the rollout where the driver and the gate disagree. In that window an unpublish fails and is retried until both are updated.

A tenant cluster whose kubernetes HelmRelease is suspended or failing keeps its old driver for longer. Drivers from before July 2025 attach through the VMI subresource, so on such a tenant every attach is refused until its release reconciles. Such a driver can also attach a volume after the migration has run and before the gate flips. The migration doesn't see that volume, and the first declarative reconcile unplugs it from the running VM.

The migration lists the VMs once and patches each one with a resourceVersion test, so a VM that changes in between fails the hook Job. The Job has backoffLimit: 3, and each retry lists again and skips the VMs already patched. Re-reading only the conflicting VM would avoid the retry; I left that out.

The new gate is listed in the CR even though it is on by default. With the old gate gone it is the only hotplug path, and the chart refuses a render that disables it. I found no other user of HotplugVolumes in the platform or in the downstream repositories.

The VMI-level fallback may have been failing already. The kcsi Role has no rule for the VMI-level removevolume (#4452), so that call likely got 403 with or without the gate. I haven't checked that on a cluster.

hack/migration-59-vmi-hotplug-volumes.bats runs the migration against a fake kubectl: a VM that needs the copy, VMs and VMIs that must be left alone (a VMI that is not Running among them), a second run, a cluster without KubeVirt and the fail-closed paths. The helm-unittests for the KubeVirt CR pin the whole gate list, accept HotplugVolumes in disabledFeatureGates and refuse DeclarativeHotplugVolumes there. The CSI test pins the flag at false, and the make update idempotency test still passes. None of that exercises hotplug itself. The real test is e2e with volume attach and detach on a tenant cluster, including a teardown. No CI lane upgrades from the previous release yet, so an upgrade with a live VMI-only volume is untested; #4463 tracks that test.

Screenshots

No UI change.

Downstream repositories

Release note

feat(kubevirt)!: KubeVirt now runs with DeclarativeHotplugVolumes instead of the deprecated HotplugVolumes feature gate, and kubevirt-csi no longer uses the VMI-level hot-unplug. Volume hotplug requests are applied through VM.spec. The upgrade copies volumes that older kubevirt-csi versions attached only to the VMI into VM.spec, so they stay attached; the check in the PR description is optional. This also applies to ephemeral volumes added to running user VMs at the VMI level, such as with virtctl addvolume without --persist: they become part of VM.spec and attach again after a VM restart. virt-api now refuses VMI-level addvolume and removevolume on a VMI owned by a VM, so such volumes are added and removed through the VM. A tenant cluster whose kubernetes release is suspended or failing keeps its old kubevirt-csi, and a driver from before July 2025 cannot attach volumes there until that release reconciles, and a volume it attaches during the upgrade itself can be unplugged again. A values override that puts DeclarativeHotplugVolumes into disabledFeatureGates now fails the kubevirt render. Clusters on the default variant do not run platform migrations and should run that check before upgrading.

Summary by CodeRabbit

  • New Features
    • Declarative hotplug volumes are enabled by default for KubeVirt.
    • Running virtual machines keep eligible hotplugged volumes and disks when their configuration is migrated.
  • Bug Fixes
    • The migration avoids adding duplicate volumes or disks and does not mark the migration complete if required operations fail.
    • The Kubernetes CSI driver no longer uses the VMI hotplug fallback.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
docs/agents/e2e-testing.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: cozystack/cozystack/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8b9f15f2-d849-41bc-88b7-9e470f961875
📥 Commits

Reviewing files that changed from the base of the PR and between 94abf69 and 8dbc7bc.

📒 Files selected for processing (11)
  • hack/migration-59-vmi-hotplug-volumes.bats
  • hack/testdata/migration-59-vmi-hotplug/kubectl
  • hack/testdata/migration-59-vmi-hotplug/vmis.json
  • hack/testdata/migration-59-vmi-hotplug/vms.json
  • packages/apps/kubernetes/templates/csi/deploy.yaml
  • packages/apps/kubernetes/tests/csi_vmi_hotplug_fallback_test.yaml
  • packages/core/platform/images/migrations/migrations/59
  • packages/core/platform/values.yaml
  • packages/system/kubevirt/templates/kubevirt-cr.yaml
  • packages/system/kubevirt/tests/kubevirt_cr_test.yaml
  • packages/system/kubevirt/values.yaml

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

Migration 59→60 copies eligible hotplug volumes and matching disks from running VMIs into their owning VM templates. The change also updates the KubeVirt feature gates and sets the Kubernetes CSI driver’s VMI hotplug fallback flag to false.

Changes

VMI hotplug handling

Layer / File(s) Summary
Backfill missing volumes and disks
packages/core/platform/images/migrations/migrations/59, packages/core/platform/values.yaml, hack/migration-59-vmi-hotplug-volumes.bats, hack/testdata/migration-59-vmi-hotplug/*
Migration 59→60 selects eligible running VMIs and adds hotplug volumes missing from their owning VM templates, along with matching disks not already present. It tests the VM resource version before patching and stamps version 60 after successful completion. Tests cover eligibility, repeated runs, absent KubeVirt, and command failures.
Configure KubeVirt feature gates
packages/system/kubevirt/templates/kubevirt-cr.yaml, packages/system/kubevirt/values.yaml, packages/system/kubevirt/tests/kubevirt_cr_test.yaml
The static and rendered enabled-gate lists replace HotplugVolumes with DeclarativeHotplugVolumes. Tests cover the updated defaults and disabledFeatureGates handling.
Set CSI fallback flag
packages/apps/kubernetes/templates/csi/deploy.yaml, packages/apps/kubernetes/tests/csi_vmi_hotplug_fallback_test.yaml
The CSI driver Deployment sets --enable-vmi-hotplug-fallback=false. The test checks that the false form is present and the true form is absent.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Migration59
  participant kubectl
  participant KubernetesAPI
  Migration59->>kubectl: Check KubeVirt CRD and list VMs and VMIs
  kubectl->>KubernetesAPI: Read CRD, VM, and VMI resources
  KubernetesAPI-->>kubectl: Return resource data
  kubectl-->>Migration59: Return resource data
  Migration59->>kubectl: Patch eligible VM with resource-version test
  kubectl->>KubernetesAPI: Apply JSON patch
  KubernetesAPI-->>kubectl: Return patch result
  Migration59->>kubectl: Apply version 60 ConfigMap
Loading

Merge Risk: 🔵 Low · up to 8dbc7

This change moves volume hotplug to the declarative path and backfills hotplugged volumes from running VMIs into their VMs. The migration has not run on a live cluster and no N-1 upgrade has been tested. It can be merged, but operators should know about that gap and the documented edge cases.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 8dbc7

The upgrade moves live disk attachments into persistent VM configuration. Failure handling protects against incomplete updates, but ownership identity and concurrent detachment can leave incorrect persistent attachments. A tenant authorization bypass has not been established.

Retained concerns

  • Medium · security · inferred: The new privileged backfill resolves a VMI's controlling VM by namespace and owner name without comparing ownerReference.uid to VM.metadata.uid. If a running VMI outlives its original VM while a same-name VM is recreated, its volumes can be persisted into the wrong VM identity. Namespace filtering limits this to same-namespace assets, and the target resourceVersion check protects concurrent target edits but does not authenticate ownership. Production reachability of this stale-owner condition remains unverified; the inspected CSI identity cannot directly mutate VMI metadata.
  • Medium · security · inferred: The backfill copies a VMI snapshot into durable VM state while guarding only the target VM resourceVersion. A legacy VMI-only hotplug detached after the snapshot can therefore be written back as a persistent attachment without changing the target VM version. This can defeat the intended attachment lifecycle and restore workload access to a disk. The pre-upgrade hook orders chart changes but does not itself freeze live VMI operations; no runtime evidence establishes how often this race is reachable.
Security review details

Security Blast Radius

  • inferred — The migration can modify eligible VMs throughout the management cluster, so rollout exposure spans multiple namespaces and tenants. Each selected target is nevertheless resolved within the source VMI namespace; the identified ownership failure is bounded to same-namespace assets, not an established arbitrary cross-namespace write primitive.

Security Findings and Attack Paths

  • inferred — A conditional attack path would require control of management-cluster VMI ownership or volume state, or a reachable stale-owner lifecycle, before an operator-triggered upgrade promotes that state using cluster-admin authority. The inspected CSI role does not grant VMI creation or metadata mutation, so that identity alone does not establish the forgery path. The retained concerns are identity and lifecycle weaknesses; no verified tenant exploit is claimed.

Trust Boundaries and Controls

  • observed — Source-to-target promotion requires Running phase, a controlling VirtualMachine owner, a KubeVirt API-version prefix, a same-namespace target, and missing hotpluggable volume names. The target resourceVersion is tested, but the source owner's UID and source VMI freshness are not validated.
  • observed — The unchanged CSI authorization is namespace-scoped for VM/VMI reads and VM volume subresource updates. Its cluster-scoped permission is read access to persistent volumes. Disabling the fallback does not itself expand these privileges or the driver's credential configuration.

Resilience and Maintainability Implications

  • observed — The migration fails on API errors, cleans temporary snapshots on exit, and advances the version only after successful processing. The chart also prevents values from disabling its pinned declarative gate, reducing configuration drift that could remove the remaining hotplug path. These controls do not resolve source identity or concurrent detachment.

Hardening Proposals

  • proposed — Require ownerReference.uid to match the target VM UID before promoting attachments. Coordinate or quiesce VMI volume operations during backfill, or use a reconciliation protocol that detects source changes and fails closed. Exercise stale-owner, concurrent-detach, partial-progress, and stamp-failure recovery scenarios before relying on the transition for tenant storage lifecycle guarantees.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. (10 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: replacing the deprecated HotplugVolumes gate with declarative volume hotplug.
Linked Issues check ✅ Passed Issue #4032 requires kubevirt-csi to stop using VMI-level removevolume before the HotplugVolumes gate is removed. The CSI Deployment now sets --enable-vmi-hotplug-fallback=false, and the driver passes…
Out of Scope Changes check ✅ Passed The migration, its fixtures and tests, the KubeVirt feature-gate changes, and the CSI fallback flag all support issue #4032. The migration also preserves user-created VMI-level hotplugs that declarati…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. (10 skipped: 10 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size/M This PR changes 30-99 lines, ignoring generated files area/virtualization Issues or PRs related to virtualization (kubevirt, cdi, vmi, vm-import) kind/breaking-change Indicates the change introduces a breaking API or behaviour change kind/feature Categorizes issue or PR as related to a new feature labels Sep 24, 2026
@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the feat/kubevirt-declarative-hotplug branch from 1204f62 to 94968e3 Compare September 24, 2026 18:07
@github-actions github-actions Bot added size/L This PR changes 100-499 lines, ignoring generated files and removed size/M This PR changes 30-99 lines, ignoring generated files labels Sep 24, 2026
@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the feat/kubevirt-declarative-hotplug branch 2 times, most recently from 45618b0 to ac6b98b Compare September 24, 2026 18:19
@github-actions github-actions Bot added size/XL This PR changes 500-999 lines, ignoring generated files and removed size/L This PR changes 100-499 lines, ignoring generated files labels Sep 24, 2026

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration itself checks out. Predicate 59:495 matches KubeVirt v1.9.0 IsDeclarativeHotplugVolume exactly, the copy set is computed from live VM.spec so a re-run or a rebase re-run finds nothing to do, reads go to the API server rather than an informer, and every patch carries a resourceVersion test so a VM that changes between list and patch fails the patch instead of clobbering it. A stopped VM has nothing to copy. The gate flip lines up with the vendored csi driver on both attach and detach: virt-api still accepts the VM-level addvolume/removevolume the driver uses, and after migration 59 no VM-owned VMI has a VMI-only volume left for the two upgrade-order windows to disagree about, so a kubevirt-versus-tenant HR skew produces transient CSI retries, not lost volumes. The red E2E (in-tree) on the head is not this change: grafana-deployment is crashlooping on a 404 from the sandbox plugin mirror, while the KubeVirt, CDI and vm-disk HelmReleases this PR touches all reported UpgradeSucceeded and the install reached 98/98.

What I want before merge is one disclosure. This is a feat!, and there is a behavior change it does not state. A hotplug attached with virtctl addvolume --persist=false on a VM-owned VMI carries the same hotpluggable: true and is indistinguishable in the VMI, so migration 59 writes it into VM.spec as a permanent disk, and the VMI-level ephemeral path stops working once the gate is off. Copying is safer than unplugging it live, so the code is right; the gap is that neither the migration header nor a breaking-change note tells an operator their scratch volume just became persistent and can now only be removed through the VM. A sentence in each closes it.

Two smaller notes, neither blocking. A tenant whose kubernetes HelmRelease is suspended or failed keeps its old csi driver, and a driver predating upstream csi-driver #130 (attach moved to the VM subresource in March 2025) has every attach rejected after the flip, so that is worth a line in the breaking-change note too. And the fail-closed hook is a single list followed by conflict-tested patches, so a busy cluster leans on the Job backoffLimit: 3 to get through. Because this flips a gate on running VMs and the attach/detach data-safety is only verified statically here, I would run an N-1 upgrade through cozystack-pr-test before merging.

| ($vols | map(.name)) as $have
| ($disks | map(.name)) as $haveDisks
| [.spec.volumes[]?
| select(.dataVolume.hotpluggable == true or .persistentVolumeClaim.hotpluggable == true)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MINOR] feat! does not disclose that ephemeral (--persist=false) VMI hotplugs become persistent and VMI-level ephemeral hotplug stops working

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a paragraph to the migration header. It says the copy makes such a volume permanent, and that with the gate off virt-api refuses VMI-level addvolume and removevolume on a VM-owned VMI, so the volume is removed through the VM from then on. The release note says the same now.

@lexfrei

Copy link
Copy Markdown
Contributor Author

IvanHunters On the other notes:

The suspended or failed tenant case is in the body and the release note now. A driver from before July 2025 attaches through the VMI, so such a tenant can't attach anything until its release reconciles.

I left the hook as it is. Each Job retry lists again and skips the VMs already patched, so it fails only if a conflict hits on every attempt. Re-reading just the conflicting VM is listed in the body as a possible improvement.

I haven't run the N-1 upgrade. It can't be started from this repo's workflows, and #4463 tracks adding that test. Whether to merge before it exists is up to you.

…otplug

Dropping the HotplugVolumes gate lets virt-controller rebuild each
running VMI's hotplug volumes from VM.spec, and it removes every
hotpluggable VMI volume the VM spec does not list. kubevirt-csi
attached tenant volumes at the VMI level until mid-2025 and live
migration carries such a VMI across KubeVirt updates, so a tenant node
that was never recreated can still hold a mounted disk only its VMI
knows about; the first reconcile after the upgrade would unplug it.

Migration 59 writes each such volume and its disk into the VM template
as they are on the VMI, before the upgrade re-points cozystack-packages
and so before the KubeVirt CR and the kubevirt-csi flag change. Hotplug
volume changes are a valid live update, so no VM needs a restart.

An ephemeral hotplug a user made at the VMI level, such as virtctl
addvolume without --persist, has the same shape, because virt-api gives
every VMI-level addvolume that shape. It is copied too and becomes
permanent: leaving it out would not keep it ephemeral, since the first
reconcile would unplug it. Only Running VMIs are considered, matching
virt-controller, which reconciles no other; a stopped VM's next start
builds its VMI from the VM spec, so its VMI-only volumes are not
resurrected.

Only the variants with migrations enabled run it. The default variant
renders no migration hook, so a cluster there that installed KubeVirt
by hand has to check for such volumes itself before upgrading.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <[email protected]>
KubeVirt 1.9 deprecates HotplugVolumes in favour of
DeclarativeHotplugVolumes, which is Beta and therefore on by default,
but virt-controller skips declarative reconciliation for as long as the
old gate is on. Keeping it was only needed because kubevirt-csi
detached through the VMI-level removevolume, which virt-api accepts on
a VM-owned VMI only with the old gate. kubevirt-csi can now skip that
call, so its fallback goes off together with the gate. The two live in
different HelmReleases and roll out one after the other; in between the
driver may issue a VMI-level call virt-api rejects, and the unpublish
fails and is retried until both sides agree.

The chart now renders --enable-vmi-hotplug-fallback=false, which a
kubevirt-csi image built before the flag existed rejects at startup.
The driver image is stamped at release preparation, so this needs the
rebuilt image that ships in the same release.

DeclarativeHotplugVolumes is now listed explicitly. With HotplugVolumes
gone it is the only hotplug path, and disabling it through
disabledFeatureGates would make virt-api refuse every addvolume, so
the chart refuses that render instead.

This changes volume hotplug platform-wide. A VM-level addvolume or
removevolume now edits VM.spec directly and virt-controller reconciles
the VMI hotplug volumes against it, instead of going through
VM.status.volumeRequests. On the first reconcile it also removes every
hotpluggable VMI volume missing from VM.spec; platform migration 59
copies those into VM.spec beforehand on the variants that run
migrations.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <[email protected]>
@lexfrei

Copy link
Copy Markdown
Contributor Author

IvanHunters I added one more case to the body and the release note. A driver from before July 2025 on a suspended or failing tenant can still attach a volume at the VMI level after migration 59 has run and before the gate flips. The migration never sees that volume, so the first declarative reconcile unplugs it from the running VM. The code is the same, the branch is only rebased onto the updated #4451.

@lexfrei

Copy link
Copy Markdown
Contributor Author

IvanHunters the ephemeral-hotplug disclosure is in the migration 59 header and in the release note at 8dbc7bc, and so is the suspended-tenant driver case. The N-1 upgrade test is tracked in #4463. Could you take another look?

IvanHunters
IvanHunters previously approved these changes Oct 2, 2026

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict

LGTM with non-blocking notes

The migration I asked for last round is disclosed and the copy path verifies clean; the notes below do not block.

Operational risks

The migration and the gate flip hold up. I re-ran the checks rather than re-reading them: the migration's jq against its own fixtures copies exactly the VMI-only hotplugged volumes into VM.spec and no more, keeps every volume and disk the VM already lists, de-dups a disk the VM already names, and leaves orphan, non-controller-owned, stopped and non-Running VMIs alone. The 7 bats cases pass, and dropping the new Running-phase guard reddens them, so the guard is actually held. helm unittest is green on both charts (kubevirt 34, kubernetes 317), the rendered CR carries DeclarativeHotplugVolumes with HotplugVolumes gone, and the only non-comment change since my last round is that Running guard, which is a safety improvement. CI is green on the head, E2E included.

The disclosure I asked for last round is here: the migration header and the release note both state that an ephemeral VMI-level hotplug (virtctl addvolume --persist=false) is copied into the VM and becomes permanent, and that with the gate off virt-api refuses VMI-level addvolume/removevolume on a VM-owned VMI, so such a volume is managed through the VM from then on. That closes it.

A few non-blocking notes, none of which hold up the merge:

The migration aborts the whole run on the first resourceVersion conflict, leaning on the Job backoffLimit: 3. On a busy fleet a VM that changes between the list and its patch fails the run; the re-run is correct and idempotent, but re-reading just the conflicting VM would avoid the dependence on retries. You flagged this as a possible improvement already.

The rollout skew window is real and disclosed: a tenant whose kubernetes release is suspended or failing keeps an old VMI-level driver, and a volume it attaches between the migration's VMI read and the gate flip is not copied and gets unplugged on the first declarative reconcile. It is in the breaking-change note now, which is the right place for it.

One thing worth a line in the PR body or a backport label: the chart now renders --enable-vmi-hotplug-fallback=false unconditionally, but the pinned kubevirt-csi-driver:v1.6.0 image predates the flag (it is only in #4451), and Go's flag package exits on an unknown flag. The release path rebuilds the image so this is the same documented lag as migrations.image, but a backport of this chart change to a 1.6.x line without #4451's driver commit would crash-loop every tenant's CSI controller.

The one real residual risk is that an N-1 upgrade carrying a live VMI-only volume has never run end to end; #4463 tracks adding that lane. The data-safety path is covered statically and fail-closed, so I am comfortable with LGTM, but I would still run it through cozystack-pr-test before merge if you can. Merge ordering: this is stacked on #4451 and merges after it.

@lexfrei Aleksei Sviridkin (lexfrei) added this to the v1.7.0 milestone Oct 3, 2026
Base automatically changed from feat/kubevirt-csi-vmi-hotplug-fallback-flag to main October 5, 2026 07:46
@IvanHunters
IvanHunters dismissed their stale review October 5, 2026 07:46

The base branch was changed.

IvanHunters added a commit that referenced this pull request Oct 5, 2026
)

<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `kind/backport` if it's a bugfix that needs to be
backported to a previous version.
-->

## What this PR does

This lets the kubevirt-csi controller detach a volume from a VM instance
that no VM owns. Right now that call is always denied. Closes #4452.

The driver has a fallback for a volume that is still attached to the VMI
but is no longer in the VM spec. It calls the VMI-level `removevolume`
subresource. The `-kcsi` Role grants only the two VM-level subresources,
so the call ends in 403. Nothing else reaches this service account with
that grant. Only its own Role and ClusterRole name it as a subject, and
the group bindings that cover every service account grant nothing on
this subresource. At v1.9.0, `kubevirt.io:default` grants only
`kubevirts`, `version` and `guestfs`. `kubevirt.io:edit` and
`kubevirt.io:admin` do include the VMI subresource, but they aggregate
only into the built-in `edit` and `admin` roles, and no tenant binding
gives those to this account. The cozystack tenant roles aggregate on
their own labels and don't list it.

The fallback is still needed after the flag in #4453 goes to false. The
flag only skips VMIs that a VM owns. When a VM is deleted with
`propagationPolicy=Orphan`, the KubeVirt VM controller drops its
finalizer and leaves the VMI running (`vm.go` at v1.9.0). The tenant
node stays up with its volumes attached. When a pod moves off that node,
the driver has to detach through the VMI, and on a 403 the hot-plug pod
keeps holding the infra volume. Until #4453 lands, the fallback also
runs for VM-owned VMIs and gets the same 403.

The grant is `update` on `virtualmachineinstances/removevolume` and
nothing more. The account can already remove a volume from any VM in the
namespace, so it gets no new reach. virt-api also rejects the VMI-level
call on a VM-owned VMI once `HotplugVolumes` is off.
`virtualmachineinstances/addvolume` stays out because the driver only
attaches through the VM.

The new helm-unittest pins the Role's `subresources.kubevirt.io` rule to
these three subresources with `update`. I haven't reproduced the 403 on
a cluster. This shows it on a tenant namespace, before and after:

```bash
kubectl auth can-i update virtualmachineinstances.subresources.kubevirt.io --subresource=removevolume \
  --namespace <tenant-ns> --as system:serviceaccount:<tenant-ns>:<release>-kcsi
```

Upstream kubevirt/csi-driver has the same gap in
`deploy/infra-cluster-service-account.yaml`.

### Screenshots

<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->

No UI change.

### Downstream repositories

<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.

Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.

IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:

1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
- [ ] [cozystack/community](https://github.com/cozystack/community) -
follow-up:

### Release note

<!--  Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->

```release-note
fix(kubernetes): the kubevirt-csi controller can now hot-unplug a volume from a VMI that no VM owns, instead of failing with 403 Forbidden.
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Improved Kubernetes CSI volume removal for virtual machine instances,
including cases where a volume is not listed in the virtual machine
specification. This helps the driver complete volume removal in
scenarios where the Kubernetes virtualization API accepts the
instance-level request. Existing permissions for adding and removing
volumes from virtual machines remain unchanged.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed after the rebase onto main. This addresses everything from the last round: the migration header, the PR body, the release-note block and the kubevirt CR comment now all state that an ephemeral virtctl addvolume --persist=false hotplug on a VM-owned VMI becomes permanent after the migration and that VMI-level ephemeral hotplug stops working once the gate is off, and the suspended-tenant / pre-July-2025 driver skew and the disabledFeatureGates render-fail are documented too.

The migration holds up on a fresh look. Its predicate (dataVolume.hotpluggable or persistentVolumeClaim.hotpluggable) is byte-for-byte the removal predicate IsDeclarativeHotplugVolume in KubeVirt v1.9.0's filterHotplugVMIVolumes, the virt-api refusal of VMI-level add/removevolume on a VM-owned VMI is gated on HotplugVolumesEnabled() so it activates exactly when the gate drops, and it runs as a pre-upgrade hook so the copy lands before the flip reaches the cluster. The rebase stamping is clean (merge base at 58, this adds 59 and bumps the target to 60). I ran the migration bats here: 7/7, including the idempotent second run and all four fail-closed abort paths, and the check-runs on the head are green.

Two non-blocking heads-ups. Enabling declarative hotplug means VM.spec.volumes now drives the running VMI, so a vm-instance whose hotplug volume was added out of band (not in .Values.disks) can have it detached live on the next chart reconcile, where before the VMI was left alone; this does not touch CAPK tenant nodes, but it is worth a line next to the ephemeral-hotplug note. And the migration is one list followed by sequential resourceVersion-tested patches, so on a large fleet under status churn it leans on the Job backoffLimit: 3 to get through.

Because the live attach/detach across the flip is only verified statically here, an N-1 upgrade through cozystack-pr-test (the #4463 check) before merge would be the thing that turns this from a careful read into a tested one. Not gating it on that.

@lexfrei

Copy link
Copy Markdown
Contributor Author

Thanks. I filed both notes as issues: #4769 for hotplug volumes outside .Values.disks on vm-instance, and #4770 for how migration 59 handles conflicts. The N-1 upgrade check was already tracked in #4463, and I added a note there that this PR went in without it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/virtualization Issues or PRs related to virtualization (kubevirt, cdi, vmi, vm-import) kind/breaking-change Indicates the change introduces a breaking API or behaviour change kind/feature Categorizes issue or PR as related to a new feature size/XL This PR changes 500-999 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kubevirt-csi depends on the deprecated HotplugVolumes gate for VMI-level detach

2 participants