Restore listNetworks behavior & clean up the code by winterhazel · Pull Request #9461 · apache/cloudstack

winterhazel · 2024-07-29T00:21:21Z

Description

PR #9184 attempted to optimize the listNetworks API by converting the multiple queries that were performed into a single one. This was done by building multiple search criterias for the individual queries, OR-adding them to an intermediary search criteria, and AND-adding the intermediary to a final one. However, this did not work as intended, because the code does not carry the joins' conditions when adding a search criteria into another one.

This PR fixes the issue, making the final query work as intended by #9184. This was done by explicitly adding the join parameters to each query's WHERE clause, which will then be included in the intermediary search criteria and in the final query. I also cleaned up the related code.

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?

My environment had the following domains, accounts, projects and networks (networks beginning with an "i" are isolated, and with a "s" are shared):

ROOT: admin (root admin; networks iadmin, created sadmin), ur (user; network iur), pr (project; network ipr; has accounts admin, ur)
ROOT/d1: d1 (domain admin; networks id1, created sd1), ud1 (user; network iud1), pd1 (project; network ipd1; has account d1)
ROOT/d2: d2 (domain admin; network id2, created sd2)

Through the UI, in every account, I listed the networks using the all/account/domain/shared filters with the project toggle on/off. I compared the behavior for the same environment in a commit before #9184, and verified that it was the same. Below is a table showing which networks were listed for each account and filter combination.

Account/filter	All, project off	Account, project off	Domain, project off	Shared, project off	All, project on	Account, project on	Domain, project on	Shared, project on
admin	id2,iud1,id1,iur,iadmin,sd2,sd1,sadmin	id2,iud1,id1,iur,iadmin	See [1]	Empty, see [2]	ipd1,ipr,sd2,sd1,sadmin	ipd1,ipr	See [1]	Empty, see [2]
ur	iur,sadmin	iur	See [1]	Empty, see [2]	ipr,sadmin	ipr	See [1]	Empty, see [2]
d1	iud1,id1,sd1,sadmin	iud1,id1	See [1]	Empty, see [2]	ipd1,sd1,sadmin	ipd1	See [1]	Empty, see [2]
ud1	iud1,sd1,sadmin	iud1	See [1]	Empty, see [2]	sd1,sadmin	Empty	See [1]	Empty, see [2]
d2	id2,sd2,sadmin	id2	See [1]	Empty, see [2]	sd2,sadmin	Empty	See [1]	Empty, see [2]

[1] When filtering by domain, I got Invalid value of networkfilter: domainpath. This was already present before #9184.
[2] This is probably not the intended behavior, and was already present before #9184. It can be fixed in a separate PR, as this one only aims to restore the original filtering behavior.

winterhazel · 2024-07-29T00:24:17Z

@blueorangutan package

blueorangutan · 2024-07-29T00:26:03Z

@winterhazel a [SL] 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 · 2024-07-29T00:29:45Z

Codecov Report

Attention: Patch coverage is 0% with 94 lines in your changes missing coverage. Please review.

Project coverage is 15.08%. Comparing base (bf11676) to head (8cf2987).
Report is 33 commits behind head on 4.19.

Files with missing lines	Patch %	Lines
...ain/java/com/cloud/network/NetworkServiceImpl.java	0.00%	94 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## 4.19 #9461 +/- ##
==========================================
Coverage 15.08% 15.08% Complexity 11189 11189 ==========================================
Files 5406 5406 Lines 472828 472830 +2 Branches 59879 60053 +174 ==========================================
+ Hits 71346 71350 +4 + Misses 393537 393536 -1 + Partials 7945 7944 -1

