Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Automatic VM volume type (cd-rom/disk) based on image type only works for first disk #5142

Closed
tserong opened this issue Feb 14, 2024 · 3 comments
Assignees
Labels
kind/bug Issues that are defects reported by users or that we know have reached a real release severity/4 Function working but has a minor issue (a minor incident with low impact)
Milestone

Comments

@tserong
Copy link
Contributor

tserong commented Feb 14, 2024

Describe the bug
When creating a new VM, on the Volumes tab, if you select an ISO Image, the image Type is automatically set to cd-rom, and the Bus to SATA:

image

If I change the image to something else (not an ISO), the Type changes to disk and the Bus to VirtIO. This automatic update only applies to the first disk. If I add a second disk and select an ISO image, the type remains disk and the bus VirtIO:

image

I'm at a loss as to understand why we'd only want this automatic disk type and bus setting to happen for the first disk. It should apply for any disk.

(Apologies for missing highlighting the bus on the screenshots)

To Reproduce
Steps to reproduce the behavior:

  1. Go to Virtual Machines and click Create.
  2. Go to the Volumes tab and add two VM images.
  3. Observe that if you select an ISO or non-ISO image for the first disk, the type and bus update as expected.
  4. Observe that this type and bus update does not happen for the second disk.

Expected behavior
The type should change to cd-rom and the bus to SATA for any disk with an ISO image (not just the first disk)

Additional context
This should fix it:

