Fix and de-risk UserVmManagerImpl external-DHCP startup scan#13635
Fix and de-risk UserVmManagerImpl external-DHCP startup scan#13635Pearl1594 wants to merge 1 commit into
Conversation
|
@blueorangutan package |
|
@Pearl1594 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 Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #13635 +/- ##
============================================
+ Coverage 16.26% 16.27% +0.01%
- Complexity 13435 13438 +3
============================================
Files 5667 5667
Lines 500731 500761 +30
Branches 60803 60804 +1
============================================
+ Hits 81430 81489 +59
+ Misses 410197 410163 -34
- Partials 9104 9109 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses management-server startup delays and instability caused by the external-DHCP startup seeding scan in UserVmManagerImpl, and fixes an NPE in NetworkModelImpl.isSharedNetworkWithoutServices when a network is concurrently removed.
Changes:
- Make external-DHCP VM-IP map seeding asynchronous during management-server startup to avoid gating agent readiness.
- Optimize seeding by batching VM lookups (
listByIds) and memoizing per-offering “without services” checks; add timing/logging. - Add a null-guard in
NetworkModelImpl.isSharedNetworkWithoutServicesand introduce unit tests for the new behaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | Asynchronous seeding on startup; batched VM lookup; memoized offering checks; timing log; guarded execution. |
| server/src/main/java/com/cloud/network/NetworkModelImpl.java | Prevent NPE by returning false when findById returns null. |
| engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDao.java | Add listByIds(List<Long> ids) DAO API for batched VM retrieval. |
| engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java | Implement listByIds using an IN search. |
| server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java | Add tests for batched/filtered seeding and async startup submission. |
| server/src/test/java/com/cloud/network/NetworkModelImplTest.java | Add test ensuring missing network returns false (null-guard). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| List<NetworkVO> networks = _networkDao.listByGuestType(Network.GuestType.Shared); | ||
| Map<Long, Boolean> offeringWithoutServices = new HashMap<>(); | ||
| int networksScanned = 0; | ||
| int nicsAdded = 0; | ||
|
|
||
| for (NetworkVO network : networks) { | ||
| boolean withoutServices = offeringWithoutServices.computeIfAbsent(network.getNetworkOfferingId(), | ||
| offeringId -> _networkModel.listNetworkOfferingServices(offeringId).isEmpty()); | ||
| if (!withoutServices) { | ||
| continue; | ||
| } | ||
| networksScanned++; |
404efec to
1f081f3
Compare
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18596 |
|
@blueorangutan package |
|
|
||
| @Override | ||
| public List<VMInstanceVO> listByIds(List<Long> ids) { | ||
| if (ids == null || ids.isEmpty()) { |
|
Description
UserVmManagerImpl.start() synchronously ran loadVmDetailsInMapForExternalDhcpIp() on the management-server startup thread. Because the CloudStack lifecycle runs every bean's start() sequentially on main before the agent listener (port 8250) is bound, this scan sat directly on the path to agent readiness.
Actual behaviour: on large, DB-contended clusters the scan blocked the agent listener bind for many minutes: 0.8 - 36.9 min observed across some prod environment and repeatedly crashed with a NullPointerException in NetworkModelImpl.isSharedNetworkWithoutServices when a network was removed mid-scan (10 occurrences on 7 hosts / 3 clusters).
Expected behaviour: the management server becomes agent-ready without this scan gating startup, and a concurrently-removed network never crashes it.
Changes:
NetworkModelImpl.isSharedNetworkWithoutServices: guard against a null network from findById (fixes the NPE for all callers).
UserVmManagerImpl.start(): seed the external-DHCP map asynchronously so it no longer gates agent readiness (it is only a warm-start cache for VmIpFetchTask).
loadVmDetailsInMapForExternalDhcpIp: replace the per-NIC findById N+1 with a batched listByIds, memoize the per-offering "without services" check, and add a Profiler timing summary line.
Add unit tests for the null-guard, the batched/filtered seeding, and the async submission.
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?