Skip to content

Fix: Condition for aborting migration, resume paused VMs on destination #12331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nvazquez wants to merge 3 commits into
base: 4.20
Choose a base branch
from
Open

Fix: Condition for aborting migration, resume paused VMs on destination #12331

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0960b4c
Fix: Condition for aborting migration, resume paused VMs on destination
nvazquez Dec 24, 2025
b204e5b
Refactor downtime logic
nvazquez Jan 15, 2026
3f1cf04
Refactor into new method
nvazquez Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 35 additions & 11 deletions ...src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,20 +243,21 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
final Future<Domain> migrateThread = executor.submit(worker);
executor.shutdown();
long sleeptime = 0;
final int migrateDowntime = libvirtComputingResource.getMigrateDowntime();
boolean isMigrateDowntimeSet = false;

while (!executor.isTerminated()) {
Thread.sleep(100);
sleeptime += 100;
if (sleeptime == 1000) { // wait 1s before attempting to set downtime on migration, since I don't know of a VIR_DOMAIN_MIGRATING state
final int migrateDowntime = libvirtComputingResource.getMigrateDowntime();
if (migrateDowntime > 0 ) {
try {
final int setDowntime = dm.migrateSetMaxDowntime(migrateDowntime);
if (setDowntime == 0 ) {
logger.debug("Set max downtime for migration of " + vmName + " to " + String.valueOf(migrateDowntime) + "ms");
}
} catch (final LibvirtException e) {
logger.debug("Failed to set max downtime for migration, perhaps migration completed? Error: " + e.getMessage());
if (!isMigrateDowntimeSet && migrateDowntime > 0 && sleeptime >= 1000) { // wait 1s before attempting to set downtime on migration, since I don't know of a VIR_DOMAIN_MIGRATING state
try {
final int setDowntime = dm.migrateSetMaxDowntime(migrateDowntime);
if (setDowntime == 0 ) {
isMigrateDowntimeSet = true;
logger.debug("Set max downtime for migration of " + vmName + " to " + String.valueOf(migrateDowntime) + "ms");
}
} catch (final LibvirtException e) {
logger.debug("Failed to set max downtime for migration, perhaps migration completed? Error: " + e.getMessage());
}
}
if (sleeptime % 1000 == 0) {
Expand All @@ -272,7 +273,7 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
} catch (final LibvirtException e) {
logger.info("Couldn't get VM domain state after " + sleeptime + "ms: " + e.getMessage());
}
if (state != null && state == DomainState.VIR_DOMAIN_RUNNING) {
if (state != null && (state == DomainState.VIR_DOMAIN_RUNNING || state == DomainState.VIR_DOMAIN_PAUSED)) {
try {
DomainJobInfo job = dm.getJobInfo();
logger.info(String.format("Aborting migration of VM [%s] with domain job [%s] due to time out after %d seconds.", vmName, job, migrateWait));
Expand Down Expand Up @@ -314,6 +315,7 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
if (logger.isDebugEnabled()) {
logger.debug(String.format("Cleaning the disks of VM [%s] in the source pool after VM migration finished.", vmName));
}
resumeDomainIfPaused(destDomain, vmName);
deleteOrDisconnectDisksOnSourcePool(libvirtComputingResource, migrateDiskInfoList, disks);
libvirtComputingResource.cleanOldSecretsByDiskDef(conn, disks);
}
Expand Down Expand Up @@ -378,6 +380,28 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
return new MigrateAnswer(command, result == null, result, null);
}

private DomainState getDestDomainState(Domain destDomain, String vmName) {
DomainState dmState = null;
try {
dmState = destDomain.getInfo().state;
} catch (final LibvirtException e) {
logger.info("Failed to get domain state for VM: " + vmName + " due to: " + e.getMessage());
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message uses logger.info() for an exception, which is inconsistent with the existing error handling pattern in this file. Line 274 uses logger.info() for a similar case, but line 388 should use String.format() for consistency with other error messages in the file (e.g., lines 285, 324, 338). Consider changing to: logger.info(String.format("Failed to get domain state for VM [%s] due to: [%s].", vmName, e.getMessage()));

Suggested change
logger.info("Failed to get domain state for VM: " + vmName + " due to: " + e.getMessage());
logger.info(String.format("Failed to get domain state for VM [%s] due to: [%s].", vmName, e.getMessage()));

Copilot uses AI. Check for mistakes.
}
return dmState;
}

private void resumeDomainIfPaused(Domain destDomain, String vmName) {
Comment on lines +383 to +393
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new methods getDestDomainState and resumeDomainIfPaused lack test coverage. Given that the test file LibvirtMigrateCommandWrapperTest.java has comprehensive test coverage for this class, these new methods should have corresponding unit tests to verify the behavior when the domain is paused, when it's running, and when LibvirtException is thrown.

Copilot uses AI. Check for mistakes.
DomainState dmState = getDestDomainState(destDomain, vmName);
if (dmState == DomainState.VIR_DOMAIN_PAUSED) {
logger.info("Resuming VM " + vmName + " on destination after migration");
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log message uses String concatenation instead of String.format() which is the established pattern in this file (e.g., lines 279, 316). For consistency with other log messages, consider: logger.info(String.format("Resuming VM [%s] on destination after migration.", vmName));

Suggested change
logger.info("Resuming VM " + vmName + " on destination after migration");
logger.info(String.format("Resuming VM %s on destination after migration", vmName));

Copilot uses AI. Check for mistakes.
try {
destDomain.resume();
} catch (final Exception e) {
logger.error("Failed to resume vm " + vmName + " on destination after migration due to : " + e.getMessage());
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message uses String concatenation instead of String.format() which is the pattern used consistently throughout this file for similar log messages (e.g., lines 279, 285, 324). Consider changing to: logger.error(String.format("Failed to resume VM [%s] on destination after migration due to: [%s].", vmName, e.getMessage())); for consistency.

Suggested change
logger.error("Failed to resume vm " + vmName + " on destination after migration due to : " + e.getMessage());
logger.error(String.format("Failed to resume VM [%s] on destination after migration due to: [%s].", vmName, e.getMessage()));

Copilot uses AI. Check for mistakes.
}
}
}

/**
* Gets the disk labels (vda, vdb...) of the disks mapped for migration on mapMigrateStorage.
* @param diskDefinitions list of all the disksDefinitions of the VM.
Expand Down
Loading