--- a/pkg/harvester/edit/kubevirt.io.virtualmachine/VirtualMachineVolume/type/vmImage.vue
+++ b/pkg/harvester/edit/kubevirt.io.virtualmachine/VirtualMachineVolume/type/vmImage.vue
@@ -141,14 +141,12 @@ export default {
     onImageChange() {
       const imageResource = this.$store.getters['harvester/all'](HCI.IMAGE)?.find( I => this.value.image === I.id);
 
-      if (this.idx === 0) {
-        if (/iso$/i.test(imageResource?.imageSuffix)) {
-          this.$set(this.value, 'type', 'cd-rom');
-          this.$set(this.value, 'bus', 'sata');
-        } else {
-          this.$set(this.value, 'type', 'disk');
-          this.$set(this.value, 'bus', 'virtio');
-        }
+      if (/iso$/i.test(imageResource?.imageSuffix)) {
+        this.$set(this.value, 'type', 'cd-rom');
+        this.$set(this.value, 'bus', 'sata');
+      } else {
+        this.$set(this.value, 'type', 'disk');
+        this.$set(this.value, 'bus', 'virtio');
       }
 
       this.update();
@tserong tserong added kind/bug Issues that are defects reported by users or that we know have reached a real release reproduce/needed Reminder to add a reproduce label and to remove this one severity/needed Reminder to add a severity label and to remove this one severity/4 Function working but has a minor issue (a minor incident with low impact) and removed reproduce/needed Reminder to add a reproduce label and to remove this one severity/needed Reminder to add a severity label and to remove this one labels Feb 14, 2024
@tserong tserong self-assigned this Mar 20, 2024
tserong added a commit to harvester/dashboard that referenced this issue Mar 20, 2024
The onImageChange() function sets the VM volume type="cd-rom" and
bus="sata" when an ISO image is selected, but it only works for
the first volume.  This commit enables the automatic type and bus
selection for all volumes attached to a VM.

Related issue: harvester/harvester#5142

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to harvester/dashboard that referenced this issue Mar 27, 2024
The onImageChange() function sets the VM volume type="cd-rom" and
bus="sata" when an ISO image is selected, but it only works for
the first volume.  This commit enables the automatic type and bus
selection for all volumes attached to a VM.

Related issue: harvester/harvester#5142

Signed-off-by: Tim Serong <tserong@suse.com>
@bk201 bk201 added this to the v1.4.0 milestone May 9, 2024
tserong added a commit to harvester/dashboard that referenced this issue May 24, 2024
The onImageChange() function sets the VM volume type="cd-rom" and
bus="sata" when an ISO image is selected, but it only works for
the first volume.  This commit enables the automatic type and bus
selection for all volumes attached to a VM.

Related issue: harvester/harvester#5142

Signed-off-by: Tim Serong <tserong@suse.com>
@harvesterhci-io-github-bot
Copy link
Collaborator

harvesterhci-io-github-bot commented May 24, 2024

Pre Ready-For-Testing Checklist

  • [ ] If labeled: require/HEP Has the Harvester Enhancement Proposal PR submitted?
    The HEP PR is at:

  • Where is the reproduce steps/test steps documented?
    The reproduce steps/test steps are at: [BUG] Automatic VM volume type (cd-rom/disk) based on image type only works for first disk #5142 (comment) (i.e. in the description of this bug)

  • Is there a workaround for the issue? If so, where is it documented?
    The workaround is at: Manually set the disk type and bus

  • Have the backend code been merged (harvester, harvester-installer, etc) (including backport-needed/*)?
    The PR is at: Take image virtual size into account when creating VMs dashboard#976

    • Does the PR include the explanation for the fix or the feature?

    • [ ] Does the PR include deployment change (YAML/Chart)? If so, where are the PRs for both YAML file and Chart?
      The PR for the YAML change is at:
      The PR for the chart change is at:

  • [ ] If labeled: area/ui Has the UI issue filed or ready to be merged?
    The UI issue/PR is at:

  • [ ] If labeled: require/doc, require/knowledge-base Has the necessary document PR submitted or merged?
    The documentation/KB PR is at:

  • If NOT labeled: not-require/test-plan Has the e2e test plan been merged? Have QAs agreed on the automation test case? If only test case skeleton w/o implementation, have you created an implementation issue?

    • The automation skeleton PR is at:
    • The automation test case PR is at:
  • [ ] If the fix introduces the code for backward compatibility Has a separate issue been filed with the label release/obsolete-compatibility?
    The compatibility issue is filed at:

@harvesterhci-io-github-bot
Copy link
Collaborator

Automation e2e test issue: harvester/tests#1290

tserong added a commit to harvester/dashboard that referenced this issue Jul 4, 2024
The onImageChange() function sets the VM volume type="cd-rom" and
bus="sata" when an ISO image is selected, but it only works for
the first volume.  This commit enables the automatic type and bus
selection for all volumes attached to a VM.

Related issue: harvester/harvester#5142

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to harvester/dashboard that referenced this issue Jul 18, 2024
The onImageChange() function sets the VM volume type="cd-rom" and
bus="sata" when an ISO image is selected, but it only works for
the first volume.  This commit enables the automatic type and bus
selection for all volumes attached to a VM.

Related issue: harvester/harvester#5142

Signed-off-by: Tim Serong <tserong@suse.com>
mergify bot pushed a commit to harvester/dashboard that referenced this issue Aug 16, 2024
The onImageChange() function sets the VM volume type="cd-rom" and
bus="sata" when an ISO image is selected, but it only works for
the first volume.  This commit enables the automatic type and bus
selection for all volumes attached to a VM.

Related issue: harvester/harvester#5142

Signed-off-by: Tim Serong <tserong@suse.com>
(cherry picked from commit 90e6535)
a110605 pushed a commit to harvester/dashboard that referenced this issue Aug 16, 2024
The onImageChange() function sets the VM volume type="cd-rom" and
bus="sata" when an ISO image is selected, but it only works for
the first volume.  This commit enables the automatic type and bus
selection for all volumes attached to a VM.

Related issue: harvester/harvester#5142

Signed-off-by: Tim Serong <tserong@suse.com>
(cherry picked from commit 90e6535)
a110605 pushed a commit to a110605/harvester-dashboard that referenced this issue Aug 21, 2024
The onImageChange() function sets the VM volume type="cd-rom" and
bus="sata" when an ISO image is selected, but it only works for
the first volume.  This commit enables the automatic type and bus
selection for all volumes attached to a VM.

Related issue: harvester/harvester#5142

Signed-off-by: Tim Serong <tserong@suse.com>
@irishgordo irishgordo self-assigned this Sep 4, 2024
@irishgordo
Copy link
Contributor

Thanks for this fix @tserong 😄 👍
This looks great in Version: v1.4.0-dev-20240830
Screenshot from 2024-09-10 14-38-52

Opened up 6540 to track an un-expected behavior I saw w/ VM creation default disk-sizes.

So I'll go ahead and close this out 👍 😄

tserong added a commit to tserong/dashboard that referenced this issue Nov 29, 2024
The onImageChange() function sets the VM volume type="cd-rom" and
bus="sata" when an ISO image is selected, but it only works for
the first volume.  This commit enables the automatic type and bus
selection for all volumes attached to a VM.

Related issue: harvester/harvester#5142

Signed-off-by: Tim Serong <tserong@suse.com>
(cherry picked from commit 90e6535)
tserong added a commit to tserong/dashboard that referenced this issue Nov 29, 2024
The onImageChange() function sets the VM volume type="cd-rom" and
bus="sata" when an ISO image is selected, but it only works for
the first volume.  This commit enables the automatic type and bus
selection for all volumes attached to a VM.

Related issue: harvester/harvester#5142

Signed-off-by: Tim Serong <tserong@suse.com>
(cherry picked from commit 90e6535)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Issues that are defects reported by users or that we know have reached a real release severity/4 Function working but has a minor issue (a minor incident with low impact)
Projects
None yet
Development

No branches or pull requests

4 participants