feat: add CD-ROM hotplug volumes by wheatdog · Pull Request #9937 · harvester/harvester

wheatdog · 2026-01-23T09:18:26Z

Problem:

To support inserting and ejecting CD-ROM volumes without a restart.

Solution:

According to the announcement, a new feature gate "DeclarativeHotplugVolumes" is introduced at KubeVirt 1.6 and the existing "HotplugVolumes" is going to be deprecated.

Migrate the feature gate from "HotplugVolumes" to "DeclarativeHotplugVolumes"
Add 2 new VM actions:
1. injectCdRomVolume: add a new entry to volumes from VM spec and volumeClaimTemplates from VM annotation
2. ejectCdRomVolume: remove the corresponding element from volumes and volumeClaimTemplates from VM. Then, delete the corresponding volume (pvc).
Remove the existing VM action ejectCdRom and related checks -> prefer the new action ejectCdRomVolume that doesn't require a restart.
~~VM mutator webhook would patch hotpluggable to true for volumes of SATA CD-ROM devices to enable hotplug volume automatically.~~ -> For flexibility, this is handled from FE.
~~For existing VMs with CD-ROM, the hotpluggable field would be patched when the VM is stopped / restarted. This is handled by vmControllerSetSataCdRomHotpluggable.~~ -> Old VMs would act as is: see docs(hep): CD-ROM Hotplug Volumes [skip ci] #9840 (comment)
Relax the live migration constrain for VM with CD-ROM devices and container disks.
Use kubernetes.Interface instead of kubernetes.Clientset for type annotations since it's an interface, which is easier to be mocked in unit tests.

Related Issue(s):

#9261
#9779

Test plan:

Feature Gate Verification: After enabling the DeclarativeHotplugVolumes feature gate, perform basic checks to ensure existing volume hot-plug functionalities are not regressed.
Volume Insertion: For a running VM with an empty CD-ROM device, verify that a new image can be successfully inserted and that the content is accessible from within the guest OS.
Volume Ejection: For a running VM with an occupied CD-ROM device, verify that the volume can be successfully ejected and is no longer accessible from within the guest OS. Additionally, the corresponding volume is deleted.
Live Migration with CD-ROM: Verify that a running VM with a CD-ROM can be successfully live-migrated.
Existing VM Compatibility: For a VM created before this enhancement, verify that after a restart, the CD-ROM volume becomes ejectable.

Additionally, please refer to the test plan in HEP: #9840

Additional documentation or context

https://kubevirt.io/user-guide/storage/hotplug_volumes/#inject-cd-rom

mergify · 2026-01-27T10:28:46Z

This pull request is now in conflict. Could you fix it @wheatdog? 🙏

Signed-off-by: Tim Liou <tim.liou@suse.com>

ibrokethecloud

lgtm. thanks.

ibrokethecloud · 2026-02-03T22:54:17Z

pkg/util/virtualmachine/virtualmachine.go

+}
+
+func SupportEjectCdRomVolume(vm *kubevirtv1.VirtualMachine) (bool, error) {
+	volumeMaps := map[string]struct{}{}


same as above

Suggested change

volumeMaps := map[string]struct{}{}

if runtime.GOARCH == "arm64" {

return false, nil

}

volumeMaps := map[string]struct{}{}

ibrokethecloud · 2026-02-03T22:59:44Z

pkg/api/vm/handler.go

 }

