Search vm snapshots using tags by Pearl1594 · Pull Request #4213 · apache/cloudstack

Pearl1594 · 2020-07-17T04:56:00Z

Description

Search VM snapshots using tags
Currently, search of VM snapshots doesn't comply with tags passed as input

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)

How Has This Been Tested?

tested via CLI (cmk) and UI

Pearl1594 · 2020-07-17T04:56:21Z

@blueorangutan package

blueorangutan · 2020-07-17T04:56:57Z

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

blueorangutan · 2020-07-17T05:26:06Z

Packaging result: ✔centos7 ✔debian. JID-1586

GabrielBrascher

Thanks for the PR @Pearl1594. I raised a few minor observations of what I think can be improved.

server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java

yadvr · 2020-07-27T08:57:12Z

@Pearl1594 can you address review comments and kick packaging/tests - assuming this is for primate, if so - can you pl change the base branch to 4.14?

Pearl1594 · 2020-07-27T13:46:49Z

@GabrielBrascher Thanks for reviewing, I've addressed your comments.
@rhtyd I've rebased it to 4.14
@blueorangutan package

davidjumani

LGTM

Pearl1594 · 2020-07-28T03:57:58Z

@blueorangutan package

blueorangutan · 2020-07-28T03:59:16Z

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

blueorangutan · 2020-07-28T04:30:04Z

Packaging result: ✔centos7 ✔debian. JID-1611

Pearl1594 · 2020-07-28T04:31:09Z

@blueorangutan test

blueorangutan · 2020-07-28T04:32:21Z

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

blueorangutan · 2020-07-28T16:46:10Z

Trillian test result (tid-2208)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42325 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4213-t2208-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 81 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_deploy_kubernetes_ha_cluster	`Error`	3613.69	test_kubernetes_clusters.py
test_04_deploy_and_upgrade_kubernetes_cluster	`Error`	0.04	test_kubernetes_clusters.py
test_05_deploy_and_upgrade_kubernetes_ha_cluster	`Error`	0.03	test_kubernetes_clusters.py
test_06_deploy_and_invalid_upgrade_kubernetes_cluster	`Error`	0.03	test_kubernetes_clusters.py
test_07_deploy_and_scale_kubernetes_cluster	`Error`	0.03	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	36.45	test_kubernetes_clusters.py
ContextSuite context=Test01DeployVM>:setup	`Error`	0.00	test_vm_life_cycle.py
ContextSuite context=Test02VMLifeCycle>:setup	`Error`	0.00	test_vm_life_cycle.py
ContextSuite context=Test03SecuredVmMigration>:setup	`Error`	0.00	test_vm_life_cycle.py

RodrigoDLopez

Hi @Pearl1594
Code looks good to me. I did not tested yet, I think you already did it.
I think if you could do those changes the code will be more readable, and add some unit tests

RodrigoDLopez · 2020-07-28T19:52:26Z

server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java

+            SearchBuilder<ResourceTagVO> tagSearch = _resourceTagDao.createSearchBuilder();
+            for (int count = 0; count < tags.size(); count++) {
+                tagSearch.or().op(ApiConstants.KEY + String.valueOf(count), tagSearch.entity().getKey(), SearchCriteria.Op.EQ);
+                tagSearch.and(ApiConstants.VALUE + String.valueOf(count), tagSearch.entity().getValue(), SearchCriteria.Op.EQ);
+                tagSearch.cp();
+            }
+            tagSearch.and(ApiConstants.RESOURCE_TYPE, tagSearch.entity().getResourceType(), SearchCriteria.Op.EQ);
+            sb.groupBy(sb.entity().getId());
+            sb.join("tagSearch", tagSearch, sb.entity().getId(), tagSearch.entity().getResourceId(), JoinBuilder.JoinType.INNER);


Could you please extract this to a method, add documentation to what this method do and why.
Also can you please create unit tests to cover those method

RodrigoDLopez · 2020-07-28T19:53:32Z

server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java

+            int count = 0;
+            sc.setJoinParameters("tagSearch", ApiConstants.RESOURCE_TYPE, ResourceTag.ResourceObjectType.VMSnapshot.toString());
+            for (String key : tags.keySet()) {
+                sc.setJoinParameters("tagSearch", ApiConstants.KEY + String.valueOf(count), key);
+                sc.setJoinParameters("tagSearch", ApiConstants.VALUE + String.valueOf(count), tags.get(key));
+                count++;
+            }


cloud you do the same here please,
extract to a new method, add documentation and unit tests.

yadvr · 2020-08-05T08:33:46Z

