NSX: Fix code smells

engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java

@@ -611,11 +611,18 @@ private boolean isValidSystemVMType(VirtualMachine vm) {
                 VirtualMachine.Type.ConsoleProxy.equals(vm.getType());
     }
 
-    protected void advanceExpunge(VMInstanceVO vm) throws ResourceUnavailableException, OperationTimedoutException, ConcurrentOperationException {
+    private boolean isVmDestroyed(VMInstanceVO vm) {
         if (vm == null || vm.getRemoved() != null) {
             if (s_logger.isDebugEnabled()) {
                 s_logger.debug("Unable to find vm or vm is destroyed: " + vm);
             }
+            return true;
+        }
+        return false;
+    }
+
+    protected void advanceExpunge(VMInstanceVO vm) throws ResourceUnavailableException, OperationTimedoutException, ConcurrentOperationException {
+        if (isVmDestroyed(vm)) {
             return;
         }
 
@@ -642,7 +649,7 @@ protected void advanceExpunge(VMInstanceVO vm) throws ResourceUnavailableExcepti
         final HypervisorGuru hvGuru = _hvGuruMgr.getGuru(vm.getHypervisorType());
 
         List<NicProfile> vmNics = profile.getNics();
-        s_logger.debug(String.format("Cleaning up NICS [%s] of %s.", vmNics.stream().map(nic -> nic.toString()).collect(Collectors.joining(", ")),vm.toString()));
+        s_logger.debug(String.format("Cleaning up NICS [%s] of %s.", vmNics.stream().map(NicProfile::toString).collect(Collectors.joining(", ")),vm.toString()));
         final List<Command> nicExpungeCommands = hvGuru.finalizeExpungeNics(vm, profile.getNics());
         _networkMgr.cleanupNics(profile);
 
@@ -686,28 +693,31 @@ protected void advanceExpunge(VMInstanceVO vm) throws ResourceUnavailableExcepti
 
         // send hypervisor-dependent commands before removing
         final List<Command> finalizeExpungeCommands = hvGuru.finalizeExpunge(vm);
-        if (CollectionUtils.isNotEmpty(finalizeExpungeCommands) || CollectionUtils.isNotEmpty(nicExpungeCommands)) {
-            if (hostId != null) {
-                final Commands cmds = new Commands(Command.OnError.Stop);
-                addAllExpungeCommandsFromList(finalizeExpungeCommands, cmds, vm);
-                addAllExpungeCommandsFromList(nicExpungeCommands, cmds, vm);
-                _agentMgr.send(hostId, cmds);
-                if (!cmds.isSuccessful()) {
-                    for (final Answer answer : cmds.getAnswers()) {
-                        if (!answer.getResult()) {
-                            s_logger.warn("Failed to expunge vm due to: " + answer.getDetails());
-                            throw new CloudRuntimeException("Unable to expunge " + vm + " due to " + answer.getDetails());
-                        }
-                    }
-                }
-            }
-        }
+        handleUnsuccessfulExpungeOperation(finalizeExpungeCommands, nicExpungeCommands, vm, hostId);
 
         if (s_logger.isDebugEnabled()) {
             s_logger.debug("Expunged " + vm);
         }
     }
 
+    private void handleUnsuccessfulExpungeOperation(List<Command> finalizeExpungeCommands, List<Command> nicExpungeCommands,
+                                                    VMInstanceVO vm, Long hostId) throws OperationTimedoutException, AgentUnavailableException {
+        if (CollectionUtils.isNotEmpty(finalizeExpungeCommands) || CollectionUtils.isNotEmpty(nicExpungeCommands) && (hostId != null)) {
+            final Commands cmds = new Commands(Command.OnError.Stop);
+            addAllExpungeCommandsFromList(finalizeExpungeCommands, cmds, vm);
+            addAllExpungeCommandsFromList(nicExpungeCommands, cmds, vm);
+            _agentMgr.send(hostId, cmds);
+            if (!cmds.isSuccessful()) {
+                for (final Answer answer : cmds.getAnswers()) {
+                    if (!answer.getResult()) {
+                        s_logger.warn("Failed to expunge vm due to: " + answer.getDetails());
+                        throw new CloudRuntimeException(String.format("Unable to expunge %s due to %s", vm, answer.getDetails()));
+                    }
+                }
+            }
+        }
+    }
+
     protected void handleUnsuccessfulCommands(Commands cmds, VMInstanceVO vm) throws CloudRuntimeException {
         String cmdsStr = cmds.toString();
         String vmToString = vm.toString();

engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java

@@ -258,6 +258,7 @@
 import com.cloud.vm.dao.UserVmDao;
 import com.cloud.vm.dao.VMInstanceDao;
 import com.googlecode.ipv6.IPv6Address;
+import org.jetbrains.annotations.NotNull;
 
 /**
  * NetworkManagerImpl implements NetworkManager.
@@ -749,20 +750,8 @@ public List<? extends Network> setupNetwork(final Account owner, final NetworkOf
                     .getBroadcastDomainType() == BroadcastDomainType.Vlan || predefined.getBroadcastDomainType() == BroadcastDomainType.Lswitch || predefined
                     .getBroadcastDomainType() == BroadcastDomainType.Vxlan)) {
                 final List<NetworkVO> configs = _networksDao.listBy(owner.getId(), offering.getId(), plan.getDataCenterId());
-                if (configs.size() > 0) {
-                    if (s_logger.isDebugEnabled()) {
-                        s_logger.debug("Found existing network configuration for offering " + offering + ": " + configs.get(0));
-                    }
-
-                    if (errorIfAlreadySetup) {
-                        final InvalidParameterValueException ex = new InvalidParameterValueException(
-                                "Found existing network configuration (with specified id) for offering (with specified id)");
-                        ex.addProxyObject(offering.getUuid(), "offeringId");
-                        ex.addProxyObject(configs.get(0).getUuid(), "networkConfigId");
-                        throw ex;
-                    } else {
-                        return configs;
-                    }
+                if (!configs.isEmpty()) {
+                    return existingConfiguration(offering, configs, errorIfAlreadySetup);
                 }
             }
 
@@ -794,11 +783,8 @@ public List<? extends Network> setupNetwork(final Account owner, final NetworkOf
                 Transaction.execute(new TransactionCallbackNoReturn() {
                     @Override
                     public void doInTransactionWithoutResult(final TransactionStatus status) {
-                        final NetworkVO vo = new NetworkVO(id, network, offering.getId(), guru.getName(), owner.getDomainId(), owner.getId(), relatedFile, name, displayText, predefined
-                                .getNetworkDomain(), offering.getGuestType(), plan.getDataCenterId(), plan.getPhysicalNetworkId(), aclType, offering.isSpecifyIpRanges(),
-                                vpcId, offering.isRedundantRouter(), predefined.getExternalId());
-                        vo.setDisplayNetwork(isDisplayNetworkEnabled == null ? true : isDisplayNetworkEnabled);
-                        vo.setStrechedL2Network(offering.isSupportingStrechedL2());
+                        final NetworkVO vo = getNetworkVO(id, offering, plan, predefined,
+                                network, guru, owner, name, displayText,relatedFile, aclType,vpcId, isDisplayNetworkEnabled);
                         final NetworkVO networkPersisted = _networksDao.persist(vo, vo.getGuestType() == Network.GuestType.Isolated,
                                 finalizeServicesAndProvidersForNetwork(offering, plan.getPhysicalNetworkId()));
                         networks.add(networkPersisted);
@@ -815,14 +801,14 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
                         }
 
                         if (domainId != null && aclType == ACLType.Domain) {
-                            _networksDao.addDomainToNetwork(id, domainId, subdomainAccess == null ? true : subdomainAccess);
+                            _networksDao.addDomainToNetwork(id, domainId, subdomainAccess == null || subdomainAccess);
                         }
                     }
                 });
                 guru.setup(network, relatedFile);
             }
 
-            if (networks.size() < 1) {
+            if (networks.isEmpty()) {
                 // see networkOfferingVO.java
                 final CloudRuntimeException ex = new CloudRuntimeException("Unable to convert network offering with specified id to network profile");
                 ex.addProxyObject(offering.getUuid(), "offeringId");
@@ -836,6 +822,37 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
         }
     }
 
+    @NotNull
+    private static NetworkVO getNetworkVO(long id, final NetworkOffering offering, final DeploymentPlan plan, final Network predefined,
+                                          Network network, final NetworkGuru guru, final Account owner,
+                                          final String name, final String displayText, long relatedFile, final ACLType aclType,
+                                          final Long vpcId, final Boolean isDisplayNetworkEnabled) {
+        final NetworkVO vo = new NetworkVO(id, network, offering.getId(), guru.getName(), owner.getDomainId(), owner.getId(),
+                relatedFile, name, displayText, predefined.getNetworkDomain(), offering.getGuestType(),
+                plan.getDataCenterId(), plan.getPhysicalNetworkId(), aclType, offering.isSpecifyIpRanges(),
+                vpcId, offering.isRedundantRouter(), predefined.getExternalId());
+        vo.setDisplayNetwork(isDisplayNetworkEnabled == null || isDisplayNetworkEnabled);
+        vo.setStrechedL2Network(offering.isSupportingStrechedL2());
+        return vo;
+    }
+
+    private List<? extends Network> existingConfiguration(final NetworkOffering offering, List<NetworkVO> configs,
+                                                          final boolean errorIfAlreadySetup) {
+        if (s_logger.isDebugEnabled()) {
+            s_logger.debug("Found existing network configuration for offering " + offering + ": " + configs.get(0));
+        }
+
+        if (errorIfAlreadySetup) {
+            final InvalidParameterValueException ex = new InvalidParameterValueException(
+                    "Found existing network configuration (with specified id) for offering (with specified id)");
+            ex.addProxyObject(offering.getUuid(), "offeringId");
+            ex.addProxyObject(configs.get(0).getUuid(), "networkConfigId");
+            throw ex;
+        } else {
+            return configs;
+        }
+    }
+
     @Override
     @DB
     public void allocate(final VirtualMachineProfile vm, final LinkedHashMap<? extends Network, List<? extends NicProfile>> networks, final Map<String, Map<Integer, String>> extraDhcpOptions) throws InsufficientCapacityException,
@@ -3929,9 +3946,8 @@ private boolean cleanupNetworkResources(final long networkId, final Account call
         }
 
         //revoke all network ACLs for network
-        // TODO: Change this when ACLs are supported for NSX
         try {
-            if (networkOffering.isForNsx() || _networkACLMgr.revokeACLItemsForNetwork(networkId)) {
+            if (_networkACLMgr.revokeACLItemsForNetwork(networkId)) {
                 s_logger.debug("Successfully cleaned up NetworkACLs for network id=" + networkId);
             } else {
                 success = false;

plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java

@@ -668,10 +668,10 @@ public String getNsxInfraServices(String ruleName, String port, String protocol,
 
             List<com.vmware.nsx_policy.model.Service> services = serviceList.getResults();
             List<String> matchedDefaultSvc = services.parallelStream().filter(svc ->
-                            (svc.getServiceEntries().get(0) instanceof L4PortSetServiceEntry) &&
-                                    ((L4PortSetServiceEntry) svc.getServiceEntries().get(0)).getDestinationPorts().get(0).equals(port)
-                    && (((L4PortSetServiceEntry) svc.getServiceEntries().get(0)).getL4Protocol().equals(protocol)))
-                    .map(svc -> ((L4PortSetServiceEntry) svc.getServiceEntries().get(0)).getDestinationPorts().get(0))
+                            (svc.getServiceEntries().get(0)._getDataValue().getField("resource_type").toString().equals("L4PortSetServiceEntry")) &&
+                                    svc.getServiceEntries().get(0)._getDataValue().getField("destination_ports").toString().equals("["+port+"]")
+                                    && svc.getServiceEntries().get(0)._getDataValue().getField("l4_protocol").toString().equals(protocol))
+                    .map(svc -> svc.getServiceEntries().get(0)._getDataValue().getField("parent_path").toString())
                     .collect(Collectors.toList());
             if (!CollectionUtils.isEmpty(matchedDefaultSvc)) {
                 return matchedDefaultSvc.get(0);

server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java

@@ -397,7 +397,7 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
                 // configure default vpc offering with NSX as network service provider in Route mode
                 if (_vpcOffDao.findByUniqueName(VpcOffering.DEFAULT_VPC_ROUTE_NSX_OFFERING_NAME) == null) {
                     s_logger.debug("Creating default VPC offering with NSX as network service provider" + VpcOffering.DEFAULT_VPC_ROUTE_NSX_OFFERING_NAME);
-                    final Map<Service, Set<Provider>> svcProviderMap = new HashMap<Service, Set<Provider>>();
+                    final Map<Service, Set<Provider>> svcProviderMap = new HashMap<>();
                     final Set<Provider> defaultProviders = Set.of(Provider.Nsx);
                     for (final Service svc : getSupportedServices()) {
                         if (List.of(Service.UserData, Service.Dhcp, Service.Dns).contains(svc)) {
@@ -471,22 +471,8 @@ public VpcOffering createVpcOffering(CreateVPCOfferingCmd cmd) {
         final Boolean forNsx = cmd.isForNsx();
         String nsxMode = cmd.getNsxMode();
         final boolean enable = cmd.getEnable();
+        nsxMode = validateNsxMode(forNsx, nsxMode);
 
-        if (Boolean.TRUE.equals(forNsx)) {
-            if (Objects.isNull(nsxMode)) {
-                throw new InvalidParameterValueException("Mode for an NSX offering needs to be specified.Valid values: " + Arrays.toString(NetworkOffering.NsxMode.values()));
-            }
-            if (!EnumUtils.isValidEnum(NetworkOffering.NsxMode.class, nsxMode)) {
-                throw new InvalidParameterValueException("Invalid mode passed. Valid values: " + Arrays.toString(NetworkOffering.NsxMode.values()));
-            }
-        } else {
-            if (Objects.nonNull(nsxMode)) {
-                if (s_logger.isTraceEnabled()) {
-                    s_logger.trace("nsxMode has is ignored for non-NSX enabled zones");
-                }
-                nsxMode = null;
-            }
-        }
         // check if valid domain
         if (CollectionUtils.isNotEmpty(cmd.getDomainIds())) {
             for (final Long domainId: cmd.getDomainIds()) {
@@ -513,6 +499,25 @@ public VpcOffering createVpcOffering(CreateVPCOfferingCmd cmd) {
                 domainIds, zoneIds, (enable ? State.Enabled : State.Disabled));
     }
 
+    private String validateNsxMode(Boolean forNsx, String nsxMode) {
+        if (Boolean.TRUE.equals(forNsx)) {
+            if (Objects.isNull(nsxMode)) {
+                throw new InvalidParameterValueException("Mode for an NSX offering needs to be specified.Valid values: " + Arrays.toString(NetworkOffering.NsxMode.values()));
+            }
+            if (!EnumUtils.isValidEnum(NetworkOffering.NsxMode.class, nsxMode)) {
+                throw new InvalidParameterValueException("Invalid mode passed. Valid values: " + Arrays.toString(NetworkOffering.NsxMode.values()));
+            }
+        } else {
+            if (Objects.nonNull(nsxMode)) {
+                if (s_logger.isTraceEnabled()) {
+                    s_logger.trace("nsxMode has is ignored for non-NSX enabled zones");
+                }
+                nsxMode = null;
+            }
+        }
+        return nsxMode;
+    }
+
     @Override
     @ActionEvent(eventType = EventTypes.EVENT_VPC_OFFERING_CREATE, eventDescription = "creating vpc offering", create = true)
     public VpcOffering createVpcOffering(final String name, final String displayText, final List<String> supportedServices, final Map<String, List<String>> serviceProviders,
@@ -1198,7 +1203,7 @@ private void allocateSourceNatIp(Vpc vpc, String sourceNatIP) {
         try {
             if (isVpcForNsx(vpc) && org.apache.commons.lang3.StringUtils.isBlank(sourceNatIP)) {
                 s_logger.debug(String.format("Reserving a source NAT IP for NSX VPC %s", vpc.getName()));
-                sourceNatIP = reserveSourceNatIpForNsxVpc(account, zone, vpc);
+                sourceNatIP = reserveSourceNatIpForNsxVpc(account, zone);
             }
             IpAddress ip = _ipAddrMgr.allocateIp(account, false, CallContext.current().getCallingAccount(), CallContext.current().getCallingUserId(), zone, null, sourceNatIP);
             this.associateIPToVpc(ip.getId(), vpc.getId());
@@ -1207,7 +1212,7 @@ private void allocateSourceNatIp(Vpc vpc, String sourceNatIP) {
         }
     }
 
-    private String reserveSourceNatIpForNsxVpc(Account account, DataCenter zone, Vpc vpc) throws ResourceAllocationException {
+    private String reserveSourceNatIpForNsxVpc(Account account, DataCenter zone) throws ResourceAllocationException {
         IpAddress ipAddress = _ntwkSvc.reserveIpAddressWithVlanDetail(account, zone, true, ApiConstants.NSX_DETAIL_KEY);
         return ipAddress.getAddress().addr();
     }

server/src/main/java/com/cloud/server/ManagementServerImpl.java

@@ -837,6 +837,7 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe
     public static final Logger s_logger = Logger.getLogger(ManagementServerImpl.class.getName());
     protected StateMachine2<State, VirtualMachine.Event, VirtualMachine> _stateMachine;
 
+    static final String FOR_SYSTEMVMS = "forsystemvms";
     static final ConfigKey<Integer> vmPasswordLength = new ConfigKey<Integer>("Advanced", Integer.class, "vm.password.length", "6", "Specifies the length of a randomly generated password", false);
     static final ConfigKey<Integer> sshKeyLength = new ConfigKey<Integer>("Advanced", Integer.class, "ssh.key.length", "2048", "Specifies custom SSH key length (bit)", true, ConfigKey.Scope.Global);
     static final ConfigKey<Boolean> humanReadableSizes = new ConfigKey<Boolean>("Advanced", Boolean.class, "display.human.readable.sizes", "true", "Enables outputting human readable byte sizes to logs and usage records.", false, ConfigKey.Scope.Global);
@@ -2562,7 +2563,7 @@ private void buildParameters(final SearchBuilder<IPAddressVO> sb, final ListPubl
         sb.and("vpcId", sb.entity().getVpcId(), SearchCriteria.Op.EQ);
         sb.and("state", sb.entity().getState(), SearchCriteria.Op.EQ);
         sb.and("display", sb.entity().isDisplay(), SearchCriteria.Op.EQ);
-        sb.and("forsystemvms", sb.entity().isForSystemVms(), SearchCriteria.Op.EQ);
+        sb.and(FOR_SYSTEMVMS, sb.entity().isForSystemVms(), SearchCriteria.Op.EQ);
 
         if (forLoadBalancing != null && forLoadBalancing) {
             final SearchBuilder<LoadBalancerVO> lbSearch = _loadbalancerDao.createSearchBuilder();
@@ -2670,9 +2671,9 @@ protected void setParameters(SearchCriteria<IPAddressVO> sc, final ListPublicIpA
         }
 
         if (IpAddressManagerImpl.getSystemvmpublicipreservationmodestrictness().value() && IpAddress.State.Free.name().equalsIgnoreCase(state)) {
-            sc.setParameters("forsystemvms", false);
+            sc.setParameters(FOR_SYSTEMVMS, false);
         } else {
-            sc.setParameters("forsystemvms", forSystemVms);
+            sc.setParameters(FOR_SYSTEMVMS, forSystemVms);
         }
     }