CLOUDSTACK-7982: KVM live migration with local storage

[marcaurele]

This is the forward port of our KVM live migration implementation in 4.4.2 to the master branch.
Feedback is welcome. I still have to write the documentation to explain some configuration for libvirt and some config value in CS.

Things left:

• Write a Marvin test scenario
• Write documentation in cloustack-docs-install & open PR with a link to this one
• Add to the upgrade SQL file:

UPDATE`cloud`.`hypervisor_capabilities`SET`storage_motion_supported`=1WHERE`hypervisor_capabilities`.`hypervisor_type`='KVM';

• Update the PR with a final description

Agent properties example

libvirt.connection.protocol=qemu+tls://
# See Libvirt flags code to compute your correct flag: https://libvirt.org/html/libvirt-libvirt-domain.html - virDomainMigrateFlags
vm.migratewithstorage.flags=12679
vm.migrate.flags=1
vm.migrate.by=hostname

Jira: https://issues.apache.org/jira/browse/CLOUDSTACK-7982

[marcaurele]

Is there a way to get the management server log of the travis job?

[yadvr]

@marcaurele yes, click on the details, go to the failing job and open the raw build log/output for debugging. Can you rebase again please, squash the changes. I can help run tests on this PR.

[marcaurele]

@rhtyd I found the log but it doesn't give the mgmt server log output, only the job's output which got a Command failed due to Internal Server Error in an API response. I'll rebase & squash.

[yadvr]

@marcaurele I had another look, you're right -- we don't have any way to see mgmt server log for Travis runs. I think rebase/squash and do a push -f to re-kick Travis run.

[wido]

Isn't this redundant since you already specify qemu:// at the beginning of LibvirtComputingResource?

[marcaurele]

It's something I have to document in the installation doc. If you want to run the migration over TLS, you have to specify here qemu+tls://

[marcaurele]

I'll remove the default value on the variable definition

[mlsorensen]

Along these lines of cancellation, I've long thought that we really need the ability to clean up long running jobs if the agent disconnects from the management server for any reason (say upgrade or restart of agent or management server, network problems, etc). Normally the management server will know the job failed but the agent keeps on trucking, causing problems, especially for things like migrations of storage. This may be an important thing to add for this feature, to avoid situations where a migration completes but CloudStack does not know about it because the management server was restarted during the migration.

Rather than forcing the management server to know that the agent work needs to be cleaned up and sending a command to the hypervisor that is tailored to each command that can fail, one solution that I've seen implemented that has worked well is for LibvirtComputingResource to hold a global List of tasks, then it overrides the disconnected() method and loops through this list, running the tasks. It then exposes methods addDisconnectHook(Runnable hook) and removeDisconnectHook(Runnable hook) so that commands that are sensitive to being interrupted can add in cancellation logic in the case of disconnect before starting and remove it when finished.

Something like:

@Override
public void disconnected() {
this._connected = false;
s_logger.info("Detected agent disconnect event, running through " + _disconnectHooks.size() + " disconnect hooks");
for (Runnable hook : _disconnectHooks) {
hook.run();
}
_disconnectHooks.clear();
}
public void addDisconnectHook(Runnable hook) {
s_logger.debug("Adding disconnect hook " + hook);
_disconnectHooks.add(hook);
}
public void removeDisconnectHook(Runnable hook) {
s_logger.debug("Removing disconnect hook " + hook);
if (_disconnectHooks.contains(hook)) {
s_logger.debug("Removing disconnect hook " + hook);
_disconnectHooks.remove(hook);
} else {
s_logger.debug("Requested removal of disconnect hook, but hook not found: " + hook);
}
}

An example hook to cancel the migration might look like this:

public class MigrationCancelHook extends Thread {
private static final Logger LOGGER = Logger.getLogger(MigrationCancelHook.class.getName());
private static final String HOOK_PREFIX = "MigrationCancelHook-";
Domain _migratingDomain;
String _vmName;
public MigrationCancelHook(Domain migratingDomain) throws LibvirtException {
super(HOOK_PREFIX + migratingDomain.getName());
_migratingDomain = migratingDomain;
_vmName = migratingDomain.getName();
}
@Override
public void run() {
LOGGER.info("Interrupted migration of " + _vmName);
try {
if (_migratingDomain.abortJob() == 0) {
LOGGER.warn("Aborted migration job for " + _vmName);
}
} catch (Exception ex) {
LOGGER.warn("Failed to abort migration job for " + _vmName, ex);
}
}
}

[marcaurele]

What you're describing is something slightly different than the case I want to address with my command. Your concern is valid, and the hook should be added too. I've sent once an email to the mailing list to ask why an agent that disconnects cannot reconnect to a management server until all its tasks are done, because the command will always be marked as failed on the management server and might have been executed successfully on the agent, causing incoherent states.

For the migration, without the cancellation command, if the migration takes a long time, the management server will cancel the job after a while and mark it as failed, thus invalidating the new host information. But the agent will keep on migrating the VM and when it has moved to the new host it becomes unreachable/unavailable since the network information hasn't been updated/committed.

[yadvr]

@marcaurele can you check the Travis failure? Thanks.

[yadvr]

@marcaurele can you rebase against latest master?

[marcaurele]

@rhtyd rebase done

[wido]

I see a few open bullet points in the PR's heading. How are we with those?

This feature would be awesome for 4.11

[marcaurele]

For the 1st one (Marvin test case) I will definitively need some help as I have no clue on how to write one. The rest will be done, I hope by next week.

[DaanHoogland]

@ustcweizhou@marcaurele will the implementations for local storage and shared storage be conflicting?
@ustcweizhou could you share your implementation? or insights on differences?

[marcaurele]

@DaanHoogland I don't know since we aren't using shared storage. I would think it should not since the API call is migrateVirtualMachineWithVolume for migration with local storage, and I would think migrateVirtualMachine would be the one with shared storage since the storage does not need to be migrated. The live migration passes a flag to inform libvirt to migrate the disk.

[DaanHoogland]

I appreciate that @marcaurele. The issue with shared storage is that, when it is cluster wide, migration may or may not still have to include volumes. So either of the call will have to include the check if the target can reach the storage in use and then delegate to the other API based on the outcome of that check.

[DaanHoogland]

@marcaurele in travis there is a unit test and a marvin test failing, both concerning migration. Can you have a look?

[marcaurele]

@DaanHoogland I know about those failures, but I cannot access the mgmt logs to get the error/exception. And since we're not using upstream, I cannot run this version in our test lab either to debug it. I would appreciate if anyone can post the proper exception thrown during those failed tests.

[DaanHoogland]

I see @marcaurele I will investigate the marvin error. the other one i a plain compilation error though:

/home/travis/build/apache/cloudstack/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java:[3842,19] error: no suitable constructor found for MigrateWithStorageAnswer(MigrateWithStorageCommand,List<VolumeObjectTO>)
[ERROR] constructor MigrateWithStorageAnswer.MigrateWithStorageAnswer(MigrateWithStorageCommand,boolean,boolean,Exception) is not applicable
(actual and formal argument lists differ in length)
constructor MigrateWithStorageAnswer.MigrateWithStorageAnswer(MigrateWithStorageCommand,List<VolumeObjectTO>,boolean,String) is not applicable
(actual and formal argument lists differ in length)
/home/travis/build/apache/cloudstack/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java:[3851,19] error: no suitable constructor found for MigrateWithStorageAnswer(MigrateWithStorageCommand,Exception)

@blueorangutan package

[yadvr]

@blueorangutan package

[blueorangutan]

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result: ✖centos6 ✖centos7 ✖debian. JID-1348

[blueorangutan]

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1366

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[blueorangutan]

Trillian test result (tid-1773)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31209 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr1709-t1773-kvm-centos7.zip
Test completed. Failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_vpc_privategw_acl	Failure	66.83	test_privategw_acl.py
test_02_vpc_privategw_static_routes	Failure	213.43	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	Failure	188.28	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	Failure	344.25	test_privategw_acl.py
test_02_create_template_with_checksum_sha1	Error	5.23	test_templates.py
test_03_create_template_with_checksum_sha256	Error	5.25	test_templates.py
test_04_create_template_with_checksum_md5	Error	5.23	test_templates.py
test_08_migrate_vm	Error	91.43	test_vm_life_cycle.py
test_01_vpc_remote_access_vpn	Failure	71.01	test_vpc_vpn.py

[yadvr]

@marcaurele can you fix the conflicts?
@DaanHoogland for reference we've another such implementation at MissionCriticalCloud/cosmic#541 and MissionCriticalCloud/cosmic#539

[wido]

How are we on this one? I'd really like to see this in master. Anything we can do to assist?

[marcaurele]

@wido I'll work on the rebase, but I need help to get it tested. The marvin tests are failing but I cannot access the management server logs to understand the issue, so I need someone who's using master in their lab to test this branch and give me a feedback on the log exceptions. Can you do that ?

[wido]

@marcaurele Yes, I should be able to do that or @GabrielBrascher might be able to help as well.

[wido]

@marcaurele Have you been able to do the rebase yet?

[marcaurele]

@wido some code has changed regarding migration with the local storage and I cannot find a way how to do a correct rebase and took a shotcut. I let you handle the tests from there, and I don't mind if you work further on this change to make it into a release.

[wido]

@marcaurele Awesome! Could you elaborate a bit about the shortcut? I kind find it right away in the changes.

[marcaurele]

@wido shortcut here with null, true as last parameters

[wido]

Thanks! @GabrielBrascher and me will take a look at this. Might take some time to bootstrap and test env, but we will test it further.

[marcaurele]

@wido I updated the PR description with the new configuration keys required on the agent side. My logic was that each agent is responsible to give the migration flags & connection mode that it is capable of. An improvement would have been to have the possibility to overwrite the migration flags from the command. I know that @remibergsma did merge it in their cosmic cloud version.

[GabrielBrascher]

@marcaurele Do you need any help with this PR? Please ping me if necessary.

[marcaurele]

@GabrielBrascher I need help here as I don't have any setup with the upstream version therefore I cannot debug the issue. I did this manual backport which I hoped could work as is, but it doesn't. If you can test the branch and see what isn't working properly. Then I'll assist you to resolve the issues.

[GabrielBrascher]

@marcaurele I will perform some tests then.

[yadvr]

@marcaurele can you rebase this branch against latest master. With introducing of CA framework in 4.11, we've extended the use to secure KVM host as well, and tls based live migration is used when both src/dest hosts are secured (tls certs setup by the CA framework). With this perhaps you can simplify/update your PR?

[marcaurele]

rebase done

[GabrielBrascher]

Closing this as the PR #2997 was merged.