Restore VMware-to-KVM migration changes from #13656 - #14256
andrijapanicsb wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14256 +/- ##
============================================
+ Coverage 19.91% 20.06% +0.15%
- Complexity 20194 20670 +476
============================================
Files 6373 6427 +54
Lines 577230 584926 +7696
Branches 70696 71661 +965
============================================
+ Hits 114942 117381 +2439
- Misses 449722 454711 +4989
- Partials 12566 12834 +268
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 |
|
@DaanHoogland I see that BO went boo boo, some other PRs and your calls of BO also failed, no response from BO - you might want to check it out. Or just use packages I build above (identical builds as "yours" all systemVM template supported |
| List<VmwareCbtMigrationDiskVO> disks = vmwareCbtMigrationDiskDao.listByMigrationId(migration.getId()); | ||
| for (VmwareCbtMigrationDiskVO disk : disks) { | ||
| for (VmwareCbtChangedDiskInfo changedDisk : changedDisks) { | ||
| if (StringUtils.isNotBlank(changedDisk.getNextChangeId()) && |
There was a problem hiding this comment.
can we have else block with logger just to get visibility of changeId is not null.
There was a problem hiding this comment.
As requested, the else block does not make much sense: a plain else would log on every non-matching disk pair in the nested loop. With two disks, for example, each disk is compared with both results, so it would generate misleading noise.
Also, 0 changed blocks and nextChangeId are different things:
- A cycle with 0 changed blocks is valid, although uncommon for a running VM.
- VMware should still return a non-blank nextChangeId.
- A blank nextChangeId would indicate an abnormal VMware response or implementation problem.
So for now - no, I prefer it stays as it is, but we can think of improvement is a follow-up PR.
There was a problem hiding this comment.
If needed, I can separate the disk match and add a warning specifically for a matched disk with a blank nextChangeId - but as a followup PR.
There was a problem hiding this comment.
The else path plays a significant role here. If the id is null, no disk would be considered by this logic, which could lead to unintended behaviour.
There was a problem hiding this comment.
Thanks for the detailed review, Rajiv. I checked the current VMware query path: it fails if the snapshot disk or its change ID is missing, before this loop runs. A plain else here would also log normal non-matches between disks. I will keep the matching and logging cleanup for a follow-up rather than change this restore PR for this point.
|
Thanks @andrijapanicsb @blueorangutan package |
|
@nvazquez 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 19342 |
|
|
||
| do { | ||
| Object diskChangeInfo = invokeQueryChangedDiskAreas(context, vmMO, snapshot, disk, startOffset); | ||
| nextChangeId = StringUtils.defaultIfBlank(getObjectStringValue(diskChangeInfo, "getChangeId"), |
There was a problem hiding this comment.
I am assuming that this method, through reflection, returns an instance of DiskChangeInfo. If that assumption is correct, DiskChangeInfo does not appear to expose a getChangeId() method. As a result, this call would always return null, and nextChangeId would never be updated from the returned object.
There was a problem hiding this comment.
Thanks, you are right: DiskChangeInfo does not expose changeId. Commit a5578d6 now reads the next per-disk checkpoint from the matching disk backing in the cycle snapshot, and fails if that ID is missing. Focused unit tests cover matching and missing IDs.
| disk.getDiskId(), commandResult.getExitValue()); | ||
| throw new IllegalStateException(commandResult.appendLastCommandOutput(details)); | ||
| } | ||
| return new VmwareCbtDiskSyncResultTO(disk.getDiskId(), disk.getTargetPath(), disk.getChangeId(), |
There was a problem hiding this comment.
It looks like we're returning the input changeId rather than an updated one. If that's the case, wouldn't subsequent incremental syncs continue to track changes from the baseline snapshot and potentially result in copying all changes accumulated since the baseline? It may be worth validating this behaviour once.
One approach could be to follow the same pattern used in VmwareCbtMigrationManagerImpl.refreshBaselineDiskChangeIds(). After the agent reports a successful sync, we could read the current changeId from each disk backing in the cycle snapshot and persist that value as the checkpoint for the next incremental cycle.
There was a problem hiding this comment.
You are right on this one, thx for pointing out. Commit incoming!
There was a problem hiding this comment.
Confirmed. The agent echoes the input changeId; after a successful sync, the management server is meant to persist the next ID. The missing ID in the VMware query result prevented that. Commit a5578d6 supplies the snapshot backing changeId, and a unit test verifies that it is persisted for the next cycle. Live multi-cycle regression on this commit is still pending.
There was a problem hiding this comment.
For information, the earlier implementation would still have produced the correct output. However, it would not have benefited from the optimisation, as it copied all changes made since the baseline snapshot rather than comparing incremental changes across subsequent snapshots.
There was a problem hiding this comment.
Yep, all clear - it's still a "bug" - cumulative copy every time - we can call it "nonsense" :)
Comming to CCC btw?
| cutoverCommand.setAllowNonInPlaceFinalization(isNonInPlaceFinalizationFallbackAllowed(storageTarget)); | ||
| cutoverCommand.setWait(getVmwareCbtMigrationAgentCommandTimeout()); | ||
|
|
||
| VmwareCbtMigrationAnswer answer = sendVmwareCbtCommand(cbtHost, cutoverCommand, "cut over", migration.getUuid()); |
There was a problem hiding this comment.
We should consider wrapping this in a try-catch block, as the method can throw an exception. Without proper handling, we may lose the opportunity to return a meaningful response that includes the underlying exception details and context.
There was a problem hiding this comment.
Good catch. If the agent call throws after the final delta, the migration can remain in CuttingOver. I am fixing this path now and adding a focused test so the failure is recorded through the normal cutover failure handling.
| cycle.setState(VmwareCbtMigrationCycle.State.Created); | ||
| cycle.setDescription("Creating final VMware CBT snapshot for cutover"); | ||
| cycle.setUpdated(new Date()); | ||
| cycle = vmwareCbtMigrationCycleDao.persist(cycle); |
There was a problem hiding this comment.
This DAO call could also be included within the try block for added safety and consistent error handling. Since the methods below already have their DAO interactions protected by the same try-catch construct, it may be beneficial to keep this call within that scope as well to handle any unexpected failure scenarios gracefully.
There was a problem hiding this comment.
You are right that this insert sits outside the catch. Simply moving it into the try block would not be safe because the catch path assumes the cycle row exists. I am handling the insert-failure case separately and adding a focused test.
|
@blueorangutan test |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-17040) |
Signed-off-by: andrijapanicsb <andrija.panic@gmail.com>
Signed-off-by: andrijapanicsb <andrija.panic@gmail.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 19349 |
|
@andrijapanicsb reviewing functionally in my labs |
This branch has not been deployed
6 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.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:
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.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.