andrijapanicsb opened a new pull request, #14496:
URL: https://github.com/apache/cloudstack/pull/14496
### 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 #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)
- [x] 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
- [x] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [x] 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 #14407 head
(`de91dd4417`), using the main-branch variant of the test change (main has no
#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: #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 (#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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
