[CLOUDSTACK-10240] ACS cannot migrate a local volume to shared storage #2425

rafaelweingartner · 2018-01-23T19:33:00Z

CloudStack is logically restricting the migration of local storages to shared storage and vice versa. This restriction is a logical one and can be removed for XenServer deployments. Therefore, we will enable migration of volumes between local-shared storages in XenServers independently of their service offering. This will work as an override mechanism to the disk offering used by volumes. If administrators want to migrate local volumes to a shared storage, they should be able to do so (the hypervisor already allows that). The same the other way around.

Extra information for reviewers:
This PS is introducing an overriding mechanism for volumes placement. We are providing a way for root administrators to override service/disk offering definitions by allocation volumes in storage pools that they could not have been allocated before if we followed their(volumes) service/disk offerings.
Therefore, it will not change the type of disk/service offerings when migrating volumes between local/shared pools. We will simply migrate them as requested by root admin. Furthermore, volumes will only be able to migrate to "suitable" storage pools. This means, storage pools that have same tags as the disk/service offering.

In summary, we are overriding placement considering location (shared/local) storage pools. However, we still consider storage tags to decide which storage pools are suitable or not.

DaanHoogland

A lot of good and welcome refactorings. to much to review in one commit IMNSHO. next time please separate the reformats, the refactorings and the logic changes out, for easier review.

small change please, the logger name.

DaanHoogland · 2018-01-24T08:37:30Z

...in/java/org/apache/cloudstack/api/command/admin/storage/FindStoragePoolsForMigrationCmd.java

