Skip to content

Commit 13eb789

Browse files
wilderrodrigueswilderrodrigues
committed
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<String> 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.
1 parent 1738ce1 commit 13eb789

File tree

3 files changed

+75
-92
lines changed

3 files changed

+75
-92
lines changed

‎plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java‎

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,6 @@
7070
import com.cloud.resource.UnableDeleteHostException;
7171
import com.cloud.utils.Pair;
7272
import com.cloud.utils.component.AdapterBase;
73-
import com.cloud.utils.exception.CloudRuntimeException;
7473
import com.cloud.vm.DomainRouterVO;
7574
import com.cloud.vm.NicProfile;
7675
import com.cloud.vm.ReservationContext;
@@ -439,7 +438,7 @@ public boolean applyIps(final Network network,
439438
break;
440439
}
441440
}
442-
boolean result = false;
441+
boolean result = true;
443442
if (canHandle) {
444443
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(
445444
network.getId(), Role.VIRTUAL_ROUTER);
@@ -454,7 +453,7 @@ public boolean applyIps(final Network network,
454453
final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
455454

456455
for (final DomainRouterVO domainRouterVO : routers) {
457-
result = networkTopology.associatePublicIP(network, ipAddress, domainRouterVO);
456+
result = result && networkTopology.associatePublicIP(network, ipAddress, domainRouterVO);
458457
}
459458
}
460459
return result;
@@ -476,19 +475,18 @@ public boolean applyStaticNats(final Network network, final List<? extends Stati
476475

477476
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
478477
final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
479-
boolean result = false;
478+
boolean result = true;
480479
for (final DomainRouterVO domainRouterVO : routers) {
481-
result = networkTopology.applyStaticNats(network, rules, domainRouterVO);
480+
result = result && networkTopology.applyStaticNats(network, rules, domainRouterVO);
482481
}
483482
return result;
484483
}
485484

486485
@Override
487486
public boolean applyPFRules(final Network network, final List<PortForwardingRule> rules)
488487
throws ResourceUnavailableException {
489-
boolean result = false;
490488
if (!canHandle(network, Service.PortForwarding)) {
491-
return result;
489+
return false;
492490
}
493491
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(
494492
network.getId(), Role.VIRTUAL_ROUTER);
@@ -498,21 +496,22 @@ public boolean applyPFRules(final Network network, final List<PortForwardingRule
498496
return true;
499497
}
500498

499+
boolean result = true;
501500
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
502501
final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
503502
for (final DomainRouterVO domainRouterVO : routers) {
504-
result = networkTopology.applyFirewallRules(network, rules, domainRouterVO);
503+
result = result && networkTopology.applyFirewallRules(network, rules, domainRouterVO);
505504
}
506505
return result;
507506
}
508507

509508
@Override
510509
public boolean applyLBRules(final Network network, final List<LoadBalancingRule> rules)
511510
throws ResourceUnavailableException {
512-
boolean result = false;
511+
boolean result = true;
513512
if (canHandle(network, Service.Lb)) {
514513
if (!canHandleLbRules(rules)) {
515-
return result;
514+
return false;
516515
}
517516

518517
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(
@@ -521,19 +520,16 @@ public boolean applyLBRules(final Network network, final List<LoadBalancingRule>
521520
s_logger.debug("Virtual router elemnt doesn't need to apply firewall rules on the backend; virtual "
522521
+ "router doesn't exist in the network "
523522
+ network.getId());
524-
result = true;
525-
return result;
523+
return true;
526524
}
527525

528526
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
529527
final NetworkTopology networkTopology = _networkTopologyContext.retrieveNetworkTopology(dcVO);
530528

531529
for (final DomainRouterVO domainRouterVO : routers) {
532-
result = networkTopology.applyLoadBalancingRules(network, rules, domainRouterVO);
530+
result = result && networkTopology.applyLoadBalancingRules(network, rules, domainRouterVO);
533531
if (!result) {
534-
throw new CloudRuntimeException(
535-
"Failed to apply load balancing rules in network "
536-
+ network.getId());
532+
s_logger.debug("Failed to apply load balancing rules in network " + network.getId());
537533
}
538534
}
539535
}

‎server/src/com/cloud/network/element/VirtualRouterElement.java‎

Lines changed: 34 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,6 @@
9595
import com.cloud.utils.crypt.DBEncryptionUtil;
9696
import com.cloud.utils.db.QueryBuilder;
9797
import com.cloud.utils.db.SearchCriteria.Op;
98-
import com.cloud.utils.exception.CloudRuntimeException;
9998
import com.cloud.utils.net.NetUtils;
10099
import com.cloud.vm.DomainRouterVO;
101100
import com.cloud.vm.NicProfile;
@@ -283,10 +282,7 @@ public boolean applyFWRules(final Network network, final List<? extends Firewall
283282
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
284283

285284
for (final DomainRouterVO domainRouterVO : routers) {
286-
result = networkTopology.applyFirewallRules(network, rules, domainRouterVO);
287-
if (!result) {
288-
throw new CloudRuntimeException("Failed to apply firewall rules in network " + network.getId());
289-
}
285+
result = result && networkTopology.applyFirewallRules(network, rules, domainRouterVO);
290286
}
291287
}
292288
return result;
@@ -406,7 +402,7 @@ public boolean validateLBRule(final Network network, final LoadBalancingRule rul
406402

407403
@Override
408404
public boolean applyLBRules(final Network network, final List<LoadBalancingRule> rules) throws ResourceUnavailableException {
409-
boolean result = false;
405+
boolean result = true;
410406
if (canHandle(network, Service.Lb)) {
411407
if (!canHandleLbRules(rules)) {
412408
return false;
@@ -422,10 +418,7 @@ public boolean applyLBRules(final Network network, final List<LoadBalancingRule>
422418
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
423419

424420
for (final DomainRouterVO domainRouterVO : routers) {
425-
result = networkTopology.applyLoadBalancingRules(network, rules, domainRouterVO);
426-
if (!result) {
427-
throw new CloudRuntimeException("Failed to apply load balancing rules in network " + network.getId());
428-
}
421+
result = result && networkTopology.applyLoadBalancingRules(network, rules, domainRouterVO);
429422
}
430423
}
431424
return result;
@@ -497,14 +490,14 @@ public boolean stopVpn(final RemoteAccessVpn vpn) throws ResourceUnavailableExce
497490

498491
@Override
499492
public boolean applyIps(final Network network, final List<? extends PublicIpAddress> ipAddress, final Set<Service> services) throws ResourceUnavailableException {
500-
boolean result = false;
501493
boolean canHandle = true;
502494
for (final Service service : services) {
503495
if (!canHandle(network, service)) {
504496
canHandle = false;
505497
break;
506498
}
507499
}
500+
boolean result = true;
508501
if (canHandle) {
509502
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
510503
if (routers == null || routers.isEmpty()) {
@@ -516,7 +509,7 @@ public boolean applyIps(final Network network, final List<? extends PublicIpAddr
516509
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
517510

518511
for (final DomainRouterVO domainRouterVO : routers) {
519-
result = networkTopology.associatePublicIP(network, ipAddress, domainRouterVO);
512+
result = result && networkTopology.associatePublicIP(network, ipAddress, domainRouterVO);
520513
}
521514
}
522515
return result;
@@ -668,14 +661,14 @@ public boolean applyStaticNats(final Network network, final List<? extends Stati
668661
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
669662
if (routers == null || routers.isEmpty()) {
670663
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());
671-
return result;
664+
return true;
672665
}
673666

674667
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
675668
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
676669

677670
for (final DomainRouterVO domainRouterVO : routers) {
678-
result = networkTopology.applyStaticNats(network, rules, domainRouterVO);
671+
result = result && networkTopology.applyStaticNats(network, rules, domainRouterVO);
679672
}
680673
}
681674
return result;
@@ -687,20 +680,21 @@ public boolean shutdown(final Network network, final ReservationContext context,
687680
if (routers == null || routers.isEmpty()) {
688681
return true;
689682
}
690-
boolean result = true;
683+
boolean stopResult = true;
684+
boolean destroyResult = true;
691685
for (final DomainRouterVO router : routers) {
692-
result = result && _routerMgr.stop(router, false, context.getCaller(), context.getAccount()) != null;
686+
stopResult = stopResult && _routerMgr.stop(router, false, context.getCaller(), context.getAccount()) != null;
687+
if (!stopResult) {
688+
s_logger.warn("Failed to stop virtual router element " + router + ", but would try to process clean up anyway.");
689+
}
693690
if (cleanup) {
694-
if (!result) {
695-
s_logger.warn("Failed to stop virtual router element " + router + ", but would try to process clean up anyway.");
696-
}
697-
result = _routerMgr.destroyRouter(router.getId(), context.getAccount(), context.getCaller().getId()) != null;
698-
if (!result) {
691+
destroyResult = destroyResult && _routerMgr.destroyRouter(router.getId(), context.getAccount(), context.getCaller().getId()) != null;
692+
if (!destroyResult) {
699693
s_logger.warn("Failed to clean up virtual router element " + router);
700694
}
701695
}
702696
}
703-
return result;
697+
return stopResult & destroyResult;
704698
}
705699

706700
@Override
@@ -760,48 +754,46 @@ public boolean savePassword(final Network network, final NicProfile nic, final V
760754

761755
@Override
762756
public boolean saveSSHKey(final Network network, final NicProfile nic, final VirtualMachineProfile vm, final String sshPublicKey) throws ResourceUnavailableException {
763-
boolean result = false;
764757
if (!canHandle(network, null)) {
765-
return result;
758+
return false;
766759
}
767760
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
768761
if (routers == null || routers.isEmpty()) {
769762
s_logger.debug("Can't find virtual router element in network " + network.getId());
770-
result = true;
771-
return result;
763+
return true;
772764
}
773765

774766
final VirtualMachineProfile uservm = vm;
775767

776768
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
777769
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
778770

771+
boolean result = true;
779772
for (final DomainRouterVO domainRouterVO : routers) {
780-
result = networkTopology.saveSSHPublicKeyToRouter(network, nic, uservm, domainRouterVO, sshPublicKey);
773+
result = result && networkTopology.saveSSHPublicKeyToRouter(network, nic, uservm, domainRouterVO, sshPublicKey);
781774
}
782775
return result;
783776
}
784777

785778
@Override
786779
public boolean saveUserData(final Network network, final NicProfile nic, final VirtualMachineProfile vm) throws ResourceUnavailableException {
787-
boolean result = false;
788780
if (!canHandle(network, null)) {
789-
return result;
781+
return false;
790782
}
791783
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
792784
if (routers == null || routers.isEmpty()) {
793785
s_logger.debug("Can't find virtual router element in network " + network.getId());
794-
result = true;
795-
return result;
786+
return true;
796787
}
797788

798789
final VirtualMachineProfile uservm = vm;
799790

800791
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
801792
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
802793

794+
boolean result = true;
803795
for (final DomainRouterVO domainRouterVO : routers) {
804-
result = networkTopology.saveUserDataToRouter(network, nic, uservm, domainRouterVO);
796+
result = result && networkTopology.saveUserDataToRouter(network, nic, uservm, domainRouterVO);
805797
}
806798
return result;
807799
}
@@ -860,23 +852,19 @@ public VirtualRouterProvider addElement(final Long nspId, final Type providerTyp
860852

861853
@Override
862854
public boolean applyPFRules(final Network network, final List<PortForwardingRule> rules) throws ResourceUnavailableException {
863-
boolean result = false;
855+
boolean result = true;
864856
if (canHandle(network, Service.PortForwarding)) {
865857
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
866858
if (routers == null || routers.isEmpty()) {
867859
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());
868-
result = true;
869-
return result;
860+
return true;
870861
}
871862

872863
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
873864
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
874865

875866
for (final DomainRouterVO domainRouterVO : routers) {
876-
result = networkTopology.applyFirewallRules(network, rules, domainRouterVO);
877-
if (!result) {
878-
throw new CloudRuntimeException("Failed to apply firewall rules in network " + network.getId());
879-
}
867+
result = result && networkTopology.applyFirewallRules(network, rules, domainRouterVO);
880868
}
881869
}
882870
return result;
@@ -978,10 +966,10 @@ public boolean removeDhcpSupportForSubnet(final Network network) throws Resource
978966
@Override
979967
public boolean addDhcpEntry(final Network network, final NicProfile nic, final VirtualMachineProfile vm, final DeployDestination dest, final ReservationContext context)
980968
throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException {
981-
boolean result = false;
969+
boolean result = true;
982970
if (canHandle(network, Service.Dhcp)) {
983971
if (vm.getType() != VirtualMachine.Type.User) {
984-
return result;
972+
return false;
985973
}
986974

987975
final VirtualMachineProfile uservm = vm;
@@ -995,7 +983,7 @@ public boolean addDhcpEntry(final Network network, final NicProfile nic, final V
995983
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
996984

997985
for (final DomainRouterVO domainRouterVO : routers) {
998-
result = networkTopology.applyDhcpEntry(network, nic, uservm, dest, domainRouterVO);
986+
result = result && networkTopology.applyDhcpEntry(network, nic, uservm, dest, domainRouterVO);
999987
}
1000988
}
1001989
return result;
@@ -1004,16 +992,15 @@ public boolean addDhcpEntry(final Network network, final NicProfile nic, final V
1004992
@Override
1005993
public boolean addPasswordAndUserdata(final Network network, final NicProfile nic, final VirtualMachineProfile vm, final DeployDestination dest,
1006994
final ReservationContext context) throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException {
1007-
boolean result = false;
995+
boolean result = true;
1008996
if (canHandle(network, Service.UserData)) {
1009997
if (vm.getType() != VirtualMachine.Type.User) {
1010-
return result;
998+
return false;
1011999
}
10121000

10131001
if (network.getIp6Gateway() != null) {
10141002
s_logger.info("Skip password and userdata service setup for IPv6 VM");
1015-
result = true;
1016-
return result;
1003+
return true;
10171004
}
10181005

10191006
final VirtualMachineProfile uservm = vm;
@@ -1028,7 +1015,7 @@ public boolean addPasswordAndUserdata(final Network network, final NicProfile ni
10281015
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);
10291016

10301017
for (final DomainRouterVO domainRouterVO : routers) {
1031-
result = networkTopology.applyUserData(network, nic, uservm, dest, domainRouterVO);
1018+
result = result && networkTopology.applyUserData(network, nic, uservm, dest, domainRouterVO);
10321019
}
10331020
}
10341021
return result;

0 commit comments

Comments
 (0)