Secondary Storage Usage Improvements by Pearl1594 · Pull Request #4053 · apache/cloudstack

Pearl1594 · 2020-05-04T04:53:08Z

Description

This feature enables the following:

Balanced migration of data objects from source Image store to destination Image store(s)
Complete migration of data
setting an image store to read-only
viewing download progress of templates across all data stores

Related Primate PR: apache/cloudstack-primate#326

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)

…toring

DaanHoogland

some intermediate comments. no blockers and not done with the full review.

DaanHoogland · 2020-05-08T10:18:14Z

+    @Override
+    public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatastores, MigrationPolicy migrationPolicy) {


this method is way too long. please modularise it into byte size chunks and factor it out to a worker - or utility class?
I can not set a rule but in my opinion any method larger than 20 line or any class larger than 300 lines is probably not separating concerns.
every comment in this method should probably be converted to a good method name and extracted, and so should many top level blocks (if-, for- and while statements

Created a utility class

It is still 112 lines and there are a lot of comments in it that could serve as method names. This would help make the code more prozaic and thus easier to read. This method could do with some modularisation.

75 lines, still big but i slowly get the picture ;)

DaanHoogland · 2020-05-18T15:12:50Z

+    @Override
+    public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatastores, MigrationPolicy migrationPolicy) {


It is still 112 lines and there are a lot of comments in it that could serve as method names. This would help make the code more prozaic and thus easier to read. This method could do with some modularisation.

blueorangutan · 2020-09-14T07:02:00Z

Packaging result: ✖centos7 ✖centos8 ✖debian. JID-1990

yadvr · 2020-09-14T07:07:44Z

@blueorangutan package

blueorangutan · 2020-09-14T07:08:14Z

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

blueorangutan · 2020-09-14T09:29:38Z

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

yadvr · 2020-09-14T10:30:57Z

@blueorangutan test

blueorangutan · 2020-09-14T10:32:18Z

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

blueorangutan · 2020-09-15T06:47:13Z

Trillian test result (tid-2733)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 70668 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4053-t2733-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 81 look OK, 4 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	358.67	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	563.47	test_privategw_acl.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	450.45	test_vpc_redundant.py
test_02_redundant_VPC_default_routes	`Failure`	447.19	test_vpc_redundant.py
test_01_redundant_vpc_site2site_vpn	`Failure`	737.25	test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_options	`Error`	695.53	test_vpc_vpn.py
test_01_vpc_site2site_vpn	`Failure`	465.04	test_vpc_vpn.py
test_hostha_enable_ha_when_host_disabled	`Error`	1.58	test_hostha_kvm.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	301.71	test_hostha_kvm.py

yadvr · 2020-09-15T07:05:34Z

Tests LGTM, I checked all are intermittent failures.

…cStgMgmt

Pearl1594 · 2020-09-16T06:37:48Z

@blueorangutan package

blueorangutan · 2020-09-16T06:38:26Z

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

blueorangutan · 2020-09-16T07:10:25Z

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

Pearl1594 · 2020-09-16T07:11:19Z

@blueorangutan test

blueorangutan · 2020-09-16T07:12:20Z

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

yadvr · 2020-09-16T07:39:42Z

@@ -0,0 +1 @@
+cks


@Pearl1594 can you remove this file?

yadvr · 2020-09-16T07:40:01Z

@Pearl1594 pl advise if/when this is ready for merge, thnx

Pearl1594 · 2020-09-16T08:17:38Z

@rhtyd post this round of smoke test, the PR will be ready for merging

blueorangutan · 2020-09-17T02:27:43Z

Trillian test result (tid-2753)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 67152 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4053-t2753-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_domain_vpc_offerings.py
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_iso.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Intermittent failure detected: /marvin/tests/smoke/test_password_server.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 80 look OK, 5 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_04_extract_Iso	`Failure`	128.37	test_iso.py
test_01_add_delete_kubernetes_supported_version	`Error`	1812.30	test_kubernetes_supported_versions.py
test_02_vpc_privategw_static_routes	`Failure`	218.84	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	207.93	test_privategw_acl.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Error`	583.06	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	476.67	test_vpc_redundant.py
test_hostha_kvm_host_fencing	`Error`	163.16	test_hostha_kvm.py

Pearl1594 · 2020-09-17T04:40:17Z

@rhtyd - ready to merge - test failures seem unrelated to this PR

harikrishna-patnala · 2020-09-17T08:30:43Z

@@ -53,6 +53,33 @@ ALTER TABLE `cloud`.`vm_instance` ADD COLUMN `backup_offering_id` bigint unsigne
 ALTER TABLE `cloud`.`vm_instance` ADD COLUMN `backup_external_id` varchar(255) DEFAULT NULL COMMENT 'ID of external backup job or container if any';
 ALTER TABLE `cloud`.`vm_instance` ADD COLUMN `backup_volumes` text DEFAULT NULL COMMENT 'details of backedup volumes';



This schema change has to be moved to schema-41400to41500.sql.
@Pearl1594 please raise a separate PR as this is already merged.

Enable migration of data between secondary storage pools - addresses feature: apache/cloudstack#4053

Enable migration of data between secondary storage pools - addresses feature: #4053 Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

Pearl Dsilva added 4 commits May 3, 2020 17:34

Secondary Storage Management

a11686d

Merge branch 'master' of github.com:shapeblue/cloudstack into SecStgMgmt

94275d6

Code clean up and logging

ac50ee6

Refactored code

87f9360

yadvr added this to the 4.15.0.0 milestone May 5, 2020

Refactored code

e4b9cca

Pearl1594 force-pushed the SecStgMgmt branch from b72a9c8 to e4b9cca Compare May 5, 2020 13:24

Pearl1594 mentioned this pull request May 6, 2020

Form to Migrate data between Image stores apache/cloudstack-primate#326

Merged

fine tuned scaling condition + allocation algorithm GS

db6a434

Pearl1594 force-pushed the SecStgMgmt branch from b0d3023 to db6a434 Compare May 6, 2020 12:57

Code changes and cleanup

ed2fe4e

Pearl1594 marked this pull request as ready for review May 7, 2020 06:03

svenvogel added type:enhancement type:new-feature component:secondary-storage labels May 7, 2020

Refactored code

082c7c7

DaanHoogland reviewed May 8, 2020

View reviewed changes

Comment thread engine/storage/src/main/java/org/apache/cloudstack/storage/image/BaseImageStoreDriverImpl.java Outdated

Pearl1594 changed the title ~~Secondary Storage Management~~ Secondary Storage Usage Improvement May 8, 2020

Pearl1594 force-pushed the SecStgMgmt branch from d71d502 to 228c703 Compare May 8, 2020 08:42

DaanHoogland changed the title ~~Secondary Storage Usage Improvement~~ Secondary Storage Usage Improvements May 8, 2020

Added check for templates already present on destination + code refac…

3ba480f

…toring

Pearl1594 force-pushed the SecStgMgmt branch from 228c703 to 3ba480f Compare May 8, 2020 09:49

DaanHoogland reviewed May 8, 2020

View reviewed changes

Pearl Dsilva added 2 commits May 11, 2020 11:55

Address review comments + make complete migration default

b173f78

Modularized storageOrchestrator class

452fe52

Pearl1594 force-pushed the SecStgMgmt branch from 5acbff9 to 452fe52 Compare May 18, 2020 03:45

Pearl1594 requested a review from DaanHoogland May 18, 2020 06:53

apache deleted a comment from blueorangutan May 18, 2020

DaanHoogland reviewed May 18, 2020

View reviewed changes

Pearl1594 force-pushed the SecStgMgmt branch from a9c5ae7 to 0a1b524 Compare May 19, 2020 07:51

Pearl Dsilva added 2 commits September 16, 2020 11:50

Merge branch 'master' of https://github.com/apache/cloudstack into Se…

40d1818

…cStgMgmt

Correct sql query to get all active commands on ssvms

716e77a

yadvr reviewed Sep 16, 2020

View reviewed changes

Removed junk file

8a85524

yadvr merged commit b464fe4 into apache:master Sep 17, 2020

harikrishna-patnala reviewed Sep 17, 2020

View reviewed changes

PaulAngus removed Severity:Critical Critical bug status:ready-for-merge type:enhancement labels Oct 18, 2020

ACSGitBot added the type:new_feature label Nov 8, 2020

Pearl1594 mentioned this pull request Nov 17, 2020

Fix: Data migration #4475

Merged

12 tasks

weizhouapache pushed a commit to apache/cloudstack-primate that referenced this pull request Jan 19, 2021

storage: Form to Migrate data between Image stores (#326)

4ff52f1

Enable migration of data between secondary storage pools - addresses feature: apache/cloudstack#4053

yadvr pushed a commit that referenced this pull request Jan 20, 2021

storage: Form to Migrate data between Image stores (#326)

7faed91

Enable migration of data between secondary storage pools - addresses feature: #4053 Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

		@Override
		public MigrationResponse migrateData(Long srcDataStoreId, List<Long> destDatastores, MigrationPolicy migrationPolicy) {

		@@ -53,6 +53,33 @@ ALTER TABLE `cloud`.`vm_instance` ADD COLUMN `backup_offering_id` bigint unsigne
		ALTER TABLE `cloud`.`vm_instance` ADD COLUMN `backup_external_id` varchar(255) DEFAULT NULL COMMENT 'ID of external backup job or container if any';
		ALTER TABLE `cloud`.`vm_instance` ADD COLUMN `backup_volumes` text DEFAULT NULL COMMENT 'details of backedup volumes';

		@@ -0,0 +1 @@
		cks

Conversation

Pearl1594 commented May 4, 2020 • edited by PaulAngus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Uh oh!

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Sep 14, 2020

Uh oh!

yadvr commented Sep 14, 2020

Uh oh!

blueorangutan commented Sep 14, 2020

Uh oh!

blueorangutan commented Sep 14, 2020

Uh oh!

yadvr commented Sep 14, 2020

Uh oh!

blueorangutan commented Sep 14, 2020

Uh oh!

blueorangutan commented Sep 15, 2020

Uh oh!

yadvr commented Sep 15, 2020

Uh oh!

Pearl1594 commented Sep 16, 2020

Uh oh!

blueorangutan commented Sep 16, 2020

Uh oh!

blueorangutan commented Sep 16, 2020

Uh oh!

Pearl1594 commented Sep 16, 2020

Uh oh!

blueorangutan commented Sep 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented Sep 16, 2020

Uh oh!

Pearl1594 commented Sep 16, 2020

Uh oh!

blueorangutan commented Sep 17, 2020

Uh oh!

Pearl1594 commented Sep 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Pearl1594 commented May 4, 2020 •

edited by PaulAngus

Loading