Skip to content

server: fix list public ip returns duplicated records#5464

Merged
yadvr merged 2 commits into
apache:4.15from
shapeblue:4.15-fix-duplicated-public-ip
Sep 20, 2021
Merged

server: fix list public ip returns duplicated records#5464
yadvr merged 2 commits into
apache:4.15from
shapeblue:4.15-fix-duplicated-public-ip

Conversation

@weizhouapache
Copy link
Copy Markdown
Member

@weizhouapache weizhouapache commented Sep 17, 2021

Description

This PR fixes the issue that "Free" public ip addresses are listed twice in api response.

there is no issue with api if domainid, account or allocatedonly=true is passed.
there is no issue on UI.

(localcloud) SBCM5> > list publicipaddresses forvirtualnetwork=true allocatedonly=false filter=ipaddress,state,
{
  "count": 36,
  "publicipaddress": [
    {
      "ipaddress": "10.0.55.9",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.8",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.7",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.6",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.5",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.4",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.3",
      "state": "Allocated"
    },
    {
      "ipaddress": "10.0.55.20",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.2",
      "state": "Allocated"
    },
    {
      "ipaddress": "10.0.55.19",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.18",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.17",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.16",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.15",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.14",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.13",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.12",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.11",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.10",
      "state": "Allocated"
    },
    {
      "ipaddress": "10.0.55.1",
      "state": "Allocated"
    },
    {
      "ipaddress": "10.0.55.9",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.8",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.7",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.6",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.5",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.4",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.20",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.19",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.18",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.17",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.16",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.15",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.14",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.13",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.12",
      "state": "Free"
    },
    {
      "ipaddress": "10.0.55.11",
      "state": "Free"
    }
  ]
}

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

weizhouapache
weizhouapache commented Sep 17, 2021
View reviewed changes
Comment thread server/src/main/java/com/cloud/server/ManagementServerImpl.java
sc2.setParameters("ids", freeAddrIds.toArray());
addrs.addAll(_publicIpAddressDao.search(sc2, searchFilter)); // Allocated + Free
}
Collections.sort(addrs, Comparator.comparing(IPAddressVO::getAddress));
Copy link
Copy Markdown
Member Author

@weizhouapache weizhouapache Sep 17, 2021

Choose a reason for hiding this comment

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

@nvazquez @rhtyd
do we need to order public ip address ?

Copy link
Copy Markdown
Member

@yadvr yadvr Sep 17, 2021

Choose a reason for hiding this comment

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

Yes it would be good

Copy link
Copy Markdown
Contributor

@nvazquez nvazquez Sep 19, 2021

Choose a reason for hiding this comment

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

+1

@yadvr yadvr added this to the 4.16.0.0 milestone Sep 17, 2021
sureshanaparti
sureshanaparti reviewed Sep 17, 2021
View reviewed changes
Comment thread server/src/main/java/com/cloud/server/ManagementServerImpl.java Outdated
}

buildParameters(sb, cmd);
buildParameters(sb, cmd, true);
Copy link
Copy Markdown
Contributor

@sureshanaparti sureshanaparti Sep 17, 2021

Choose a reason for hiding this comment

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

@weizhouapache If isAllocated is hardcoded in both the buildParameters() calls, where exactly we use the 'allocatedonly' parameter from listPublicIpAddresses API cmd.

Copy link
Copy Markdown
Member Author

@weizhouapache weizhouapache Sep 17, 2021

Choose a reason for hiding this comment

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

@sureshanaparti good point.
isAllocated is used in line 2136.
I updated this pr to fix an issue with shared networks.

@weizhouapache
Copy link
Copy Markdown
Member Author

weizhouapache commented Sep 17, 2021

@blueorangutan package

sureshanaparti
sureshanaparti approved these changes Sep 17, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@sureshanaparti sureshanaparti left a comment

Choose a reason for hiding this comment

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

code LGTM

nvazquez
nvazquez approved these changes Sep 19, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

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

LGTM, can you target it for main branch?

@weizhouapache
Copy link
Copy Markdown
Member Author

weizhouapache commented Sep 19, 2021

blueorangutan

LGTM, can you target it for main branch?

@nvazquez 4.15.2.0 will be released soon. should we commit bug fix to 4.15 ?

@weizhouapache
Copy link
Copy Markdown
Member Author

weizhouapache commented Sep 19, 2021

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Sep 19, 2021

@weizhouapache a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Sep 19, 2021

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 1313

@nvazquez
Copy link
Copy Markdown
Contributor

nvazquez commented Sep 19, 2021

@weizhouapache IMO it shouldn't as it is not a blocker, let's ask @rhtyd

@weizhouapache
Copy link
Copy Markdown
Member Author

weizhouapache commented Sep 19, 2021

@weizhouapache IMO it shouldn't as it is not a blocker, let's ask @rhtyd

@nvazquez 4.15 branch has version 4.15.3.0-SNAPSHOT now.

@weizhouapache
Copy link
Copy Markdown
Member Author

weizhouapache commented Sep 19, 2021

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Sep 19, 2021

@weizhouapache a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

davidjumani
davidjumani approved these changes Sep 20, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@davidjumani davidjumani left a comment

Choose a reason for hiding this comment

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

Verified, records are not duplicated

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Sep 20, 2021

Trillian test result (tid-2120)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32162 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5464-t2120-kvm-centos7.zip
Smoke tests completed. 86 look OK, 1 have errors
Only failed tests results shown below:

Test Result Time (s) Test File
test_01_nic Error 56.30 test_nic.py

@yadvr yadvr merged commit f50cc27 into apache:4.15 Sep 20, 2021
This was referenced Sep 21, 2021
Merged
Merged
@weizhouapache weizhouapache mentioned this pull request Sep 29, 2021
Closed
7 tasks
yadvr pushed a commit to shapeblue/cloudstack that referenced this pull request May 18, 2022
@weizhouapache @yadvr
server: fix list public ip returns duplicated records (apache#5464)
5fe2332 
* server: fix list public ip returns duplicated records

* update apache#5464: fix shared network

(cherry picked from commit f50cc27)
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
@weizhouapache weizhouapache deleted the 4.15-fix-duplicated-public-ip branch December 9, 2022 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr left review comments

@nvazquez nvazquez nvazquez approved these changes

@davidjumani davidjumani davidjumani approved these changes

@sureshanaparti sureshanaparti sureshanaparti approved these changes

Assignees

No one assigned

Labels

Severity:Minor type:bug

Projects

None yet

Milestone

4.16.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@weizhouapache @blueorangutan @nvazquez @yadvr @davidjumani @sureshanaparti