migrateVMWithVolume: Handle migrate vm with volume where data disk is not based off a template by Pearl1594 · Pull Request #4771 · apache/cloudstack

Pearl1594 · 2021-03-08T11:38:59Z

Description

This PR fixes the failure noticed during live migration of an instance with a data volume not based off a template. The issue is noticed on centos 8 which causes the TestKVMLiveMigration::test_test_02_migrate_VM_with_two_data_disks to fail

nosetests --with-xunit --xunit-file=results.xml --with-marvin --marvin-config=/marvin/ref-trl-320-k-M8-pearl-dsilva-advanced-cfg -s -a tags=xx --hypervisor=KVM /marvin/tests/smoke/test_vm_life_cycle.py

==== Marvin Init Started ====

=== Marvin Parse Config Successful ===

=== Marvin Setting TestData Successful===

==== Log Folder Path: /marvin/MarvinLogs/Apr_05_2021_15_27_52_4FERLF. All logs will be available here ====

=== Marvin Init Logging Successful===

==== Marvin Init Successful ====
=== TestName: test_02_migrate_VM_with_two_data_disks | Status : EXCEPTION ===

===final results are now copied to: /marvin//MarvinLogs/test_vm_life_cycle_MW3N4E===

Corresponding exception observed on MS:

2021-04-05 21:20:46,925 DEBUG [o.a.c.s.v.VolumeServiceImpl] (Work-Job-Executor-1:ctx-2907ea33 job-3946/job-3947 ctx-fa575848) (logid:a7e59bcb) Failed to copy volume
com.cloud.utils.exception.CloudRuntimeException: Copy operation failed in 'StorageSystemDataMotionStrategy.copyAsync': null
	at org.apache.cloudstack.storage.motion.StorageSystemDataMotionStrategy.copyAsync(StorageSystemDataMotionStrategy.java:1901)
	at org.apache.cloudstack.storage.motion.DataMotionServiceImpl.copyAsync(DataMotionServiceImpl.java:119)
	at org.apache.cloudstack.storage.volume.VolumeServiceImpl.migrateVolumes(VolumeServiceImpl.java:1645)
	at org.apache.cloudstack.engine.orchestration.VolumeOrchestrator.migrateVolumes(VolumeOrchestrator.java:1181)
	at com.cloud.vm.VirtualMachineManagerImpl.orchestrateMigrateWithStorage(VirtualMachineManagerImpl.java:3000)
	at com.cloud.vm.VirtualMachineManagerImpl.orchestrateMigrateWithStorage(VirtualMachineManagerImpl.java:5524)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at com.cloud.vm.VmWorkJobHandlerProxy.handleVmWorkJob(VmWorkJobHandlerProxy.java:107)
	at com.cloud.vm.VirtualMachineManagerImpl.handleVmWorkJob(VirtualMachineManagerImpl.java:5631)
	at com.cloud.vm.VmWorkJobDispatcher.runJob(VmWorkJobDispatcher.java:102)
	at org.apache.cloudstack.framework.jobs.impl.AsyncJobManagerImpl$5.runInContext(AsyncJobManagerImpl.java:620)
	at org.apache.cloudstack.managed.context.ManagedContextRunnable$1.run(ManagedContextRunnable.java:48)
	at org.apache.cloudstack.managed.context.impl.DefaultManagedContext$1.call(DefaultManagedContext.java:55)
	at org.apache.cloudstack.managed.context.impl.DefaultManagedContext.callWithContext(DefaultManagedContext.java:102)
	at org.apache.cloudstack.managed.context.impl.DefaultManagedContext.runWithContext(DefaultManagedContext.java:52)
	at org.apache.cloudstack.managed.context.ManagedContextRunnable.run(ManagedContextRunnable.java:45)
	at org.apache.cloudstack.framework.jobs.impl.AsyncJobManagerImpl$5.run(AsyncJobManagerImpl.java:568)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: java.lang.NullPointerException
	at org.apache.cloudstack.storage.motion.KvmNonManagedStorageDataMotionStrategy.copyTemplateToTargetFilesystemStorageIfNeeded(KvmNonManagedStorageDataMotionStrategy.java:198)
	at org.apache.cloudstack.storage.motion.StorageSystemDataMotionStrategy.copyAsync(StorageSystemDataMotionStrategy.java:1793)
	at org.apache.cloudstack.storage.motion.DataMotionServiceImpl.copyAsync(DataMotionServiceImpl.java:119)
	... 23 more

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Bug Severity

How Has This Been Tested?

Run the test for live-migrating a VM with its volumes comprising of root volume + 2 data disks

nosetests --with-xunit --xunit-file=results.xml --with-marvin --marvin-config=/marvin/ref-trl-320-k-M8-pearl-dsilva-advanced-cfg -s -a tags=xx --hypervisor=KVM /marvin/tests/smoke/test_vm_life_cycle.py

==== Marvin Init Started ====

=== Marvin Parse Config Successful ===

=== Marvin Setting TestData Successful===

==== Log Folder Path: /marvin/MarvinLogs/Apr_05_2021_15_57_59_1THXV7. All logs will be available here ====

=== Marvin Init Logging Successful===

==== Marvin Init Successful ====
=== TestName: test_02_migrate_VM_with_two_data_disks | Status : SUCCESS ===

===final results are now copied to: /marvin//MarvinLogs/test_vm_life_cycle_YMHKU5===

…late

Pearl1594 · 2021-03-08T11:40:01Z

@blueorangutan package

blueorangutan · 2021-03-08T11:40:59Z

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

blueorangutan · 2021-03-08T12:14:54Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2885

harikrishna-patnala · 2021-03-08T12:43:10Z

+            sourceVolumeTemplateStoragePoolVO = vmTemplatePoolDao.findByPoolTemplate(destStoragePool.getId(), srcVolumeInfo.getTemplateId(), null);
+        }
        if (sourceVolumeTemplateStoragePoolVO == null && destStoragePool.getPoolType() == StoragePoolType.Filesystem) {
            DataStore sourceTemplateDataStore = dataStoreManagerImpl.getRandomImageStore(srcVolumeInfo.getDataCenterId());


@Pearl1594 can you help me understanding on which object we are getting NPE. I'm seeing only srcVolumeInfo as possible object but it would have failed earlier at 198 Line.

@harikrishna-patnala I've updated the exception, you're right, the exception happens at line198 - I had added a few debug lines which was why it showed the exception occuring at L200

@harikrishna-patnala The problem is that findByPoolTemplate receives a long (primitive type) to template and getTemplateId returns a Long (object type). Java will do an autounboxing, but as getTemplateId returns null for non-template volume (data disks), it will generate a NPE when trying to autounbox this value.

@GutoVeronezi wouldn't srcVolumeInfo.getTemplateId() != null prevents what you mentioned?

@GabrielBrascher It will. I mention it to explain why it is happening.

GutoVeronezi · 2021-03-08T13:28:10Z

I would ask you to take a look at the PR #4698. As it was proposed before this PR, I think it would be interesting to consider it first for this problem.

DaanHoogland · 2021-03-08T13:33:31Z

@GutoVeronezi I see that code is changed in different places than the code in this PR. Are you saying that PR makes this obsolete or do they merit merging each in their own right?

GabrielBrascher · 2021-03-08T13:40:10Z

Thanks for opening this PR @Pearl1594.

This PR seems to address the same issue as PR #4698 but in a different way. I think that it is important to evaluate which one suits better before moving on with this or the other PR.

It is important to raise also that it seems that @GutoVeronezi will only work to the master, so we can have also a PR specifically for 4.15, or cherry-pick hist commit (if we decide that his option is the better).

GabrielBrascher · 2021-03-08T13:51:27Z

@DaanHoogland @GutoVeronezi you were quicker than me, I pressed enter to send the message and then your comments appear.

Taking a deeper look, I think that both handle different situations. Very similar, but still complementary situations.
This one prevents also issue due to eventually null srcVolumeInfo.

GutoVeronezi · 2021-03-08T13:55:00Z

@DaanHoogland @GutoVeronezi you were quicker than me, I pressed enter to send the message and then your comments appear.

Taking a deeper look, I think that both handle different situations. Very similar, but still complementary situations.
This one prevents also issue due to eventually null srcVolumeInfo.

@GabrielBrascher The problem is that findByPoolTemplate receives a long (primitive type) to template and getTemplateId returns a Long (object type). Java will do an autounboxing, but as getTemplateId returns null for non-template volume (data disks), it will generate a NPE when trying to autounbox this value.

GutoVeronezi · 2021-03-08T14:09:37Z

@GutoVeronezi I see that code is changed in different places than the code in this PR. Are you saying that PR makes this obsolete or do they merit merging each in their own right?

@DaanHoogland Both PRs address the same problem, which is to handle the template id in case it is null (for data disks); the main difference is that, that this PR made validations out of the scope of the method, where it seems more interesting, as it verifies if we are handling a volume created from a template before calling the copy template method. Further, we added some logging to facilitate the life of operators if they ever need to troubleshoot this process.

GabrielBrascher · 2021-03-08T14:10:03Z

@GutoVeronezi I know how Long and long work.

That is why I already mentioned about srcVolumeInfo validations that this PR does: srcVolumeInfo != null and srcVolumeInfo.getTemplateId() != null.

GabrielBrascher · 2021-03-08T14:27:06Z

Anyway, I see one check that this PR does and #4698 does not. Which is checking if srcVolumeInfo is not null.

However looking at the code, I don't see any way that it can be null. If srcVolumeInfo is null then we will see NPE previously on the caller, precisely at StorageSystemDataMotionStrategy.java#L1806.

With that said, I am +1 on @GutoVeronezi solution; he adds quite interesting log messages.

Pearl1594 · 2021-03-08T14:39:53Z

Thanks @GutoVeronezi and @GabrielBrascher. I hadn't noticed that the issue was already addressed. And as mentioned it does add useful logs. Ill close this PR. Thanks!

DaanHoogland · 2021-03-08T14:43:35Z

Reviewed @GutoVeronezi and prefer that one as well

blueorangutan · 2021-03-08T21:53:50Z

Trillian test result (tid-3673)
Environment: kvm-centos8 (x2), Advanced Networking with Mgmt server 8
Total time taken: 31236 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4771-t3673-kvm-centos8.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_outofbandmanagement.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 84 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_oobm_background_powerstate_sync	`Failure`	21.52	test_outofbandmanagement.py
test_oobm_enabledisable_across_clusterzones	`Error`	36.58	test_outofbandmanagement.py
test_oobm_issue_power_cycle	`Error`	19.58	test_outofbandmanagement.py
test_oobm_issue_power_off	`Error`	19.59	test_outofbandmanagement.py
test_oobm_issue_power_on	`Error`	18.55	test_outofbandmanagement.py
test_oobm_issue_power_reset	`Error`	19.57	test_outofbandmanagement.py
test_oobm_issue_power_soft	`Error`	19.59	test_outofbandmanagement.py
test_oobm_issue_power_status	`Error`	19.56	test_outofbandmanagement.py
test_oobm_multiple_mgmt_server_ownership	`Failure`	27.63	test_outofbandmanagement.py
test_oobm_zchange_password	`Error`	6.27	test_outofbandmanagement.py
test_hostha_kvm_host_degraded	`Error`	9.55	test_hostha_kvm.py
test_hostha_kvm_host_fencing	`Error`	9.65	test_hostha_kvm.py
test_hostha_kvm_host_recovering	`Error`	8.59	test_hostha_kvm.py

Handle migrate vm with volume where data disk is not based off a temp…

4cf7e99

…late

Pearl1594 requested a review from DaanHoogland March 8, 2021 11:39

Pearl1594 added this to the 4.15.1.0 milestone Mar 8, 2021

Pearl1594 added the component:migration label Mar 8, 2021

Pearl1594 mentioned this pull request Mar 8, 2021

issues with live migration on centos (was: Trillian test failures) #4757

Closed

harikrishna-patnala reviewed Mar 8, 2021

View reviewed changes

DaanHoogland approved these changes Mar 8, 2021

View reviewed changes

Pearl1594 closed this Mar 8, 2021

Conversation

Pearl1594 commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

How Has This Been Tested?

Uh oh!

Pearl1594 commented Mar 8, 2021

Uh oh!

blueorangutan commented Mar 8, 2021

Uh oh!

blueorangutan commented Mar 8, 2021

Uh oh!

harikrishna-patnala Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

Pearl1594 Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi commented Mar 8, 2021

Uh oh!

DaanHoogland commented Mar 8, 2021

Uh oh!

GabrielBrascher commented Mar 8, 2021

Uh oh!

GabrielBrascher commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GutoVeronezi commented Mar 8, 2021

Uh oh!

GutoVeronezi commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GabrielBrascher commented Mar 8, 2021

Uh oh!

GabrielBrascher commented Mar 8, 2021

Uh oh!

Pearl1594 commented Mar 8, 2021

Uh oh!

DaanHoogland commented Mar 8, 2021

Uh oh!

blueorangutan commented Mar 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Pearl1594 commented Mar 8, 2021 •

edited

Loading

GabrielBrascher commented Mar 8, 2021 •

edited

Loading

GutoVeronezi commented Mar 8, 2021 •

edited

Loading