-
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
api,server,ui: improve listing public ip for associate #11591
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #11591 +/- ##
=========================================
Coverage 17.56% 17.56%
- Complexity 15538 15544 +6
=========================================
Files 5909 5909
Lines 529099 529106 +7
Branches 64623 64625 +2
=========================================
+ Hits 92913 92926 +13
+ Misses 425732 425726 -6
Partials 10454 10454
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.
harikrishna-patnala
left a comment
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 |
DaanHoogland
left a comment
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
server/src/main/java/com/cloud/server/ManagementServerImpl.java
Outdated
Show resolved
Hide resolved
|
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
|
@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 15751 |
| import org.apache.commons.codec.binary.Base64; | ||
| import org.apache.commons.collections.CollectionUtils; | ||
| import org.apache.commons.collections.MapUtils; | ||
| import org.apache.commons.lang3.EnumUtils; |
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.
please let com.cloud.utils.EnumUtils inherit org.apache.commons.lang3.EnumUtils and import that one, to keep related utilities in one place and serve as a proxy in case dependencies need updating/replacing.
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
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. |
DaanHoogland
left a comment
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.
this is what I meant. in this case it makes it very simple.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15760 |
Signed-off-by: Abhishek Kumar <[email protected]>
|
@blueorangutan package |
|
@DaanHoogland 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 15769 |
DaanHoogland
left a comment
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, some nitpicking applied
| 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)) { |
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 cannot have a null here, so no biggy but maybe reverse the checks and even use Arrays?
| } | ||
| } 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)) { |
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.
same here
| 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)) { |
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.
and here
| isAllocatedOrReserved = true; | ||
| } | ||
| boolean isAllocatedOrReserved = isAllocated || | ||
| (states.size() == 1 && IpAddress.State.Reserved.equals(states.get(0))); |
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.
| (states.size() == 1 && IpAddress.State.Reserved.equals(states.get(0))); | |
| (states.size() == 1 && states.contains(IpAddress.State.Reserved)); |
|
@blueorangutan test keepEnv |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14835)
|
|
tested in lab env by defining a 1021 long network range. acquire dialog nicely scrolls through them without preloading. |
|
[SF] Trillian test result (tid-14863)
|
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?