You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/02/10 10:36:36 UTC
[GitHub] [cloudstack] ravening opened a new pull request #4675: Bug fix in displaying public IP address of shared networks
ravening opened a new pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675
### Description
Public IP address dedicated to one domain should not be accessed
by other domains. Also root admin should be able to display all
public ip address in system.
Currently following issues exist
1. Public IP address assigned to one domain can be accessed by
other sibling domains
If use.system.public.ip is false then child domains should not
see public ip of ROOT domain
Before fix
```
(test1) mgt01 > list publicipaddresses listall=true fordisplay=true allocatedonly=false forvirtualnetwork=true filter=ipaddress,
{
"count": 59,
"publicipaddress": [
```
After fix
```
(test) mgt01 > list publicipaddresses listall=true fordisplay=true allocatedonly=false forvirtualnetwork=true filter=ipaddress,
{
"count": 10,
```
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ********************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] 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
- [X] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [X] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-787947605
There's also a conflict @ravening
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-806381023
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-806381493
@shwstppr a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd merged pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-779045555
@ravening conflict here. Should this go in 4.15.1.0 as it is a bug fix?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#discussion_r576873859
##########
File path: engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java
##########
@@ -208,5 +208,10 @@ void allocateNicValues(NicProfile nic, DataCenter dc, VirtualMachineProfile vm,
void releasePodIp(Long id) throws CloudRuntimeException;
boolean isUsageHidden(IPAddressVO address);
+
+ List<IPAddressVO> listAvailablePublicIps(final long dcId, final Long podId, final List<Long> vlanDbIds, final Account owner, final VlanType vlanUse, final Long guestNetworkId,
+ final boolean sourceNat, final boolean assign, final boolean allocate, final String requestedIp, final boolean isSystem,
+ final Long vpcId, final Boolean displayIp, final boolean forSystemVms, final boolean lockOneRow)
+ throws InsufficientAddressCapacityException;
Review comment:
```suggestion
List<IPAddressVO> listAvailablePublicIps(final long dcId,
final Long podId,
final List<Long> vlanDbIds,
final Account owner,
final VlanType vlanUse,
final Long guestNetworkId,
final boolean sourceNat,
final boolean assign,
final boolean allocate,
final String requestedIp,
final boolean isSystem,
final Long vpcId,
final Boolean displayIp,
final boolean forSystemVms,
final boolean lockOneRow)
throws InsufficientAddressCapacityException;
```
##########
File path: server/src/main/java/com/cloud/api/ApiResponseHelper.java
##########
@@ -910,6 +910,41 @@ public IPAddressResponse createIPAddressResponse(ResponseView view, IpAddress ip
}
}
+ // show vm info for shared networks
Review comment:
this comment should be a method call to `showVmInfoForSharedNetworks()` and the bit below factorred out in a hierarchy of methods.
##########
File path: server/src/main/java/com/cloud/api/ApiResponseHelper.java
##########
@@ -910,6 +910,41 @@ public IPAddressResponse createIPAddressResponse(ResponseView view, IpAddress ip
}
}
+ // show vm info for shared networks
+ if (!forVirtualNetworks) {
+ NicVO nic = ApiDBUtils.findByIp4AddressAndNetworkId(ipAddr.getAddress().toString(), ipAddr.getNetworkId());
+ if (nic != null && nic.getVmType() == VirtualMachine.Type.User) {
+ UserVm vm = ApiDBUtils.findUserVmById(nic.getInstanceId());
+ if (vm != null) {
+ ipResponse.setVirtualMachineId(vm.getUuid());
+ ipResponse.setVirtualMachineName(vm.getHostName());
+ if (vm.getDisplayName() != null) {
+ ipResponse.setVirtualMachineDisplayName(vm.getDisplayName());
+ } else {
+ ipResponse.setVirtualMachineDisplayName(vm.getHostName());
+ }
+ }
+ } else if (nic != null && nic.getVmType() == VirtualMachine.Type.DomainRouter) {
+ ipResponse.setIsSystem(true);
+ }
+ if (nic == null) { // find in nic_secondary_ips, user vm only
+ NicSecondaryIpVO secondaryIp =
+ ApiDBUtils.findSecondaryIpByIp4AddressAndNetworkId(ipAddr.getAddress().toString(), ipAddr.getNetworkId());
+ if (secondaryIp != null) {
+ UserVm vm = ApiDBUtils.findUserVmById(secondaryIp.getVmId());
+ if (vm != null) {
+ ipResponse.setVirtualMachineId(vm.getUuid());
+ ipResponse.setVirtualMachineName(vm.getHostName());
+ if (vm.getDisplayName() != null) {
+ ipResponse.setVirtualMachineDisplayName(vm.getDisplayName());
+ } else {
+ ipResponse.setVirtualMachineDisplayName(vm.getHostName());
+ }
+ }
+ }
+ }
Review comment:
i would turn around the logic:
```
if (nic != null && nic.getVmType() == VirtualMachine.Type.User) {
} else if (nic != null && nic.getVmType() == VirtualMachine.Type.DomainRouter) {
}
if (nic == null) { // find in nic_secondary_ips, user vm only
}
```
to
```
if (nic == null) { // find in nic_secondary_ips, user vm only
} else if (VirtualMachine.Type.User.equals(nic.getVmType())) {
} else if (VirtualMachine.Type.DomainRouter.equals(nic.getVmType())) {
}
```
also the
```
// find in nic_secondary_ips, user vm only
```
is never checked for vmtype. maybe not relevant
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-781245192
> @ravening can you look at those failures ^^?
@DaanHoogland will look into it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening removed a comment on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
ravening removed a comment on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-797361250
> @ravening still some issue
@shwstppr but I dont think the issue is with ip management.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on a change in pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
shwstppr commented on a change in pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#discussion_r596611620
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -2004,42 +2023,101 @@ private void abstractDataStoreClustersList(List<StoragePool> storagePools, List<
@Override
public Pair<List<? extends IpAddress>, Integer> searchForIPAddresses(final ListPublicIpAddressesCmd cmd) {
- final Object keyword = cmd.getKeyword();
- final Long physicalNetworkId = cmd.getPhysicalNetworkId();
final Long associatedNetworkId = cmd.getAssociatedNetworkId();
- final Long sourceNetworkId = cmd.getNetworkId();
final Long zone = cmd.getZoneId();
- final String address = cmd.getIpAddress();
final Long vlan = cmd.getVlanId();
final Boolean forVirtualNetwork = cmd.isForVirtualNetwork();
- final Boolean forLoadBalancing = cmd.isForLoadBalancing();
final Long ipId = cmd.getId();
- final Boolean sourceNat = cmd.isSourceNat();
- final Boolean staticNat = cmd.isStaticNat();
+ final Long networkId = cmd.getNetworkId();
final Long vpcId = cmd.getVpcId();
- final Boolean forDisplay = cmd.getDisplay();
- final Map<String, String> tags = cmd.getTags();
final String state = cmd.getState();
Boolean isAllocated = cmd.isAllocatedOnly();
if (isAllocated == null) {
- isAllocated = Boolean.TRUE;
-
- if (state != null) {
+ if (state != null && state.equalsIgnoreCase(IpAddress.State.Free.name())) {
isAllocated = Boolean.FALSE;
+ } else {
+ isAllocated = Boolean.TRUE; // default
+ }
+ } else {
+ if (state != null && state.equalsIgnoreCase(IpAddress.State.Free.name())) {
+ if (isAllocated) {
+ throw new InvalidParameterValueException("Conflict: allocatedonly is true but state is Free");
+ }
+ } else if (state != null && state.equalsIgnoreCase(IpAddress.State.Allocated.name())) {
+ isAllocated = Boolean.TRUE;
Review comment:
@ravening what if API was passed `allocatedonly=False` and `state=Allocated`? Shouldn't we raise exception like above case (allocatedonly=true & state=Free)
This block can be simplified (or maybe complicated :smile:) with something like,
```
if (Boolean.TRUE.equals(isAllocated) && IpAddress.State.Free.name().equals(state)) {
throw new InvalidParameterValueException("Conflict: allocatedonly is true but state is Free");
}
if (Boolean.FALSE.equals(isAllocated) && IpAddress.State.Allocated.name().equals(state)) {
throw new InvalidParameterValueException("Conflict: allocatedonly is false but state is Allocated");
}
if (isAllocated == null) {
isAllocated = !IpAddress.State.Free.name().equals(state);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-801676318
@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd removed a comment on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
rhtyd removed a comment on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-805731850
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-806407312
<b>Trillian test result (tid-253)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40948 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4675-t253-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 84 look OK, 2 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_deploy_kubernetes_cluster | `Failure` | 3602.73 | test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 71.83 | test_kubernetes_clusters.py
test_01_migrate_VM_and_root_volume | `Error` | 67.20 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 51.12 | test_vm_life_cycle.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-796642802
> @rhtyd I've fixed the conflicts but test failures from previous run doesn't seem intermittent and would need investigation/changes cc @ravening
>
> ```
> 2021-02-16 21:39:22,806 - CRITICAL - FAILED: test_public_ip_admin_account: ['Traceback (most recent call last):\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n testMethod()\n', ' File "/marvin/tests/smoke/test_network.py", line 236, in test_public_ip_admin_account\n self.fail("list public ip response is not empty")\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 450, in fail\n raise self.failureException(msg)\n', 'AssertionError: list public ip response is not empty\n']
> 2021-02-16 21:39:25,098 - CRITICAL - FAILED: test_public_ip_user_account: ['Traceback (most recent call last):\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n testMethod()\n', ' File "/marvin/tests/smoke/test_network.py", line 285, in test_public_ip_user_account\n "Check if disassociated IP Address is no longer available"\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 553, in assertEqual\n assertion_func(first, second, msg=msg)\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 546, in _baseAssertEqual\n raise self.failureException(msg)\n', 'AssertionError: Check if disassociated IP Address is no longer available\n']
> 2021-02-16 21:56:14,340 - CRITICAL - FAILED: test_releaseIP: ['Traceback (most recent call last):\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n testMethod()\n', ' File "/marvin/tests/smoke/test_network.py", line 894, in test_releaseIP\n "Failed to disassociate IP address")\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 462, in assertTrue\n raise self.failureException(msg)\n', 'AssertionError: Failed to disassociate IP address\n']
> ```
>
@shwstppr I have pushed a commit. can you retrigger the tests to see if that fixes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-801676057
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-779877775
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-797360468
@ravening still some issue
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-780017780
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-807010234
<b>Trillian test result (tid-259)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35585 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4675-t259-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 84 look OK, 2 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 70.88 | test_kubernetes_clusters.py
test_01_migrate_VM_and_root_volume | `Error` | 67.17 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 50.06 | test_vm_life_cycle.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-805732860
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-796663347
Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 90
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-796651482
@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-796576099
@rhtyd I've fixed the conflicts but test failures from previous run doesn't seem intermittent and would need investigation/changes cc @ravening
```
2021-02-16 21:39:22,806 - CRITICAL - FAILED: test_public_ip_admin_account: ['Traceback (most recent call last):\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n testMethod()\n', ' File "/marvin/tests/smoke/test_network.py", line 236, in test_public_ip_admin_account\n self.fail("list public ip response is not empty")\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 450, in fail\n raise self.failureException(msg)\n', 'AssertionError: list public ip response is not empty\n']
2021-02-16 21:39:25,098 - CRITICAL - FAILED: test_public_ip_user_account: ['Traceback (most recent call last):\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n testMethod()\n', ' File "/marvin/tests/smoke/test_network.py", line 285, in test_public_ip_user_account\n "Check if disassociated IP Address is no longer available"\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 553, in assertEqual\n assertion_func(first, second, msg=msg)\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 546, in _baseAssertEqual\n raise self.failureException(msg)\n', 'AssertionError: Check if disassociated IP Address is no longer available\n']
2021-02-16 21:56:14,340 - CRITICAL - FAILED: test_releaseIP: ['Traceback (most recent call last):\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n testMethod()\n', ' File "/marvin/tests/smoke/test_network.py", line 894, in test_releaseIP\n "Failed to disassociate IP address")\n', ' File "/usr/lib64/python2.7/unittest/case.py", line 462, in assertTrue\n raise self.failureException(msg)\n', 'AssertionError: Failed to disassociate IP address\n']
```
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-781244455
@ravening can you look at those failures ^^?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#discussion_r599400282
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -2004,42 +2023,101 @@ private void abstractDataStoreClustersList(List<StoragePool> storagePools, List<
@Override
public Pair<List<? extends IpAddress>, Integer> searchForIPAddresses(final ListPublicIpAddressesCmd cmd) {
- final Object keyword = cmd.getKeyword();
- final Long physicalNetworkId = cmd.getPhysicalNetworkId();
final Long associatedNetworkId = cmd.getAssociatedNetworkId();
- final Long sourceNetworkId = cmd.getNetworkId();
final Long zone = cmd.getZoneId();
- final String address = cmd.getIpAddress();
final Long vlan = cmd.getVlanId();
final Boolean forVirtualNetwork = cmd.isForVirtualNetwork();
- final Boolean forLoadBalancing = cmd.isForLoadBalancing();
final Long ipId = cmd.getId();
- final Boolean sourceNat = cmd.isSourceNat();
- final Boolean staticNat = cmd.isStaticNat();
+ final Long networkId = cmd.getNetworkId();
final Long vpcId = cmd.getVpcId();
- final Boolean forDisplay = cmd.getDisplay();
- final Map<String, String> tags = cmd.getTags();
final String state = cmd.getState();
Boolean isAllocated = cmd.isAllocatedOnly();
if (isAllocated == null) {
- isAllocated = Boolean.TRUE;
-
- if (state != null) {
+ if (state != null && state.equalsIgnoreCase(IpAddress.State.Free.name())) {
isAllocated = Boolean.FALSE;
+ } else {
+ isAllocated = Boolean.TRUE; // default
+ }
+ } else {
+ if (state != null && state.equalsIgnoreCase(IpAddress.State.Free.name())) {
+ if (isAllocated) {
+ throw new InvalidParameterValueException("Conflict: allocatedonly is true but state is Free");
+ }
+ } else if (state != null && state.equalsIgnoreCase(IpAddress.State.Allocated.name())) {
+ isAllocated = Boolean.TRUE;
Review comment:
> @ravening what if API was passed `allocatedonly=False` and `state=Allocated`? Shouldn't we raise exception like above case (allocatedonly=true & state=Free)
>
> This block can be simplified (or maybe complicated 😄) with something like,
>
> ```
> if (Boolean.TRUE.equals(isAllocated) && IpAddress.State.Free.name().equals(state)) {
> throw new InvalidParameterValueException("Conflict: allocatedonly is true but state is Free");
> }
> if (Boolean.FALSE.equals(isAllocated) && IpAddress.State.Allocated.name().equals(state)) {
> throw new InvalidParameterValueException("Conflict: allocatedonly is false but state is Allocated");
> }
> if (isAllocated == null) {
> isAllocated = !IpAddress.State.Free.name().equals(state);
> }
> ```
I dont think we need exception when `allocatedonly=false` and `state=allocated`. since `allocatedonly=false` indicates it can return both allocated and non allocated and in the returned list we are filtering by allocated state.
If `allocatedonly=true` and `state=free`, we can throw exception since we know that the list wont contain free ip's anyway
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-797126584
<b>[S] Trillian test result (tid-90)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37102 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4675-t90-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 84 look OK, 2 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_releaseIP | `Failure` | 668.16 | test_network.py
test_01_migrate_VM_and_root_volume | `Error` | 76.34 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 51.23 | test_vm_life_cycle.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-796714627
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-805731850
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-810736169
Manually tested, fixes #4723
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-796714927
@shwstppr a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests [S]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-805575953
Packaging result: :heavy_multiplication_x: centos7 :heavy_multiplication_x: centos8 :heavy_multiplication_x: debian. SL-JID 230
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-797361250
> @ravening still some issue
@shwstppr but I dont think the issue is with ip management.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-796651280
okay @ravening
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-779903988
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2717
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-805566255
Packaging result: :heavy_multiplication_x: centos7 :heavy_multiplication_x: centos8 :heavy_multiplication_x: debian. SL-JID 222
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-796562950
@shwstppr can you help resolve conflict and then kick tests. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-805734490
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-780284016
<b>Trillian test result (tid-3556)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34467 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4675-t3556-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_public_ip_admin_account | `Failure` | 32.31 | test_network.py
test_public_ip_user_account | `Failure` | 2.29 | test_network.py
test_releaseIP | `Failure` | 663.16 | test_network.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-779878586
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-809279923
Ping @ravening @shwstppr @Spaceman1984 what's the update on this? Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-780017085
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
ravening commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-797375897
> @ravening still some issue
@shwstppr found the issue.. without the change, if ip address is in free state, its not displayed by `list publicipaddress` command but with this change, it will be displayed. so need to pass an extra param `state=Allocated`. Then its not displayed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-805752126
Packaging result: :heavy_check_mark: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 233
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-805569447
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-801685809
Packaging result: :heavy_multiplication_x: centos7 :heavy_check_mark: centos8 :heavy_check_mark: debian. SL-JID 168
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Spaceman1984 commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
Spaceman1984 commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-797369613
> @Spaceman1984 can you help manually test/verify this? cc @shwstppr Thanks.
I'll test @rhtyd
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-796594451
Packaging result: :heavy_check_mark: centos7 :heavy_multiplication_x: centos8 :heavy_check_mark: debian. SL-JID 87
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-796576441
@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-797327950
@Spaceman1984 can you help manually test/verify this? cc @shwstppr Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4675: Bug fix in displaying public IP address of shared networks
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4675:
URL: https://github.com/apache/cloudstack/pull/4675#issuecomment-805569932
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org