-
Notifications
You must be signed in to change notification settings - Fork 1.2k
api,server,ui: improve listing public ip for associate #11591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11591 +/- ##
===========================================
Coverage 17.54% 17.55%
- Complexity 15483 15523 +40
===========================================
Files 5897 5907 +10
Lines 527484 528631 +1147
Branches 64432 64550 +118
===========================================
+ Hits 92566 92801 +235
- Misses 424508 425397 +889
- Partials 10410 10433 +23
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:
|
@blueorangutan package |
@shwstppr 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 14878 |
@blueorangutan test keepEnv |
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
server/src/main/java/com/cloud/server/ManagementServerImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request enhances the public IP address listing functionality in CloudStack by enabling iterative loading through infinite scroll and supporting multiple states. The primary improvement is in the UI's IP acquisition/association interface, which previously loaded only 500 addresses synchronously.
Key changes include:
- Modified listPublicIpAddresses API to accept comma-separated states parameter
- Replaced static select component with infinite scroll select in UI
- Updated server-side filtering to handle multiple states
- Enhanced test coverage for new multi-state functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
ui/src/views/network/IpAddressesTab.vue | Replaces static select with InfiniteScrollSelect component and removes manual IP loading logic |
ui/src/components/widgets/InfiniteScrollSelect.vue | Adds support for custom label functions and auto-selection of first option |
server/src/main/java/com/cloud/server/ManagementServerImpl.java | Implements comma-separated state parameter parsing and multi-state filtering |
api/src/main/java/org/apache/cloudstack/api/command/user/address/ListPublicIpAddressesCmd.java | Updates API documentation to reflect comma-separated state support |
server/src/test/java/com/cloud/server/ManagementServerImplTest.java | Updates test methods to accommodate new multi-state parameter structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code LGTM
@blueorangutan package |
@harikrishna-patnala 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 14949 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks good, just one remark, no blocker
List<IpAddress.State> states = new ArrayList<>(); | ||
if (StringUtils.isNotBlank(state)) { | ||
for (String s : StringUtils.split(state, ",")) { | ||
try { | ||
states.add(IpAddress.State.valueOf(s)); | ||
} catch (IllegalArgumentException e) { | ||
throw new InvalidParameterValueException("Invalid state: " + s); | ||
} | ||
} | ||
} | ||
Boolean isAllocated = cmd.isAllocatedOnly(); | ||
if (isAllocated == null) { | ||
if (state != null && (state.equalsIgnoreCase(IpAddress.State.Free.name()) || state.equalsIgnoreCase(IpAddress.State.Reserved.name()))) { | ||
if (states.contains(IpAddress.State.Free) || states.contains(IpAddress.State.Reserved)) { | ||
isAllocated = Boolean.FALSE; | ||
} else { | ||
isAllocated = Boolean.TRUE; // default | ||
} | ||
} else { | ||
if (state != null && (state.equalsIgnoreCase(IpAddress.State.Free.name()) || state.equalsIgnoreCase(IpAddress.State.Reserved.name()))) { | ||
if (states.contains(IpAddress.State.Free) || states.contains(IpAddress.State.Reserved)) { | ||
if (isAllocated) { | ||
throw new InvalidParameterValueException("Conflict: allocatedonly is true but state is Free"); | ||
} | ||
} else if (state != null && state.equalsIgnoreCase(IpAddress.State.Allocated.name())) { | ||
} else if (states.contains(IpAddress.State.Allocated)) { | ||
isAllocated = Boolean.TRUE; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move this logic to its own method?
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
@blueorangutan package |
@shwstppr 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 15327 |
}, | ||
listApiParamsForAssociate () { | ||
const params = this.listApiParams | ||
params.state = 'Free,Reserved' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shwstppr I tested the changes, it is not listing any IPs now

Seems to be problem with these params
http://10.0.32.194:8080/client/api/?page=1&pagesize=20&zoneid=eb21bc9a-4c08-4456-938b-af605ef36aae&domainid=d3a3e350-a40d-11f0-b82c-1e009c000155&account=admin&forvirtualnetwork=true&allocatedonly=false& state=Free,Reserved &showicon=true&command=listPublicIpAddresses&response=json&sessionkey=fSa-6YW14skm_KeFEJvTnLPpQQs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking @harikrishna-patnala. Will check and fix. Moving to draft for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harikrishna-patnala changes should work now
Currently, acquire or associate IP action in the UI will load only 500 addresses. ALso, it will load all 500 of them together. This PR uses InfiniteScrollSelect to iteratively load more addresses. To facilitate retrieving both Free and Reserved addresses api and server change has been made for the listPublicIpAddresses API to pass a comma-separated list of states for state parameter. Signed-off-by: Abhishek Kumar <[email protected]>
f7999bc
to
8e414a6
Compare
@blueorangutan package |
@shwstppr 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 ✖️ debian ✖️ suse15. SL-JID 15370 |
Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan package |
@shwstppr 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 15387 |
server/src/main/java/com/cloud/server/ManagementServerImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan package |
@shwstppr 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 15420 |
return null; | ||
} | ||
|
||
public static <T extends Enum<T>> T getEnumIgnoreCase(Class<T> enumClass, String name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use org.apache.commons.lang3.EnumUtils
's getEnumIgnoreCase
instead of this which does the exact the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so let’s make this class a child of the commons one (keeping the enum related utils in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @vishesh92
|
Signed-off-by: Abhishek Kumar <[email protected]>
@blueorangutan package |
@shwstppr 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm. didn't test.
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✖️ suse15. SL-JID 15436 |
Description
Currently, acquire or associate IP action in the UI will load only 500 addresses (or whatever defined by config
default.page.size
). Also, it will load all 500 of them together. This PR uses InfiniteScrollSelect to iteratively load more addresses. To facilitate retrieving both Free and Reserved addresses api and server change has been made for the listPublicIpAddresses API to pass a comma-separated list of states for state parameter.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?