-func (h *vmActionHandler) ejectCdRom(ctx context.Context, name, namespace string, diskNames []string) error {
+func getSataCdRomDiskPos(vm *kubevirtv1.VirtualMachine, deviceName string) (int, error) {


I am a bit confused by this check and why we need to return device position

based on my reading https://kubevirt.io/user-guide/storage/hotplug_volumes/ as long as there is a cdrom available in the vm.spec.template.spec.devices[*].disk of type cdrom with a sata bus, we just need to add a hot pluggable volume in vm.spec.template.volumes with the same name as disk found from vm.spec.template.spec.devices[*].disk.

If there is no volume already attached to the cdrom disk, then we can hot plug a disk or if there is one present we can unplug the disk.

Looking and using the position to insert the volume is redundant.

thanks, sorry I missed this comment. And yes, this turns out not just redundant but also problematic for cases like error out when inserting image to the last one of multiple empty cdrom. I've removed this and simplified the logic.

already addressed in 3528e33

wheatdog · 2026-02-06T07:59:15Z

pkg/api/vm/handler.go


 // addVolume add a hotplug volume with given volume source and disk name.
 // name -> VM name, namespace -> VM namespace, input.VolumeSourceName -> PVC name
 func (h *vmActionHandler) addVolume(ctx context.Context, namespace, name string, input AddVolumeInput) error {


I found a regression to "Add Volume". The API return errors but the volume is actually attached successfully.

failed to update vm default/hp, error: Operation cannot be fulfilled on virtualmachines.kubevirt.io "hp": the object has been modified; please apply your changes to the latest version and try again

Screen.Recording.2026-02-06.at.3.51.23.PM.mov

It seems that "Detach Volume" is fine.

I'll take a look.

This error comes from

harvester/pkg/api/vm/handler.go

Lines 957 to 965 in 5c83246

func (h *vmActionHandler) updateVMVolumeClaimTemplate(vm *kubevirtv1.VirtualMachine, updateVolumeClaimTemplate func([]corev1.PersistentVolumeClaim) ([]corev1.PersistentVolumeClaim, bool)) error {

var volumeClaimTemplates []corev1.PersistentVolumeClaim

vmCopy := vm.DeepCopy()

anno := vmCopy.GetAnnotations()

if volumeClaimTemplatesJSON, ok := anno[util.AnnotationVolumeClaimTemplates]; ok {

if err := json.Unmarshal([]byte(volumeClaimTemplatesJSON), &volumeClaimTemplates); err != nil {

return fmt.Errorf("failed to unserialize %s, error: %v", util.AnnotationVolumeClaimTemplates, err)

}

}

@w13915984028 @ibrokethecloud in 1d28d54, I've migrated the existing addVolume/removeVolume functions to the new declarative way such that update is called once to prevent the error.

Screen.Recording.2026-02-06.at.6.22.34.PM.mov

Signed-off-by: Tim Liou <tim.liou@suse.com>

mergify · 2026-02-10T03:37:12Z

This pull request is now in conflict. Could you fix it @wheatdog? 🙏

Signed-off-by: Tim Liou <tim.liou@suse.com>

w13915984028

A few last questions, thanks.

w13915984028 · 2026-02-11T07:53:00Z

pkg/api/vm/formatter.go

 }
+
+func canInsertCdRomVolume(vm *kubevirtv1.VirtualMachine) bool {
+	if runtime.GOARCH == "arm64" {


it is better if the arm64 is defined as a const for public usage

fixed by 2fdd817

w13915984028 · 2026-02-11T07:55:38Z

pkg/util/virtualmachine/virtualmachine.go

+			volumeMaps[volume.Name] = struct{}{}
+		}
+	}
+


L222 check len(volumeMaps) == 0 to return early

fixed by 5762271

w13915984028 · 2026-02-11T08:03:16Z

pkg/util/virtualmachine/virtualmachine.go

+			continue
+		}
+		useSata := isDiskSataCdRom(&disk)
+		if !useSata {


this function is used for both UI menu render and webhook check, there are some differences:

UI render does not care error, as long as the first target is found, it could return directly, e.g. L236 could trigger return directly; and L233 could also cause UI render failure

webhook/controller check all devices to ensure no error

could we split it into two functions for each purpose?

The idea is to separate 2 intentions:

Capability of insertion/ejection

Correctness of VM spec

Though, as offline discussed, I would like to stick with the current implementation for now. Thanks for the suggestion still!

I still kinda prefer having one function to handle both intentions since

They are quite related. To determine the capability of insertion/ejection, we also need to validate the correctness of the corresponding part of VM spec.

Their implementations are quite similar. They iterate volumes and disks in VM spec.

As for UI, it has its own logic canDoVolumeHotplugAction in UI PR instead of relying on VM action APIs.

w13915984028 · 2026-02-11T08:14:22Z

pkg/api/vm/handler.go

-	if !reflect.DeepEqual(vm, vmCopy) {
-		if _, err := h.vms.Update(vmCopy); err != nil {
-			return err
+	imgSize := max(vmImage.Status.VirtualSize, vmImage.Status.Size)


could we put L329~L366 to a new function and make insertCdRomVolume() shorter

I prefer keeping it as is. The most of the space it took is related to initialization of pvc and kubevirt volume, which is the main point of this function.

I have no strong options here, as long as CI does not complain.

Signed-off-by: Tim Liou <tim.liou@suse.com>

pkg/api/vm/formatter.go

Signed-off-by: Tim Liou <tim.liou@suse.com>

mergify · 2026-02-12T09:32:21Z

This pull request is now in conflict. Could you fix it @wheatdog? 🙏

w13915984028

LGTM, thanks.

BTW, please squash the commits manually or utlize the gihtub merge & cut off some commit messages to make the final commit message shorter.

mergify · 2026-02-13T07:00:28Z

This pull request is now in conflict. Could you fix it @wheatdog? 🙏

w13915984028

LGTM, thanks.

wheatdog changed the title ~~feat: add cdrom hotplug volumes~~ feat: add CD-ROM hotplug volumes Jan 23, 2026

wheatdog force-pushed the 9261-hp-cdrom branch from 7100a20 to 668dd26 Compare January 23, 2026 10:12

wheatdog marked this pull request as ready for review January 23, 2026 10:19

wheatdog requested review from a team, Vicente-Cheng, WebberHuang1118, Yu-Jack, ibrokethecloud, rrajendran17, starbops, votdev and w13915984028 as code owners January 23, 2026 10:19

github-actions bot assigned wheatdog Jan 23, 2026

wheatdog force-pushed the 9261-hp-cdrom branch 3 times, most recently from cea8ae0 to e30a0ec Compare January 26, 2026 06:16

wheatdog added 13 commits January 29, 2026 18:17

replace HotplugVolumes with DeclarativeHotplugVolumes

3905670

Signed-off-by: Tim Liou <tim.liou@suse.com>

feat: add actions to inject / eject cdrom volume

3787061

Signed-off-by: Tim Liou <tim.liou@suse.com>

fix: properly remove from annotation

8ef2934

Signed-off-by: Tim Liou <tim.liou@suse.com>

dev: fix insert

03000d2

Signed-off-by: Tim Liou <tim.liou@suse.com>

feat: patch hotpluggable in mutator

a51e0cd

Signed-off-by: Tim Liou <tim.liou@suse.com>

Set sata cdrom hotpluggable for existing VMs during restarts

af890b7

Signed-off-by: Tim Liou <tim.liou@suse.com>

handler: directly remove cdrom volume pvc upon ejection

dc8e486

Signed-off-by: Tim Liou <tim.liou@suse.com>

chores: rename injectCdRomVolume to insertCdRomVolume

9d20b90

Signed-off-by: Tim Liou <tim.liou@suse.com>

fix: relax live-migration constrain

eea8092

Signed-off-by: Tim Liou <tim.liou@suse.com>

feat: remove old EjectCdRom action

171b594

Signed-off-by: Tim Liou <tim.liou@suse.com>

fix: rename VolumeName to DeviceName

0ce316d

Signed-off-by: Tim Liou <tim.liou@suse.com>

refactor: setSataCdRomHotpluggable

e56383d

Signed-off-by: Tim Liou <tim.liou@suse.com>

test: patchSataCdRomHotpluggable

0442d62

Signed-off-by: Tim Liou <tim.liou@suse.com>

ibrokethecloud self-requested a review February 5, 2026 22:51

ibrokethecloud previously approved these changes Feb 5, 2026

View reviewed changes

wheatdog commented Feb 6, 2026

View reviewed changes

wheatdog added 2 commits February 6, 2026 16:25

fix: clean up leftover ejectCdRom

ee33112

Signed-off-by: Tim Liou <tim.liou@suse.com>

fix: declarative way of addVolume and removeVolume

1d28d54

Signed-off-by: Tim Liou <tim.liou@suse.com>

wheatdog dismissed ibrokethecloud’s stale review via 1d28d54 February 6, 2026 10:16

wheatdog mentioned this pull request Feb 9, 2026

feat: support cdrom hotplug volume harvester/terraform-provider-harvester#153

Merged

wheatdog added 2 commits February 10, 2026 15:54

Merge remote-tracking branch 'upstream/master' into 9261-hp-cdrom

b14e8d7

fix: make linter happy

48d4564

Signed-off-by: Tim Liou <tim.liou@suse.com>

w13915984028 reviewed Feb 11, 2026

View reviewed changes

refactor: use util.GoArchArm64 instead of "arm64"

2fdd817

Signed-off-by: Tim Liou <tim.liou@suse.com>

brandboat reviewed Feb 11, 2026

View reviewed changes

pkg/api/vm/formatter.go Outdated Show resolved Hide resolved

fix: shortcut on empty volumeMaps

5762271

Signed-off-by: Tim Liou <tim.liou@suse.com>

Merge remote-tracking branch 'upstream/master' into 9261-hp-cdrom

f449210

w13915984028 previously approved these changes Feb 12, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/master' into 9261-hp-cdrom

79f317c

ibrokethecloud previously approved these changes Feb 13, 2026

View reviewed changes

wheatdog dismissed stale reviews from ibrokethecloud and w13915984028 via 79f317c February 13, 2026 07:23

ibrokethecloud approved these changes Feb 13, 2026

View reviewed changes

w13915984028 approved these changes Feb 13, 2026

View reviewed changes

Vicente-Cheng approved these changes Feb 13, 2026

View reviewed changes

wheatdog merged commit ef1bcad into harvester:master Feb 13, 2026
12 checks passed

wheatdog deleted the 9261-hp-cdrom branch February 13, 2026 08:35

This was referenced Feb 13, 2026

[ENHANCEMENT] Relax the live migration feasibility check for VMs while using CD-ROM #9779

Open

[GUI] [FEATURE] Support CD-ROM hotpluggable inject/eject #9837

Open

[FEATURE] Support CD-ROM hotpluggable inject/eject #9261

Open

	func (h vmActionHandler) updateVMVolumeClaimTemplate(vm kubevirtv1.VirtualMachine, updateVolumeClaimTemplate func([]corev1.PersistentVolumeClaim) ([]corev1.PersistentVolumeClaim, bool)) error {
	var volumeClaimTemplates []corev1.PersistentVolumeClaim
	vmCopy := vm.DeepCopy()
	anno := vmCopy.GetAnnotations()
	if volumeClaimTemplatesJSON, ok := anno[util.AnnotationVolumeClaimTemplates]; ok {
	if err := json.Unmarshal([]byte(volumeClaimTemplatesJSON), &volumeClaimTemplates); err != nil {
	return fmt.Errorf("failed to unserialize %s, error: %v", util.AnnotationVolumeClaimTemplates, err)
	}
	}

Conversation

wheatdog commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem:

Solution:

Related Issue(s):

Test plan:

Additional documentation or context

Uh oh!

mergify bot commented Jan 27, 2026

Uh oh!

ibrokethecloud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wheatdog Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wheatdog Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wheatdog Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Feb 10, 2026

Uh oh!

w13915984028 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mergify bot commented Feb 12, 2026

Uh oh!

w13915984028 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Feb 13, 2026

Uh oh!

w13915984028 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

wheatdog commented Jan 23, 2026 •

edited

Loading

wheatdog Feb 5, 2026 •

edited

Loading

wheatdog Feb 6, 2026 •

edited

Loading

wheatdog Feb 6, 2026 •

edited

Loading