[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4675: Bug fix in displaying public IP address of shared networks

[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