Skip to content

Add pagination to cloudstack api calls - #102

Open
vishesh92 wants to merge 1 commit into
apache:mainfrom
shapeblue:add-pagination
Open

Add pagination to cloudstack api calls#102
vishesh92 wants to merge 1 commit into
apache:mainfrom
shapeblue:add-pagination

Conversation

@vishesh92

Copy link
Copy Markdown
Member

Fixes #99

Copilot AI lite review requested due to automatic review settings August 21, 2026 07:07
@codecov-commenter

codecov-commenter commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.93%. Comparing base (0fdca7b) to head (fbcb035).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cloudstack_loadbalancer.go 89.18% 4 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
+ Coverage   50.05%   53.93%   +3.88%     
==========================================
  Files           4        5       +1     
  Lines         975     1042      +67     
==========================================
+ Hits          488      562      +74     
+ Misses        473      459      -14     
- Partials       14       21       +7     

☔ 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses CloudStack API list truncation (default default.page.size, typically 500) by introducing a reusable pagination helper and applying it to load balancer–related list calls so large clusters can still correctly discover VMs/rules and reconcile load balancer membership (fixing #99).

Changes:

  • Add a generic listAll pagination helper for CloudStack List*Params and unit tests covering paging behavior and edge cases.
  • Apply paging to load balancer rule lookup, rule instance listing, VM listing for host verification, firewall rule listing, and network ACL listing.
  • Add/extend tests to cover pagination regressions and duplicate entries across pages during reconciliation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
pagination.go Introduces generic pagination helper (listAll) for CloudStack list APIs.
pagination_test.go Adds focused unit tests validating paging behavior and error handling for listAll.
cloudstack_loadbalancer.go Switches several CloudStack list calls to use listAll and adds deduplication where paging may return duplicates.
cloudstack_loadbalancer_test.go Adds regression tests for VM/rule-instance pagination and duplicate handling in reconciliation logic.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pagination.go Outdated
Copilot AI review requested due to automatic review settings August 21, 2026 12:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

cloudstack_loadbalancer.go:967

  • listAll can return the same firewall rule on multiple pages if rules are created/deleted while paging (offset shifts). Because updateFirewallRule tracks candidates in a map keyed by *FirewallRule, duplicate entries with the same rule.Id won’t dedupe and can cause the “matching” rule to still be deleted. Deduplicate firewall rules by Id before returning them from listFirewallRules (or change filtering to key by rule.Id).
	return rules, nil
}

Comment thread cloudstack_loadbalancer.go Outdated

var hostIDs []string
var networkID string
seen := map[string]bool{}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment to justify this variable

Comment thread cloudstack_loadbalancer.go Outdated

var hostIDs []string
var networkID string
seen := map[string]bool{}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
seen := map[string]bool{}
seen := map[string]bool{} // used to check whether the changing set of VMs contains one we had already seen in another page.

Comment thread cloudstack_loadbalancer.go Outdated
Comment on lines +545 to +546
// Paging over a set of VMs that is changing underneath us can return
// the same VM on more than one page.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Paging over a set of VMs that is changing underneath us can return
// the same VM on more than one page.

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core code looks good. I just wonder if we can structure the test code a little better, as in the loadbalancer test, the new methods are 140 and 110 lines long.

Copilot AI review requested due to automatic review settings September 2, 2026 10:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

listAll can hit the page cap and return partial results with no error, which can cause reconciliation logic to act on incomplete inventories (e.g., create/delete decisions).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread pagination.go
Comment on lines +58 to +62
if page > maxListPages {
klog.Warningf("stopped paging after %d pages holding %d of %d records; results may be incomplete",
maxListPages, len(collected), count)
break
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Load Balancer doesn't get created when there are too many VMs

4 participants