This is an automated email from the ASF dual-hosted git repository. pearl11594 pushed a commit to branch nsx-code-smell-2 in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit 4f6f39ec8c98f610e4eb4aab9f529a1987469ecf Author: Pearl Dsilva <[email protected]> AuthorDate: Wed Jan 3 09:11:14 2024 -0500 NSX: Fix code smells --- .../com/cloud/vm/VirtualMachineManagerImpl.java | 46 +++++++++------- .../engine/orchestration/NetworkOrchestrator.java | 62 ++++++++++++++-------- .../java/com/cloud/network/vpc/VpcManagerImpl.java | 41 +++++++------- .../com/cloud/server/ManagementServerImpl.java | 7 +-- 4 files changed, 94 insertions(+), 62 deletions(-) diff --git a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java index 1b19ffee2b5..d0b6dbb19d9 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -611,11 +611,18 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac 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 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac 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 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac // 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(); diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index b9873470a53..05fc2e27ae4 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java @@ -258,6 +258,7 @@ import com.cloud.vm.dao.NicSecondaryIpVO; 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 class NetworkOrchestrator extends ManagerBase implements NetworkOrchestra .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 class NetworkOrchestrator extends ManagerBase implements NetworkOrchestra 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 class NetworkOrchestrator extends ManagerBase implements NetworkOrchestra } 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 class NetworkOrchestrator extends ManagerBase implements NetworkOrchestra } } + @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 @@ public class NetworkOrchestrator extends ManagerBase implements NetworkOrchestra } //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; diff --git a/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java b/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java index 4b507c5273d..17277b2f7dd 100644 --- a/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java @@ -397,7 +397,7 @@ public class VpcManagerImpl extends ManagerBase implements VpcManager, VpcProvis // 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 class VpcManagerImpl extends ManagerBase implements VpcManager, VpcProvis 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 class VpcManagerImpl extends ManagerBase implements VpcManager, VpcProvis 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 @@ public class VpcManagerImpl extends ManagerBase implements VpcManager, VpcProvis 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 @@ public class VpcManagerImpl extends ManagerBase implements VpcManager, VpcProvis } } - 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(); } diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 95bad927c63..966b593542f 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/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 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe 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 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe } 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); } }
