Skip to content

feat: support cdrom hotplug volume#153

Merged
wheatdog merged 7 commits intoharvester:masterfrom
wheatdog:9261-tf
Feb 13, 2026
Merged

feat: support cdrom hotplug volume#153
wheatdog merged 7 commits intoharvester:masterfrom
wheatdog:9261-tf

Conversation

@wheatdog
Copy link
Member

@wheatdog wheatdog commented Feb 3, 2026

Problem:

To support cdrom hotplug volume: image insertion and ejection.

Solution:

  • properly handle empty cdrom

Related Issue(s):

harvester/harvester#9261

Test plan:

Check out integration test: TestAccVirtualMachine_hotplug_cdrom_volume.

Additional documentation or context

HEP: harvester/harvester#9840
BE: harvester/harvester#9937

Signed-off-by: Tim Liou <tim.liou@suse.com>
Signed-off-by: Tim Liou <tim.liou@suse.com>
@wheatdog wheatdog force-pushed the 9261-tf branch 2 times, most recently from cdb6c3c to 4ff7305 Compare February 9, 2026 04:09
@wheatdog wheatdog changed the title [WIP] feat: support cdrom hotplug volume feat: support cdrom hotplug volume Feb 9, 2026
Signed-off-by: Tim Liou <tim.liou@suse.com>
@wheatdog wheatdog marked this pull request as ready for review February 9, 2026 04:21
},
},
})
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Although we got the "Complex Method" notice from CodeFactor, it's intentional to have TestAccVirtualMachine_hotplug_cdrom_volume perform

  1. create VM with empty cdrom
  2. create a VM image and insert it into the VM
  3. eject the media out of the VM

Therefore, I think we can ignore this warning from CodeFactor.

Copy link
Member

@m-ildefons m-ildefons left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd prefer if the "Complex Method" warning got fixed.

Signed-off-by: Tim Liou <tim.liou@suse.com>
Signed-off-by: Tim Liou <tim.liou@suse.com>
Signed-off-by: Tim Liou <tim.liou@suse.com>
m-ildefons
m-ildefons previously approved these changes Feb 10, 2026
Signed-off-by: Tim Liou <tim.liou@suse.com>
@wheatdog
Copy link
Member Author

@m-ildefons sorry that I found that vmiUid in testAccCheckVmiUid should be passed as pointer such that it can get the updated vmiUid from the first step. Fixed by 76b1927

I tried it after the fix, it pass. Could you help me review it again? Thanks.

❯ git diff | cat
diff --git a/scripts/test b/scripts/test
index 43f883d..305bdfa 100755
--- a/scripts/test
+++ b/scripts/test
@@ -7,7 +7,7 @@ if [ -f ./kubeconfig_test.yaml ] ; then
   export KUBECONFIG="$(pwd)/kubeconfig_test.yaml"
   export TF_ACC=1
   # Avoid timeout after 10 minutes https://pkg.go.dev/cmd/go#hdr-Testing_flags
-  export EXTRA_OPTIONS=("-timeout" "0")
+  export EXTRA_OPTIONS=("-timeout" "0" "-run" "TestAccVirtualMachine_hotplug_cdrom_volume")
 fi

 echo Running tests:

❯ ./scripts/test
Running tests:
        github.com/harvester/terraform-provider-harvester               coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/config               coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider             coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider/bootstrap           coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider/cloudinitsecret             coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider/clusternetwork              coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider/image               coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider/ippool              coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider/keypair             coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider/loadbalancer                coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider/network             coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider/setting             coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider/storageclass                coverage: 0.0% of statements
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok      github.com/harvester/terraform-provider-harvester/internal/provider/virtualmachine      0.020s  coverage: 0.0% of statements [no tests to run]
        github.com/harvester/terraform-provider-harvester/internal/provider/vlanconfig          coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/internal/provider/volume              coverage: 0.0% of statements
=== RUN   TestAccVirtualMachine_hotplug_cdrom_volume
--- PASS: TestAccVirtualMachine_hotplug_cdrom_volume (107.30s)
PASS
coverage: [no statements]
ok      github.com/harvester/terraform-provider-harvester/internal/tests        107.330s        coverage: [no statements]
        github.com/harvester/terraform-provider-harvester/internal/util         coverage: 0.0% of statements
        github.com/harvester/terraform-provider-harvester/pkg/client            coverage: 0.0% of statements
