[andrijapanicsb]

Description

This PR fixes the device ID of the ROOT volume when an existing KVM disk is adopted with importVm and importsource=shared or importsource=local.

The shared/local KVM import allocates the root volume at device ID 0, then overwrites it with an explicit ID 1 during import finalization (long deviceId = 1L passed to updateImportedVolume()). Pass no explicit device ID so the existing root default remains 0, matching the other import paths (unmanaged/VMware-to-KVM via importDisk(), external KVM via importExternalDisk()).

• The production change is limited to importKvmVirtualMachineFromDisk(): the explicit 1L is removed and null is passed in both the shared and the local root-import calls. updateImportedVolume() already maps ROOT + null to device ID 0; no null device ID is stored.
• The shared/local path imports only the root disk, so no data-disk IDs change.
• Scope: future imports only. Existing VMs, their volumes and device IDs are not modified.

Tests:

• UnmanagedVMsManagerImplTest.importFromDisk(...), which is used by the four shared/local import tests, now verifies that updateImportedVolume() receives Volume.Type.ROOT and a null device ID.
  • The updateImportedVolume() stub added by StorPool support importing standalone disk volumes and importVm from SHARED #14389 now accepts a null device ID (nullable(Long.class)).
  • The import helpers use the stub's returned profile (profile.getVolumeId()), so a stub limited to anyLong() would make the tests fail with a NullPointerException instead of on the verification.
• VolumeOrchestratorTest gets two focused tests for updateImportedVolume(). They check the volume handed to VolumeDao.update():
  • ROOT with no device ID is saved with device ID 0.
  • An explicit data-disk device ID is kept.

Related: #14407 (device ID conflicts between data volumes, CD-ROMs and the config drive ISO) changes data-disk allocation in the other import paths. It is independent of this fix, and neither needs the other to be merged first.

Fixes: #13771

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

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

How Has This Been Tested?

Unit tests:

• Before the fix, with the updated tests only, on 4.22 at c4748669bf: UnmanagedVMsManagerImplTest ran 93 tests with 4 failures and 0 errors.
  • The failures were exactly importFromLocalDisk, importFromsharedStorage, importFromsharedStorageOnRbdPool and importFromsharedStorageOnZoneWidePool.
  • Each failed on the verification: updateImportedVolume(ROOT, ..., 1L, ...) was called where isNull() was expected.
• After the fix, on 4.22 at c4748669bf, and again after rebasing onto 4.22 at 30ed411cff:
  • UnmanagedVMsManagerImplTest: 93 tests, 0 failures.
  • VolumeOrchestratorTest: 38 tests, 0 failures, including the two new tests.
  • Checkstyle: 0 violations.

Combination with main and #14407:

• Applied locally to main (1abb539242) together with the server,kvm: Avoid device id conflicts between data volumes, CD-ROMs and the config drive ISO #14407 head (de91dd4417), using the main-branch variant of the test change (main has no StorPool support importing standalone disk volumes and importVm from SHARED #14389 stub yet).
• Ran UnmanagedVMsManagerImplTest, VolumeOrchestratorTest, VolumeApiServiceImplTest, TemplateManagerImplTest, KVMGuruTest and LibvirtVMDefTest.
• Results, all 0 failures and 0 errors:
  • UnmanagedVMsManagerImplTest: 128
  • VolumeOrchestratorTest: 49
  • VolumeApiServiceImplTest: 165
  • TemplateManagerImplTest: 74
  • KVMGuruTest: 33
  • LibvirtVMDefTest: 29
• Included in those runs: server,kvm: Avoid device id conflicts between data volumes, CD-ROMs and the config drive ISO #14407's getReservedDeviceIdsAlwaysReservesTheCdromOfConfigDrive and getNextDeviceIdSkipsReservedDeviceIds, the four shared/local import tests with the stricter verification, and the two new VolumeOrchestratorTest tests.

Forward merge to main:

• UnmanagedVMsManagerImplTest merges without conflicts and brings the nullable(Long.class) stub along with the stricter verification.
• UnmanagedVMsManagerImpl has one conflict in importKvmVirtualMachineFromDisk(). It already exists between 4.22 and main without this change (StorPool support importing standalone disk volumes and importVm from SHARED #14389's refactor) and is not at the changed lines.
• VolumeOrchestratorTest may conflict in the import list only (main has extra imports); keep both sides.

Not tested: live imports. No guest boot or libvirt XML checks have been run yet.

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

• Checked that the tests fail on the old behaviour for the right reason: on the device-ID argument, not with a fixture error.
• Checked that the explicit device-ID path (data disks) is unchanged.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 18.26%. Comparing base (30ed411) to head (5eecd77).

Additional details and impacted files

@@             Coverage Diff              @@
##               4.22   #14496      +/-   ##
============================================
- Coverage     18.26%   18.26%   -0.01%     
  Complexity    16608    16608              
============================================
  Files          5939     5939              
  Lines        537697   537694       -3     
  Branches      65922    65922              
============================================
- Hits          98210    98205       -5     
+ Misses       428299   428297       -2     
- Partials      11188    11192       +4     

Flag	Coverage Δ
uitests	4.08% <ø> (ø)
unittests	19.35% <ø> (-0.01%)	⬇️

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

☔ View full report in Codecov by Harness.
📢 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.

[andrijapanicsb]

@blueorangutan package

[blueorangutan]

@andrijapanicsb 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 19728

[DaanHoogland]

@andrijapanicsb is your home-bot running tests as well?

[andrijapanicsb]

@andrijapanicsb is your home-bot running tests as well?

Not yet @DaanHoogland I still have some minor bugs, but otherwise 99.5% ready, yes.

At the moment I'm busy bringing openVDDK(.com) into an open source foundation, so people can have VDDK back ("clean-room" implentation, with much better performance etc) - so it might become part of "dnf install ....." :)

[andrijapanicsb]

@ACSHomeBot package G

[ACSHomeBot]

Accepted: package kvm G for 5eecd7740f7d.

Planned: RPM + DEB, KVM SystemVM.

Status: Completed. Packaging results.

[ACSHomeBot]

Packaging results:

Result	Artifact	Platform	Build Time
PASS	RPM	EL (EL8/9/10)	32 min
PASS	DEB	Ubuntu, Debian	20 min

Test packages are available at:

• EL9/RPM: https://packages.v2kmigrate.com/cloudstack/pr/14496/el9/
• Debian/APT: https://packages.v2kmigrate.com/cloudstack/pr/14496/debian/

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-17275)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 53118 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14496-t17275-kvm-ol8.zip
Smoke tests completed. 149 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[DaanHoogland]

clgtm and makes functional sense.

[DaanHoogland]

@weizhouapache , can you think of any edge casses that might be touched?

[weizhouapache]

@weizhouapache , can you think of any edge casses that might be touched?

changes make sense

when deviceId is null, it is set in updateImportedVolume method as below:

        if (deviceId != null) {
            vol.setDeviceId(deviceId);
        } else if (type.equals(Type.ROOT)) {
            vol.setDeviceId(0l);
        } else {
            vol.setDeviceId(1l);
        }