listHosts: add 'core' details#13444
Conversation
|
@blueorangutan package |
|
@nvazquez 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.22 #13444 +/- ##
============================================
- Coverage 17.67% 17.67% -0.01%
+ Complexity 15792 15790 -2
============================================
Files 5922 5922
Lines 533167 533178 +11
Branches 65210 65213 +3
============================================
- Hits 94246 94220 -26
- Misses 428276 428313 +37
Partials 10645 10645
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:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18291 |
|
@nvazquez |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new opt-in details=core option for the listHosts API, intended to return a lightweight “core” host response and avoid per-host (N+1) lookups performed by the existing response builder.
Changes:
- Added
coretoApiConstants.HostDetailssolistHostscan acceptdetails=core. - Implemented a new “core” host response path in
HostJoinDaoImpl.newHostResponse(...)viasetNewCoreHostResponse(...).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
server/src/main/java/com/cloud/api/query/dao/HostJoinDaoImpl.java |
Adds a new lightweight host response builder and routes details=core requests to it. |
api/src/main/java/org/apache/cloudstack/api/ApiConstants.java |
Adds core to the HostDetails enum (allowed values for details). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| hostResponse.setZoneId(host.getZoneUuid()); | ||
| hostResponse.setZoneName(host.getZoneName()); | ||
| hostResponse.setPodId(host.getPodUuid()); | ||
| hostResponse.setPodName(host.getPodName()); | ||
| if (host.getClusterId() > 0) { |
| // msid is returned as-is; callers resolve it to avoid a per-host lookup | ||
| if (host.getManagementServerId() != null) { | ||
| hostResponse.setManagementServerId(host.getManagementServerId().toString()); | ||
| } |
There was a problem hiding this comment.
Reproduced while testing. In core the managementserverid comes back as the raw numeric id, while every other mode returns the UUID - so the same field changes format depending on the detail flag, which could be a risk for consumers reading it. From a QA perspective, omitting it (as suggested above) is the safer of the two - a missing field is easier to handle than a value that looks valid but is in the wrong/unexpected format.
| if (details.contains(HostDetails.core)) { | ||
| setNewCoreHostResponse(host, hostResponse); | ||
| } else { | ||
| setNewHostResponseBase(host, details, hostResponse); | ||
| } |
There was a problem hiding this comment.
Confirmed in testing. Any request that mixes core with another value (core,stats ; core,capacity ; core,events; core,all; core,min) returns the lightweight core set and silently drops the other requested fields. So if it fails quietly - the caller gets fewer fields than they asked for, with no error.
|
@nvazquez in the description you mentioned avoiding requests when getting details like cpu usage, but I don't see this parameter in specific being added to the response, am I missing something? |
|
|
||
| public enum HostDetails { | ||
| all, capacity, events, stats, min; | ||
| all, capacity, events, stats, min, core; |
There was a problem hiding this comment.
| all, capacity, events, stats, min, core; | |
| all, capacity, core, events, stats, min; |
|
Also, the copilot left a review regarding the MS ID that I kind of agree. This API is admin only, so shouldn't be a big deal returning the internal ID, however, this same api returns MS UUID when other filters are passed, having this one be different may cause confusion |
|
Thanks @RosiKyu @GaOrtiga @weizhouapache - I have refactored the code according to your review comments and Copilot comments. The only pending item is the msid return value, I'm keeping this PR in draft while this is not addressed and will put back to review state once its sorted out |
|
@blueorangutan package |
|
@nvazquez 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 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18351 |
Description
This PR adds a detail
corefor thelistHostsAPI, which makes it so we don't make N+1 queries when getting details (cpu usage, clusterid, etc.).This will drastically reduce the time it takes to listHosts and allows us to get a list of hosts & their state, which can take lot of time for the API to complete if the core detail is not passed
This is strictly opt-in with
listHosts details=coreand the default bahaviour is unchanged.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?