Fix templates and ISOs listing pagination by winterhazel · Pull Request #7974 · apache/cloudstack

winterhazel · 2023-09-18T20:35:43Z

Description

After the changes in #4774, when an account other than the root admin lists templates/ISOs with pagination, count gets set to the number of items returned in that page. For example, when the user lists 10 templates/ISOs and 15 entries are found, in the first page, count is set to 10 (the number of items being returned), as opposed to 15 (the total number of items). As a result, the deploy VM wizard only shows the first page of templates/ISOs.

This PR fixes this behavior and makes count return the correct number of found entries.

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

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Everything has been tested in a local lab.

In the tested scenario, the following domains have templates:

ROOT: 13 public and featured templates;
ROOT/test: 4 public and featured templates.

CloudMonkey command used to list templates.

list templates templatefilter=featured page=1 pageSize=5

In every test, the expected number of templates/ISOs was returned.

With share.public.templates.with.other.domains set to true in both domains:

In the root admin account, I listed the templates. I verified that the expected value of 17 was returned;
In a user account belonging to ROOT, I listed the templates. I verified that the expected value of 17 was returned;
In a user account belonging to ROOT/test, I listed the templates. I verified that the expected value of 17 was returned;
I marked a template belonging to ROOT as not public. Then, in a user account belonging to ROOT, I listed the templates. I verified that the expected value of 16 was returned;
I marked a template belonging to ROOT as not public. Then, in a user account belonging to ROOT/test, I listed the templates. I verified that the expected value of 16 was returned;
I marked a template belonging to ROOT/test as not public. Then, in a user account belonging to ROOT, I listed the templates. I verified that the expected value of 16 was returned;
I marked a template belonging to ROOT/test as not public. Then, in a user account belonging to ROOT/test, I listed the template. I verified that the expected value of 16 was returned.

With share.public.templates.with.other.domains set to false in both domains:

In the root admin account, I listed the templates. I verified that the expected value of 17 was returned;
In a user account belonging to ROOT, I listed the templates. I verified that the expected value of 13 was returned;
In a user account belonging to ROOT/test, I listed the templates. I verified that the expected value of 4 was returned;
I marked a template belonging to ROOT as not public. Then, in a user account belonging to ROOT, I listed the templates. I verified that the expected value of 12 was returned;
I marked a template belonging to ROOT as not public. Then, in a user account belonging to ROOT/test, I listed the templates. I verified that the expected value of 4 was returned;
I marked a template belonging to ROOT/test as not public. Then, in a user account belonging to ROOT, I listed the templates. I verified that the expected value of 13 was returned;
I marked a template belonging to ROOT/test as not public. Then, in a user account belonging to ROOT/test, I listed the template. I verified that the expected value of 3 was returned.

I also verified the behavior of the listIsos API. In the tested scenario, the following domains have ISOs:

ROOT: 11 public and featured ISOs;
ROOT/test: 2 public and featured ISOs.

CloudMonkey command used to list ISOs.

list isos page=1 pageSize=5 isoFilter=featured bootable=true

With share.public.templates.with.other.domains set to true in both domains:

In the root admin account, I listed the ISOs. I verified that the expected value of 13 was returned;
In a user account belonging to ROOT, I listed the ISOs. I verified that the expected value of 13 was returned;
In a user account belonging to ROOT/test, I listed the ISOs. I verified that the expected value of 13 was returned;
I marked an ISO belonging to ROOT as not public. Then, in a user account belonging to ROOT, I listed the ISOs. I verified that the expected value of 12 was returned;
I marked an ISO belonging to ROOT as not public. Then, in a user account belonging to ROOT/test, I listed the ISOs. I verified that the expected value of 12 was returned;
I marked an ISO belonging to ROOT/test as not public. Then, in a user account belonging to ROOT, I listed the ISOs. I verified that the expected value of 12 was returned;
I marked an ISO belonging to ROOT/test as not public. Then, in a user account belonging to ROOT/test, I listed the ISOs. I verified that the expected value of 12 was returned.

With share.public.templates.with.other.domains set to false in both domains:

