Copilot commented on code in PR #13792:
URL: https://github.com/apache/cloudstack/pull/13792#discussion_r3719559258
##########
plugins/network-elements/ovs/src/main/java/com/cloud/network/ovs/OvsTunnelManagerImpl.java:
##########
@@ -684,25 +690,28 @@ private void handleVmStateChange(VMInstanceVO vm) {
}
for (Long vpcId: vpcIds) {
- VpcVO vpc = _vpcDao.findById(vpcId);
- // nothing to do if the VPC is not setup for distributed routing
- if (vpc == null || !vpc.usesDistributedRouter()) {
- return;
+ if (!isOvsDistributedRouterVpc(vpcId)) {
+ continue;
}
- // get the list of hosts on which VPC spans (i.e hosts that need
to be aware of VPC topology change update)
- List<Long> vpcSpannedHostIds =
_ovsNetworkToplogyGuru.getVpcSpannedHosts(vpcId);
- String bridgeName=generateBridgeNameForVpc(vpcId);
-
- OvsVpcPhysicalTopologyConfigCommand topologyConfigCommand =
prepareVpcTopologyUpdate(vpcId);
-
topologyConfigCommand.setSequenceNumber(getNextTopologyUpdateSequenceNumber(vpcId));
-
- // send topology change update to VPC spanned hosts
- for (Long id: vpcSpannedHostIds) {
- if (!sendVpcTopologyChangeUpdate(topologyConfigCommand, id,
bridgeName)) {
- logger.debug("Failed to send VPC topology change update to
host : " + id + ". Moving on " +
- "with rest of the host update.");
+ try {
+ // get the list of hosts on which VPC spans (i.e hosts that
need to be aware of VPC topology change update)
+ List<Long> vpcSpannedHostIds =
_ovsNetworkToplogyGuru.getVpcSpannedHosts(vpcId);
Review Comment:
The failure-isolation goal is not fully met because
`isOvsDistributedRouterVpc(vpcId)` (DAO/provider lookup) executes outside the
per‑VPC `try/catch`. If `_vpcDao.findById` or
`_vpcMgr.isProviderSupportServiceInVpc` throws for one VPC, it can still abort
processing of the remaining VPCs. Move the OVS/distributed eligibility check
inside the `try/catch`, or wrap just that call in its own `try/catch` that logs
and continues.
##########
plugins/network-elements/ovs/src/test/java/com/cloud/network/ovs/OvsTunnelManagerImplTest.java:
##########
@@ -0,0 +1,379 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.network.ovs;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import com.cloud.agent.AgentManager;
+import com.cloud.agent.api.OvsVpcPhysicalTopologyConfigCommand;
+import com.cloud.host.dao.HostDao;
+import com.cloud.network.Network;
+import com.cloud.network.Networks.BroadcastDomainType;
+import com.cloud.network.dao.NetworkDao;
+import com.cloud.network.dao.NetworkVO;
+import com.cloud.network.ovs.dao.VpcDistributedRouterSeqNoDao;
+import com.cloud.network.ovs.dao.VpcDistributedRouterSeqNoVO;
+import com.cloud.network.vpc.VpcManager;
+import com.cloud.network.vpc.VpcVO;
+import com.cloud.network.vpc.dao.VpcDao;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.StateMachine2;
+import com.cloud.vm.NicVO;
+import com.cloud.vm.VMInstanceVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.NicDao;
+import com.cloud.vm.dao.VMInstanceDao;
+
+public class OvsTunnelManagerImplTest {
+ private static final long VPC_ID = 7L;
+ private static final long SECOND_VPC_ID = 8L;
+
+ private OvsTunnelManagerImpl manager;
+ private VpcDao vpcDao;
+ private VpcManager vpcManager;
+ private OvsNetworkTopologyGuru topologyGuru;
+ private NicDao nicDao;
+ private VpcDistributedRouterSeqNoVO sequenceNumber;
+
+ @Before
+ public void setUp() {
+ manager = new OvsTunnelManagerImpl();
+ vpcDao = mock(VpcDao.class);
+ vpcManager = mock(VpcManager.class);
+ topologyGuru = mock(OvsNetworkTopologyGuru.class);
+ nicDao = mock(NicDao.class);
+ manager._vpcDao = vpcDao;
+ manager._vpcMgr = vpcManager;
+ manager._ovsNetworkToplogyGuru = topologyGuru;
+ manager._nicDao = nicDao;
+ manager._hostDao = mock(HostDao.class);
+ manager._vmInstanceDao = mock(VMInstanceDao.class);
+ manager._networkDao = mock(NetworkDao.class);
+ manager._vpcDrSeqNoDao = mock(VpcDistributedRouterSeqNoDao.class);
+ manager._agentMgr = mock(AgentManager.class);
+ }
+
+ @Test
+ public void testIsOvsDistributedRouterVpcReturnsFalseWhenVpcIsMissing() {
+ assertFalse(manager.isOvsDistributedRouterVpc(VPC_ID));
+ verify(vpcManager, never()).isProviderSupportServiceInVpc(anyLong(),
+ org.mockito.ArgumentMatchers.any(Network.Service.class),
+ org.mockito.ArgumentMatchers.any(Network.Provider.class));
+ }
+
+ @Test
+ public void
testIsOvsDistributedRouterVpcReturnsFalseWhenVpcIsNotDistributed() {
+ VpcVO vpc = mock(VpcVO.class);
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.usesDistributedRouter()).thenReturn(false);
+
+ assertFalse(manager.isOvsDistributedRouterVpc(VPC_ID));
+ }
+
+ @Test
+ public void
testIsOvsDistributedRouterVpcReturnsFalseForNsxDistributedVpc() {
+ VpcVO vpc = mock(VpcVO.class);
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.usesDistributedRouter()).thenReturn(true);
+ when(vpcManager.isProviderSupportServiceInVpc(VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(false);
+
+ assertFalse(manager.isOvsDistributedRouterVpc(VPC_ID));
+ }
+
+ @Test
+ public void
testIsOvsDistributedRouterVpcReturnsTrueForOvsConnectivityDistributedVpc() {
+ VpcVO vpc = mock(VpcVO.class);
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.usesDistributedRouter()).thenReturn(true);
+ when(vpcManager.isProviderSupportServiceInVpc(VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(true);
+
+ assertTrue(manager.isOvsDistributedRouterVpc(VPC_ID));
+ }
+
+ @Test
+ public void testPostStateTransitionEventIgnoresNsxDistributedVpc() {
+ VpcVO vpc = mock(VpcVO.class);
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ @SuppressWarnings("unchecked")
+ StateMachine2.Transition<VirtualMachine.State, VirtualMachine.Event>
transition = mock(StateMachine2.Transition.class);
+ when(vm.getId()).thenReturn(11L);
+
when(topologyGuru.getVpcIdsVmIsPartOf(11L)).thenReturn(List.of(VPC_ID));
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.usesDistributedRouter()).thenReturn(true);
+ when(vpcManager.isProviderSupportServiceInVpc(VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(false);
+
when(transition.getCurrentState()).thenReturn(VirtualMachine.State.Starting);
+
when(transition.getEvent()).thenReturn(VirtualMachine.Event.OperationSucceeded);
+ when(transition.getToState()).thenReturn(VirtualMachine.State.Running);
+
+ assertTrue(manager.postStateTransitionEvent(transition, vm, true,
null));
+
+ verify(topologyGuru, never()).getVpcSpannedHosts(anyLong());
+ verify(vpcManager, never()).getVpcNetworks(anyLong());
+ }
+
+ @Test
+ public void testPostStateTransitionEventContinuesAfterNonOvsVpc() {
+ VpcVO firstVpc = mock(VpcVO.class);
+ VpcVO secondVpc = mock(VpcVO.class);
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ @SuppressWarnings("unchecked")
+ StateMachine2.Transition<VirtualMachine.State, VirtualMachine.Event>
transition = mock(StateMachine2.Transition.class);
+ when(vm.getId()).thenReturn(11L);
+ when(topologyGuru.getVpcIdsVmIsPartOf(11L)).thenReturn(List.of(VPC_ID,
SECOND_VPC_ID));
+ when(vpcDao.findById(VPC_ID)).thenReturn(firstVpc);
+ when(vpcDao.findById(SECOND_VPC_ID)).thenReturn(secondVpc);
+ when(firstVpc.usesDistributedRouter()).thenReturn(true);
+ when(secondVpc.usesDistributedRouter()).thenReturn(true);
+ when(vpcManager.isProviderSupportServiceInVpc(VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(false);
+ when(vpcManager.isProviderSupportServiceInVpc(SECOND_VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(false);
+
when(transition.getCurrentState()).thenReturn(VirtualMachine.State.Starting);
+
when(transition.getEvent()).thenReturn(VirtualMachine.Event.OperationSucceeded);
+ when(transition.getToState()).thenReturn(VirtualMachine.State.Running);
+
+ assertTrue(manager.postStateTransitionEvent(transition, vm, true,
null));
+
+ verify(vpcDao).findById(SECOND_VPC_ID);
+ }
+
+ @Test
+ public void
testPostStateTransitionEventContainsMalformedOvsTopologyAndContinues() {
+ VpcVO firstVpc = mock(VpcVO.class);
+ VpcVO secondVpc = mock(VpcVO.class);
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ @SuppressWarnings("unchecked")
+ StateMachine2.Transition<VirtualMachine.State, VirtualMachine.Event>
transition = mock(StateMachine2.Transition.class);
+ when(vm.getId()).thenReturn(11L);
+ when(topologyGuru.getVpcIdsVmIsPartOf(11L)).thenReturn(List.of(VPC_ID,
SECOND_VPC_ID));
+ when(vpcDao.findById(VPC_ID)).thenReturn(firstVpc);
+ when(vpcDao.findById(SECOND_VPC_ID)).thenReturn(secondVpc);
+ when(firstVpc.usesDistributedRouter()).thenReturn(true);
+ when(secondVpc.usesDistributedRouter()).thenReturn(true);
+ when(vpcManager.isProviderSupportServiceInVpc(VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(true);
+ when(vpcManager.isProviderSupportServiceInVpc(SECOND_VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(false);
+
when(transition.getCurrentState()).thenReturn(VirtualMachine.State.Starting);
+
when(transition.getEvent()).thenReturn(VirtualMachine.Event.OperationSucceeded);
+ when(transition.getToState()).thenReturn(VirtualMachine.State.Running);
+ Network malformedNetwork = mock(Network.class);
+
when(topologyGuru.getVpcSpannedHosts(VPC_ID)).thenReturn(Collections.emptyList());
+
when(topologyGuru.getAllActiveVmsInVpc(VPC_ID)).thenReturn(Collections.emptyList());
+
doReturn(List.of(malformedNetwork)).when(vpcManager).getVpcNetworks(VPC_ID);
+ when(firstVpc.getUuid()).thenReturn("vpc-uuid");
+ when(firstVpc.getCidr()).thenReturn("10.0.0.0/16");
+ when(malformedNetwork.getUuid()).thenReturn("network-uuid");
+
when(malformedNetwork.getBroadcastDomainType()).thenReturn(BroadcastDomainType.NSX);
+
+ assertTrue(manager.postStateTransitionEvent(transition, vm, true,
null));
+
+ verify(vpcDao).findById(SECOND_VPC_ID);
+ }
+
+ @Test
+ public void testPostStateTransitionEventBuildsTopologyForOvsVpc() {
+ VpcVO vpc = mock(VpcVO.class);
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ @SuppressWarnings("unchecked")
+ StateMachine2.Transition<VirtualMachine.State, VirtualMachine.Event>
transition = mock(StateMachine2.Transition.class);
+ when(vm.getId()).thenReturn(11L);
+
when(topologyGuru.getVpcIdsVmIsPartOf(11L)).thenReturn(List.of(VPC_ID));
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.usesDistributedRouter()).thenReturn(true);
+ when(vpc.getUuid()).thenReturn("vpc-uuid");
+ when(vpc.getCidr()).thenReturn("10.0.0.0/16");
+ when(vpcManager.isProviderSupportServiceInVpc(VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(true);
+
when(transition.getCurrentState()).thenReturn(VirtualMachine.State.Starting);
+
when(transition.getEvent()).thenReturn(VirtualMachine.Event.OperationSucceeded);
+ when(transition.getToState()).thenReturn(VirtualMachine.State.Running);
+
when(topologyGuru.getVpcSpannedHosts(VPC_ID)).thenReturn(Collections.emptyList());
+
when(topologyGuru.getAllActiveVmsInVpc(VPC_ID)).thenReturn(Collections.emptyList());
+
doReturn(Collections.emptyList()).when(vpcManager).getVpcNetworks(VPC_ID);
+ prepareSequenceNumber(VPC_ID);
+
+ assertTrue(manager.postStateTransitionEvent(transition, vm, true,
null));
+
+ verify(vpcManager).getVpcNetworks(VPC_ID);
+ verify(manager._vpcDrSeqNoDao).update(1L, sequenceNumber);
+ }
+
+ @Test
+ public void testNetworkAclSubscriberIgnoresNsxDistributedVpc() {
+ VpcVO vpc = mock(VpcVO.class);
+ NetworkVO network = mock(NetworkVO.class);
+ when(network.getVpcId()).thenReturn(VPC_ID);
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.usesDistributedRouter()).thenReturn(true);
+ when(vpcManager.isProviderSupportServiceInVpc(VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(false);
+
+ manager.new NetworkAclEventsSubscriber().onPublishMessage("sender",
"Network_ACL_Replaced", network);
+
+ verify(topologyGuru, never()).getVpcSpannedHosts(anyLong());
+ verify(vpcManager, never()).getVpcNetworks(anyLong());
+ }
+
+ @Test
+ public void testNetworkAclSubscriberBuildsPolicyForOvsVpc() {
+ VpcVO vpc = mock(VpcVO.class);
+ NetworkVO network = mock(NetworkVO.class);
+ when(network.getVpcId()).thenReturn(VPC_ID);
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.usesDistributedRouter()).thenReturn(true);
+ when(vpc.getUuid()).thenReturn("vpc-uuid");
+ when(vpc.getCidr()).thenReturn("10.0.0.0/16");
+ when(vpcManager.isProviderSupportServiceInVpc(VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(true);
+ doReturn(List.of(network)).when(vpcManager).getVpcNetworks(VPC_ID);
+ when(network.getNetworkACLId()).thenReturn(null);
+
when(topologyGuru.getVpcSpannedHosts(VPC_ID)).thenReturn(Collections.emptyList());
+ prepareSequenceNumber(VPC_ID);
+
+ manager.new NetworkAclEventsSubscriber().onPublishMessage("sender",
"Network_ACL_Replaced", network);
+
+ verify(vpcManager).getVpcNetworks(VPC_ID);
+ verify(manager._vpcDrSeqNoDao).update(1L, sequenceNumber);
+ }
+
+ @Test
+ public void testPrepareVpcTopologyUpdateRejectsNonVswitchTier() {
+ VpcVO vpc = mock(VpcVO.class);
+ Network network = mock(Network.class);
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.getUuid()).thenReturn("vpc-uuid");
+ doReturn(List.of(network)).when(vpcManager).getVpcNetworks(VPC_ID);
+
when(topologyGuru.getVpcSpannedHosts(VPC_ID)).thenReturn(Collections.emptyList());
+
when(topologyGuru.getAllActiveVmsInVpc(VPC_ID)).thenReturn(Collections.emptyList());
+ when(network.getUuid()).thenReturn("network-uuid");
+
when(network.getBroadcastDomainType()).thenReturn(BroadcastDomainType.NSX);
+
+ assertThrows(CloudRuntimeException.class, () ->
manager.prepareVpcTopologyUpdate(VPC_ID));
+ }
+
+ @Test
+ public void testPrepareVpcTopologyUpdateRejectsBroadcastKeyForAnotherVpc()
{
+ Network network = prepareVswitchNetwork("8.123");
+
+ assertThrows(CloudRuntimeException.class, () ->
manager.prepareVpcTopologyUpdate(VPC_ID));
+
+ verify(nicDao, never()).findByIp4AddressAndNetworkId("10.0.1.1", 13L);
+ }
+
+ @Test
+ public void testPrepareVpcTopologyUpdateRejectsNonNumericGreKey() {
+ prepareVswitchNetwork("7.invalid");
+
+ assertThrows(CloudRuntimeException.class, () ->
manager.prepareVpcTopologyUpdate(VPC_ID));
+ }
+
+ @Test
+ public void
testPrepareVpcTopologyUpdateRejectsRepeatedDelimiterInBroadcastKey() {
+ prepareVswitchNetwork("7..123");
+
+ assertThrows(CloudRuntimeException.class, () ->
manager.prepareVpcTopologyUpdate(VPC_ID));
+ }
+
+ @Test
+ public void
testPrepareVpcTopologyUpdateRejectsLeadingDelimiterInBroadcastKey() {
+ prepareVswitchNetwork(".7.123");
+
+ assertThrows(CloudRuntimeException.class, () ->
manager.prepareVpcTopologyUpdate(VPC_ID));
+ }
+
+ @Test
+ public void
testPrepareVpcTopologyUpdateRejectsTrailingDelimiterInBroadcastKey() {
+ prepareVswitchNetwork("7.123.");
+
+ assertThrows(CloudRuntimeException.class, () ->
manager.prepareVpcTopologyUpdate(VPC_ID));
+ }
+
+ @Test
+ public void testPrepareVpcTopologyUpdateRejectsGreKeyOutsideIntegerRange()
{
+ prepareVswitchNetwork("7.2147483648");
+
+ assertThrows(CloudRuntimeException.class, () ->
manager.prepareVpcTopologyUpdate(VPC_ID));
+ }
Review Comment:
This test encodes the current (incorrect) behavior that GRE keys above
`Integer.MAX_VALUE` are rejected. Once `greKey` parsing is corrected to support
unsigned 32-bit values, update this test to assert acceptance (and add boundary
tests for `4294967295` and out-of-range `4294967296`, plus negative/zero if
disallowed) so validation matches the GRE key domain.
##########
plugins/network-elements/ovs/src/test/java/com/cloud/network/ovs/OvsTunnelManagerImplTest.java:
##########
@@ -0,0 +1,379 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.network.ovs;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import com.cloud.agent.AgentManager;
+import com.cloud.agent.api.OvsVpcPhysicalTopologyConfigCommand;
+import com.cloud.host.dao.HostDao;
+import com.cloud.network.Network;
+import com.cloud.network.Networks.BroadcastDomainType;
+import com.cloud.network.dao.NetworkDao;
+import com.cloud.network.dao.NetworkVO;
+import com.cloud.network.ovs.dao.VpcDistributedRouterSeqNoDao;
+import com.cloud.network.ovs.dao.VpcDistributedRouterSeqNoVO;
+import com.cloud.network.vpc.VpcManager;
+import com.cloud.network.vpc.VpcVO;
+import com.cloud.network.vpc.dao.VpcDao;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.fsm.StateMachine2;
+import com.cloud.vm.NicVO;
+import com.cloud.vm.VMInstanceVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.dao.NicDao;
+import com.cloud.vm.dao.VMInstanceDao;
+
+public class OvsTunnelManagerImplTest {
+ private static final long VPC_ID = 7L;
+ private static final long SECOND_VPC_ID = 8L;
+
+ private OvsTunnelManagerImpl manager;
+ private VpcDao vpcDao;
+ private VpcManager vpcManager;
+ private OvsNetworkTopologyGuru topologyGuru;
+ private NicDao nicDao;
+ private VpcDistributedRouterSeqNoVO sequenceNumber;
+
+ @Before
+ public void setUp() {
+ manager = new OvsTunnelManagerImpl();
+ vpcDao = mock(VpcDao.class);
+ vpcManager = mock(VpcManager.class);
+ topologyGuru = mock(OvsNetworkTopologyGuru.class);
+ nicDao = mock(NicDao.class);
+ manager._vpcDao = vpcDao;
+ manager._vpcMgr = vpcManager;
+ manager._ovsNetworkToplogyGuru = topologyGuru;
+ manager._nicDao = nicDao;
+ manager._hostDao = mock(HostDao.class);
+ manager._vmInstanceDao = mock(VMInstanceDao.class);
+ manager._networkDao = mock(NetworkDao.class);
+ manager._vpcDrSeqNoDao = mock(VpcDistributedRouterSeqNoDao.class);
+ manager._agentMgr = mock(AgentManager.class);
+ }
+
+ @Test
+ public void testIsOvsDistributedRouterVpcReturnsFalseWhenVpcIsMissing() {
+ assertFalse(manager.isOvsDistributedRouterVpc(VPC_ID));
+ verify(vpcManager, never()).isProviderSupportServiceInVpc(anyLong(),
+ org.mockito.ArgumentMatchers.any(Network.Service.class),
+ org.mockito.ArgumentMatchers.any(Network.Provider.class));
+ }
+
+ @Test
+ public void
testIsOvsDistributedRouterVpcReturnsFalseWhenVpcIsNotDistributed() {
+ VpcVO vpc = mock(VpcVO.class);
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.usesDistributedRouter()).thenReturn(false);
+
+ assertFalse(manager.isOvsDistributedRouterVpc(VPC_ID));
+ }
+
+ @Test
+ public void
testIsOvsDistributedRouterVpcReturnsFalseForNsxDistributedVpc() {
+ VpcVO vpc = mock(VpcVO.class);
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.usesDistributedRouter()).thenReturn(true);
+ when(vpcManager.isProviderSupportServiceInVpc(VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(false);
+
+ assertFalse(manager.isOvsDistributedRouterVpc(VPC_ID));
+ }
+
+ @Test
+ public void
testIsOvsDistributedRouterVpcReturnsTrueForOvsConnectivityDistributedVpc() {
+ VpcVO vpc = mock(VpcVO.class);
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.usesDistributedRouter()).thenReturn(true);
+ when(vpcManager.isProviderSupportServiceInVpc(VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(true);
+
+ assertTrue(manager.isOvsDistributedRouterVpc(VPC_ID));
+ }
+
+ @Test
+ public void testPostStateTransitionEventIgnoresNsxDistributedVpc() {
+ VpcVO vpc = mock(VpcVO.class);
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ @SuppressWarnings("unchecked")
+ StateMachine2.Transition<VirtualMachine.State, VirtualMachine.Event>
transition = mock(StateMachine2.Transition.class);
+ when(vm.getId()).thenReturn(11L);
+
when(topologyGuru.getVpcIdsVmIsPartOf(11L)).thenReturn(List.of(VPC_ID));
+ when(vpcDao.findById(VPC_ID)).thenReturn(vpc);
+ when(vpc.usesDistributedRouter()).thenReturn(true);
+ when(vpcManager.isProviderSupportServiceInVpc(VPC_ID,
Network.Service.Connectivity, Network.Provider.Ovs))
+ .thenReturn(false);
+
when(transition.getCurrentState()).thenReturn(VirtualMachine.State.Starting);
+
when(transition.getEvent()).thenReturn(VirtualMachine.Event.OperationSucceeded);
+ when(transition.getToState()).thenReturn(VirtualMachine.State.Running);
+
+ assertTrue(manager.postStateTransitionEvent(transition, vm, true,
null));
+
+ verify(topologyGuru, never()).getVpcSpannedHosts(anyLong());
+ verify(vpcManager, never()).getVpcNetworks(anyLong());
+ }
+
+ @Test
+ public void testPostStateTransitionEventContinuesAfterNonOvsVpc() {
+ VpcVO firstVpc = mock(VpcVO.class);
+ VpcVO secondVpc = mock(VpcVO.class);
Review Comment:
There’s coverage for continuation after a non-OVS VPC and after malformed
topology data, but there’s no test ensuring the loop continues when the
provider/VPC eligibility lookup itself throws (the code currently performs that
lookup before the `try/catch`). Add a test where the first VPC causes
`_vpcMgr.isProviderSupportServiceInVpc(...)` (or `_vpcDao.findById(...)`) to
throw, and assert the second (valid) VPC still reaches topology
generation/sending.
##########
plugins/network-elements/ovs/src/main/java/com/cloud/network/ovs/OvsTunnelManagerImpl.java:
##########
@@ -754,20 +763,32 @@ OvsVpcPhysicalTopologyConfigCommand
prepareVpcTopologyUpdate(long vpcId) {
}
for (Network network: vpcNetworks) {
+ if (network.getBroadcastDomainType() !=
BroadcastDomainType.Vswitch || network.getBroadcastUri() == null) {
+ throw new CloudRuntimeException(String.format(
+ "OVS distributed-router VPC %s contains network %s
without a Vswitch broadcast URI",
+ vpc.getUuid(), network.getUuid()));
+ }
String key = network.getBroadcastUri().getAuthority();
- long gre_key;
- if (key.contains(".")) {
- String[] parts = key.split("\\.");
- gre_key = Long.parseLong(parts[1]);
- } else {
- try {
- gre_key =
Long.parseLong(BroadcastDomainType.getValue(key));
- } catch (Exception e) {
- return null;
- }
+ String expectedPrefix = vpcId + ".";
+ if (key == null || !key.startsWith(expectedPrefix) ||
key.indexOf('.', expectedPrefix.length()) >= 0) {
+ throw new CloudRuntimeException(String.format(
+ "OVS distributed-router network %s has invalid
broadcast key %s for VPC %s",
+ network.getUuid(), key, vpc.getUuid()));
+ }
+ int greKey;
+ try {
+ greKey =
Integer.parseInt(key.substring(expectedPrefix.length()));
+ } catch (NumberFormatException e) {
+ throw new CloudRuntimeException(String.format(
+ "OVS distributed-router network %s has non-numeric GRE
key %s",
+ network.getUuid(),
key.substring(expectedPrefix.length())), e);
Review Comment:
`greKey` is parsed as `int`, which rejects valid 32-bit unsigned GRE keys in
the range 2147483648–4294967295 and also allows negative values. Parse as
`long` (e.g., `Long.parseLong`) and explicitly validate the value is within the
unsigned 32-bit GRE key range (commonly 1..4294967295, or 0..4294967295 if 0 is
allowed), then pass the `long` into `Tier` so keys above `Integer.MAX_VALUE`
are accepted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