@blueorangutan package

blueorangutan · 2020-08-05T08:35:03Z

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

blueorangutan · 2020-08-05T09:07:16Z

Packaging result: ✖centos7 ✔debian. JID-1656

Pearl1594 · 2020-08-05T10:27:48Z

@blueorangutan package

blueorangutan · 2020-08-05T10:28:57Z

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

blueorangutan · 2020-08-05T11:16:50Z

Packaging result: ✔centos7 ✔debian. JID-1662

yadvr · 2020-08-10T07:44:27Z

@blueorangutan package

blueorangutan · 2020-08-10T07:44:57Z

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

DaanHoogland · 2020-08-11T07:03:28Z

@blueorangutan package

blueorangutan · 2020-08-11T07:03:57Z

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

blueorangutan · 2020-08-11T08:08:44Z

Packaging result: ✔centos7 ✔debian. JID-1705

DaanHoogland · 2020-08-12T07:06:42Z

@blueorangutan test

blueorangutan · 2020-08-12T07:08:02Z

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

blueorangutan · 2020-08-12T19:48:27Z

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

Test	Result	Time (s)	Test File

yadvr · 2020-08-13T10:08:57Z

@Pearl1594 pl address code review comments, send another PR for refactoring as necessary

davidjumani mentioned this pull request Jul 17, 2020

Consolidated Bug List from Testing/Review apache/cloudstack-primate#528

Closed

svenvogel added type:bug component:api component:primary-storage labels Jul 19, 2020

GabrielBrascher reviewed Jul 21, 2020

View reviewed changes

yadvr added this to the 4.15.0.0 milestone Jul 27, 2020

Pearl1594 force-pushed the vmsnap_search_by_tags branch from 2a5b533 to f79dc4e Compare July 27, 2020 13:13

Pearl1594 changed the base branch from master to 4.14 July 27, 2020 13:14

Pearl Dsilva added 2 commits July 27, 2020 18:47

search vm snapshots with tags

9a2d235

Review comments

2f01c8b

Pearl1594 force-pushed the vmsnap_search_by_tags branch from f79dc4e to 2f01c8b Compare July 27, 2020 13:18

davidjumani approved these changes Jul 27, 2020

View reviewed changes

RodrigoDLopez suggested changes Jul 28, 2020

View reviewed changes

yadvr closed this Aug 10, 2020

yadvr reopened this Aug 10, 2020

yadvr approved these changes Aug 13, 2020

View reviewed changes

yadvr merged commit b68be66 into apache:4.14 Aug 13, 2020

DaanHoogland added the Severity:Minor label Nov 10, 2020

Conversation

Pearl1594 commented Jul 17, 2020

Description

Types of changes

How Has This Been Tested?

Uh oh!

Pearl1594 commented Jul 17, 2020

Uh oh!

blueorangutan commented Jul 17, 2020

Uh oh!

blueorangutan commented Jul 17, 2020

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yadvr commented Jul 27, 2020

Uh oh!

Pearl1594 commented Jul 27, 2020

Uh oh!

davidjumani left a comment

Choose a reason for hiding this comment

Uh oh!

Pearl1594 commented Jul 28, 2020

Uh oh!

blueorangutan commented Jul 28, 2020

Uh oh!

blueorangutan commented Jul 28, 2020

Uh oh!

Pearl1594 commented Jul 28, 2020

Uh oh!

blueorangutan commented Jul 28, 2020

Uh oh!

blueorangutan commented Jul 28, 2020

Uh oh!

RodrigoDLopez left a comment

Choose a reason for hiding this comment

Uh oh!

RodrigoDLopez Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

RodrigoDLopez Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

yadvr commented Aug 5, 2020

Uh oh!

blueorangutan commented Aug 5, 2020

Uh oh!

blueorangutan commented Aug 5, 2020

Uh oh!

Pearl1594 commented Aug 5, 2020

Uh oh!

blueorangutan commented Aug 5, 2020

Uh oh!

blueorangutan commented Aug 5, 2020

Uh oh!

yadvr commented Aug 10, 2020

Uh oh!

blueorangutan commented Aug 10, 2020

Uh oh!

DaanHoogland commented Aug 11, 2020

Uh oh!

blueorangutan commented Aug 11, 2020

Uh oh!

blueorangutan commented Aug 11, 2020

Uh oh!

DaanHoogland commented Aug 12, 2020

Uh oh!

blueorangutan commented Aug 12, 2020

Uh oh!

blueorangutan commented Aug 12, 2020

Uh oh!

yadvr commented Aug 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects