Fix NSX SDK list handling to fetch all paginated results#12834
Fix NSX SDK list handling to fetch all paginated results#12834sureshanaparti merged 1 commit intoapache:4.22from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes incomplete NSX SDK “list” handling by introducing a generic pagination helper that follows NSX cursor values across pages, merges results, and returns a single combined result to callers (so list operations return the full dataset, not just the first page).
Changes:
- Added a
PagedFetcherutility to iterate through cursor-based pages and accumulate all items. - Updated several NSX
list(...)call sites inNsxApiClientto usePagedFetcherand return merged results. - Added unit tests covering single-page, empty-cursor, multi-page, and null-first-page-items scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/PagedFetcher.java |
New pagination helper that fetches pages via cursor and merges items into the first-page result. |
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java |
Switches multiple NSX list operations to fetch and merge all paginated results; also updates some logging and “first item” lookups to use Optional. |
plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/PagedFetcherTest.java |
New JUnit tests validating pagination and edge cases for the helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12834 +/- ##
============================================
+ Coverage 17.61% 17.95% +0.34%
- Complexity 15706 16531 +825
============================================
Files 5921 6023 +102
Lines 532392 541495 +9103
Branches 65109 66352 +1243
============================================
+ Hits 93788 97238 +3450
- Misses 428015 433287 +5272
- Partials 10589 10970 +381
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ZhyliaievD
force-pushed
the
fix/nsx-sdk-list-pagenation
branch
from
f60b99d to
48e63a3
Compare
|
Hi guys Would appreciate if you share details on PR process, e.g. if I need to provide anything else to get this merged, what is the release schedule and any other potentially missing items needed to complete contribution? Maybe there is a doc regarding this? |
@ZhyliaievD , the process is that 2 people have reviewed (some lobby may be required) one of which at least has tested it, and smoke tests pass. When text only changes we sometimes skip that last test. Not many people have smoke test infra structure. (or at least admit to it.) |
|
@weizhouapache @winterhazel a kind reminder about this PR 😅 |
winterhazel
left a comment
winterhazel
left a comment
There was a problem hiding this comment.
The code looks ok to me, but I don't have access to NSX (nor knowledge in it) to validate it. @Pearl1594 @nvazquez could you have a look at this?
|
@blueorangutan package |
|
@winterhazel 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17373 |
nvazquez
left a comment
nvazquez
left a comment
There was a problem hiding this comment.
Thanks @ZhyliaievD code LGTM
IMO there haven't been significant changes to the NSX plugin since 4.19 so I would expect the fixes to work properly. Would it be possible for you to test the improvements against main branch?
Unfortunately our current env is 4.19 |
|
Hi
Environmnet: PS: There is no diff in nsx module between main branch and tag 4.22.0.0 |
|
@winterhazel @weizhouapache at your discretion. That said @ZhyliaievD @nvazquez , does it make sense to then merge it against 22? cc @Pearl1594 |
weizhouapache
left a comment
weizhouapache
left a comment
There was a problem hiding this comment.
code lgtm
better to fix in 4.22.1.0
cc @sureshanaparti @rajujith
@ZhyliaievD can re-target to 4.22? |
… and non-empty `cursor` field when more pages are available. The previous implementation only fetched the first page and ignored pagination. This change updates the list retrieval flow to: - follow the `cursor` chain until no further pages exist - accumulate items from all pages - return a single merged result to the caller This ensures that list operations return the complete dataset rather than just the first page.
ZhyliaievD
force-pushed
the
fix/nsx-sdk-list-pagenation
branch
from
48e63a3 to
8d70de6
Compare
|
@sureshanaparti Done |
|
@blueorangutan package |
|
Thanks @ZhyliaievD @DaanHoogland @sureshanaparti @weizhouapache - LGTM for 4.22 branch |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 17509 |
|
@blueorangutan test |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15891)
|
github-project-automation
bot
moved this from In Progress
to Done
in Apache CloudStack 4.22.1
Description
NSX SDK list operations are pageable: the API returns a non-null and non-empty
cursorfield when more pages are available. The previous implementation only fetched the first page and ignored pagination.This change updates the list retrieval flow to:
cursorchain until no further pages existThis ensures that list operations return the complete dataset rather than just the first page.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity