CLVM/ISO: Ensure volume is created on the same host as the VM being deployed on - #13748
CLVM/ISO: Ensure volume is created on the same host as the VM being deployed on#13748Pearl1594 wants to merge 1 commit into
Conversation
86fa58e to
734640d
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses CLVM/CLVM-NG deployment failures for VMs deployed from ISO by attempting to ensure the newly created (templateless) volume is created/locked on the same host that will start the VM, avoiding LV exclusive-activation conflicts across hosts.
Changes:
- Add CLVM pool detection in the
templateId == null(templateless/ISO-style) volume creation path. - Set
destinationHostIdand persist a CLVM lock-host hint prior tocreateVolumeAsync(...)for that path.
Comments suppressed due to low confidence (1)
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:2033
- The CLVM host pinning here uses
vm.getVirtualMachine().getHostId(), which may be null or reflect a previous host during deployment/recreate flows. In this method you already have the planned deploy destination (DeployDestination dest) and usedest.getHost()elsewhere; the CLVM lock should be based on the destination host to ensure the LV is activated on the same host that will start the VM (the issue being fixed).
StoragePoolVO poolVO = _storagePoolDao.findById(destPool.getId());
if (poolVO != null && ClvmPoolManager.isClvmPoolType(poolVO.getPoolType())) {
Long hostId = vm.getVirtualMachine().getHostId();
if (hostId != null) {
volume.setDestinationHostId(hostId);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // CLVM: pin templateless volume creation to the deploy host | ||
| StoragePoolVO poolVO = _storagePoolDao.findById(destPool.getId()); | ||
| if (poolVO != null && ClvmPoolManager.isClvmPoolType(poolVO.getPoolType())) { | ||
| Long hostId = vm.getVirtualMachine().getHostId(); | ||
| if (hostId != null) { |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13748 +/- ##
============================================
- Coverage 19.65% 19.64% -0.01%
+ Complexity 19792 19790 -2
============================================
Files 6368 6368
Lines 574881 574889 +8
Branches 70351 70353 +2
============================================
- Hits 112970 112963 -7
- Misses 449639 449655 +16
+ Partials 12272 12271 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.50% |
| Branch coverage | 18.67% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18712 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16658)
|
RosiKyu
left a comment
RosiKyu
left a comment
There was a problem hiding this comment.
LGTM
Tested on KVM against both CLVM and CLVM-NG pools. Started on a 2 host setup, then extended the cluster to 4 hosts to check placement at scale, and also ran a host failure (HA) test. Everything worked as expected
What was tested (all passed):
- Deploy a VM from an ISO onto a CLVM pool and onto a CLVM-NG pool. The new disk gets a lock host record that matches the host the VM actually starts on, and the disk is only active on that one host.
- Checked both directions (VM landing on host A vs host B) and by pinning the VM to a chosen host. The lock host record always matched.
- Deploy the VM stopped first, then start it later. The fix still kicks in at start time and pins the disk to the correct host.
- Deploy from a template (not ISO) onto CLVM and CLVM-NG. Still sets the lock host correctly.
- Deploy onto a non-CLVM (NFS) pool. Correctly does nothing extra and writes no lock host record.
- Stop and start a CLVM VM. Comes back on the same host, disk active only there.
- Live migrate a CLVM VM between hosts. The lock moves to the new host and the disk follows.
- Attach a separate CLVM data disk to a running VM, reinstall a VM from ISO, and move a disk between CLVM-NG pools. All kept the lock on the correct host.
- Ran 16 deployments at the same time across all 4 hosts (mix of CLVM and CLVM-NG). Every one had its lock host record match its running host, and no disk was ever active on two hosts at once.
- Put a host into maintenance so its CLVM VMs live migrated off. All VMs moved and their locks transferred to the new hosts.
- Host failure test: hard crashed a host that was running an HA enabled CLVM VM. The disk was never active on two hosts at any point. The VM was kept down until the lock could be taken safely, which is the correct behaviour, and it started cleanly again once the failed host recovered.
Notes (not blockers, not caused by this PR):
- For plain CLVM (not CLVM-NG), the lock host record can lag the actual disk move by a few seconds during a host evacuation. It catches up on its own and the disk is always safe in the meantime, so it is cosmetic.
- The lock host records for deleted disks are not cleaned up when a VM is expunged. This already happened before this PR, it just adds a few more.
One review note:
- The fix reads the host id from the VM. In every case I tested that value was correct, but since the code only acts when that id is set, using the deploy destination host directly (as Copilot suggested) would remove any chance of it being empty. Worth taking either way.
|
cool, thanks @RosiKyu |
Description
This PR fixes: #13747
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?