Refactor and test NetworkHelper#sendCommandsToRouterWithNoAnswers
Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/2802d3b7 Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/2802d3b7 Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/2802d3b7 Branch: refs/heads/master Commit: 2802d3b75bd420458e1c184fd2f19158f2004e16 Parents: e08cb11 Author: Antonio Fornie <afor...@schubergphilis.com> Authored: Thu Aug 21 03:31:02 2014 -0500 Committer: wilderrodrigues <wrodrig...@schubergphilis.com> Committed: Tue Oct 14 15:02:14 2014 +0200 ---------------------------------------------------------------------- .../spring-server-core-managers-context.xml | 3 - .../cloud/network/router/NetworkHelperImpl.java | 18 +-- .../network/router/RouterControlHelper.java | 2 +- .../VpcVirtualNetworkApplianceManagerImpl.java | 8 +- .../topology/AdvancedNetworkTopology.java | 1 + .../network/topology/BasicNetworkTopology.java | 1 + .../network/router/NetworkHelperImplTest.java | 156 +++++++++++++++++++ .../network/router/RouterControlHelperTest.java | 105 +++++++++++++ 8 files changed, 274 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cloudstack/blob/2802d3b7/server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml ---------------------------------------------------------------------- diff --git a/server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml b/server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml index 2b46ca3..7a9044b 100644 --- a/server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml +++ b/server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml @@ -206,9 +206,6 @@ <bean id="vpcNetworkHelper" class="com.cloud.network.router.VpcNetworkHelperImpl" /> - <bean id="vpcVirtualNetworkHelper" - class="com.cloud.network.router.VpcVirtualNetworkHelperImpl" /> - <bean id="nicProfileHelper" class="com.cloud.network.router.NicProfileHelperImpl" /> http://git-wip-us.apache.org/repos/asf/cloudstack/blob/2802d3b7/server/src/com/cloud/network/router/NetworkHelperImpl.java ---------------------------------------------------------------------- diff --git a/server/src/com/cloud/network/router/NetworkHelperImpl.java b/server/src/com/cloud/network/router/NetworkHelperImpl.java index ece0c0f..689ef3b 100644 --- a/server/src/com/cloud/network/router/NetworkHelperImpl.java +++ b/server/src/com/cloud/network/router/NetworkHelperImpl.java @@ -195,22 +195,16 @@ public class NetworkHelperImpl implements NetworkHelper { throw new AgentUnavailableException("Unable to send commands to virtual router ", router.getHostId(), e); } - if (answers == null) { - return false; - } - - if (answers.length != cmds.size()) { + if (answers == null || answers.length != cmds.size()) { return false; } // FIXME: Have to return state for individual command in the future boolean result = true; - if (answers.length > 0) { - for (final Answer answer : answers) { - if (!answer.getResult()) { - result = false; - break; - } + for (final Answer answer : answers) { + if (!answer.getResult()) { + result = false; + break; } } return result; @@ -253,7 +247,7 @@ public class NetworkHelperImpl implements NetworkHelper { final int connRouterPR = getRealPriority(connectedRouter); final int disconnRouterPR = getRealPriority(disconnectedRouter); if (connRouterPR < disconnRouterPR) { - //connRouterPR < disconnRouterPR, they won't equal at anytime + //connRouterPR < disconnRouterPR, they won't equal at any time if (!connectedRouter.getIsPriorityBumpUp()) { final BumpUpPriorityCommand command = new BumpUpPriorityCommand(); command.setAccessDetail(NetworkElementCommand.ROUTER_IP, getRouterControlIp(connectedRouter.getId())); http://git-wip-us.apache.org/repos/asf/cloudstack/blob/2802d3b7/server/src/com/cloud/network/router/RouterControlHelper.java ---------------------------------------------------------------------- diff --git a/server/src/com/cloud/network/router/RouterControlHelper.java b/server/src/com/cloud/network/router/RouterControlHelper.java index 583bd58..68fd6e3 100644 --- a/server/src/com/cloud/network/router/RouterControlHelper.java +++ b/server/src/com/cloud/network/router/RouterControlHelper.java @@ -35,7 +35,7 @@ public class RouterControlHelper { private static final Logger logger = Logger.getLogger(RouterControlHelper.class); @Inject - private final DomainRouterDao routerDao = null; + private DomainRouterDao routerDao; @Inject private NetworkDao networkDao; http://git-wip-us.apache.org/repos/asf/cloudstack/blob/2802d3b7/server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java ---------------------------------------------------------------------- diff --git a/server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java b/server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java index 1761699..da1447a 100644 --- a/server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java +++ b/server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java @@ -135,7 +135,7 @@ public class VpcVirtualNetworkApplianceManagerImpl extends VirtualNetworkApplian private EntityManager _entityMgr; @Inject - private NicProfileHelper vpcHelper; + private NicProfileHelper nicProfileHelper; @Override public boolean configure(final String name, final Map<String, Object> params) throws ConfigurationException { @@ -323,7 +323,7 @@ public class VpcVirtualNetworkApplianceManagerImpl extends VirtualNetworkApplian int i = 0; for (final PublicIpAddress ipAddr : ipAddrList) { - boolean add = (ipAddr.getState() == IpAddress.State.Releasing ? false : true); + boolean add = ipAddr.getState() == IpAddress.State.Releasing ? false : true; String macAddress = vlanMacAddress.get(BroadcastDomainType.getValue(BroadcastDomainType.fromString(ipAddr.getVlanTag()))); @@ -431,7 +431,7 @@ public class VpcVirtualNetworkApplianceManagerImpl extends VirtualNetworkApplian public boolean finalizeCommandsOnStart(final Commands cmds, final VirtualMachineProfile profile) { DomainRouterVO router = _routerDao.findById(profile.getId()); - boolean isVpc = (router.getVpcId() != null); + boolean isVpc = router.getVpcId() != null; if (!isVpc) { return super.finalizeCommandsOnStart(cmds, profile); } @@ -621,7 +621,7 @@ public class VpcVirtualNetworkApplianceManagerImpl extends VirtualNetworkApplian if (router.getVpcId() != null) { if (_networkModel.isProviderSupportServiceInNetwork(guestNetworkId, Service.NetworkACL, Provider.VPCVirtualRouter)) { List<NetworkACLItemVO> networkACLs = _networkACLMgr.listNetworkACLItems(guestNetworkId); - if ((networkACLs != null) && !networkACLs.isEmpty()) { + if (networkACLs != null && !networkACLs.isEmpty()) { s_logger.debug("Found " + networkACLs.size() + " network ACLs to apply as a part of VPC VR " + router + " start for guest network id=" + guestNetworkId); createNetworkACLsCommands(networkACLs, router, cmds, guestNetworkId, false); http://git-wip-us.apache.org/repos/asf/cloudstack/blob/2802d3b7/server/src/org/apache/cloudstack/network/topology/AdvancedNetworkTopology.java ---------------------------------------------------------------------- diff --git a/server/src/org/apache/cloudstack/network/topology/AdvancedNetworkTopology.java b/server/src/org/apache/cloudstack/network/topology/AdvancedNetworkTopology.java index 8b2a5a6..892a81a 100644 --- a/server/src/org/apache/cloudstack/network/topology/AdvancedNetworkTopology.java +++ b/server/src/org/apache/cloudstack/network/topology/AdvancedNetworkTopology.java @@ -285,6 +285,7 @@ public class AdvancedNetworkTopology extends BasicNetworkTopology { } if (!connectedRouters.isEmpty()) { + // Shouldn't we include this check inside the method? if (!isZoneBasic && !disconnectedRouters.isEmpty() && disconnectedRouters.get(0).getIsRedundantRouter()) { // These disconnected redundant virtual routers are out of sync // now, stop them for synchronization http://git-wip-us.apache.org/repos/asf/cloudstack/blob/2802d3b7/server/src/org/apache/cloudstack/network/topology/BasicNetworkTopology.java ---------------------------------------------------------------------- diff --git a/server/src/org/apache/cloudstack/network/topology/BasicNetworkTopology.java b/server/src/org/apache/cloudstack/network/topology/BasicNetworkTopology.java index 0873d9a..ec52911 100644 --- a/server/src/org/apache/cloudstack/network/topology/BasicNetworkTopology.java +++ b/server/src/org/apache/cloudstack/network/topology/BasicNetworkTopology.java @@ -412,6 +412,7 @@ public class BasicNetworkTopology implements NetworkTopology { } if (!connectedRouters.isEmpty()) { + // Shouldn't we include this check inside the method? if (!isZoneBasic && !disconnectedRouters.isEmpty() && disconnectedRouters.get(0).getIsRedundantRouter()) { // These disconnected redundant virtual routers are out of sync // now, stop them for synchronization http://git-wip-us.apache.org/repos/asf/cloudstack/blob/2802d3b7/server/test/com/cloud/network/router/NetworkHelperImplTest.java ---------------------------------------------------------------------- diff --git a/server/test/com/cloud/network/router/NetworkHelperImplTest.java b/server/test/com/cloud/network/router/NetworkHelperImplTest.java new file mode 100644 index 0000000..04c5067 --- /dev/null +++ b/server/test/com/cloud/network/router/NetworkHelperImplTest.java @@ -0,0 +1,156 @@ +package com.cloud.network.router; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Matchers; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; + +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.Command; +import com.cloud.agent.manager.Commands; +import com.cloud.exception.AgentUnavailableException; +import com.cloud.exception.OperationTimedoutException; +import com.cloud.utils.exception.CloudRuntimeException; + + +@RunWith(MockitoJUnitRunner.class) +public class NetworkHelperImplTest { + + private static final long HOST_ID = 10L; + + @Mock + protected AgentManager agentManager; + + @InjectMocks + protected NetworkHelperImpl nwHelper = new NetworkHelperImpl(); + + @Test(expected=CloudRuntimeException.class) + public void testSendCommandsToRouterWrongRouterVersion() + throws AgentUnavailableException, OperationTimedoutException { + // Prepare + NetworkHelperImpl nwHelperUT = spy(this.nwHelper); + VirtualRouter vr = mock(VirtualRouter.class); + doReturn(false).when(nwHelperUT).checkRouterVersion(vr); + + // Execute + nwHelperUT.sendCommandsToRouter(vr, null); + + // Assert + verify(this.agentManager, times(0)).send((Long) Matchers.anyObject(), (Command) Matchers.anyObject()); + } + + @Test + public void testSendCommandsToRouter() + throws AgentUnavailableException, OperationTimedoutException { + // Prepare + NetworkHelperImpl nwHelperUT = spy(this.nwHelper); + VirtualRouter vr = mock(VirtualRouter.class); + when(vr.getHostId()).thenReturn(HOST_ID); + doReturn(true).when(nwHelperUT).checkRouterVersion(vr); + + Commands commands = mock(Commands.class); + when(commands.size()).thenReturn(3); + Answer answer1 = mock(Answer.class); + Answer answer2 = mock(Answer.class); + Answer answer3 = mock(Answer.class); + // In the second iteration it should match and return, without invoking the third + Answer[] answers = {answer1, answer2, answer3}; + when(answer1.getResult()).thenReturn(true); + when(answer2.getResult()).thenReturn(false); + when(answer3.getResult()).thenReturn(false); + when(this.agentManager.send(HOST_ID, commands)).thenReturn(answers); + + // Execute + final boolean result = nwHelperUT.sendCommandsToRouter(vr, commands); + + // Assert + verify(this.agentManager, times(1)).send(HOST_ID, commands); + verify(answer1, times(1)).getResult(); + verify(answer2, times(1)).getResult(); + verify(answer3, times(0)).getResult(); + assertFalse(result); + } + + /** + * The only way result can be true is if each and every command receive a true result + * + * @throws AgentUnavailableException + * @throws OperationTimedoutException + */ + @Test + public void testSendCommandsToRouterWithTrueResult() + throws AgentUnavailableException, OperationTimedoutException { + // Prepare + NetworkHelperImpl nwHelperUT = spy(this.nwHelper); + VirtualRouter vr = mock(VirtualRouter.class); + when(vr.getHostId()).thenReturn(HOST_ID); + doReturn(true).when(nwHelperUT).checkRouterVersion(vr); + + Commands commands = mock(Commands.class); + when(commands.size()).thenReturn(3); + Answer answer1 = mock(Answer.class); + Answer answer2 = mock(Answer.class); + Answer answer3 = mock(Answer.class); + // In the second iteration it should match and return, without invoking the third + Answer[] answers = {answer1, answer2, answer3}; + when(answer1.getResult()).thenReturn(true); + when(answer2.getResult()).thenReturn(true); + when(answer3.getResult()).thenReturn(true); + when(this.agentManager.send(HOST_ID, commands)).thenReturn(answers); + + // Execute + final boolean result = nwHelperUT.sendCommandsToRouter(vr, commands); + + // Assert + verify(this.agentManager, times(1)).send(HOST_ID, commands); + verify(answer1, times(1)).getResult(); + verify(answer2, times(1)).getResult(); + verify(answer3, times(1)).getResult(); + assertTrue(result); + } + + /** + * If the number of answers is different to the number of commands the result is false + * + * @throws AgentUnavailableException + * @throws OperationTimedoutException + */ + @Test + public void testSendCommandsToRouterWithNoAnswers() + throws AgentUnavailableException, OperationTimedoutException { + // Prepare + NetworkHelperImpl nwHelperUT = spy(this.nwHelper); + VirtualRouter vr = mock(VirtualRouter.class); + when(vr.getHostId()).thenReturn(HOST_ID); + doReturn(true).when(nwHelperUT).checkRouterVersion(vr); + + Commands commands = mock(Commands.class); + when(commands.size()).thenReturn(3); + Answer answer1 = mock(Answer.class); + Answer answer2 = mock(Answer.class); + // In the second iteration it should match and return, without invoking the third + Answer[] answers = {answer1, answer2}; + when(this.agentManager.send(HOST_ID, commands)).thenReturn(answers); + + // Execute + final boolean result = nwHelperUT.sendCommandsToRouter(vr, commands); + + // Assert + verify(this.agentManager, times(1)).send(HOST_ID, commands); + verify(answer1, times(0)).getResult(); + assertFalse(result); + } + +} http://git-wip-us.apache.org/repos/asf/cloudstack/blob/2802d3b7/server/test/com/cloud/network/router/RouterControlHelperTest.java ---------------------------------------------------------------------- diff --git a/server/test/com/cloud/network/router/RouterControlHelperTest.java b/server/test/com/cloud/network/router/RouterControlHelperTest.java new file mode 100644 index 0000000..46d0fb2 --- /dev/null +++ b/server/test/com/cloud/network/router/RouterControlHelperTest.java @@ -0,0 +1,105 @@ +package com.cloud.network.router; + +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; + +import com.cloud.network.Networks.TrafficType; +import com.cloud.network.dao.NetworkDao; +import com.cloud.network.dao.NetworkVO; +import com.cloud.vm.DomainRouterVO; +import com.cloud.vm.NicVO; +import com.cloud.vm.dao.DomainRouterDao; +import com.cloud.vm.dao.NicDao; + +@RunWith(MockitoJUnitRunner.class) +public class RouterControlHelperTest { + + private static final String DIDN_T_GET_THE_EXPECTED_IP4_ADDRESS = "Didn't get the expected IP4 address"; + private static final String IP4_ADDRES1 = "IP4Addres1"; + private static final String IP4_ADDRES2 = "IP4Addres2"; + protected static final long ROUTER_ID = 1L; + protected static final long NW_ID_1 = 11L; + protected static final long NW_ID_2 = 12L; + protected static final long NW_ID_3 = 13L; + + @Mock + protected NicDao nicDao; + @Mock + protected NetworkDao nwDao; + @Mock + protected DomainRouterDao routerDao; + + @InjectMocks + protected RouterControlHelper routerControlHelper = new RouterControlHelper(); + + @Test + public void testGetRouterControlIp() { + // Prepare + List<NicVO> nics = new ArrayList<>(); + NicVO nic1 = mock(NicVO.class); + NicVO nic2 = mock(NicVO.class); + // Actually the third one will never be used, but we must assert that is not + NicVO nic3 = mock(NicVO.class); + when(nic1.getNetworkId()).thenReturn(NW_ID_1); + when(nic2.getNetworkId()).thenReturn(NW_ID_2); + when(nic2.getIp4Address()).thenReturn(IP4_ADDRES1); + when(nic3.getNetworkId()).thenReturn(NW_ID_3); + when(nic3.getIp4Address()).thenReturn(IP4_ADDRES2); + nics.add(nic1); + nics.add(nic2); + nics.add(nic3); + when(this.nicDao.listByVmId(ROUTER_ID)).thenReturn(nics); + + NetworkVO nw1 = mock(NetworkVO.class); + when(nw1.getTrafficType()).thenReturn(TrafficType.Public); + NetworkVO nw2 = mock(NetworkVO.class); + when(nw2.getTrafficType()).thenReturn(TrafficType.Control); + NetworkVO nw3 = mock(NetworkVO.class); + when(nw3.getTrafficType()).thenReturn(TrafficType.Control); + when(this.nwDao.findById(NW_ID_1)).thenReturn(nw1); + when(this.nwDao.findById(NW_ID_2)).thenReturn(nw2); + when(this.nwDao.findById(NW_ID_3)).thenReturn(nw3); + + // Execute + final String ip4address = this.routerControlHelper.getRouterControlIp(ROUTER_ID); + + // Assert + assertEquals(DIDN_T_GET_THE_EXPECTED_IP4_ADDRESS, IP4_ADDRES1, ip4address); + } + + @Test + public void testGetRouterControlIpWithRouterIp() { + // Prepare + List<NicVO> nics = new ArrayList<>(); + NicVO nic1 = mock(NicVO.class); + when(nic1.getNetworkId()).thenReturn(NW_ID_1); + when(nic1.getIp4Address()).thenReturn(null); + nics.add(nic1); + when(this.nicDao.listByVmId(ROUTER_ID)).thenReturn(nics); + + NetworkVO nw1 = mock(NetworkVO.class); + when(nw1.getTrafficType()).thenReturn(TrafficType.Public); + when(this.nwDao.findById(NW_ID_1)).thenReturn(nw1); + + DomainRouterVO router = mock(DomainRouterVO.class); + when(this.routerDao.findById(ROUTER_ID)).thenReturn(router); + when(router.getPrivateIpAddress()).thenReturn(IP4_ADDRES1); + + // Execute + final String ip4address = this.routerControlHelper.getRouterControlIp(ROUTER_ID); + + // Assert + assertEquals(DIDN_T_GET_THE_EXPECTED_IP4_ADDRESS, IP4_ADDRES1, ip4address); + } + +}