In the root admin account, I listed the ISOs. I verified that the expected value of 13 was returned;
In a user account belonging to ROOT, I listed the ISOs. I verified that the expected value of 11 was returned;
In a user account belonging to ROOT/test, I listed the ISOs. I verified that the expected value of 2 was returned;
I marked an ISO belonging to ROOT as not public. Then, in a user account belonging to ROOT, I listed the ISOs. I verified that the expected value of 10 was returned;
I marked an ISO belonging to ROOT as not public. Then, in a user account belonging to ROOT/test, I listed the ISOs. I verified that the expected value of 2 was returned;
I marked an ISO belonging to ROOT/test as not public. Then, in a user account belonging to ROOT, I listed the ISOs. I verified that the expected value of 11 was returned;
I marked an ISO belonging to ROOT/test as not public. Then, in a user account belonging to ROOT/test, I listed the ISOs. I verified that the expected value of 1 was returned.

I also verified the changes in the UI. Before, the deploy VM wizard showed only one page of templates/ISOs. After the changes, all pages are shown as expected.

DaanHoogland · 2023-09-19T06:53:59Z

@blueorangutan package

blueorangutan · 2023-09-19T06:56:04Z

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

codecov · 2023-09-19T07:54:40Z

Codecov Report

Merging #7974 (0fc8748) into main (c3aeba1) will increase coverage by 7.89%.
The diff coverage is 86.66%.

@@ Coverage Diff @@## main #7974 +/- ##
============================================
+ Coverage 21.20% 29.09% +7.89% - Complexity 20501 30378 +9877 
============================================
Files 4968 5102 +134 Lines 336855 358767 +21912 Branches 48389 52356 +3967 ============================================
+ Hits 71416 104379 +32963 + Misses 255044 240066 -14978 - Partials 10395 14322 +3927

