Skip to content

CLOUDSTACK-8829 : Consecutive cold migration fails #797

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

Closed
maneesha-p wants to merge 1 commit into from
Closed

CLOUDSTACK-8829 : Consecutive cold migration fails #797

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 11 additions & 7 deletions engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1744,9 +1744,6 @@ public void storageMigration(final String vmUuid, final StoragePool destPool) {

private void orchestrateStorageMigration(final String vmUuid, final StoragePool destPool) {
final VMInstanceVO vm = _vmDao.findByUuid(vmUuid);
final Long srchostId = vm.getHostId() != null ? vm.getHostId() : vm.getLastHostId();
final HostVO srcHost = _hostDao.findById(srchostId);
final Long srcClusterId = srcHost.getClusterId();

try {
stateTransitTo(vm, VirtualMachine.Event.StorageMigrationRequested, null);
Expand Down Expand Up @@ -1776,19 +1773,26 @@ private void orchestrateStorageMigration(final String vmUuid, final StoragePool
// If VM was cold migrated between clusters belonging to two different VMware DCs,
// unregister the VM from the source host and cleanup the associated VM files.
if (vm.getHypervisorType().equals(HypervisorType.VMware)) {
Long srcClusterId = null;
Long srcHostId = vm.getHostId() != null ? vm.getHostId() : vm.getLastHostId();
if (srcHostId != null) {
HostVO srcHost = _hostDao.findById(srcHostId);

@alexandrelimassantana alexandrelimassantana Feb 14, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hello @maneesha-p
I got a little confused by this if statement. Are you actually doing anything inside it? If Im not mistaken both variables are local inside this if-block and won't do anything outside it's scope, is this intended?

@GabrielBrascher GabrielBrascher Feb 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@alexandrelimassantana I don't see any problem with that if (line 1778). Both variables (srcHostId and srcClusterId) are used.

The conditional at the line 1784 uses srcClusterId.

The method at line 1793 sends the UnregisterVMCommand to a given host with the srcHostId.

@maneesha-p maneesha-p Feb 24, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@alexandrelimassantana 'srcHost' variable is local to the if-block but 'srcClusterId' variable that is populated inside the if-block is outside the scope of it and as @GabrielBrascher mentioned its being used in line 1784.

srcClusterId = srcHost.getClusterId();
}

final Long destClusterId = destPool.getClusterId();
if (srcClusterId != null && destClusterId != null && ! srcClusterId.equals(destClusterId)) {
final String srcDcName = _clusterDetailsDao.getVmwareDcName(srcClusterId);
final String destDcName = _clusterDetailsDao.getVmwareDcName(destClusterId);
if (srcDcName != null && destDcName != null && !srcDcName.equals(destDcName)) {
s_logger.debug("Since VM's storage was successfully migrated across VMware Datacenters, unregistering VM: " + vm.getInstanceName() +
" from source host: " + srcHost.getId());
" from source host: " + srcHostId);
final UnregisterVMCommand uvc = new UnregisterVMCommand(vm.getInstanceName());
uvc.setCleanupVmFiles(true);
try {
_agentMgr.send(srcHost.getId(), uvc);
} catch (final Exception e) {
throw new CloudRuntimeException("Failed to unregister VM: " + vm.getInstanceName() + " from source host: " + srcHost.getId() +
_agentMgr.send(srcHostId, uvc);
} catch (final AgentUnavailableException | OperationTimedoutException e) {
throw new CloudRuntimeException("Failed to unregister VM: " + vm.getInstanceName() + " from source host: " + srcHostId +

@pedro-martins pedro-martins Feb 22, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hi @maneesha-p

How about you replace the catch Exception at line 1794 to ( AgentUnavailableException | OperationTimedoutException )?

@maneesha-p maneesha-p Mar 21, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for pointing out.Updated.

" after successfully migrating VM's storage across VMware Datacenters");
}
}
Expand Down