+@APICommand(name = "findStoragePoolsForMigration", description = "Lists storage pools available for migration of a volume.", responseObject = StoragePoolResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
 public class FindStoragePoolsForMigrationCmd extends BaseListCmd {
-    public static final Logger s_logger = Logger.getLogger(FindStoragePoolsForMigrationCmd.class.getName());
+    public static final Logger s_logger = Logger.getLogger(FindStoragePoolsForMigrationCmdTest.class.getName());


this is not the test class!

Thanks! Fixed.

it is stil there, did you push?

I forgot to push :(
my bad!

rafaelweingartner · 2018-01-27T09:07:47Z

@daan, thanks for the feedback here!
I am starting to doubt that we are all using the same code formatter (I am using this one [1]). I can remove the code formatting changes and leave only the methods that I created if you prefer.

[1] https://cwiki.apache.org/confluence/download/attachments/29687985/ApacheCloudStack.xml?version=1&modificationDate=1382960469000&api=v2

DaanHoogland · 2018-01-27T09:55:19Z

@rafaelweingartner that is the only true formatter for cloudstack, isn't it. still i'd like to have reformattings in a different commit. the mindset of reviewing reformatting is a bit different than that of reviewing semantic/logic changes.

rafaelweingartner · 2018-01-27T11:53:15Z

I would say that there is only one formatting style for ACS. However, I have seen so many PRs like this one where we see a lot of formatting changes that I am starting to doubt if we are all using the same.

I agree with you. My intention here was never to do code formatting. My eclipse is configured to format only edit lines, but for some reason it formatted all of the Java files I touched (maybe I pressed a ctrl+shift+f without noticing). Would you like me to remove the formatting and leave only the real changes I am introducing here?

DaanHoogland · 2018-01-27T18:50:38Z

no, never mind. (next time better!)

rafaelweingartner · 2018-01-27T18:58:58Z

ok @DaanHoogland.
Thanks!

…torage. CloudStack is logically restricting the migration of local storages to shared storage and vice versa. This restriction is a logical one and can be removed for XenServer deployments. Therefore, we will enable migration of volumes between local-shared storages in XenServers independently of their service offering. This will work as an override mechanism to the disk offering used by volumes. If administrators want to migrate local volumes to a shared storage, they should be able to do so (the hypervisor already allows that). The same the other way around.

rafaelweingartner · 2018-02-15T17:56:43Z

@DaanHoogland I separated the code changes from formatting one as your suggestion. I think it is way better now.

blueorangutan · 2018-03-02T12:06:14Z

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

blueorangutan · 2018-03-02T20:37:19Z

Trillian test result (tid-2303)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29005 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2425-t2303-kvm-centos7.zip
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_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 60 look OK, 7 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	`Error`	1.06	test_primary_storage.py
test_01_primary_storage_nfs	`Error`	0.17	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.29	test_primary_storage.py
test_04_rvpc_privategw_static_routes	`Failure`	346.33	test_privategw_acl.py
test_02_list_snapshots_with_removed_data_store	`Error`	1.26	test_snapshots.py
test_08_migrate_vm	`Error`	125.28	test_vm_life_cycle.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	289.85	test_vpc_redundant.py
test_01_cancel_host_maintenace_with_no_migration_jobs	`Failure`	0.19	test_host_maintenance.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	3.50	test_host_maintenance.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	3.14	test_hostha_kvm.py

rafaelweingartner · 2018-03-02T21:03:46Z

Now the failures increased... :)

DaanHoogland · 2018-03-05T08:48:37Z

:( jenkins had crashed due to disk full, should not be related and should not be. acting insane:
@blueorangutan test

blueorangutan · 2018-03-05T08:48:46Z

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

blueorangutan · 2018-03-05T17:18:15Z

Trillian test result (tid-2312)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28936 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2425-t2312-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_outofbandmanagement.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. 66 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_hostha_enable_ha_when_host_in_maintenance	`Error`	1.86	test_hostha_kvm.py

rafaelweingartner · 2018-03-06T17:05:08Z

@DaanHoogland this test test_hostha_enable_ha_when_host_in_maintenance error seems to be persistent in other PRs as well. Can I consider that the tests finished successfully?

rafaelweingartner · 2018-03-06T17:43:42Z

@khos2ow would you mind reviewing this PR?

DaanHoogland · 2018-03-07T08:56:44Z

@rafaelweingartner , yes you can. It seems to be a timing issue. the host does not have a state needed in some occurences and does have that state in other runs.

rafaelweingartner · 2018-03-07T10:10:41Z

Thanks @DaanHoogland!

khos2ow

I haven't had the chance to manual test this myself, but the code LGTM. +1.

rafaelweingartner · 2018-03-07T18:53:05Z

If we have no objections I will be merging this one latter today.

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.

…on (#2486) * [CLOUDSTACK-10323] Allow changing disk offering during volume migration 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. * Code formatting * Adding a test to cover migration with new disk offering (#4) * Adding a test to cover migration with new disk offering * Update test_volumes.py * Update test_volumes.py * fix test_11_migrate_volume_and_change_offering * Fix typo in Java doc

DaanHoogland requested changes Jan 24, 2018

View reviewed changes

rafaelweingartner force-pushed the CLOUDSTACK-10240 branch 5 times, most recently from 758f21f to 13cad9f Compare January 26, 2018 22:19

DaanHoogland approved these changes Jan 27, 2018

View reviewed changes

rafaelweingartner self-assigned this Jan 29, 2018

rafaelweingartner added component:XenServer component:primary-storage type:improvement labels Jan 30, 2018

rafaelweingartner added this to the 4.12 milestone Jan 30, 2018

rafaelweingartner force-pushed the CLOUDSTACK-10240 branch from 13cad9f to d0baf33 Compare February 15, 2018 17:51

rafaelweingartner force-pushed the CLOUDSTACK-10240 branch 10 times, most recently from 67aad8e to 26c559a Compare February 17, 2018 02:15

rafaelweingartner mentioned this pull request Mar 5, 2018

[CLOUDSTACK-10197] Update DisplayText of XenServer tools ISO entry in the database when it already exists #2470

Merged

rafaelweingartner requested a review from GabrielBrascher March 5, 2018 20:21

khos2ow approved these changes Mar 7, 2018

View reviewed changes

rafaelweingartner merged commit f2efbce into apache:master Mar 7, 2018

rafaelweingartner mentioned this pull request Mar 14, 2018

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

Merged

rafaelweingartner mentioned this pull request Jul 23, 2018

Add managed storage pool constraints to MigrateWithVolume API method #2761

Merged

12 tasks

PaulAngus mentioned this pull request Aug 16, 2018

Uploaded Volume cannot be attached to VM with local storage #2730

Closed

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

JoaoJandre mentioned this pull request Jun 13, 2024

Fix migration from local storage to NFS in KVM #8909

Merged

13 tasks

[CLOUDSTACK-10240] ACS cannot migrate a local volume to shared storage #2425

[CLOUDSTACK-10240] ACS cannot migrate a local volume to shared storage #2425

Uh oh!

Conversation

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

Uh oh!

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner commented Jan 27, 2018

Uh oh!

DaanHoogland commented Jan 27, 2018

Uh oh!

rafaelweingartner commented Jan 27, 2018

Uh oh!

DaanHoogland commented Jan 27, 2018

Uh oh!

rafaelweingartner commented Jan 27, 2018

Uh oh!

rafaelweingartner commented Feb 15, 2018

Uh oh!

blueorangutan commented Mar 2, 2018

Uh oh!

blueorangutan commented Mar 2, 2018

Uh oh!

rafaelweingartner commented Mar 2, 2018

Uh oh!

DaanHoogland commented Mar 5, 2018

Uh oh!

blueorangutan commented Mar 5, 2018

Uh oh!

blueorangutan commented Mar 5, 2018

Uh oh!

rafaelweingartner commented Mar 6, 2018

Uh oh!

rafaelweingartner commented Mar 6, 2018

Uh oh!

DaanHoogland commented Mar 7, 2018

Uh oh!

rafaelweingartner commented Mar 7, 2018

Uh oh!

khos2ow left a comment

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner commented Mar 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rafaelweingartner commented Jan 23, 2018 •

edited

Loading