You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by pe...@apache.org on 2024/01/03 14:11:25 UTC
(cloudstack) 01/01: NSX: Fix code smells
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 <pe...@gmail.com>
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);
}
}