Keep volume policies after migrating it to another primary storage by GutoVeronezi · Pull Request #5067 · apache/cloudstack

GutoVeronezi · 2021-06-02T13:36:35Z

Description

In ACS, there are two ways to migrate a volume between storages:

migrating only the volume (migrateVolume);
migrating a VM with volume (migrateVirtualMachineWithVolume);

Both, although are in differents flows, work similarly: they create a new volume in the destination storage and then delete the old volume in the source storage; When we delete the old volume, we also delete the volume policies (snapshot scheduling).

Therefore, the new volume will not have the snapshot schedules and we will lose this information. This PR intends to copy the policies from source to destination volume before deleting the old volume;

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

Feature/Enhancement Scale

Major
Minor

How Has This Been Tested?

It has been tested locally in a test lab.

I created a VM;
I configured snapshot schedules in the volume;
I migrated :
- volume (with migrateVolume);
- VM and volume (with migrateVirtualMachineWithVolume);
I checked if the policies were copied to new volume;

-- After applying this patch, the policies were copied.
Also, I created Java unit tests for the methods I changed.

… not have yet

…ation

sureshanaparti · 2021-06-04T11:31:32Z

engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java


+    @Override
+    public boolean copyPoliciesBetweenVolumesAndDestroySourceVolumeAfterMigration(Event destinationEvent, Answer destinationEventAnswer, VolumeInfo sourceVolume,
+      VolumeInfo destinationVolume, boolean retryExpungeVolumeAsync) {


@GutoVeronezi I think, it is better to separate copy policies and destroy volume.

@sureshanaparti I extracted the code that destroy the source volume to a method and added tests to it; However I kept this method, calling both copy policies and destroy volume to facilitate and guide the process.

GutoVeronezi · 2021-06-14T14:47:25Z

Can anyone review this?

DaanHoogland

This is a lot of good improvements in log and exception messages as well as a solid looking re-factor of the volume object and - service. I t will need extensive regression testing on anything that relates to that. I don;t see hypervisor specific risks but these may be hidden in there.
CLGTM

DaanHoogland · 2021-06-15T21:27:42Z

engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java

+     * @param destinationEvent
+     * @param destinationEventAnswer
+     * @param sourceVolume
+     * @param destinationVolume
+     * @param retryExpungeVolumeAsync


maybe remove these if there is no sensible description to be given.

@DaanHoogland removed, thanks.

DaanHoogland · 2021-06-15T21:41:25Z

engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeObjectTest.java

-    /**
-     * Tests the following scenario:
-     * If the volume gets deleted by another thread (cleanup) and the cleanup is attempted again, the volume isnt found in DB and hence NPE occurs
-     * during transition


why delete this javadoc. it seems to add vallue to the method name, which doesn't describe what it does. validateHandleProcessEventCopyCmdAnswerNotPrimaryStoreDoNotSetSize is a bit cryptic.

@DaanHoogland This whole test class, as well as those in test package in this module were deleted. The tests in this module were already disabled for a long time; therefore we considered that they were irrelevant, as they were a bunch of code that were not being used.

ah right, it isn't a change but an addition and a deletion, sorry and thanks

blueorangutan · 2021-06-21T09:12:32Z

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 311

blueorangutan · 2021-06-22T07:57:16Z

Trillian Build Failed (tid-1033)

GutoVeronezi · 2021-07-20T12:38:01Z

Can anyone review this?

DaanHoogland · 2021-07-20T13:28:18Z

@GutoVeronezi a trick for getting the right person is running git blame on the files and see if there is the name of an active contributor there to ask directly.

GutoVeronezi · 2021-07-20T16:36:00Z

@GutoVeronezi a trick for getting the right person is running git blame on the files and see if there is the name of an active contributor there to ask directly.

Thanks for the tip @DaanHoogland.

blueorangutan · 2021-07-28T13:50:11Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 671

blueorangutan · 2021-07-28T23:47:32Z

Trillian test result (tid-1395)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33932 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5067-t1395-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland · 2021-07-29T09:42:43Z

@sureshanaparti does this LGTY? cc @nvazquez

nvazquez · 2021-07-29T17:40:01Z

Looking good, I think it will need manual testing for migrations and also volumes regression testing, @sureshanaparti is this something you can check please?

GutoVeronezi · 2021-08-16T12:40:33Z

@nvazquez any update about this one?

nvazquez · 2021-08-25T03:33:12Z

@weizhouapache can you please review/verify this improvement?

weizhouapache · 2021-09-02T10:52:24Z

@blueorangutan package

blueorangutan · 2021-09-02T10:53:03Z

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

blueorangutan · 2021-09-02T11:48:18Z

Packaging result: ✔️ el7 ✖️ el8 ✔️ debian ✔️ suse15. SL-JID 1105

weizhouapache · 2021-09-02T11:52:55Z

@blueorangutan package

blueorangutan · 2021-09-02T11:54:03Z

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

blueorangutan · 2021-09-02T12:21:27Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1109

weizhouapache · 2021-09-02T12:22:05Z

@blueorangutan test

blueorangutan · 2021-09-02T12:23:02Z

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

blueorangutan · 2021-09-03T00:56:28Z

Trillian test result (tid-1941)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43228 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5067-t1941-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

weizhouapache

tested ok on kvm.

(1) both root and data disk
(2) offline volume migration (migratevolume)
(3) online volume migration (migratevirtualmachinewithvolume)

yadvr · 2021-09-03T11:34:32Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2021-09-03T11:35:02Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

blueorangutan · 2021-09-03T23:11:50Z

Trillian test result (tid-1962)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40370 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5067-t1962-vmware-67u3.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

nvazquez

LGTM

Daniel Augusto Veronezi Salvador added 8 commits June 1, 2021 13:22

Add commons-lang3 to Utils

e923cb9

Create an util to provide methods that ReflectionToStringBuilder does…

b088f82

… not have yet

Create method to retrieve map of tags from resource

5b79c17

Enable tests on volume components and remove useless tests

47bc12a

Refactor VolumeObject and add unit tests

673573f

Extract createPolicy in several methods

9b94ddc

Create method to copy policies between volumes and add unit tests

2c8a8a9

Copy policies to new volume before removing old volume on volume migr…

d89bff3

…ation

sureshanaparti reviewed Jun 4, 2021

View reviewed changes

Extract "destroySourceVolumeAfterMigration" to a method and test it

389a30e

DaanHoogland approved these changes Jun 18, 2021

View reviewed changes

Remove javadoc @param with no sensible information

02e9ea7

apache deleted a comment from blueorangutan Jun 21, 2021

yadvr requested review from harikrishna-patnala and nvazquez June 29, 2021 07:29

sureshanaparti added this to the 4.16.0.0 milestone Jul 16, 2021

Rename method name to a generic name

2bac481

nvazquez added component:migration status:needs-testing type:enhancement labels Aug 25, 2021

weizhouapache approved these changes Sep 3, 2021

View reviewed changes

nvazquez approved these changes Sep 8, 2021

View reviewed changes

nvazquez merged commit 8ffba83 into apache:main Sep 8, 2021

DaanHoogland removed the status:needs-testing label Jan 28, 2026

Conversation

GutoVeronezi commented Jun 2, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

How Has This Been Tested?

Uh oh!

sureshanaparti Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi commented Jun 14, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jun 21, 2021

Uh oh!

blueorangutan commented Jun 22, 2021

Uh oh!

GutoVeronezi commented Jul 20, 2021

Uh oh!

DaanHoogland commented Jul 20, 2021

Uh oh!

GutoVeronezi commented Jul 20, 2021

Uh oh!

blueorangutan commented Jul 28, 2021

Uh oh!

blueorangutan commented Jul 28, 2021

Uh oh!

DaanHoogland commented Jul 29, 2021

Uh oh!

nvazquez commented Jul 29, 2021

Uh oh!

GutoVeronezi commented Aug 16, 2021

Uh oh!

nvazquez commented Aug 25, 2021

Uh oh!

weizhouapache commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 2, 2021

Uh oh!

weizhouapache commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 2, 2021

Uh oh!

weizhouapache commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 2, 2021

Uh oh!

blueorangutan commented Sep 3, 2021

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Sep 3, 2021

Uh oh!

blueorangutan commented Sep 3, 2021