ONOS-6164 Fix the add and remove command to the virtual network store
We fix the add and remove command to be more semantic:
1. When remove a virtual network element, all the other elements
that depend on it should also be removed.
2. Virtual link and virtual host should not be created until the virtual
ports they depend on are created.
3. Some bugs about NullPointerException are fixed.
Change-Id: I0346dfb54dbb9a388cd4a39637ee57601fecff02
diff --git a/incubator/net/src/test/java/org/onosproject/incubator/net/virtual/impl/VirtualNetworkHostManagerTest.java b/incubator/net/src/test/java/org/onosproject/incubator/net/virtual/impl/VirtualNetworkHostManagerTest.java
index 01611f3..d585138 100644
--- a/incubator/net/src/test/java/org/onosproject/incubator/net/virtual/impl/VirtualNetworkHostManagerTest.java
+++ b/incubator/net/src/test/java/org/onosproject/incubator/net/virtual/impl/VirtualNetworkHostManagerTest.java
@@ -25,6 +25,7 @@
import org.onosproject.common.event.impl.TestEventDispatcher;
import org.onosproject.core.CoreService;
import org.onosproject.incubator.net.virtual.TenantId;
+import org.onosproject.incubator.net.virtual.VirtualDevice;
import org.onosproject.incubator.net.virtual.VirtualHost;
import org.onosproject.incubator.net.virtual.VirtualNetwork;
import org.onosproject.incubator.store.virtual.impl.DistributedVirtualNetworkStore;
@@ -90,6 +91,19 @@
private VirtualNetwork setupVnet() {
manager.registerTenantId(TenantId.tenantId(tenantIdValue1));
VirtualNetwork virtualNetwork = manager.createVirtualNetwork(TenantId.tenantId(tenantIdValue1));
+
+ VirtualDevice virtualDevice1 =
+ manager.createVirtualDevice(virtualNetwork.id(), DID1);
+ VirtualDevice virtualDevice2 =
+ manager.createVirtualDevice(virtualNetwork.id(), DID2);
+
+ ConnectPoint hostCp1 = new ConnectPoint(DID1, P1);
+ ConnectPoint hostCp2 = new ConnectPoint(DID2, P2);
+ manager.createVirtualPort(virtualNetwork.id(), hostCp1.deviceId(), hostCp1.port(),
+ new ConnectPoint(virtualDevice1.id(), hostCp1.port()));
+ manager.createVirtualPort(virtualNetwork.id(), hostCp2.deviceId(), hostCp2.port(),
+ new ConnectPoint(virtualDevice2.id(), hostCp2.port()));
+
manager.createVirtualHost(virtualNetwork.id(), HID1, MAC1, VLAN1, LOC1, IPSET1);
manager.createVirtualHost(virtualNetwork.id(), HID2, MAC2, VLAN2, LOC2, IPSET2);
return virtualNetwork;
@@ -102,7 +116,21 @@
*/
private VirtualNetwork setupEmptyVnet() {
manager.registerTenantId(TenantId.tenantId(tenantIdValue1));
- return manager.createVirtualNetwork(TenantId.tenantId(tenantIdValue1));
+ VirtualNetwork virtualNetwork = manager.createVirtualNetwork(TenantId.tenantId(tenantIdValue1));
+
+ VirtualDevice virtualDevice1 =
+ manager.createVirtualDevice(virtualNetwork.id(), DID1);
+ VirtualDevice virtualDevice2 =
+ manager.createVirtualDevice(virtualNetwork.id(), DID2);
+
+ ConnectPoint hostCp1 = new ConnectPoint(DID1, P1);
+ ConnectPoint hostCp2 = new ConnectPoint(DID2, P2);
+ manager.createVirtualPort(virtualNetwork.id(), hostCp1.deviceId(), hostCp1.port(),
+ new ConnectPoint(virtualDevice1.id(), hostCp1.port()));
+ manager.createVirtualPort(virtualNetwork.id(), hostCp2.deviceId(), hostCp2.port(),
+ new ConnectPoint(virtualDevice2.id(), hostCp2.port()));
+
+ return virtualNetwork;
}
/**
diff --git a/incubator/net/src/test/java/org/onosproject/incubator/net/virtual/impl/VirtualNetworkManagerTest.java b/incubator/net/src/test/java/org/onosproject/incubator/net/virtual/impl/VirtualNetworkManagerTest.java
index 2d125d1..46b717e 100644
--- a/incubator/net/src/test/java/org/onosproject/incubator/net/virtual/impl/VirtualNetworkManagerTest.java
+++ b/incubator/net/src/test/java/org/onosproject/incubator/net/virtual/impl/VirtualNetworkManagerTest.java
@@ -427,6 +427,17 @@
}
/**
+ * Tests adding a virtual host where no virtual port exists.
+ */
+ @Test(expected = IllegalStateException.class)
+ public void testCreateVirtualHostWithNoVirtualPort() {
+ manager.registerTenantId(TenantId.tenantId(tenantIdValue1));
+ VirtualNetwork virtualNetwork1 =
+ manager.createVirtualNetwork(TenantId.tenantId(tenantIdValue1));
+ manager.createVirtualHost(virtualNetwork1.id(), HID1, MAC1, VLAN1, LOC1, IPSET1);
+ }
+
+ /**
* Tests add and remove of virtual hosts.
*/
@Test
@@ -436,6 +447,19 @@
manager.createVirtualNetwork(TenantId.tenantId(tenantIdValue1));
VirtualNetwork virtualNetwork2 =
manager.createVirtualNetwork(TenantId.tenantId(tenantIdValue1));
+
+ VirtualDevice virtualDevice1 =
+ manager.createVirtualDevice(virtualNetwork1.id(), DID1);
+ VirtualDevice virtualDevice2 =
+ manager.createVirtualDevice(virtualNetwork2.id(), DID2);
+
+ ConnectPoint hostCp1 = new ConnectPoint(DID1, P1);
+ ConnectPoint hostCp2 = new ConnectPoint(DID2, P2);
+ manager.createVirtualPort(virtualNetwork1.id(), hostCp1.deviceId(), hostCp1.port(),
+ new ConnectPoint(virtualDevice1.id(), hostCp1.port()));
+ manager.createVirtualPort(virtualNetwork2.id(), hostCp2.deviceId(), hostCp2.port(),
+ new ConnectPoint(virtualDevice2.id(), hostCp2.port()));
+
manager.createVirtualHost(virtualNetwork1.id(), HID1, MAC1, VLAN1, LOC1, IPSET1);
manager.createVirtualHost(virtualNetwork2.id(), HID2, MAC2, VLAN2, LOC2, IPSET2);
@@ -592,6 +616,70 @@
}
/**
+ * Tests when a virtual element is removed, all the other elements depending on it are also removed.
+ */
+ @Test
+ public void testRemoveAllElements() {
+ manager.registerTenantId(TenantId.tenantId(tenantIdValue1));
+ VirtualNetwork virtualNetwork1 =
+ manager.createVirtualNetwork(TenantId.tenantId(tenantIdValue1));
+ VirtualDevice virtualDevice1 =
+ manager.createVirtualDevice(virtualNetwork1.id(), DID1);
+ VirtualDevice virtualDevice2 =
+ manager.createVirtualDevice(virtualNetwork1.id(), DID2);
+ ConnectPoint src = new ConnectPoint(virtualDevice1.id(), PortNumber.portNumber(1));
+ manager.createVirtualPort(virtualNetwork1.id(), src.deviceId(), src.port(),
+ new ConnectPoint(PHYDID1, PortNumber.portNumber(1)));
+
+ ConnectPoint dst = new ConnectPoint(virtualDevice2.id(), PortNumber.portNumber(2));
+ manager.createVirtualPort(virtualNetwork1.id(), dst.deviceId(), dst.port(),
+ new ConnectPoint(PHYDID2, PortNumber.portNumber(2)));
+
+ manager.createVirtualLink(virtualNetwork1.id(), src, dst);
+ manager.createVirtualLink(virtualNetwork1.id(), dst, src);
+
+ ConnectPoint hostCp = new ConnectPoint(DID1, P1);
+ manager.createVirtualPort(virtualNetwork1.id(), hostCp.deviceId(), hostCp.port(),
+ new ConnectPoint(PHYDID1, P1));
+ manager.createVirtualHost(virtualNetwork1.id(), HID1, MAC1, VLAN1, LOC1, IPSET1);
+
+ //When a virtual port is removed, all virtual links connected to it should also be removed.
+ manager.removeVirtualPort(virtualNetwork1.id(), DID1, PortNumber.portNumber(1));
+ Set<VirtualLink> virtualLinks = manager.getVirtualLinks(virtualNetwork1.id());
+ assertTrue("The virtual link set should be empty.", virtualLinks.isEmpty());
+
+ //When a virtual port is removed, all virtual hosts located to it should also be removed.
+ manager.removeVirtualPort(virtualNetwork1.id(), DID1, P1);
+ Set<VirtualHost> virtualHosts = manager.getVirtualHosts(virtualNetwork1.id());
+ assertTrue("The virtual host set should be empty.", virtualHosts.isEmpty());
+
+ manager.createVirtualPort(virtualNetwork1.id(), src.deviceId(), src.port(),
+ new ConnectPoint(PHYDID1, PortNumber.portNumber(1)));
+ manager.createVirtualLink(virtualNetwork1.id(), src, dst);
+ manager.createVirtualLink(virtualNetwork1.id(), dst, src);
+ manager.createVirtualPort(virtualNetwork1.id(), hostCp.deviceId(), hostCp.port(),
+ new ConnectPoint(PHYDID1, P1));
+ manager.createVirtualHost(virtualNetwork1.id(), HID1, MAC1, VLAN1, LOC1, IPSET1);
+
+ //When a virtual device is removed, all virtual ports, hosts and links depended on it should also be removed.
+ manager.removeVirtualDevice(virtualNetwork1.id(), DID1);
+ Set<VirtualPort> virtualPorts = manager.getVirtualPorts(virtualNetwork1.id(), DID1);
+ assertTrue("The virtual port set of DID1 should be empty", virtualPorts.isEmpty());
+ virtualLinks = manager.getVirtualLinks(virtualNetwork1.id());
+ assertTrue("The virtual link set should be empty.", virtualLinks.isEmpty());
+ virtualHosts = manager.getVirtualHosts(virtualNetwork1.id());
+ assertTrue("The virtual host set should be empty.", virtualHosts.isEmpty());
+
+ //When a tenantId is removed, all the virtual networks belonging to it should also be removed.
+ manager.unregisterTenantId(TenantId.tenantId(tenantIdValue1));
+ manager.registerTenantId(TenantId.tenantId(tenantIdValue1));
+ Set<VirtualNetwork> virtualNetworks = manager.getVirtualNetworks(TenantId.tenantId(tenantIdValue1));
+ assertNotNull("The virtual network set should not be null", virtualNetworks);
+ assertTrue("The virtual network set should be empty.", virtualNetworks.isEmpty());
+ }
+
+
+ /**
* Tests the addOrUpdateIntent() method in the store with a null intent.
*/
@Test(expected = NullPointerException.class)
diff --git a/incubator/store/src/main/java/org/onosproject/incubator/store/virtual/impl/DistributedVirtualNetworkStore.java b/incubator/store/src/main/java/org/onosproject/incubator/store/virtual/impl/DistributedVirtualNetworkStore.java
index f525028..4b79cf5 100644
--- a/incubator/store/src/main/java/org/onosproject/incubator/store/virtual/impl/DistributedVirtualNetworkStore.java
+++ b/incubator/store/src/main/java/org/onosproject/incubator/store/virtual/impl/DistributedVirtualNetworkStore.java
@@ -73,6 +73,7 @@
import java.util.HashSet;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.BiFunction;
@@ -303,6 +304,12 @@
@Override
public void removeTenantId(TenantId tenantId) {
+ //Remove all the virtual networks of this tenant
+ Set<VirtualNetwork> networkIdSet = getNetworks(tenantId);
+ if (networkIdSet != null) {
+ networkIdSet.forEach(virtualNetwork -> removeNetwork(virtualNetwork.id()));
+ }
+
tenantIdSet.remove(tenantId);
}
@@ -347,6 +354,11 @@
public void removeNetwork(NetworkId networkId) {
// Make sure that the virtual network exists before attempting to remove it.
if (networkExists(networkId)) {
+ //Remove all the devices of this network
+ Set<VirtualDevice> deviceSet = getDevices(networkId);
+ if (deviceSet != null) {
+ deviceSet.forEach(virtualDevice -> removeDevice(networkId, virtualDevice.id()));
+ }
//TODO update both maps in one transaction.
VirtualNetwork virtualNetwork = networkIdVirtualNetworkMap.remove(networkId);
@@ -401,6 +413,11 @@
@Override
public void removeDevice(NetworkId networkId, DeviceId deviceId) {
checkState(networkExists(networkId), "The network has not been added.");
+ //Remove all the virtual ports of the this device
+ Set<VirtualPort> virtualPorts = getPorts(networkId, deviceId);
+ if (virtualPorts != null) {
+ virtualPorts.forEach(virtualPort -> removePort(networkId, deviceId, virtualPort.number()));
+ }
//TODO update both maps in one transaction.
Set<DeviceId> deviceIdSet = new HashSet<>();
@@ -421,13 +438,14 @@
deviceIdVirtualDeviceMap.remove(deviceId);
}
- //TODO remove virtual links and ports when removing the virtual device
}
@Override
public VirtualHost addHost(NetworkId networkId, HostId hostId, MacAddress mac,
VlanId vlan, HostLocation location, Set<IpAddress> ips) {
checkState(networkExists(networkId), "The network has not been added.");
+ checkState(virtualPortExists(networkId, location.deviceId(), location.port()),
+ "The virtual port has not been created.");
Set<HostId> hostIdSet = networkIdHostIdSetMap.get(networkId);
if (hostIdSet == null) {
hostIdSet = new HashSet<>();
@@ -465,10 +483,33 @@
}
}
+ /**
+ * Returns if the given virtual port exists.
+ *
+ * @param networkId network identifier
+ * @param deviceId virtual device Id
+ * @param portNumber virtual port number
+ * @return true if the virtual port exists, false otherwise.
+ */
+ private boolean virtualPortExists(NetworkId networkId, DeviceId deviceId, PortNumber portNumber) {
+ Set<VirtualPort> virtualPortSet = networkIdVirtualPortSetMap.get(networkId);
+ if (virtualPortSet != null) {
+ return virtualPortSet.stream().anyMatch(
+ p -> p.element().id().equals(deviceId) &&
+ p.number().equals(portNumber));
+ } else {
+ return false;
+ }
+ }
+
@Override
public VirtualLink addLink(NetworkId networkId, ConnectPoint src, ConnectPoint dst,
Link.State state, TunnelId realizedBy) {
checkState(networkExists(networkId), "The network has not been added.");
+ checkState(virtualPortExists(networkId, src.deviceId(), src.port()),
+ "The source virtual port has not been added.");
+ checkState(virtualPortExists(networkId, dst.deviceId(), dst.port()),
+ "The destination virtual port has not been added.");
Set<VirtualLink> virtualLinkSet = networkIdVirtualLinkSetMap.get(networkId);
if (virtualLinkSet == null) {
virtualLinkSet = new HashSet<>();
@@ -546,10 +587,8 @@
VirtualDevice device = deviceIdVirtualDeviceMap.get(deviceId);
checkNotNull(device, "The device has not been created for deviceId: " + deviceId);
- boolean exist = virtualPortSet.stream().anyMatch(
- p -> p.element().id().equals(deviceId) &&
- p.number().equals(portNumber));
- checkState(!exist, "The requested Port Number is already in use");
+ checkState(!virtualPortExists(networkId, deviceId, portNumber),
+ "The requested Port Number has been added.");
VirtualPort virtualPort = new DefaultVirtualPort(networkId, device,
portNumber, realizedBy);
@@ -567,15 +606,16 @@
Set<VirtualPort> virtualPortSet = networkIdVirtualPortSetMap
.get(networkId);
- VirtualPort vPort = virtualPortSet.stream().filter(
+ Optional<VirtualPort> virtualPortOptional = virtualPortSet.stream().filter(
p -> p.element().id().equals(deviceId) &&
- p.number().equals(portNumber)).findFirst().get();
- checkNotNull(vPort, "The virtual port has not been added.");
+ p.number().equals(portNumber)).findFirst();
+ checkState(virtualPortOptional.isPresent(), "The virtual port has not been added.");
VirtualDevice device = deviceIdVirtualDeviceMap.get(deviceId);
checkNotNull(device, "The device has not been created for deviceId: "
+ deviceId);
+ VirtualPort vPort = virtualPortOptional.get();
virtualPortSet.remove(vPort);
vPort = new DefaultVirtualPort(networkId, device, portNumber, realizedBy);
virtualPortSet.add(vPort);
@@ -591,6 +631,11 @@
checkNotNull(device, "The device has not been created for deviceId: "
+ deviceId);
+ if (networkIdVirtualPortSetMap.get(networkId) == null) {
+ log.warn("No port has been created for NetworkId: {}", networkId);
+ return;
+ }
+
Set<VirtualPort> virtualPortSet = new HashSet<>();
networkIdVirtualPortSetMap.get(networkId).forEach(port -> {
if (port.element().id().equals(deviceId) && port.number().equals(portNumber)) {
@@ -613,6 +658,30 @@
new VirtualNetworkEvent(VirtualNetworkEvent.Type.VIRTUAL_PORT_REMOVED,
networkId, device, virtualPort)
));
+
+ //Remove all the virtual links connected to this virtual port
+ Set<VirtualLink> existingVirtualLinks = networkIdVirtualLinkSetMap.get(networkId);
+ if (existingVirtualLinks != null && !existingVirtualLinks.isEmpty()) {
+ Set<VirtualLink> virtualLinkSet = new HashSet<>();
+ ConnectPoint cp = new ConnectPoint(deviceId, portNumber);
+ existingVirtualLinks.forEach(virtualLink -> {
+ if (virtualLink.src().equals(cp) || virtualLink.dst().equals(cp)) {
+ virtualLinkSet.add(virtualLink);
+ }
+ });
+ virtualLinkSet.forEach(virtualLink ->
+ removeLink(networkId, virtualLink.src(), virtualLink.dst()));
+ }
+
+ //Remove all the hosts connected to this virtual port
+ Set<HostId> hostIdSet = new HashSet<>();
+ hostIdVirtualHostMap.forEach((hostId, virtualHost) -> {
+ if (virtualHost.location().deviceId().equals(deviceId) &&
+ virtualHost.location().port().equals(portNumber)) {
+ hostIdSet.add(hostId);
+ }
+ });
+ hostIdSet.forEach(hostId -> removeHost(networkId, hostId));
}
}
}