server: refactor listNetworks api database retrievals by shwstppr · Pull Request #9184 · apache/cloudstack

shwstppr · 2024-06-06T10:47:26Z

Description

This PR attempts to optimize database queries during listNetworks API call. It should fix pagination-related issues as well while querying desired networks from the database. Only in the following cases, it may query all networks:

API param canusefordeploy is passed as true
Any service specified with the supportedservices API parameter

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)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

codecov · 2024-06-06T10:55:52Z

Codecov Report

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

Project coverage is 14.98%. Comparing base (631d6ad) to head (3a97fae).
Report is 102 commits behind head on 4.19.

Files	Patch %	Lines
...ain/java/com/cloud/network/NetworkServiceImpl.java	0.00%	70 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## 4.19 #9184 +/- ##
============================================
+ Coverage 14.96% 14.98% +0.02% - Complexity 10991 11048 +57 
============================================
Files 5373 5389 +16 Lines 469203 470603 +1400 Branches 60225 58405 -1820 ============================================
+ Hits 70198 70517 +319 - Misses 391232 392251 +1019 - Partials 7773 7835 +62

Flag	Coverage Δ
uitests	`4.28% <ø> (-0.03%)`	⬇️
unittests	`15.69% <0.00%> (+0.02%)`	⬆️

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.

weizhouapache · 2024-06-06T11:12:15Z

good idea

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2024-06-06T18:55:57Z

@blueorangutan package

blueorangutan · 2024-06-06T18:58:04Z

@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-06-06T20:05:11Z

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

shwstppr · 2024-06-06T20:08:59Z

@blueorangutan test

blueorangutan · 2024-06-06T20:10:04Z

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

blueorangutan · 2024-06-07T10:08:53Z

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

Test	Result	Time (s)	Test File

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

weizhouapache · 2024-06-10T07:50:40Z

@blueorangutan package

blueorangutan · 2024-06-10T07:52:04Z

@weizhouapache 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-06-10T08:54:01Z

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

shwstppr · 2024-06-10T09:41:32Z

In my testing, listing networks is working fine with the changes and smoke tests didn't report any error. Marking it ready for review

yadvr

LGTM, didn't test it but needs testing on:

networks by roles: admin, domain admin, normal user
list networks by projects, accounts, domains
list networks in env with variety of networks across roles; isolated networks, VPC tier/network, shared network, L2 network and user-shared network

sureshanaparti

clgtm

sureshanaparti · 2024-06-24T07:40:42Z

@blueorangutan package

blueorangutan · 2024-06-24T07:42:03Z

@sureshanaparti 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.

vishesh92 · 2024-06-25T07:27:01Z

-networksToReturn.addAll(listAccountSpecificNetworks(buildNetworkSearchCriteria(sb, keyword, id, isSystem, zoneId, guestIpType, trafficType, physicalNetworkId, networkOfferingId,
-aclType, skipProjectNetworks, restartRequired, specifyIpRanges, vpcId, tags, display, vlanId, associatedNetworkId), searchFilter, permittedAccounts));
+additionalSearchCriteria.addOr("id", SearchCriteria.Op.SC,
+getAccountSpecificNetworksSearchCriteria(sb, permittedAccounts, skipProjectNetworks));


If getAccountSpecificNetworksSearchCriteria returns null, the request fails with a bad sql query.

Thanks @vishesh92 will check

@vishesh92 it was due to empty searchcriteria at line 2392. Should be fixed now.

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2024-06-26T07:51:20Z

@blueorangutan package

blueorangutan · 2024-06-26T07:52: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-06-26T08:07:27Z

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

shwstppr · 2024-06-26T08:23:21Z

@blueorangutan package

blueorangutan · 2024-06-26T08:24: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-06-26T09:26:19Z

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

yadvr · 2024-06-26T09:46:17Z

@blueorangutan test

blueorangutan · 2024-06-26T09:48:04Z

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

borisstoyanov

LGTM, checked manually for admin/user visibility of shared networks and for networks deployed within projects

blueorangutan · 2024-06-26T20:32:54Z

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

Test	Result	Time (s)	Test File
ContextSuite context=TestTemplateHierarchy>:setup	`Error`	8.27	test_accounts.py
test_01_events_resource	`Failure`	0.02	test_events_resource.py
ContextSuite context=TestDeployVmWithAffinityGroup>:setup	`Error`	0.00	test_affinity_groups.py
ContextSuite context=TestMultipleVolumeAttach>:setup	`Error`	0.00	test_attach_multiple_volumes.py
ContextSuite context=TestDummyBackupAndRecovery>:setup	`Error`	0.00	test_backup_recovery_dummy.py
ContextSuite context=TestVeeamBackupAndRecovery>:setup	`Error`	0.00	test_backup_recovery_veeam.py
test_01_condensed_drs_algorithm	`Error`	0.00	test_cluster_drs.py
test_02_balanced_drs_algorithm	`Error`	0.00	test_cluster_drs.py
ContextSuite context=TestConsoleEndpoint>:setup	`Error`	0.00	test_console_endpoint.py
test_01_deploy_vm_with_extraconfig_throws_exception_kvm	`Error`	0.10	test_deploy_vm_extra_config_data.py
test_02_deploy_vm_with_extraconfig_kvm	`Error`	0.07	test_deploy_vm_extra_config_data.py
test_03_update_vm_with_extraconfig_kvm	`Error`	0.06	test_deploy_vm_extra_config_data.py
ContextSuite context=TestDeployVmRootSize>:setup	`Error`	0.00	test_deploy_vm_root_resize.py
ContextSuite context=TestHostControlState>:setup	`Error`	27.24	test_host_control_state.py
ContextSuite context=TestImportAndUnmanageVolumes>:setup	`Error`	0.00	test_import_unmanage_volumes.py
test_11_test_unmanaged_cluster_lifecycle	`Error`	2.30	test_kubernetes_clusters.py
ContextSuite context=TestListVolumes>:setup	`Error`	0.00	test_list_volumes.py
ContextSuite context=TestNetworkMigration>:setup	`Error`	0.00	test_migration.py
test_03_network_operations_on_created_vm_of_otheruser	`Error`	1.20	test_network_permissions.py
test_04_deploy_vm_for_other_user_and_test_vm_operations	`Failure`	0.05	test_network_permissions.py
ContextSuite context=TestSharedNetwork>:setup	`Error`	56.59	test_network.py
test_01_nic	`Error`	0.05	test_nic.py
test_01_non_strict_host_anti_affinity	`Error`	2.17	test_nonstrict_affinity_group.py
test_02_non_strict_host_affinity	`Error`	1.13	test_nonstrict_affinity_group.py
ContextSuite context=TestL2PersistentNetworks>:setup	`Error`	0.00	test_persistent_network.py
test_01_add_primary_storage_disabled_host	`Failure`	0.06	test_primary_storage.py
test_01_primary_storage_iscsi	`Failure`	0.06	test_primary_storage.py
test_01_primary_storage_nfs	`Failure`	0.06	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.15	test_primary_storage.py
ContextSuite context=TestPrivateGwACLOvsGRE>:setup	`Error`	0.00	test_privategw_acl_ovs_gre.py
ContextSuite context=TestTemplates>:setup	`Error`	36.51	test_templates.py
test_01_positive_tests_usage	`Failure`	2.93	test_usage_events.py
ContextSuite context=TestLBRuleUsage>:setup	`Error`	2.24	test_usage.py
ContextSuite context=TestNatRuleUsage>:setup	`Error`	2.28	test_usage.py
ContextSuite context=TestPublicIPUsage>:setup	`Error`	2.32	test_usage.py
ContextSuite context=TestSnapshotUsage>:setup	`Error`	2.38	test_usage.py
ContextSuite context=TestTemplateUsage>:setup	`Error`	2.43	test_usage.py
ContextSuite context=TestVmUsage>:setup	`Error`	2.48	test_usage.py
ContextSuite context=TestVolumeUsage>:setup	`Error`	2.57	test_usage.py
ContextSuite context=TestVpnUsage>:setup	`Error`	2.62	test_usage.py
ContextSuite context=TestVmAutoScaling>:setup	`Error`	0.00	test_vm_autoscaling.py
test_01_deploy_vm_on_specific_host	`Error`	0.05	test_vm_deployment_planner.py
test_02_deploy_vm_on_specific_cluster	`Error`	0.04	test_vm_deployment_planner.py
test_03_deploy_vm_on_specific_pod	`Error`	0.07	test_vm_deployment_planner.py
test_04_deploy_vm_on_host_override_pod_and_cluster	`Error`	0.08	test_vm_deployment_planner.py
test_05_deploy_vm_on_cluster_override_pod	`Error`	0.05	test_vm_deployment_planner.py
ContextSuite context=TestDeployVM>:setup	`Error`	0.00	test_vm_life_cycle.py
ContextSuite context=TestKVMLiveMigration>:setup	`Error`	0.00	test_vm_life_cycle.py
ContextSuite context=TestMigrateVMwithVolume>:setup	`Error`	0.00	test_vm_life_cycle.py
ContextSuite context=TestSecuredVmMigration>:setup	`Error`	0.00	test_vm_life_cycle.py
ContextSuite context=TestVMLifeCycle>:setup	`Error`	0.06	test_vm_life_cycle.py
ContextSuite context=TestUnmanageVM>:setup	`Error`	0.00	test_vm_lifecycle_unmanage_import.py
ContextSuite context=TestVMSchedule>:setup	`Error`	0.00	test_vm_schedule.py
ContextSuite context=TestVmSnapshot>:setup	`Error`	0.07	test_vm_snapshots.py
ContextSuite context=TestVnfTemplates>:setup	`Error`	0.00	test_vnf_templates.py
ContextSuite context=TestCreateVolume>:setup	`Error`	0.00	test_volumes.py
ContextSuite context=TestVolumeEncryption>:setup	`Error`	0.00	test_volumes.py
ContextSuite context=TestVolumes>:setup	`Error`	0.00	test_volumes.py
test_02_cancel_host_maintenace_with_migration_jobs	`Failure`	0.14	test_host_maintenance.py
test_03_cancel_host_maintenace_with_migration_jobs_failure	`Error`	0.19	test_host_maintenance.py
ContextSuite context=TestHostMaintenanceAgents>:setup	`Error`	0.26	test_host_maintenance.py

