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

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:

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.

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.

@blueorangutan package

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

ok @rafaelweingartner, let me run some tests

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

@blueorangutan test

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

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

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

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

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

any idea? @rafaelweingartner

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

@blueorangutan package

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

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

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

debian packaging failed and so did jenkins and travis!

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

@blueorangutan test

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

@blueorangutan test

@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

@DaanHoogland now everything is ok again.

thanks @rafaelweingartner , didn't notice the relation

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

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

@blueorangutan test

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

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

@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.

yup, let me run it again @blueorangutan test

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

@blueorangutan test

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

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

LGTM

@nitin-maharana are you LGTM here?

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.

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.

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

rafaelweingartner and 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 4b87733 Compare 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:master Apr 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

-                      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.

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

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

Uh oh!

[CLOUDSTACK-10323] Allow changing disk offering during volume migration - #2486

[CLOUDSTACK-10323] Allow changing disk offering during volume migration #2486rafaelweingartner merged 5 commits intoapache:masterapache/cloudstack:masterfrom rafaelweingartner:CLOUDSTACK-10323rafaelweingartner/cloudstack:CLOUDSTACK-10323Copy head branch name to clipboard

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!

[CLOUDSTACK-10323] Allow changing disk offering during volume migration #2486
rafaelweingartner merged 5 commits into
apache:masterfrom
rafaelweingartner:CLOUDSTACK-10323

rafaelweingartner commented Mar 14, 2018 •
edited
Loading