Restore VMware-to-KVM migration changes from #13656 - #14256
andrijapanicsb wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14256 +/- ##
============================================
+ Coverage 19.91% 20.09% +0.18%
- Complexity 20194 20730 +536
============================================
Files 6373 6428 +55
Lines 577230 585089 +7859
Branches 70696 71684 +988
============================================
+ Hits 114942 117595 +2653
- Misses 449722 454613 +4891
- Partials 12566 12881 +315
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:
|
|
Packages build from the identical code: Packaging results:
Test packages are available at: |
|
ShapeBlue clean/successfull packaging pass - link: #13656 (comment) |
|
ShapeBlue BlueOrangutan (Marvin tests - all 156 passed with zero failures) - link: #13656 (comment) |
|
@alexandremattioli cant see you from the dropdown in "reviewers" so just pinging you this way, especially if you have any capacity for testing etc. |
|
@ACSHomeBot package G |
Packaging results:
Test packages are available at:
The packages previously published for |
|
Building (as you can see above) new packages for this one @DaanHoogland - just to have it officially/fresh |
|
@blueorangutan package |
1 similar comment
|
@blueorangutan package |
|
@winterhazel thx for the review request |
|
@blueorangutan package |
|
@andrijapanicsb 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19349 |
|
@andrijapanicsb reviewing functionally in my labs |
Port the production fix from apache/cloudstack PR apache#14195, source commit 0cb24ba (merged into 4.22 as 273b2ea). Reuse the supplied template and load its details without a findById lookup that filters removed records. Preserve template CPU-detail inheritance and explicit VM overrides. Add six regression tests for dummy and active templates, non-VO templates, CPU override precedence and missing templates. Co-authored-by: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com>
|
@ACSHomeBot package G |
Packaging results:
Test packages are available at: |
|
@blueorangutan package |
|
@andrijapanicsb 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19364 |
|
@blueorangutan test |
|
@NuxRo a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-17057) |
Skip target cleanup on cancel and delete when an imported VM is recorded, including imports that completed after cancellation. Re-read the migration before cleanup to protect VM references recorded since the caller loaded it. Keep source snapshot cleanup and migration record deletion independent. Add regression coverage for both cleanup entry points, all migration states, stale caller records, and record-only deletion with source cleanup. This guard does not resolve imports racing after the final read. Signed-off-by: andrijapanicsb <andrija.panic@gmail.com>
|
@ACSHomeBot package G |
|
Accepted: Planned: RPM + DEB, KVM SystemVM. Status: Queued for worker G. Jobs ahead: 0. |
|
Package building started on worker G. |
Packaging results:
Test packages are available at: |
Published package retention updateThe packages previously published for |
| // Import may finish after cancellation and record the VM without changing the Cancelled state. | ||
| // Its target disks now belong to that VM, so neither cancel nor delete may remove them. | ||
| // This does not prevent source snapshot cleanup or deletion of the migration record. |
There was a problem hiding this comment.
| // Import may finish after cancellation and record the VM without changing the Cancelled state. | |
| // Its target disks now belong to that VM, so neither cancel nor delete may remove them. | |
| // This does not prevent source snapshot cleanup or deletion of the migration record. |
There was a problem hiding this comment.
Thanks, @DaanHoogland . I’m keeping these comments. They document a non-obvious cancellation/import interaction and explain why cleanup must preserve the imported VM’s disks. If anything in that explanation is technically inaccurate, please point it out; the deletion suggestions don’t identify an error.
Given that the earlier PR was reverted over concerns about independent testing, it is rather surprising to see review time spent removing explanations instead of helping close that testing gap. With prebuilt packages available, an independent CBT migration test would be considerably more useful than making the source four comment lines shorter.
If you have time to contribute further, could you help verify that workflow and report any functional issues? That would address the concern that actually held this work back.
| // This does not prevent source snapshot cleanup or deletion of the migration record. | ||
| Long importedVmId = migration.getVmId(); | ||
| if (importedVmId == null) { | ||
| // Re-read before cleanup: an import may have recorded its VM since this caller loaded the migration. |
There was a problem hiding this comment.
| // Re-read before cleanup: an import may have recorded its VM since this caller loaded the migration. |
There was a problem hiding this comment.
Thanks, @DaanHoogland . I’m keeping these comments. They document a non-obvious cancellation/import interaction and explain why cleanup must preserve the imported VM’s disks. If anything in that explanation is technically inaccurate, please point it out; the deletion suggestions don’t identify an error.
Given that the earlier PR was reverted over concerns about independent testing, it is rather surprising to see review time spent removing explanations instead of helping close that testing gap. With prebuilt packages available, an independent CBT migration test would be considerably more useful than making the source four comment lines shorter.
If you have time to contribute further, could you help verify that workflow and report any functional issues? That would address the concern that actually held this work back.
rp-
left a comment
rp-
left a comment
There was a problem hiding this comment.
Hey Andrija,
sorry but I did review again the Linstor parts and found 3 issues that should be probably fixed before merging.
Use read-only controller metadata for listing and single-volume inspection without temporary resource placement or DRBD property changes. Validate the selected pool resource group in management-server ROOT/DATA import paths and agent inspection. Add regression coverage for remote replicas, unknown or busy resources, and cross-group imports.
|
Package building started on worker G. |
Packaging results:
Test packages are available at: |
| List<String> names = path == null ? Collections.emptyList() : Collections.singletonList(resourceName(path)); | ||
| List<ResourceDefinition> result = new ArrayList<>(); | ||
| for (int offset = 0; ; offset += PAGE_SIZE) { | ||
| List<ResourceDefinition> page = api.resourceDefinitionList(names, true, null, PAGE_SIZE, offset); |
There was a problem hiding this comment.
this is mixed up, resourceDefinitionList signature is offset, limit
| List<ResourceDefinition> page = api.resourceDefinitionList(names, true, null, PAGE_SIZE, offset); | |
| List<ResourceDefinition> page = api.resourceDefinitionList(names, true, null, offset, PAGE_SIZE); |
or drop the paging completely and query all.
| List<ResourceWithVolumes> page = api.viewResources(Collections.emptyList(), names, | ||
| Collections.emptyList(), null, PAGE_SIZE, offset); |
| private static Set<String> onlineNodes(DevelopersApi api) throws ApiException { | ||
| Set<String> result = new HashSet<>(); | ||
| for (int offset = 0; ; offset += PAGE_SIZE) { | ||
| List<Node> page = api.nodeList(Collections.emptyList(), Collections.emptyList(), PAGE_SIZE, offset); |
This branch has not been deployed
9 participants
Description
This PR restores the exact content of #13656, which was merged as
0a5bf30af32bdea5a209f3f993cbd6a43301d0f9and then reverted by510d0ec3785efe3cce65ccd1247682b91f4492d0.The restoration commit
44a8713671d3a8830342762e88975ad3fd3426c7reproduced the original merge's Git tree (d3be509ec552975ff628e8f2ada6f4d46d1f109d) and stable patch ID (43eeb0cbbadf2e566bc43780ee1c5244888451d0).A subsequent focused commit,
a5578d6f53b51e2b67c7c537d3b7217c6d44eb3d, fixes the per-disk checkpoint used between warm CBT delta cycles. VMware'sDiskChangeInfodoes not contain a change ID; the new checkpoint is now read from the cycle snapshot's disk backing. A second focused commit,6fa7d30b47, handles failures when the final agent command throws or the final CBT cycle cannot be recorded. The original restoration is otherwise unchanged.A further focused commit,
2a3598ad6a, ports the upstream dummy-template import fix described below, with regression tests.The complete feature description and implementation details remain available in
the original PR:
#13656
The original change, before these focused fixes, was approved by two independent committers:
Existing upstream importVM regression
During the new QA run, VM import without a supplied template failed with
Unable to find template with id ... for virtual machine import. This is an existing upstream regression, not introduced by the VMware-to-KVM restoration in this PR.PR #12793, merged as
a01fb0be34b2774d8fb7853703b36364444398e4, added template-detail loading so imported VMs inherit settings such asguest.cpu.mode=host-passthrough. However, its additionalfindById(template.getId())lookup excludes soft-deleted records. The defaultVM Import Default Templateis deliberately stored in the removed state, so this lookup returns null and rejects an otherwise valid import.This was already fixed on the
4.22branch by PR #14195, using source commit0cb24ba8967e14e9577dde7754eeb4a93e6e23e7(merged as273b2ea32bfa24396431b9bb0df055e0ac6c1c02). That fix is still absent frommainas checked on 1 October 2026 at1a48a87587c03472feefb081cfac71b2ebd0f407, which retains the failing lookup.This PR ports the same production-code fix: load details on the template object already supplied to
importVM, rather than look it up again. Template CPU-detail inheritance is preserved, and explicit VM CPU settings continue to override template defaults. Six regression tests cover the removed dummy template, active-template CPU inheritance, non-VO templates, explicit CPU model/mode overrides, and rejection of a genuinely missing template.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate)
The acceptance report attached to #13656 includes representative cold VDDK and
warm CBT migration screenshots:
https://github.com/user-attachments/files/32426132/PR13656-acceptance-report.pdf
How Has This Been Tested?
The original change completed its full functional acceptance run:
Blueorangutan smoke testing also passed 156/156 tests:
#13656 (comment)
The original acceptance and smoke results document the restored implementation, but predate the focused fixes in
a5578d6f53b51e2b67c7c537d3b7217c6d44eb3dand6fa7d30b47. For the checkpoint fix, four targeted unit tests, the 38-module Maven package build, and Checkstyle passed. For the cutover failure handling, the 26-module server test reactor and Checkstyle passed (33 targeted CBT tests, including two new tests). Live multi-cycle CBT regression and functional cutover tests have not yet been run on the updated commits. Normal CI and smoke tests should run for this PR.For the dummy-template port in
2a3598ad6a, all sixUserVmImportTemplateTestregression tests passed, and the 26-module server test reactor completed with zero Checkstyle violations. The local command wasmvn -B -pl server -am -Dtest=UserVmImportTemplateTest -Dsurefire.failIfNoSpecifiedTests=false -Dexec.skip=true test. The unrelated schema shell test was skipped viaexec.skipbecause Windows CRLF line endings prevent that script from running locally; this is not a claim that the full test suite passed. Fresh functional acceptance testing is still in progress on the QA environment with the same production fix hot-patched into both management servers. The historical acceptance report above must not be read as a completed acceptance run for this new commit.How did you try to break this feature and the system with this change?
The original acceptance run covered cold and warm migration to NFS, Ceph/RBD
and Linstor, cancellation, retries, invalid state transitions, ownership,
network validation, existing-volume adoption, Windows Server migration,
cleanup and backend leak checks. Full details and evidence are in #13656 and
the linked acceptance report.