From 13eb78938820489efc0251cf85ed4e7fd01f685d Mon Sep 17 00:00:00 2001 From: Wilder Rodrigues Date: Mon, 7 Dec 2015 13:40:12 +0100 Subject: [PATCH] CLOUDSTACK-9106 - Makes the router commands call more consistent. - Checks the result of a call against the previous result. Either both are true or the method returns false. - Do not thrown exceptions because some calls are not handling/rethrowing them. It would cause runtime problems. - When doing a list.addAll(Arrays.asList(String[]{}) will cause problems when trying to cast the list.toArray() into an aray of String It would only work if instead of calling addAll() I would pass it straight into the constructor: e.g. List l = new ArrayList(Arrays.asList(new String[]{}); Stirng [] s = (String[]) l.toArray(); But I did not like that implementation because it would require 2 arrays of string and combine them at the end. --- .../com/cloud/network/element/OvsElement.java | 28 +++---- .../network/element/VirtualRouterElement.java | 81 ++++++++----------- .../element/VpcVirtualRouterElement.java | 58 ++++++------- 3 files changed, 75 insertions(+), 92 deletions(-) diff --git a/plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java b/plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java index ddf8833f249..906431e0ec1 100644 --- a/plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java +++ b/plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java @@ -70,7 +70,6 @@ import com.cloud.resource.ServerResource; import com.cloud.resource.UnableDeleteHostException; import com.cloud.utils.Pair; import com.cloud.utils.component.AdapterBase; -import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.vm.DomainRouterVO; import com.cloud.vm.NicProfile; import com.cloud.vm.ReservationContext; @@ -439,7 +438,7 @@ StaticNatServiceProvider, IpDeployer { break; } } - boolean result = false; + boolean result = true; if (canHandle) { final List routers = _routerDao.listByNetworkAndRole( network.getId(), Role.VIRTUAL_ROUTER); @@ -454,7 +453,7 @@ StaticNatServiceProvider, IpDeployer { final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO); for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.associatePublicIP(network, ipAddress, domainRouterVO); + result = result && networkTopology.associatePublicIP(network, ipAddress, domainRouterVO); } } return result; @@ -476,9 +475,9 @@ StaticNatServiceProvider, IpDeployer { final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId()); final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO); - boolean result = false; + boolean result = true; for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.applyStaticNats(network, rules, domainRouterVO); + result = result && networkTopology.applyStaticNats(network, rules, domainRouterVO); } return result; } @@ -486,9 +485,8 @@ StaticNatServiceProvider, IpDeployer { @Override public boolean applyPFRules(final Network network, final List rules) throws ResourceUnavailableException { - boolean result = false; if (!canHandle(network, Service.PortForwarding)) { - return result; + return false; } final List routers = _routerDao.listByNetworkAndRole( network.getId(), Role.VIRTUAL_ROUTER); @@ -498,10 +496,11 @@ StaticNatServiceProvider, IpDeployer { return true; } + boolean result = true; final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId()); final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO); for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.applyFirewallRules(network, rules, domainRouterVO); + result = result && networkTopology.applyFirewallRules(network, rules, domainRouterVO); } return result; } @@ -509,10 +508,10 @@ StaticNatServiceProvider, IpDeployer { @Override public boolean applyLBRules(final Network network, final List rules) throws ResourceUnavailableException { - boolean result = false; + boolean result = true; if (canHandle(network, Service.Lb)) { if (!canHandleLbRules(rules)) { - return result; + return false; } final List routers = _routerDao.listByNetworkAndRole( @@ -521,19 +520,16 @@ StaticNatServiceProvider, IpDeployer { s_logger.debug("Virtual router elemnt doesn't need to apply firewall rules on the backend; virtual " + "router doesn't exist in the network " + network.getId()); - result = true; - return result; + return true; } final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId()); final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO); for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.applyLoadBalancingRules(network, rules, domainRouterVO); + result = result && networkTopology.applyLoadBalancingRules(network, rules, domainRouterVO); if (!result) { - throw new CloudRuntimeException( - "Failed to apply load balancing rules in network " - + network.getId()); + s_logger.debug("Failed to apply load balancing rules in network " + network.getId()); } } } diff --git a/server/src/com/cloud/network/element/VirtualRouterElement.java b/server/src/com/cloud/network/element/VirtualRouterElement.java index 9f50256fe51..7d198f58d6a 100644 --- a/server/src/com/cloud/network/element/VirtualRouterElement.java +++ b/server/src/com/cloud/network/element/VirtualRouterElement.java @@ -95,7 +95,6 @@ import com.cloud.utils.component.AdapterBase; import com.cloud.utils.crypt.DBEncryptionUtil; import com.cloud.utils.db.QueryBuilder; import com.cloud.utils.db.SearchCriteria.Op; -import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.net.NetUtils; import com.cloud.vm.DomainRouterVO; import com.cloud.vm.NicProfile; @@ -283,10 +282,7 @@ NetworkMigrationResponder, AggregatedCommandExecutor { final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.applyFirewallRules(network, rules, domainRouterVO); - if (!result) { - throw new CloudRuntimeException("Failed to apply firewall rules in network " + network.getId()); - } + result = result && networkTopology.applyFirewallRules(network, rules, domainRouterVO); } } return result; @@ -406,7 +402,7 @@ NetworkMigrationResponder, AggregatedCommandExecutor { @Override public boolean applyLBRules(final Network network, final List rules) throws ResourceUnavailableException { - boolean result = false; + boolean result = true; if (canHandle(network, Service.Lb)) { if (!canHandleLbRules(rules)) { return false; @@ -422,10 +418,7 @@ NetworkMigrationResponder, AggregatedCommandExecutor { final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.applyLoadBalancingRules(network, rules, domainRouterVO); - if (!result) { - throw new CloudRuntimeException("Failed to apply load balancing rules in network " + network.getId()); - } + result = result && networkTopology.applyLoadBalancingRules(network, rules, domainRouterVO); } } return result; @@ -497,7 +490,6 @@ NetworkMigrationResponder, AggregatedCommandExecutor { @Override public boolean applyIps(final Network network, final List ipAddress, final Set services) throws ResourceUnavailableException { - boolean result = false; boolean canHandle = true; for (final Service service : services) { if (!canHandle(network, service)) { @@ -505,6 +497,7 @@ NetworkMigrationResponder, AggregatedCommandExecutor { break; } } + boolean result = true; if (canHandle) { final List routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER); if (routers == null || routers.isEmpty()) { @@ -516,7 +509,7 @@ NetworkMigrationResponder, AggregatedCommandExecutor { final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.associatePublicIP(network, ipAddress, domainRouterVO); + result = result && networkTopology.associatePublicIP(network, ipAddress, domainRouterVO); } } return result; @@ -668,14 +661,14 @@ NetworkMigrationResponder, AggregatedCommandExecutor { final List routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER); if (routers == null || routers.isEmpty()) { s_logger.debug("Virtual router elemnt doesn't need to apply static nat on the backend; virtual " + "router doesn't exist in the network " + network.getId()); - return result; + return true; } final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId()); final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.applyStaticNats(network, rules, domainRouterVO); + result = result && networkTopology.applyStaticNats(network, rules, domainRouterVO); } } return result; @@ -687,20 +680,21 @@ NetworkMigrationResponder, AggregatedCommandExecutor { if (routers == null || routers.isEmpty()) { return true; } - boolean result = true; + boolean stopResult = true; + boolean destroyResult = true; for (final DomainRouterVO router : routers) { - result = result && _routerMgr.stop(router, false, context.getCaller(), context.getAccount()) != null; + stopResult = stopResult && _routerMgr.stop(router, false, context.getCaller(), context.getAccount()) != null; + if (!stopResult) { + s_logger.warn("Failed to stop virtual router element " + router + ", but would try to process clean up anyway."); + } if (cleanup) { - if (!result) { - s_logger.warn("Failed to stop virtual router element " + router + ", but would try to process clean up anyway."); - } - result = _routerMgr.destroyRouter(router.getId(), context.getAccount(), context.getCaller().getId()) != null; - if (!result) { + destroyResult = destroyResult && _routerMgr.destroyRouter(router.getId(), context.getAccount(), context.getCaller().getId()) != null; + if (!destroyResult) { s_logger.warn("Failed to clean up virtual router element " + router); } } } - return result; + return stopResult & destroyResult; } @Override @@ -760,15 +754,13 @@ NetworkMigrationResponder, AggregatedCommandExecutor { @Override public boolean saveSSHKey(final Network network, final NicProfile nic, final VirtualMachineProfile vm, final String sshPublicKey) throws ResourceUnavailableException { - boolean result = false; if (!canHandle(network, null)) { - return result; + return false; } final List routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER); if (routers == null || routers.isEmpty()) { s_logger.debug("Can't find virtual router element in network " + network.getId()); - result = true; - return result; + return true; } final VirtualMachineProfile uservm = vm; @@ -776,23 +768,22 @@ NetworkMigrationResponder, AggregatedCommandExecutor { final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId()); final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); + boolean result = true; for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.saveSSHPublicKeyToRouter(network, nic, uservm, domainRouterVO, sshPublicKey); + result = result && networkTopology.saveSSHPublicKeyToRouter(network, nic, uservm, domainRouterVO, sshPublicKey); } return result; } @Override public boolean saveUserData(final Network network, final NicProfile nic, final VirtualMachineProfile vm) throws ResourceUnavailableException { - boolean result = false; if (!canHandle(network, null)) { - return result; + return false; } final List routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER); if (routers == null || routers.isEmpty()) { s_logger.debug("Can't find virtual router element in network " + network.getId()); - result = true; - return result; + return true; } final VirtualMachineProfile uservm = vm; @@ -800,8 +791,9 @@ NetworkMigrationResponder, AggregatedCommandExecutor { final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId()); final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); + boolean result = true; for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.saveUserDataToRouter(network, nic, uservm, domainRouterVO); + result = result && networkTopology.saveUserDataToRouter(network, nic, uservm, domainRouterVO); } return result; } @@ -860,23 +852,19 @@ NetworkMigrationResponder, AggregatedCommandExecutor { @Override public boolean applyPFRules(final Network network, final List rules) throws ResourceUnavailableException { - boolean result = false; + boolean result = true; if (canHandle(network, Service.PortForwarding)) { final List routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER); if (routers == null || routers.isEmpty()) { s_logger.debug("Virtual router elemnt doesn't need to apply firewall rules on the backend; virtual " + "router doesn't exist in the network " + network.getId()); - result = true; - return result; + return true; } final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId()); final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.applyFirewallRules(network, rules, domainRouterVO); - if (!result) { - throw new CloudRuntimeException("Failed to apply firewall rules in network " + network.getId()); - } + result = result && networkTopology.applyFirewallRules(network, rules, domainRouterVO); } } return result; @@ -978,10 +966,10 @@ NetworkMigrationResponder, AggregatedCommandExecutor { @Override public boolean addDhcpEntry(final Network network, final NicProfile nic, final VirtualMachineProfile vm, final DeployDestination dest, final ReservationContext context) throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { - boolean result = false; + boolean result = true; if (canHandle(network, Service.Dhcp)) { if (vm.getType() != VirtualMachine.Type.User) { - return result; + return false; } final VirtualMachineProfile uservm = vm; @@ -995,7 +983,7 @@ NetworkMigrationResponder, AggregatedCommandExecutor { final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.applyDhcpEntry(network, nic, uservm, dest, domainRouterVO); + result = result && networkTopology.applyDhcpEntry(network, nic, uservm, dest, domainRouterVO); } } return result; @@ -1004,16 +992,15 @@ NetworkMigrationResponder, AggregatedCommandExecutor { @Override public boolean addPasswordAndUserdata(final Network network, final NicProfile nic, final VirtualMachineProfile vm, final DeployDestination dest, final ReservationContext context) throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { - boolean result = false; + boolean result = true; if (canHandle(network, Service.UserData)) { if (vm.getType() != VirtualMachine.Type.User) { - return result; + return false; } if (network.getIp6Gateway() != null) { s_logger.info("Skip password and userdata service setup for IPv6 VM"); - result = true; - return result; + return true; } final VirtualMachineProfile uservm = vm; @@ -1028,7 +1015,7 @@ NetworkMigrationResponder, AggregatedCommandExecutor { final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.applyUserData(network, nic, uservm, dest, domainRouterVO); + result = result && networkTopology.applyUserData(network, nic, uservm, dest, domainRouterVO); } } return result; diff --git a/server/src/com/cloud/network/element/VpcVirtualRouterElement.java b/server/src/com/cloud/network/element/VpcVirtualRouterElement.java index 6f7a06fc69c..6ed5c1b8eab 100644 --- a/server/src/com/cloud/network/element/VpcVirtualRouterElement.java +++ b/server/src/com/cloud/network/element/VpcVirtualRouterElement.java @@ -16,6 +16,8 @@ // under the License. package com.cloud.network.element; +import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -116,8 +118,7 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc @Qualifier("vpcNetworkHelper") private VpcNetworkHelperImpl _vpcNetWprkHelper; - @Inject - private RouterDeploymentDefinitionBuilder routerDeploymentDefinitionBuilder; + @Inject RouterDeploymentDefinitionBuilder routerDeploymentDefinitionBuilder; @Override protected boolean canHandle(final Network network, final Service service) { @@ -278,13 +279,13 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc @Override public boolean shutdown(final Network network, final ReservationContext context, final boolean cleanup) throws ConcurrentOperationException, ResourceUnavailableException { - boolean success = true; final Long vpcId = network.getVpcId(); if (vpcId == null) { s_logger.debug("Network " + network + " doesn't belong to any vpc, so skipping unplug nic part"); - return success; + return true; } + boolean success = true; final List routers = _routerDao.listByVpcId(vpcId); for (final VirtualRouter router : routers) { // 1) Check if router is already a part of the network @@ -306,13 +307,13 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc @Override public boolean destroy(final Network config, final ReservationContext context) throws ConcurrentOperationException, ResourceUnavailableException { - boolean success = true; final Long vpcId = config.getVpcId(); if (vpcId == null) { s_logger.debug("Network " + config + " doesn't belong to any vpc, so skipping unplug nic part"); - return success; + return true; } + boolean success = true; final List routers = _routerDao.listByVpcId(vpcId); for (final VirtualRouter router : routers) { // 1) Check if router is already a part of the network @@ -412,18 +413,15 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc @Override public boolean createPrivateGateway(final PrivateGateway gateway) throws ConcurrentOperationException, ResourceUnavailableException { - boolean result = false; - if (gateway.getType() != VpcGateway.Type.Private) { s_logger.warn("Type of vpc gateway is not " + VpcGateway.Type.Private); - return result; + return true; } final List routers = _vpcRouterMgr.getVpcRouters(gateway.getVpcId()); if (routers == null || routers.isEmpty()) { s_logger.debug(getName() + " element doesn't need to create Private gateway on the backend; VPC virtual " + "router doesn't exist in the vpc id=" + gateway.getVpcId()); - result = true; - return result; + return true; } s_logger.info("Adding VPC routers to Guest Network: " + routers.size() + " to be added!"); @@ -431,6 +429,7 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc final DataCenterVO dcVO = _dcDao.findById(gateway.getZoneId()); final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); + boolean result = true; final Network network = _networkDao.findById(gateway.getNetworkId()); final boolean isPrivateGateway = true; @@ -438,13 +437,10 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc if (networkTopology.setupPrivateGateway(gateway, domainRouterVO)) { try { final List rules = _networkACLItemDao.listByACL(gateway.getNetworkACLId()); - result = networkTopology.applyNetworkACLs(network, rules, domainRouterVO, isPrivateGateway); - if (!result) { - throw new CloudRuntimeException("Failed to apply network acl in network " + network.getId()); - } + result = result && networkTopology.applyNetworkACLs(network, rules, domainRouterVO, isPrivateGateway); } catch (final Exception ex) { s_logger.debug("Failed to apply network acl id " + gateway.getNetworkACLId() + " on gateway "); - return result; + return false; } } } @@ -486,20 +482,20 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc break; } } - boolean result = false; + boolean result = true; if (canHandle) { final List routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER); if (routers == null || routers.isEmpty()) { s_logger.debug(getName() + " element doesn't need to associate ip addresses on the backend; VPC virtual " + "router doesn't exist in the network " + network.getId()); - return result; + return false; } final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId()); final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.associatePublicIP(network, ipAddress, domainRouterVO); + result = result && networkTopology.associatePublicIP(network, ipAddress, domainRouterVO); } } return result; @@ -512,7 +508,7 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc final List routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER); if (routers == null || routers.isEmpty()) { s_logger.debug("Virtual router elemnt doesn't need to apply firewall rules on the backend; virtual " + "router doesn't exist in the network " + network.getId()); - return result; + return true; } final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId()); @@ -520,7 +516,7 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc for (final DomainRouterVO domainRouterVO : routers) { try { - result = networkTopology.applyNetworkACLs(network, rules, domainRouterVO, false); + result = result && networkTopology.applyNetworkACLs(network, rules, domainRouterVO, false); } catch (final Exception ex) { s_logger.debug("Failed to apply network acl in network " + network.getId()); } @@ -569,10 +565,7 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc boolean result = true; for (final DomainRouterVO domainRouterVO : routers) { - result = networkTopology.applyNetworkACLs(network, rules, domainRouterVO, isPrivateGateway); - if (!result) { - throw new CloudRuntimeException("Failed to apply network acl in network " + network.getId()); - } + result = result && networkTopology.applyNetworkACLs(network, rules, domainRouterVO, isPrivateGateway); } return result; } @@ -642,24 +635,31 @@ public class VpcVirtualRouterElement extends VirtualRouterElement implements Vpc @Override public String[] applyVpnUsers(final RemoteAccessVpn vpn, final List users) throws ResourceUnavailableException { - if (vpn.getVpcId() == null) { + final Long vpcId = vpn.getVpcId(); + if (vpcId == null) { return null; } - final List routers = _vpcRouterMgr.getVpcRouters(vpn.getVpcId()); + final List routers = _vpcRouterMgr.getVpcRouters(vpcId); if (routers == null) { - s_logger.debug("Cannot apply vpn users on the backend; virtual router doesn't exist in the network " + vpn.getVpcId()); + s_logger.debug("Cannot apply vpn users on the backend; virtual router doesn't exist in the network " + vpcId); return null; } - final Vpc vpc = _entityMgr.findById(Vpc.class, vpn.getVpcId()); + final Vpc vpc = _entityMgr.findById(Vpc.class, vpcId); final DataCenterVO dcVO = _dcDao.findById(vpc.getZoneId()); final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO); String[] result = null; + final List combinedResults = new ArrayList(); for (final DomainRouterVO domainRouterVO : routers) { result = networkTopology.applyVpnUsers(vpn, users, domainRouterVO); + combinedResults.addAll(Arrays.asList(result)); } + result = new String[combinedResults.size()]; + final Object [] resultCast = combinedResults.toArray(); + System.arraycopy(resultCast, 0, result, 0, resultCast.length); + return result; }