?       github.com/harvester/terraform-provider-harvester/pkg/constants [no test files]
        github.com/harvester/terraform-provider-harvester/pkg/conversion                coverage: 0.0% of statements
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok      github.com/harvester/terraform-provider-harvester/pkg/helper    0.002s  coverage: 0.0% of statements [no tests to run]
testing: warning: no tests to run
PASS
coverage: 0.0% of statements
ok      github.com/harvester/terraform-provider-harvester/pkg/importer  0.021s  coverage: 0.0% of statements [no tests to run]

Copy link
Member

@brandboat brandboat left a comment

Choose a reason for hiding this comment

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

Overall LGTM, only have some questions.
Thanks for the thorough e2e test!

Comment on lines -276 to -278
if volume.Name != disk.Name {
continue
}
Copy link
Member

@brandboat brandboat Feb 11, 2026

Choose a reason for hiding this comment

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

So this fixed the missing hotplug disk issue, right? The original behavior is iterating over volumes, and skip disk if its name not match the volume name. That could be a problem since hotplug empty cd-rom is not defined in the volume section.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly! This change is for empty cdrom.

Comment on lines +77 to +83
if oldFirmware := vmBuilder.VirtualMachine.Spec.Template.Spec.Domain.Firmware; oldFirmware != nil {
if firmware == nil {
firmware = &kubevirtv1.Firmware{}
}
firmware.UUID = oldFirmware.UUID
firmware.Serial = oldFirmware.Serial
}
Copy link
Member

Choose a reason for hiding this comment

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

could you please explain a bit more about this? Is this related to cdrom hotplug?

Copy link
Member Author

@wheatdog wheatdog Feb 13, 2026

Choose a reason for hiding this comment

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

This change is important for hotplug actions (not just for the cdrom hotplug but also for the existing hotplug volume). Without this change, the updated VM will have different firmware serial and uuid, causing the RestartRequired condition.

Take this example using binary from master branch (hotplug volume),

❯ cat main.tf
resource "harvester_volume" "mount-disk" {
  name      = "mount-disk"
  namespace = "default"

  size = "1Gi"
}

resource "harvester_virtualmachine" "fedora" {
  name                 = "fedora"
  namespace            = "default"

  cpu    = 1
  memory = "2Gi"

  efi         = true
  secure_boot = true

  hostname        = "fedora"

  network_interface {
    name           = "nic-1"
    wait_for_lease = true
  }

  disk {
    name       = "rootdisk"
    type       = "disk"
    bus        = "virtio"
    boot_order = 1

    container_image_name = "kubevirt/fedora-cloud-container-disk-demo:v0.35.0"
  }

  # uncomment for the second terraform apply to do volume hotplug 
  # disk {
  #   name = "mount-disk"
  #   type = "disk"
  #   bus  = "scsi"
  #
  #   existing_volume_name = harvester_volume.mount-disk.name
  #   auto_delete          = false
  #   hot_plug             = true
  # }

}

❯ terraform apply

❯ kubectl get vm fedora -o yaml > /tmp/first.yaml

/* edit main.tf to uncomment for the second terraform apply to do volume hotplug  */

❯ terraform apply

❯ kubectl get vm fedora -o yaml > /tmp/second.yaml

❯ diff /tmp/first.yaml /tmp/second.yaml
6a7
>     harvesterhci.io/volumeClaimTemplates: '[]'
13c14
<   generation: 1
---
>   generation: 3
19c20
<   resourceVersion: "1157979"
---
>   resourceVersion: "1161276"
26c27
<         harvesterhci.io/sshNames: '[]'
---
>         harvesterhci.io/sshNames: "null"
55a57,59
>           - disk:
>               bus: scsi
>             name: mount-disk
69,70c73
<           serial: 78ff9029-d200-4ed7-972f-8222e81d3de4
<           uuid: 8c491cef-9ab6-4644-abe8-22876906a927
---
>           uuid: 98f07cdd-96da-5880-b6c7-1a5700b73dc4
92a96,99
>       - name: mount-disk
>         persistentVolumeClaim:
>           claimName: mount-disk
>           hotpluggable: true
106a114,118
>   - lastProbeTime: null
>     lastTransitionTime: "2026-02-13T00:42:15Z"
>     message: a non-live-updatable field was changed in the template spec
>     status: "True"
>     type: RestartRequired
108c120
<   desiredGeneration: 1
---
>   desiredGeneration: 3
116a129,130
>   - enabled: true
>     name: mount-disk

For cdrom hotplug, check the integration test.

This change fixed it by preserving the old firmware's uuid and serial.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments