[CLOUDSTACK-10323] Allow changing disk offering during volume migration by rafaelweingartner · Pull Request #2486 · apache/cloudstack

rafaelweingartner · 2018-03-14T00:42:51Z

This is a continuation of work developed on PR #2425 (CLOUDSTACK-10240), which provided root admins an override mechanism to move volumes between storage systems types (local/shared) even when the disk offering would not allow such operation. To complete the work, we will now provide a way for administrators to enter a new disk offering that can reflect the new placement of the volume. We will add an extra parameter to allow the root admin inform a new disk offering for the volume. Therefore, when the volume is being migrated, it will be possible to replace the disk offering to reflect the new placement of the volume.

The API method will have the following parameters:

storageid (required)
volumeid (required)
livemigrate(optional)
newdiskofferingid (optional) – this is the new parameter

The expected behavior is the following:

If “newdiskofferingid” is not provided the current behavior is maintained. Override mechanism will also keep working as we have seen so far.
If the “newdiskofferingid” is provided by the admin, we will execute the following checks
- new disk offering mode (local/shared) must match the target storage mode. If it does not match, an exception will be thrown and the operator will receive a message indicating the problem.
- we will check if the new disk offering tags match the target storage tags. If it does not match, an exception will be thrown and the operator will receive a message indicating the problem.
- check if the target storage has the capacity for the new volume. If it does not have enough space, then an exception is thrown and the operator will receive a message indicating the problem.
- check if the size of the volume is the same as the size of the new disk offering. If it is not the same, we will ALLOW the change of the service offering, and a warning message will be logged.

We execute the change of the Disk offering as soon as the migration of the volume finishes. Therefore, if an error happens during the migration and the volume remains in the original storage system, the disk offering will keep reflecting this situation.

Attached is a screenshot to show how the migrate option will look like:

borisstoyanov · 2018-03-14T12:08:08Z

thanks for this PR @rafaelweingartner, are you looking into adding a marvin tests for this ?
I'll wait on your response and kick the smoketests.

rafaelweingartner · 2018-03-14T12:10:10Z

I added unit test for the code I changed. I was not planning to create a Marvin tests though.
If you guys think it is a requirement, I can look into it.

borisstoyanov · 2018-03-14T12:24:52Z

@blueorangutan package

blueorangutan · 2018-03-14T12:25:27Z

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

borisstoyanov · 2018-03-14T12:25:32Z

ok @rafaelweingartner, let me run some tests

blueorangutan · 2018-03-14T13:05:28Z

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

borisstoyanov · 2018-03-14T13:19:59Z

@blueorangutan test

blueorangutan · 2018-03-14T13:20:27Z

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

blueorangutan · 2018-03-14T22:30:35Z

Trillian test result (tid-2363)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31355 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2486-t2363-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_public_ip_range.py
Intermitten failure detected: /marvin/tests/smoke/test_ssvm.py
Intermitten failure detected: /marvin/tests/smoke/test_templates.py
Intermitten failure detected: /marvin/tests/smoke/test_usage.py
Intermitten failure detected: /marvin/tests/smoke/test_volumes.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 62 look OK, 5 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_04_extract_template	`Failure`	128.33	test_templates.py
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
test_06_download_detached_volume	`Failure`	137.59	test_volumes.py
test_01_redundant_vpc_site2site_vpn	`Failure`	325.32	test_vpc_vpn.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	0.45	test_hostha_kvm.py

borisstoyanov · 2018-03-15T09:48:25Z

@rafaelweingartner I've created a marvin test but I'm not able to push to your repo. How can I contribute?

rafaelweingartner · 2018-03-15T10:22:21Z

Thanks!
You can open a PR against my branch, then I can merge it here.

borisstoyanov · 2018-03-15T10:53:39Z

I'm not sure I'm doing this right.. but git says there's nothing to compare..

any idea? @rafaelweingartner

rafaelweingartner · 2018-03-15T10:59:54Z

Did you pushed your changes to your branch in the remote repository?
I am checking your branch, but there is no new commit there.

borisstoyanov · 2018-03-15T11:41:02Z

@blueorangutan package

blueorangutan · 2018-03-15T11:41:31Z

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

blueorangutan · 2018-03-15T12:07:16Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-1781

blueorangutan · 2018-03-15T13:16:31Z

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

DaanHoogland · 2018-03-15T14:17:47Z

debian packaging failed and so did jenkins and travis!

rafaelweingartner · 2018-03-15T14:18:41Z

Yes. However, that is a consequence of the last merge and is fixed with: #2491

borisstoyanov · 2018-03-16T07:58:05Z

@blueorangutan test

blueorangutan · 2018-03-16T07:58:34Z

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

borisstoyanov · 2018-03-16T08:34:07Z

@blueorangutan test

blueorangutan · 2018-03-16T08:34:34Z

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

This is a continuation of work developed on PR apache#2425 (CLOUDSTACK-10240), which provided root admins an override mechanism to move volumes between storage systems types (local/shared) even when the disk offering would not allow such operation. To complete the work, we will now provide a way for administrators to enter a new disk offering that can reflect the new placement of the volume. We will add an extra parameter to allow the root admin inform a new disk offering for the volume. Therefore, when the volume is being migrated, it will be possible to replace the disk offering to reflect the new placement of the volume. The API method will have the following parameters: * storageid (required) * volumeid (required) * livemigrate(optional) * newdiskofferingid (optional) – this is the new parameter The expected behavior is the following: * If “newdiskofferingid” is not provided the current behavior is maintained. Override mechanism will also keep working as we have seen so far. * If the “newdiskofferingid” is provided by the admin, we will execute the following checks ** new disk offering mode (local/shared) must match the target storage mode. If it does not match, an exception will be thrown and the operator will receive a message indicating the problem. ** we will check if the new disk offering tags match the target storage tags. If it does not match, an exception will be thrown and the operator will receive a message indicating the problem. ** check if the target storage has the capacity for the new volume. If it does not have enough space, then an exception is thrown and the operator will receive a message indicating the problem. ** check if the size of the volume is the same as the size of the new disk offering. If it is not the same, we will ALLOW the change of the service offering, and a warning message will be logged. We execute the change of the Disk offering as soon as the migration of the volume finishes. Therefore, if an error happens during the migration and the volume remains in the original storage system, the disk offering will keep reflecting this situation.

* Adding a test to cover migration with new disk offering * Update test_volumes.py * Update test_volumes.py

rafaelweingartner · 2018-03-16T10:45:56Z

@DaanHoogland now everything is ok again.

DaanHoogland · 2018-03-16T11:08:23Z

thanks @rafaelweingartner , didn't notice the relation

blueorangutan · 2018-04-07T19:01:35Z

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

blueorangutan · 2018-04-07T19:31:47Z

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

DaanHoogland · 2018-04-08T18:25:23Z

@blueorangutan test

blueorangutan · 2018-04-08T18:26:36Z

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

blueorangutan · 2018-04-09T20:27:44Z

Trillian test result (tid-2471)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 91858 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2486-t2471-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_certauthority_root.py
Intermitten failure detected: /marvin/tests/smoke/test_primary_storage.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_routers.py
Intermitten failure detected: /marvin/tests/smoke/test_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 61 look OK, 6 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	`Error`	0.61	test_primary_storage.py
test_01_primary_storage_nfs	`Error`	0.13	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.24	test_primary_storage.py
test_04_restart_network_wo_cleanup	`Failure`	3.27	test_routers.py
test_02_list_snapshots_with_removed_data_store	`Error`	1.15	test_snapshots.py
test_08_migrate_vm	`Error`	17.79	test_vm_life_cycle.py
test_01_cancel_host_maintenace_with_no_migration_jobs	`Failure`	6.34	test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	2.29	test_host_maintenance.py
test_hostha_kvm_host_fencing	`Failure`	1491.43	test_hostha_kvm.py
test_hostha_kvm_host_recovering	`Failure`	616.59	test_hostha_kvm.py

rafaelweingartner · 2018-04-09T20:29:44Z

@borisstoyanov with that change in the test case, now the test we created for this PR passed. Some tests failed, but they are different from the last run.

borisstoyanov · 2018-04-10T06:55:47Z

yup, let me run it again @blueorangutan test

blueorangutan · 2018-04-10T06:56:36Z

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

PaulAngus · 2018-04-10T12:14:37Z

@blueorangutan test

blueorangutan · 2018-04-10T12:15:47Z

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

blueorangutan · 2018-04-11T15:56:37Z

Trillian test result (tid-2488)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 97660 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2486-t2488-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_certauthority_root.py
Intermitten failure detected: /marvin/tests/smoke/test_primary_storage.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_routers.py
Intermitten failure detected: /marvin/tests/smoke/test_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_ssvm.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 61 look OK, 6 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	`Error`	0.74	test_primary_storage.py
test_01_primary_storage_nfs	`Error`	0.12	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.21	test_primary_storage.py
test_04_restart_network_wo_cleanup	`Failure`	4.11	test_routers.py
test_02_list_snapshots_with_removed_data_store	`Error`	1.16	test_snapshots.py
test_08_migrate_vm	`Error`	18.91	test_vm_life_cycle.py
test_01_cancel_host_maintenace_with_no_migration_jobs	`Failure`	1.14	test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	2.31	test_host_maintenance.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	1.56	test_hostha_kvm.py
test_hostha_kvm_host_degraded	`Failure`	1503.52	test_hostha_kvm.py
test_hostha_kvm_host_fencing	`Failure`	619.78	test_hostha_kvm.py
test_hostha_kvm_host_recovering	`Failure`	621.92	test_hostha_kvm.py

borisstoyanov

LGTM

rafaelweingartner · 2018-04-16T14:04:41Z

@nitin-maharana are you LGTM here?

GabrielBrascher

Thanks for the PR @rafaelweingartner.
I have just two minor observations on Javadoc.

The code is well documented and covered with unit tests; additionally, it has passed on integration tests. Based on that, it LGTM.

GabrielBrascher · 2018-04-26T19:32:28Z

- StoragePool destPool = (StoragePool)dataStoreMgr.getDataStore(destPoolId, DataStoreRole.Primary);
- assert (destPool != null);
+ /**
+ * Retrieve the new disk offering UUID that might be sent to replace the current one in the volume being migrated.


I think that it should be "Retrieves", right?

If you read as "it (the method) retrieves the new .....". Then, yes, I guess it should be "Retrieves".
I will change that.

GabrielBrascher · 2018-04-26T20:41:37Z

+ }
+
+ /**
+ * Retrieve the storage pool tags as a {@link String}. If the storage pool does not have tags we return a null value.


I think that "Retrieves" would best fit here.

…l selected After using the feature introduced by apache#2486 in production, we felt the need for an improvement in the UI. It is interesting to filter the displayed disk offerings according to the type of storage selected (local/shared) to migrate the volume to.

…l selected (#2612) After using the feature introduced by #2486 in production, we felt the need for an improvement in the UI. It is interesting to filter the displayed disk offerings according to the type of storage selected (local/shared) to migrate the volume to.

rafaelweingartner added this to the 4.12.0.0 milestone Mar 14, 2018

rafaelweingartnerand others added 3 commits March 16, 2018 06:53

Code formatting
040752c

Adding a test to cover migration with new disk offering (#4)
4b87733
* Adding a test to cover migration with new disk offering * Update test_volumes.py * Update test_volumes.py

rafaelweingartner force-pushed the CLOUDSTACK-10323 branch from 3f90089 to 4b87733Compare March 16, 2018 09:53

apache deleted a comment from nitin-maharana Apr 10, 2018

borisstoyanov approved these changes Apr 12, 2018
View reviewed changes

mike-tutkowski mentioned this pull request Apr 12, 2018
Updates to capacity management #2499
Merged
11 tasks

rafaelweingartner added type:enhancement component:management-server labels Apr 20, 2018

GabrielBrascher approved these changes Apr 26, 2018
View reviewed changes

Fix typo in Java doc
06f3421

rafaelweingartner merged commit d6cbd77 into apache:masterApr 26, 2018

rafaelweingartner mentioned this pull request Apr 27, 2018
Allow changing disk offering of VMs' root volume during volume migration #2607
Merged
12 tasks

rafaelweingartner mentioned this pull request Apr 27, 2018
[migrateVolume API method] Filter disk offerings based on target storage pool selected #2612
Merged
12 tasks

rafaelweingartner mentioned this pull request Sep 22, 2020
Removes unnecessary validations #4283
Closed
5 tasks

sureshanaparti mentioned this pull request Nov 20, 2020
Find suitable storage for migration regression #4400
Closed

Uh oh!

Conversation

rafaelweingartner commented Mar 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borisstoyanov commented Mar 14, 2018

Uh oh!

rafaelweingartner commented Mar 14, 2018

Uh oh!

borisstoyanov commented Mar 14, 2018

Uh oh!

blueorangutan commented Mar 14, 2018

Uh oh!

borisstoyanov commented Mar 14, 2018

Uh oh!

blueorangutan commented Mar 14, 2018

Uh oh!

borisstoyanov commented Mar 14, 2018

Uh oh!

blueorangutan commented Mar 14, 2018

Uh oh!

blueorangutan commented Mar 14, 2018

Uh oh!

borisstoyanov commented Mar 15, 2018

Uh oh!

rafaelweingartner commented Mar 15, 2018

Uh oh!

borisstoyanov commented Mar 15, 2018

Uh oh!

rafaelweingartner commented Mar 15, 2018

Uh oh!

borisstoyanov commented Mar 15, 2018

Uh oh!

blueorangutan commented Mar 15, 2018

Uh oh!

blueorangutan commented Mar 15, 2018

Uh oh!

blueorangutan commented Mar 15, 2018

Uh oh!

DaanHoogland commented Mar 15, 2018

Uh oh!

rafaelweingartner commented Mar 15, 2018

Uh oh!

borisstoyanov commented Mar 16, 2018

Uh oh!

blueorangutan commented Mar 16, 2018

Uh oh!

borisstoyanov commented Mar 16, 2018

Uh oh!

blueorangutan commented Mar 16, 2018

Uh oh!

rafaelweingartner commented Mar 16, 2018

Uh oh!

DaanHoogland commented Mar 16, 2018

Uh oh!

blueorangutan commented Apr 7, 2018

Uh oh!

blueorangutan commented Apr 7, 2018

Uh oh!

DaanHoogland commented Apr 8, 2018

Uh oh!

blueorangutan commented Apr 8, 2018

Uh oh!

blueorangutan commented Apr 9, 2018

Uh oh!

rafaelweingartner commented Apr 9, 2018

Uh oh!

borisstoyanov commented Apr 10, 2018

Uh oh!

blueorangutan commented Apr 10, 2018

Uh oh!

PaulAngus commented Apr 10, 2018

Uh oh!

blueorangutan commented Apr 10, 2018

Uh oh!

blueorangutan commented Apr 11, 2018

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner commented Mar 14, 2018•
edited
Loading