Restore listNetworks behavior & clean up the code#9461
Restore listNetworks behavior & clean up the code#9461DaanHoogland merged 2 commits intoapache:4.19apache/cloudstack:4.19from scclouds:fix-listnetworks-queryscclouds/cloudstack:fix-listnetworks-queryCopy head branch name to clipboard
Conversation
|
@blueorangutan package |
|
@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 ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10496 |
|
@blueorangutan test |
|
@shwstppr a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
@blueorangutan package |
|
@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. |
|
@shwstppr could we run the CI again? There seems to have been a problem with the last run |
|
@winterhazel sure, will do that once we have the new packages |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10549 |
|
@blueorangutan test |
|
@shwstppr a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
@blueorangutan test |
|
@shwstppr a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-11025)
|
|
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>
eb48413 to
8cf2987
Compare
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10613 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-11057)
|
JoaoJandre
left a comment
There was a problem hiding this comment.
CLGTM, did not test it.
shwstppr
left a comment
There was a problem hiding this comment.
Code LGTM. Reported manual and smoke tests also look good.
It would be great if we could have some more manual QA
|
cc @kiranchavala @rajujith @NuxRo @vladimirpetrov @borisstoyanov @JoaoJandre @GutoVeronezi or others - anybody able to manually QA this? Thanks. |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11027 |
|
@blueorangutan test keepEnv |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-11402)
|
|
@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? |
|
Hey @DaanHoogland When I opened this PR, the This problem was introduced in an attempt to optimize the However, #9184 was reverted in Therefore, we just need to test whether the |
DaanHoogland
left a comment
There was a problem hiding this comment.
lgtm (tested and reviewed)
Co-authored-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Co-authored-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Description
PR #9184 attempted to optimize the
listNetworksAPI 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
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
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):
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.
[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.