Skip to content

CLOUDSTACK-9106 - As a Developer I want the Redundant VPC private gateway feature fixed #1179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wilderrodrigues wants to merge 3 commits into from
Closed

CLOUDSTACK-9106 - As a Developer I want the Redundant VPC private gateway feature fixed #1179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae325f6
CLOUDSTACK-9106 - Reduces the amount of iterations through the router…
wilderrodrigues Dec 4, 2015
78e15d1
CLOUDSTACK-9106 - Refactor the createPrivateNicProfileForGateway() me…
wilderrodrigues Dec 5, 2015
6dab361
CLOUDSTACK-9106 - Enables private gateway tests on Redundant VPCs
wilderrodrigues Dec 5, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 72 additions & 62 deletions plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java
View file
Open in desktop

Large diffs are not rendered by default.

106 changes: 65 additions & 41 deletions server/src/com/cloud/network/element/VirtualRouterElement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ public boolean prepare(final Network network, final NicProfile nic, final Virtua

@Override
public boolean applyFWRules(final Network network, final List<? extends FirewallRule> rules) throws ResourceUnavailableException {
boolean result = true;
if (canHandle(network, Service.Firewall)) {
final List<DomainRouterVO> routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (routers == null || routers.isEmpty()) {
Expand All @@ -278,14 +279,14 @@ public boolean applyFWRules(final Network network, final List<? extends Firewall
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);

if (!networkTopology.applyFirewallRules(network, rules, routers)) {
throw new CloudRuntimeException("Failed to apply firewall rules in network " + network.getId());
} else {
return true;
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());
}
}
} else {
return true;
}
return result;
}

