Batch network lookups per NIC in NetworkOrchestrator to eliminate N+1 queries during migration by vishesh92 · Pull Request #13354 · apache/cloudstack

vishesh92 · 2026-06-05T09:14:40Z

Description

Three methods called _networksDao.findById(nic.getNetworkId()) per NIC in a loop: setHypervisorHostname, prepareNicForMigration, prepareAllNicsForMigration. Now batch-loads all networks for a VM's NICs in a single WHERE id IN (...) query.

Add NetworkDao.listByIds(List) for batch ID lookup
Add getNetworkMapForNics helper returning Map<Long, NetworkVO>
Replace per-NIC findById with map lookup at 3 sites
Add null guard for deleted networks (pre-existing NPE risk on all 3 sites)

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
Test (unit or integration test code)

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?

vishesh92 · 2026-06-05T09:16:34Z

@blueorangutan package

blueorangutan · 2026-06-05T09:18:04Z

@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

codecov · 2026-06-05T09:29:23Z

Codecov Report

❌ Patch coverage is 0% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.41%. Comparing base (a951ac6) to head (7acfc1a).

Files with missing lines	Patch %	Lines
...tack/engine/orchestration/NetworkOrchestrator.java	0.00%	30 Missing ⚠️
...ain/java/com/cloud/network/dao/NetworkDaoImpl.java	0.00%	10 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.22   #13354      +/-   ##
============================================
+ Coverage     17.67%   19.41%   +1.74%     
- Complexity    15797    19155    +3358     
============================================
  Files          5923     6284     +361     
  Lines        533349   564880   +31531     
  Branches      65248    68921    +3673     
============================================
+ Hits          94253   109685   +15432     
- Misses       428437   443294   +14857     
- Partials      10659    11901    +1242

Flag	Coverage Δ
uitests	`3.46% <ø> (-0.24%)`	⬇️
unittests	`20.67% <0.00%> (+1.92%)`	⬆️

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

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

blueorangutan · 2026-06-05T10:01:57Z

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

vishesh92 · 2026-06-08T09:35:18Z

@blueorangutan test

blueorangutan · 2026-06-08T09:36:04Z

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

blueorangutan · 2026-06-09T00:21:14Z

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

Test	Result	Time (s)	Test File

Copilot

Pull request overview

This PR reduces N+1 database lookups in NetworkOrchestrator during VM migration by batch-loading NetworkVO records for a VM’s NICs (via a new NetworkDao.listByIds method) and then using an in-memory map for per-NIC network access, with added null-guards when a referenced network no longer exists.

Changes:

Added NetworkDao.listByIds(List<Long>) and implemented it in NetworkDaoImpl using an IN search criteria.
Introduced a batchLoadNetworksForNics helper in NetworkOrchestrator and replaced per-NIC findById calls at three migration-related call sites.
Updated the test mock MockNetworkDaoImpl to implement the new DAO method.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
server/src/test/java/com/cloud/vpc/dao/MockNetworkDaoImpl.java	Adds `listByIds` to satisfy the updated `NetworkDao` interface.
engine/schema/src/main/java/com/cloud/network/dao/NetworkDaoImpl.java	Implements batch network lookup via `id IN (...)`.
engine/schema/src/main/java/com/cloud/network/dao/NetworkDao.java	Extends the DAO contract with `listByIds(List<Long>)`.
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java	Uses a batch-loaded map to avoid per-NIC network DAO calls during migration prep/hostname update.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    @Override
+    public List<NetworkVO> listByIds(List<Long> ids) {
+        return null;
+    }


+    private Map<Long, NetworkVO> batchLoadNetworksForNics(List<NicVO> nics) {
+        List<Long> networkIds = new ArrayList<>(nics.size());
+        for (NicVO nic : nics) {
+            networkIds.add(nic.getNetworkId());
+        }
+        Map<Long, NetworkVO> result = new HashMap<>(networkIds.size());
+        if (!networkIds.isEmpty()) {
+            for (NetworkVO network : _networksDao.listByIds(networkIds)) {
+                result.put(network.getId(), network);
+            }
+        }
+        return result;
+    }


+    public List<NetworkVO> listByIds(List<Long> ids) {
+        if (ids == null || ids.isEmpty()) {
+            return new ArrayList<>();
+        }
+        SearchBuilder<NetworkVO> sb = createSearchBuilder();
+        sb.and("id", sb.entity().getId(), Op.IN);
+        sb.done();
+        SearchCriteria<NetworkVO> sc = sb.create();
+        sc.setParameters("id", ids.toArray());
+        return listBy(sc);


nvazquez

Thanks @vishesh92 - can you please rebase to branch 4.22 and check if Copilot comments are applicable?

… queries during migration Three methods called _networksDao.findById(nic.getNetworkId()) per NIC in a loop: setHypervisorHostname, prepareNicForMigration, prepareAllNicsForMigration. Now batch-loads all networks for a VM's NICs in a single WHERE id IN (...) query. - Add NetworkDao.listByIds(List<Long>) for batch ID lookup - Add getNetworkMapForNics helper returning Map<Long, NetworkVO> - Replace per-NIC findById with map lookup at 3 sites - Add null guard for deleted networks (pre-existing NPE risk on all 3 sites) - Site 2 (prepare) left unchanged — findById only on error path, not N+1

boring-cyborg Bot added component:networking component:orchestration component:vpc labels Jun 5, 2026

github-actions Bot mentioned this pull request Jun 5, 2026

[repo-status] Daily Status Report – June 5, 2026 #13360

Closed

weizhouapache added this to the 4.22.2 milestone Jun 29, 2026

sureshanaparti requested a review from Copilot July 6, 2026 10:57

Copilot started reviewing on behalf of sureshanaparti July 6, 2026 10:58 View session

Copilot AI reviewed Jul 6, 2026

View reviewed changes

sureshanaparti marked this pull request as ready for review July 7, 2026 10:44

sureshanaparti requested a review from nvazquez July 7, 2026 10:46

nvazquez reviewed Jul 7, 2026

View reviewed changes

vishesh92 force-pushed the batch-network-lookups branch from 81ca4c9 to 7acfc1a Compare July 7, 2026 12:02

vishesh92 changed the base branch from main to 4.22 July 7, 2026 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Batch network lookups per NIC in NetworkOrchestrator to eliminate N+1 queries during migration#13354

Batch network lookups per NIC in NetworkOrchestrator to eliminate N+1 queries during migration#13354
vishesh92 wants to merge 1 commit into
apache:4.22from
shapeblue:batch-network-lookups

vishesh92 commented Jun 5, 2026

Uh oh!

vishesh92 commented Jun 5, 2026

Uh oh!

blueorangutan commented Jun 5, 2026

Uh oh!

codecov Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

blueorangutan commented Jun 5, 2026

Uh oh!

vishesh92 commented Jun 8, 2026

Uh oh!

blueorangutan commented Jun 8, 2026

Uh oh!

blueorangutan commented Jun 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

nvazquez left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

vishesh92 commented Jun 5, 2026

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!

vishesh92 commented Jun 5, 2026

Uh oh!

blueorangutan commented Jun 5, 2026

Uh oh!

codecov Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Jun 5, 2026

Uh oh!

vishesh92 commented Jun 8, 2026

Uh oh!

blueorangutan commented Jun 8, 2026

Uh oh!

blueorangutan commented Jun 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Jun 5, 2026 •

edited

Loading