Flag	Coverage Δ
simulator-marvin-tests	`25.01% <60.00%> (+2.46%)`	⬆️
uitests	`4.84% <ø> (ø)`
unit-tests	`14.51% <70.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...a/com/cloud/api/query/dao/TemplateJoinDaoImpl.java	`47.96% <100.00%> (+2.59%)`	⬆️
...ain/java/com/cloud/api/query/QueryManagerImpl.java	`45.61% <83.33%> (+7.48%)`	⬆️

... and 1471 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

blueorangutan · 2023-09-19T07:55:33Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7057

DaanHoogland · 2023-09-19T08:18:21Z

@blueorangutan test

blueorangutan · 2023-09-19T08:20:04Z

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

blueorangutan · 2023-09-19T20:41:32Z

[SF] Trillian test result (tid-7688)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43052 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7974-t7688-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_upgrade_kubernetes_cluster	`Failure`	575.22	test_kubernetes_clusters.py

DaanHoogland

clgtm

DaanHoogland · 2023-09-28T07:06:27Z

@blueorangutan package

blueorangutan · 2023-09-28T07:08:03Z

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-09-28T08:14:02Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7149

DaanHoogland · 2023-09-28T09:17:25Z

@blueorangutan test

blueorangutan · 2023-09-28T09:18:04Z

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

blueorangutan · 2023-09-29T00:26:55Z

[SF] Trillian test result (tid-7756)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 52974 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7974-t7756-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_invalid_upgrade_kubernetes_cluster	`Failure`	3623.09	test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster	`Failure`	3615.24	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	0.06	test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster	`Failure`	0.03	test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster	`Failure`	0.03	test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster	`Failure`	0.03	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.03	test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster	`Failure`	50.90	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	126.40	test_kubernetes_clusters.py

GutoVeronezi

CLGTM

GutoVeronezi · 2023-10-09T11:21:25Z

@shwstppr @DaanHoogland it would be interesting to have this fix on 4.19

DaanHoogland · 2023-10-09T15:21:13Z

@hsato03 are you allright with this PR like this?
@winterhazel , did you get some 3rd party testing?

hsato03 · 2023-10-09T17:24:19Z

Code LGTM

winterhazel · 2023-10-09T17:35:05Z

@winterhazel , did you get some 3rd party testing?

@DaanHoogland yes, some users have tested these changes; I will ask them to report the results here.

DaanHoogland · 2023-10-09T17:57:30Z

@blueorangutan package

blueorangutan · 2023-10-09T17:58:03Z

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-10-09T18:53:31Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7285

DaanHoogland · 2023-10-10T06:23:21Z

@blueorangutan test

blueorangutan · 2023-10-10T06:24:03Z

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

blueorangutan · 2023-10-10T18:13:49Z

[SF] Trillian test result (tid-7896)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41089 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7974-t7896-kvm-centos7.zip
Smoke tests completed. 113 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland · 2023-10-11T07:37:00Z

@winterhazel , did you get some 3rd party testing?
@DaanHoogland yes, some users have tested these changes; I will ask them to report the results here.

It would be nice to see those, thanks.

gp-santos · 2023-10-18T15:24:33Z

I have tested this PR by replicating the tests on my local lab. First, I tested on the current main and verified the problem was present. Then, I applied the changes and verified it had been fixed.

My scenario had the same amount of domains, templates and ISOs (ROOT with 13 and 11 and ROOT/test with 4 and 2), and I used the same commands to list templates and ISOs.

Templates:

Without the changes

With share.public.templates.with.other.domains set to true in both domains:

Domain	Account	`count`
ROOT	Admin	17
ROOT	User	5
ROOT/test	TestAdmin	5
ROOT/test	TestUser	5

I marked one of the ROOT templates as non-public. It stopped showing in templatefilter=featured for all accounts:

Domain	Account	`count`
ROOT	Admin	16
ROOT	User	5
ROOT/test	TestAdmin	5
ROOT/test	TestUser	5

With share.public.templates.with.other.domains set to false in both domains:

Domain	Account	`count`
ROOT	Admin	17
ROOT	User	5
ROOT/test	TestAdmin	Empty
ROOT/test	TestUser	Empty

With the changes

With share.public.templates.with.other.domains set to true in both domains:

Domain	Account	`count`
ROOT	Admin	17
ROOT	User	17
ROOT/test	TestAdmin	17
ROOT/test	TestUser	17

I marked one of the templates as non-public. It stopped showing in templatefilter=featured for all accounts. The first column indicates the domain of the non-public template:

Template Domain	Domain	Account	`count`
ROOT	ROOT	Admin	16
ROOT	ROOT	User	16
ROOT	ROOT/test	TestAdmin	16
ROOT	ROOT/test	TestUser	16
ROOT/test	ROOT	Admin	16
ROOT/test	ROOT	User	16
ROOT/test	ROOT/test	TestAdmin	16
ROOT/test	ROOT/test	TestUser	16

With share.public.templates.with.other.domains set to false in both domains:

Domain	Account	`count`
ROOT	Admin	17
ROOT	User	13
ROOT/test	TestAdmin	4
ROOT/test	TestUser	4

With share.public.templates.with.other.domains set to false in both domains. The first column indicates the domain of the non-public template:

Template Domain	Domain	Account	`count`
ROOT	ROOT	Admin	16
ROOT	ROOT	User	12
ROOT	ROOT/test	TestAdmin	4
ROOT	ROOT/test	TestUser	4
ROOT/test	ROOT	Admin	16
ROOT/test	ROOT	User	13
ROOT/test	ROOT/test	TestAdmin	3
ROOT/test	ROOT/test	TestUser	3

ISOs:

Without the changes

With share.public.templates.with.other.domains set to true in both domains:

Domain	Account	Count
ROOT	Admin	13
ROOT	User	5
ROOT/test	TestAdmin	5
ROOT/test	TestUser	5

I marked one of the ROOT ISOs as non-public. It stopped showing in isofilter=featured for all accounts:

Domain	Account	`count`
ROOT	Admin	12
ROOT	User	5
ROOT/test	TestAdmin	5
ROOT/test	TestUser	5

With share.public.templates.with.other.domains set to false in both domains:

Domain	Account	`count`
ROOT	Admin	13
ROOT	User	5
ROOT/test	TestAdmin	Empty
ROOT/test	TestUser	Empty

With the changes

With share.public.templates.with.other.domains set to true in both domains:

Domain	Account	`count`
ROOT	Admin	13
ROOT	User	13
ROOT/test	TestAdmin	13
ROOT/test	TestUser	13

I marked one of the ISOs as non-public. It stopped showing in isofilter=featured, for all accounts. The first column indicates the domain of the non-public ISO:

ISO Domain	Domain	Account	Count
ROOT	ROOT	Admin	12
ROOT	ROOT	User	12
ROOT	ROOT/test	TestAdmin	12
ROOT	ROOT/test	TestUser	12
ROOT/test	ROOT	Admin	12
ROOT/test	ROOT	User	12
ROOT/test	ROOT/test	TestAdmin	12
ROOT/test	ROOT/test	TestUser	12

With share.public.templates.with.other.domains set to false in both domains:

Domain	Account	Count
ROOT	Admin	13
ROOT	User	11
ROOT/test	TestAdmin	2
ROOT/test	TestUser	2

I marked one of the ISOs as non-public. It stopped showing with isofilter=featured for all accounts. The first column indicates the domain of the non-public ISO:

ISO Domain	Domain	Account	Count
ROOT	ROOT	Admin	12
ROOT	ROOT	User	10
ROOT	ROOT/test	TestAdmin	2
ROOT	ROOT/test	TestUser	2
ROOT/test	ROOT	Admin	12
ROOT/test	ROOT	User	11
ROOT/test	ROOT/test	TestAdmin	1
ROOT/test	ROOT/test	TestUser	1

My testing yielded the same results. The only caveat is that the featured not-public templates do not show under the featured filter to anyone. However, this situation was observed even without the patch; therefore, it is outside the scope of this PR.

DaanHoogland · 2023-10-19T07:49:00Z

@hsato03 Do you think this is ready for merge, now?

hsato03 · 2023-10-19T22:24:47Z

@DaanHoogland Yes, didn't test but CLGTM.

hsato03 reviewed Sep 26, 2023
View reviewed changes

Comment threadserver/src/main/java/com/cloud/api/query/QueryManagerImpl.java Outdated

DaanHoogland approved these changes Sep 27, 2023
View reviewed changes

DaanHoogland added component:api status:needs-testing labels Sep 27, 2023

DaanHoogland added this to the 4.19.0.0 milestone Sep 27, 2023

GutoVeronezi approved these changes Oct 9, 2023
View reviewed changes

shwstppr requested a review from soreana October 9, 2023 11:28

winterhazel force-pushed the fix-list-templates-isos-pagination branch from 1d228d3 to 15f5279Compare October 9, 2023 17:47

boring-cyborgBot added component:agent component:build component:integration labels Oct 9, 2023

winterhazel changed the base branch from 4.18 to mainOctober 9, 2023 17:47

Fix ISOs and templates listing pagination
0fc8748

winterhazel force-pushed the fix-list-templates-isos-pagination branch from 15f5279 to 0fc8748Compare October 9, 2023 17:53

DaanHoogland merged commit d3d3027 into apache:mainOct 20, 2023

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

Uh oh!

Conversation

winterhazel commented Sep 18, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

DaanHoogland commented Sep 19, 2023

Uh oh!

blueorangutan commented Sep 19, 2023

Uh oh!

codecovBot commented Sep 19, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Sep 19, 2023

Uh oh!

DaanHoogland commented Sep 19, 2023

Uh oh!

blueorangutan commented Sep 19, 2023

Uh oh!

blueorangutan commented Sep 19, 2023

Uh oh!

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Sep 28, 2023

Uh oh!

blueorangutan commented Sep 28, 2023

Uh oh!

blueorangutan commented Sep 28, 2023

Uh oh!

DaanHoogland commented Sep 28, 2023

Uh oh!

blueorangutan commented Sep 28, 2023

Uh oh!

blueorangutan commented Sep 29, 2023

Uh oh!

GutoVeronezi left a comment

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi commented Oct 9, 2023

Uh oh!

DaanHoogland commented Oct 9, 2023

Uh oh!

hsato03 commented Oct 9, 2023

Uh oh!

winterhazel commented Oct 9, 2023

Uh oh!

DaanHoogland commented Oct 9, 2023

Uh oh!

blueorangutan commented Oct 9, 2023

Uh oh!

blueorangutan commented Oct 9, 2023

Uh oh!

DaanHoogland commented Oct 10, 2023

Uh oh!

blueorangutan commented Oct 10, 2023

Uh oh!

blueorangutan commented Oct 10, 2023

Uh oh!

DaanHoogland commented Oct 11, 2023

Uh oh!

gp-santos commented Oct 18, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland commented Oct 19, 2023

Uh oh!

hsato03 commented Oct 19, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

codecovBot commented Sep 19, 2023•
edited
Loading

gp-santos commented Oct 18, 2023•
edited
Loading

hsato03 commented Oct 19, 2023•
edited
Loading