kvm: honour disk controller for vm during attach volume

[shwstppr]

Description

Fixes #12090

Some years back #4569 allowed honouring dataDiskController for KVM VMs during start but the same behviour is not seen while attaching a volume to a running VM.
This PR first checks in the AttachCommand if a valid controller is present. If a valid controller/bus type is not present than existing logic is used.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

[codecov]

Codecov Report

❌ Patch coverage is 0% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.23%. Comparing base (42f1e19) to head (3e908f8).
⚠️ Report is 54 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...ud/hypervisor/kvm/storage/KVMStorageProcessor.java	0.00%	22 Missing ⚠️
...om/cloud/hypervisor/kvm/resource/LibvirtVMDef.java	0.00%	6 Missing ⚠️
...ervisor/kvm/resource/LibvirtComputingResource.java	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.20   #12452   +/-   ##
=========================================
  Coverage     16.23%   16.23%           
- Complexity    13380    13381    +1     
=========================================
  Files          5657     5657           
  Lines        499039   499056   +17     
  Branches      60567    60569    +2     
=========================================
+ Hits          81024    81032    +8     
- Misses       408978   408986    +8     
- Partials       9037     9038    +1     

Flag	Coverage Δ
uitests	4.03% <ø> (ø)
unittests	17.09% <0.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[DaanHoogland]

clgtm

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16399

[shwstppr]

@blueorangutan package

[blueorangutan]

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16438

[kiranchavala]

LGTM

Followed the steps mentioned here

#12090 (comment)

After adding the sixth disk , it was correctly assigned scsi driver

    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2' cache='none'/>
      <source file='/mnt/9dde54d6-ebe4-3f4e-b185-c88bf7419bfd/bd33ea5a-df96-4449-9b12-f58c1ff4a9ac' index='6'/>
      <backingStore/>
      <target dev='sdb' bus='scsi'/>
      <serial>bd33ea5adf9644499b12</serial>
      <alias name='scsi0-0-0-1'/>
      <address type='drive' controller='0' bus='0' target='0' unit='1'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2' cache='none'/>
      <source file='/mnt/9dde54d6-ebe4-3f4e-b185-c88bf7419bfd/2837aa76-1adb-4d0b-b287-b8eabb8ece77' index='5'/>
      <backingStore/>
      <target dev='sdc' bus='scsi'/>
      <serial>2837aa761adb4d0bb287</serial>
      <alias name='scsi0-0-0-2'/>
      <address type='drive' controller='0' bus='0' target='0' unit='2'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2' cache='none'/>
      <source file='/mnt/9dde54d6-ebe4-3f4e-b185-c88bf7419bfd/a3ccfebd-b3f6-4a23-975f-6fc178cd54d4' index='4'/>
      <backingStore/>
      <target dev='sde' bus='scsi'/>
      <serial>a3ccfebdb3f64a23975f</serial>
      <alias name='scsi0-0-0-4'/>
      <address type='drive' controller='0' bus='0' target='0' unit='4'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2' cache='none'/>
      <source file='/mnt/734738cb-cf21-3b12-aa39-f007af1044ea/64f2bfa6-a7c3-4d44-a195-5eda2be1e59b' index='3'/>
      <backingStore/>
      <target dev='sdf' bus='scsi'/>
      <serial>64f2bfa6a7c34d44a195</serial>
      <alias name='scsi0-0-0-5'/>
      <address type='drive' controller='0' bus='0' target='0' unit='5'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2' cache='none'/>
      <source file='/mnt/9dde54d6-ebe4-3f4e-b185-c88bf7419bfd/9007dd03-6806-410c-8c1e-94ac0e2e4e28' index='2'/>
      <backingStore/>
      <target dev='sdg' bus='scsi'/>
      <serial>9007dd036806410c8c1e</serial>
      <alias name='scsi0-0-0-6'/>
      <address type='drive' controller='0' bus='0' target='0' unit='6'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2' cache='none' discard='unmap'/>
      <source file='/mnt/9dde54d6-ebe4-3f4e-b185-c88bf7419bfd/97b78eb6-7be1-4081-a52d-faa7c8056ee7' index='9'/>
      <backingStore/>
      <target dev='sdh' bus='scsi'/>
      <serial>97b78eb67be14081a52d</serial>
      <alias name='scsi1-0-0-0'/>
      <address type='drive' controller='1' bus='0' target='0' unit='0'/>
    </disk>

[DaanHoogland]

this is ready for merge, @shwstppr ?

[shwstppr]

@DaanHoogland I've marked it ready though I'm not completely sure if this would solve the original issue reported in #12090 but definitely solves Kiran's case

[DaanHoogland]

ok, will not close the issue (yet) but doesn’t hurt to merge than.