sureshanaparti · 2024-06-28T10:31:27Z

@blueorangutan package

blueorangutan · 2024-06-28T10:32:03Z

@sureshanaparti 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-06-28T11:38:27Z

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

sureshanaparti · 2024-06-28T11:48:54Z

Merging based on manual tests & reviews.

* 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>

* server, api, ui: access improvements and assorted fixes Fixes domain-admin access check to prevent unauthorized access. Co-authored-by: Fabricio Duarte <fabricio.duarte.jr@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> Co-authored-by: Abhishek Kumar <abhishek.mrt22@gmail.com> * Revert "server: refactor listNetworks api database retrievals (#9184)" This reverts commit c7f1ba5. * Fix snapshot chain being deleted on XenServer (#9447) Using XenServer as the hypervisor, when deleting a snapshot that has a parent, that parent will also get erased on storage, causing data loss. This behavior was introduced with #7873, where the list of snapshot states that can be deleted was changed to add BackedUp snapshots. This PR changes the states list back to the original list, and swaps the while loop for a do while loop to account for the changes in #7873. Fixes#9446 * UI: Display Firewall, LB and Port Forwading rules tab for CKS clusters deployed on isolated networks (#9458) --------- Co-authored-by: nvazquez <nicovazquez90@gmail.com> Co-authored-by: Fabricio Duarte <fabricio.duarte.jr@gmail.com> Co-authored-by: João Jandre <48719461+JoaoJandre@users.noreply.github.com> Co-authored-by: Pearl Dsilva <pearl1594@gmail.com>

* 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>

* server, api, ui: access improvements and assorted fixes Fixes domain-admin access check to prevent unauthorized access. Co-authored-by: Fabricio Duarte <fabricio.duarte.jr@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com> Co-authored-by: Abhishek Kumar <abhishek.mrt22@gmail.com> * Revert "server: refactor listNetworks api database retrievals (apache#9184)" This reverts commit c7f1ba5. * Fix snapshot chain being deleted on XenServer (apache#9447) Using XenServer as the hypervisor, when deleting a snapshot that has a parent, that parent will also get erased on storage, causing data loss. This behavior was introduced with apache#7873, where the list of snapshot states that can be deleted was changed to add BackedUp snapshots. This PR changes the states list back to the original list, and swaps the while loop for a do while loop to account for the changes in apache#7873. Fixesapache#9446 * UI: Display Firewall, LB and Port Forwading rules tab for CKS clusters deployed on isolated networks (apache#9458) --------- Co-authored-by: nvazquez <nicovazquez90@gmail.com> Co-authored-by: Fabricio Duarte <fabricio.duarte.jr@gmail.com> Co-authored-by: João Jandre <48719461+JoaoJandre@users.noreply.github.com> Co-authored-by: Pearl Dsilva <pearl1594@gmail.com>

* 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>

server: refactor listNetworks api database retrievals
e95b72d
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

boring-cyborgBot added the component:networking label Jun 6, 2024

shwstppr mentioned this pull request Jun 6, 2024
listNetworks optimization #9096
Closed
13 tasks

DaanHoogland added this to the 4.19.1.0 milestone Jun 6, 2024

fixes
f627ecd
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr added 2 commits June 10, 2024 12:43

remove unused methods
3f15867
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

imports
11ca762
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr changed the title ~~[WIP] server: refactor listNetworks api database retrievals~~server: refactor listNetworks api database retrievalsJun 10, 2024

shwstppr marked this pull request as ready for review June 10, 2024 09:41

shwstppr requested review from abh1sar, weizhouapache and yadvr June 12, 2024 11:37

yadvr approved these changes Jun 12, 2024
View reviewed changes

yadvr added the status:needs-testing label Jun 12, 2024

yadvr assigned borisstoyanov Jun 12, 2024

sureshanaparti approved these changes Jun 24, 2024
View reviewed changes

vishesh92 reviewed Jun 25, 2024
View reviewed changes

shwstppr added 2 commits June 26, 2024 13:18

fix empty searchcriteria issue
038787f
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

refactor
3a97fae
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

borisstoyanov approved these changes Jun 26, 2024
View reviewed changes

sureshanaparti removed the status:needs-testing label Jun 26, 2024

apache deleted a comment from blueorangutan Jun 27, 2024

sureshanaparti merged commit c7f1ba5 into apache:4.19Jun 28, 2024

DaanHoogland deleted the refactor-listnetworks-execution branch July 1, 2024 06:52

winterhazel mentioned this pull request Jul 29, 2024
Restore listNetworks behavior & clean up the code #9461
Merged
7 tasks

weizhouapache mentioned this pull request Aug 5, 2024
New feature: Dynamic and Static Routing #9470
Merged
14 tasks

Uh oh!

Conversation

shwstppr commented Jun 6, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

weizhouapache commented Jun 6, 2024

Uh oh!

shwstppr commented Jun 6, 2024

Uh oh!

blueorangutan commented Jun 6, 2024

Uh oh!

blueorangutan commented Jun 6, 2024

Uh oh!

shwstppr commented Jun 6, 2024

Uh oh!

blueorangutan commented Jun 6, 2024

Uh oh!

blueorangutan commented Jun 7, 2024

Uh oh!

weizhouapache commented Jun 10, 2024

Uh oh!

blueorangutan commented Jun 10, 2024

Uh oh!

blueorangutan commented Jun 10, 2024

Uh oh!

shwstppr commented Jun 10, 2024

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Jun 24, 2024

Uh oh!

blueorangutan commented Jun 24, 2024

Uh oh!

vishesh92Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

shwstpprJun 25, 2024

Choose a reason for hiding this comment

Uh oh!

shwstpprJun 26, 2024

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Jun 26, 2024

Uh oh!

blueorangutan commented Jun 26, 2024

Uh oh!

blueorangutan commented Jun 26, 2024

Uh oh!

shwstppr commented Jun 26, 2024

Uh oh!

blueorangutan commented Jun 26, 2024

Uh oh!

blueorangutan commented Jun 26, 2024

Uh oh!

yadvr commented Jun 26, 2024

Uh oh!

blueorangutan commented Jun 26, 2024

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jun 26, 2024

Uh oh!

sureshanaparti commented Jun 28, 2024

Uh oh!

codecovBot commented Jun 6, 2024•
edited
Loading