/*
Expand Down Expand Up @@ -402,6 +403,7 @@ public boolean validateLBRule(final Network network, final LoadBalancingRule rul

@Override
public boolean applyLBRules(final Network network, final List<LoadBalancingRule> rules) throws ResourceUnavailableException {
boolean result = false;
if (canHandle(network, Service.Lb)) {
if (!canHandleLbRules(rules)) {
return false;
Expand All @@ -416,14 +418,14 @@ public boolean applyLBRules(final Network network, final List<LoadBalancingRule>
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);

if (!networkTopology.applyLoadBalancingRules(network, rules, routers)) {
throw new CloudRuntimeException("Failed to apply load balancing rules in network " + network.getId());
} else {
return true;
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());
}
}
} else {
return false;
}
return result;
}

@Override
Expand Down Expand Up @@ -492,6 +494,7 @@ public boolean stopVpn(final RemoteAccessVpn vpn) throws ResourceUnavailableExce

@Override
public boolean applyIps(final Network network, final List<? extends PublicIpAddress> ipAddress, final Set<Service> services) throws ResourceUnavailableException {
boolean result = false;
boolean canHandle = true;
for (final Service service : services) {
if (!canHandle(network, service)) {
Expand All @@ -509,10 +512,11 @@ public boolean applyIps(final Network network, final List<? extends PublicIpAddr
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);

return networkTopology.associatePublicIP(network, ipAddress, routers);
} else {
return false;
for (final DomainRouterVO domainRouterVO : routers) {
result = networkTopology.associatePublicIP(network, ipAddress, domainRouterVO);
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Dec 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&= these constructs only return the last result.

}
}
return result;
}

@Override
Expand Down Expand Up @@ -656,20 +660,22 @@ private static Map<Service, Map<Capability, String>> setCapabilities() {

@Override
public boolean applyStaticNats(final Network network, final List<? extends StaticNat> rules) throws ResourceUnavailableException {
boolean result = true;
if (canHandle(network, Service.StaticNat)) {
final List<DomainRouterVO> 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 true;
return result;
}

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

return networkTopology.applyStaticNats(network, rules, routers);
} else {
return true;
for (final DomainRouterVO domainRouterVO : routers) {
result = networkTopology.applyStaticNats(network, rules, domainRouterVO);
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Dec 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&=

}
}
return result;
}

@Override
Expand Down Expand Up @@ -732,7 +738,7 @@ public boolean savePassword(final Network network, final NicProfile nic, final V
// save the password in DB
for (final VirtualRouter router : routers) {
if (router.getState() == State.Running) {
return networkTopology.savePasswordToRouter(network, nic, uservm, routers);
return networkTopology.savePasswordToRouter(network, nic, uservm, router);
}
}
final String password = (String) uservm.getParameter(VirtualMachineProfile.Param.VmPassword);
Expand All @@ -751,40 +757,50 @@ public boolean savePassword(final Network network, final NicProfile nic, final V

@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 false;
return result;
}
final List<DomainRouterVO> 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());
return true;
result = true;
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Dec 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh? why not return true;?

Copy link
Copy Markdown
Contributor Author

@wilderrodrigues wilderrodrigues Dec 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I do not want to return a literal. And the same for the other comments.

return result;
}

final VirtualMachineProfile uservm = vm;

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

return networkTopology.saveSSHPublicKeyToRouter(network, nic, uservm, routers, sshPublicKey);
for (final DomainRouterVO domainRouterVO : routers) {
result = networkTopology.saveSSHPublicKeyToRouter(network, nic, uservm, domainRouterVO, sshPublicKey);
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Dec 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&=

}
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 false;
return result;
}
final List<DomainRouterVO> 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());
return true;
result = true;
return result;
}

final VirtualMachineProfile uservm = vm;

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

return networkTopology.saveUserDataToRouter(network, nic, uservm, routers);
for (final DomainRouterVO domainRouterVO : routers) {
result = networkTopology.saveUserDataToRouter(network, nic, uservm, domainRouterVO);
}
return result;
}

@Override
Expand Down Expand Up @@ -841,24 +857,26 @@ public VirtualRouterProvider addElement(final Long nspId, final Type providerTyp

@Override
public boolean applyPFRules(final Network network, final List<PortForwardingRule> rules) throws ResourceUnavailableException {
boolean result = false;
if (canHandle(network, Service.PortForwarding)) {
final List<DomainRouterVO> 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 true;
result = true;
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Dec 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return true; would work wouldn't it?

Copy link
Copy Markdown
Contributor Author

@wilderrodrigues wilderrodrigues Dec 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would. But I won't change because I don't agree with the approach of returning true/false in several places.

  1. A method that returns something should have only one point where it actually returns. So those several returns is a bad practice. A way to make it not so bad is to assign the return to a variable, so people looking at the code in the future won't miss a hidden "return true" somewhere. I did not change all of it because in some places it requires a better refactor.
  2. If a method execution cannot proceed due to some condition, it should fail - exception. A return false should be used when one wants to check is something is valid/exists or not.

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Dec 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your consideration and I am not forcing you to change to win my lgtm. I just want to make sure we discuss the consideration here for posterity and change only if a simple good alternative comes up.

I noticed line 874 and wondered about this and the &= thingy upstairs. I would use a combination of first trying all of them and then throw the exception if any of them failed

return result;
}

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

if (!networkTopology.applyFirewallRules(network, rules, routers)) {
throw new CloudRuntimeException("Failed to apply firewall rules in network " + network.getId());
} else {
return true;
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());
}
}
} else {
return true;
}
return result;
}

@Override
Expand Down Expand Up @@ -957,13 +975,13 @@ public boolean removeDhcpSupportForSubnet(final Network network) throws Resource
@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;
if (canHandle(network, Service.Dhcp)) {
if (vm.getType() != VirtualMachine.Type.User) {
return false;
return result;
}

final VirtualMachineProfile uservm = vm;

final List<DomainRouterVO> routers = getRouters(network, dest);

if (routers == null || routers.size() == 0) {
Expand All @@ -973,22 +991,26 @@ public boolean addDhcpEntry(final Network network, final NicProfile nic, final V
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);

return networkTopology.applyDhcpEntry(network, nic, uservm, dest, routers);
for (final DomainRouterVO domainRouterVO : routers) {
result = networkTopology.applyDhcpEntry(network, nic, uservm, dest, domainRouterVO);
}
}
return false;
return result;
}

@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;
if (canHandle(network, Service.UserData)) {
if (vm.getType() != VirtualMachine.Type.User) {
return false;
return result;
}

if (network.getIp6Gateway() != null) {
s_logger.info("Skip password and userdata service setup for IPv6 VM");
return true;
result = true;
return result;
}

final VirtualMachineProfile uservm = vm;
Expand All @@ -1002,9 +1024,11 @@ public boolean addPasswordAndUserdata(final Network network, final NicProfile ni
final DataCenterVO dcVO = _dcDao.findById(network.getDataCenterId());
final NetworkTopology networkTopology = networkTopologyContext.retrieveNetworkTopology(dcVO);

return networkTopology.applyUserData(network, nic, uservm, dest, routers);
for (final DomainRouterVO domainRouterVO : routers) {
result = networkTopology.applyUserData(network, nic, uservm, dest, domainRouterVO);
}
}
return false;
return result;
}

protected List<DomainRouterVO> getRouters(final Network network, final DeployDestination dest) {
Expand Down
Loading