Flag	Coverage Δ
uitests	`4.30% <ø> (ø)`
unittests	`15.80% <0.00%> (+<0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

blueorangutan · 2024-07-29T01:26:37Z

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

shwstppr · 2024-07-29T18:14:16Z

@blueorangutan test

blueorangutan · 2024-07-29T18:16:03Z

@shwstppr a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

winterhazel · 2024-08-02T12:26:55Z

@blueorangutan package

blueorangutan · 2024-08-02T12:28:03Z

@winterhazel a [SL] 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.

winterhazel · 2024-08-02T12:28:04Z

@shwstppr could we run the CI again? There seems to have been a problem with the last run

shwstppr · 2024-08-02T13:07:14Z

@winterhazel sure, will do that once we have the new packages

blueorangutan · 2024-08-02T13:28:17Z

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

shwstppr · 2024-08-02T13:30:00Z

@blueorangutan test

blueorangutan · 2024-08-02T13:32:03Z

@shwstppr a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

shwstppr · 2024-08-05T07:45:45Z

@blueorangutan test

blueorangutan · 2024-08-05T07:48:03Z

@shwstppr a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2024-08-05T21:48:08Z

[SF] Trillian test result (tid-11025)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 48187 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9461-t11025-kvm-ol8.zip
Smoke tests completed. 131 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_secure_vm_migration	`Error`	133.32	test_vm_life_cycle.py
test_01_secure_vm_migration	`Error`	133.33	test_vm_life_cycle.py

github-actions · 2024-08-06T17:40:18Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

* server: refactor listNetworks api database retrievals * fixes * remove unused methods * imports * fix empty searchcriteria issue * refactor Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2024-08-12T04:53:06Z

@blueorangutan package

blueorangutan · 2024-08-12T04:54:03Z

@shwstppr a [SL] 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 · 2024-08-12T05:54:15Z

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

DaanHoogland · 2024-08-12T11:30:23Z

@blueorangutan test

blueorangutan · 2024-08-12T11:32:03Z

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

blueorangutan · 2024-08-13T01:28:31Z

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

Test	Result	Time (s)	Test File

JoaoJandre

CLGTM, did not test it.

shwstppr

Code LGTM. Reported manual and smoke tests also look good.
It would be great if we could have some more manual QA

yadvr · 2024-09-05T17:23:14Z

cc @kiranchavala @rajujith @NuxRo @vladimirpetrov @borisstoyanov @JoaoJandre @GutoVeronezi or others - anybody able to manually QA this? Thanks.

DaanHoogland · 2024-09-06T06:39:29Z

@blueorangutan package

blueorangutan · 2024-09-06T06:40:03Z

@DaanHoogland a [SL] 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 · 2024-09-06T08:01:24Z

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

DaanHoogland · 2024-09-06T09:06:35Z

@blueorangutan test keepEnv

blueorangutan · 2024-09-06T09:08:05Z

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

blueorangutan · 2024-09-06T22:47:35Z

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

Test	Result	Time (s)	Test File
test_04_nonsecured_to_secured_vm_migration	`Error`	375.77	test_vm_life_cycle.py

DaanHoogland · 2024-09-09T07:04:21Z

@winterhazel , I tested very slightly (list networks does not give errors with no, or with an isolated and a shared network) but do not understand what the situation is you are trying to solve. Is there any specific situation I should test?

winterhazel · 2024-09-09T14:38:35Z

Hey @DaanHoogland

When I opened this PR, the listNetworks filters were not working properly and allowed all users to see every network in the system (#9456).

This problem was introduced in an attempt to optimize the listNetworks queries (#9184) because the optimized query was not being generated as expected. I opened this PR to make it work as #9184 intended, and did some code clean-up.

However, #9184 was reverted in 4.19.1.1 (after I opened this), so the problem is not present anymore in the latest version. Hence, I adapted this PR to restore the optimizations made by #9184, but with the mentioned problem fixed and the clean-up done.

Therefore, we just need to test whether the listNetworks's response has not changed for the different filters.

DaanHoogland

lgtm (tested and reviewed)

Co-authored-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

boring-cyborgBot added the component:networking label Jul 29, 2024

winterhazel mentioned this pull request Jul 29, 2024
ACS 4.19.1 | Full Network List visible for Domain Admin #9456
Closed

yadvr added this to the 4.19.1.1 milestone Jul 29, 2024

winterhazel requested review from nvazquez and shwstppr July 29, 2024 12:13

github-actionsBot added the status:has-conflicts label Aug 6, 2024

yadvr modified the milestones: 4.19.1.1, 4.19.2.0 Aug 8, 2024

shwstpprand others added 2 commits August 11, 2024 17:45

server: refactor listNetworks api database retrievals (apache#9184)
c3b3fe2
* server: refactor listNetworks api database retrievals * fixes * remove unused methods * imports * fix empty searchcriteria issue * refactor Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

Fix listNetworks queries & clean up the code
8cf2987

winterhazel force-pushed the fix-listnetworks-query branch from eb48413 to 8cf2987Compare August 11, 2024 21:05

github-actionsBot removed the status:has-conflicts label Aug 11, 2024

DaanHoogland added the status:needs-testing label Aug 13, 2024

JoaoJandre approved these changes Aug 20, 2024
View reviewed changes

yadvr assigned shwstppr Sep 4, 2024

yadvr requested a review from DaanHoogland September 4, 2024 06:49

shwstppr approved these changes Sep 4, 2024
View reviewed changes

shwstppr removed their assignment Sep 4, 2024

DaanHoogland approved these changes Sep 9, 2024
View reviewed changes

DaanHoogland merged commit a0932b0 into apache:4.19Sep 9, 2024

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Sep 23, 2024
Restore listNetworks behavior & clean up the code (apache#9461)
f2545a2
Co-authored-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr added a commit to shwstppr/cloudstack that referenced this pull request Oct 13, 2024
Restore listNetworks behavior & clean up the code (apache#9461)
bc42264
Co-authored-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

DaanHoogland removed the status:needs-testing label Jan 31, 2025

Uh oh!

Conversation

winterhazel commented Jul 29, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

How Has This Been Tested?

Uh oh!

winterhazel commented Jul 29, 2024

Uh oh!

blueorangutan commented Jul 29, 2024

Uh oh!

codecovBot commented Jul 29, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Jul 29, 2024

Uh oh!

shwstppr commented Jul 29, 2024

Uh oh!

blueorangutan commented Jul 29, 2024

Uh oh!

winterhazel commented Aug 2, 2024

Uh oh!

blueorangutan commented Aug 2, 2024

Uh oh!

winterhazel commented Aug 2, 2024

Uh oh!

shwstppr commented Aug 2, 2024

Uh oh!

blueorangutan commented Aug 2, 2024

Uh oh!

shwstppr commented Aug 2, 2024

Uh oh!

blueorangutan commented Aug 2, 2024

Uh oh!

shwstppr commented Aug 5, 2024

Uh oh!

blueorangutan commented Aug 5, 2024

Uh oh!

blueorangutan commented Aug 5, 2024

Uh oh!

github-actionsBot commented Aug 6, 2024

Uh oh!

shwstppr commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 12, 2024

Uh oh!

DaanHoogland commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 12, 2024

Uh oh!

blueorangutan commented Aug 13, 2024

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Sep 5, 2024

Uh oh!

DaanHoogland commented Sep 6, 2024

Uh oh!

blueorangutan commented Sep 6, 2024

Uh oh!

blueorangutan commented Sep 6, 2024

Uh oh!

DaanHoogland commented Sep 6, 2024

Uh oh!

blueorangutan commented Sep 6, 2024

Uh oh!

blueorangutan commented Sep 6, 2024

Uh oh!

DaanHoogland commented Sep 9, 2024

Uh oh!

winterhazel commented Jul 29, 2024•
edited
Loading

codecovBot commented Jul 29, 2024•
edited
Loading

winterhazel commented Sep 9, 2024•
edited
Loading