[VOL-4497] Do not update cpStatus map if the cp has been removed during device cleanup
Change-Id: I6c97ccb04dcf8879a297f620629bd7a117a8ed3b
diff --git a/impl/src/main/java/org/opencord/olt/impl/OltFlowService.java b/impl/src/main/java/org/opencord/olt/impl/OltFlowService.java
index f4e859f..64d0745 100644
--- a/impl/src/main/java/org/opencord/olt/impl/OltFlowService.java
+++ b/impl/src/main/java/org/opencord/olt/impl/OltFlowService.java
@@ -82,9 +82,11 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.util.ArrayList;
import java.util.Dictionary;
import java.util.HashMap;
import java.util.Iterator;
+import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Properties;
@@ -381,7 +383,7 @@
@Override
public ImmutableMap<ServiceKey, UniTagInformation> getProgrammedSubscribers() {
// NOTE we might want to remove this conversion and directly use cpStatus as it contains
- // all the required information about suscribers
+ // all the required information about subscribers
Map<ServiceKey, UniTagInformation> subscribers =
new HashMap<>();
try {
@@ -394,6 +396,7 @@
sk.getPort().connectPoint().port()) &&
// not EAPOL flow
!sk.getService().equals(defaultEapolUniTag)
+
) {
subscribers.put(sk, sk.getService());
}
@@ -656,9 +659,16 @@
@Override
public void purgeDeviceFlows(DeviceId deviceId) {
log.debug("Purging flows on device {}", deviceId);
- flowRuleService.purgeFlowRules(deviceId);
+ try {
+ flowRuleService.purgeFlowRules(deviceId);
+ } catch (Exception e) {
+ log.error("Cannot purge flow rules", e);
+ }
// removing the status from the cpStatus map
+ if (log.isTraceEnabled()) {
+ log.trace("Clearing cp status from device {}", deviceId);
+ }
try {
cpStatusWriteLock.lock();
Iterator<Map.Entry<ServiceKey, OltPortStatus>> iter = cpStatus.entrySet().iterator();
@@ -668,11 +678,16 @@
cpStatus.remove(entry.getKey());
}
}
+ } catch (Exception e) {
+ log.error("Cannot wipe out cpStatus", e);
} finally {
cpStatusWriteLock.unlock();
}
// removing subscribers from the provisioned map
+ if (log.isTraceEnabled()) {
+ log.trace("Clearing provisioned subscribers from device {}", deviceId);
+ }
try {
provisionedSubscribersWriteLock.lock();
Iterator<Map.Entry<ServiceKey, Boolean>> iter = provisionedSubscribers.entrySet().iterator();
@@ -682,9 +697,12 @@
provisionedSubscribers.remove(entry.getKey());
}
}
+ } catch (Exception e) {
+ log.error("Cannot wipe out subscribers", e);
} finally {
provisionedSubscribersWriteLock.unlock();
}
+ log.debug("Done clearing up device flows and subscribers");
}
@Override
@@ -1473,11 +1491,34 @@
protected void updateConnectPointStatus(ServiceKey key, OltFlowsStatus eapolStatus,
OltFlowsStatus subscriberFlowsStatus, OltFlowsStatus dhcpStatus) {
+ if (log.isTraceEnabled()) {
+ log.trace("Updating cpStatus {} with values: eapolFlow={}, subscriberFlows={}, dhcpFlow={}",
+ key, eapolStatus, subscriberFlowsStatus, dhcpStatus);
+ }
try {
cpStatusWriteLock.lock();
OltPortStatus status = cpStatus.get(key);
+
if (status == null) {
+ // if we don't have status for the connectPoint
+ // and we're only updating status to PENDING_REMOVE or ERROR
+ // do not create it. This is because this case will only happen when a device is removed
+ // and it's status cleaned
+ List<OltFlowsStatus> statusesToIgnore = new ArrayList<>();
+ statusesToIgnore.add(OltFlowsStatus.PENDING_REMOVE);
+ statusesToIgnore.add(OltFlowsStatus.ERROR);
+
+ if (
+ (statusesToIgnore.contains(subscriberFlowsStatus) && dhcpStatus == null) ||
+ (subscriberFlowsStatus == null && statusesToIgnore.contains(dhcpStatus))
+ ) {
+ if (log.isTraceEnabled()) {
+ log.trace("Ignoring cpStatus update as status is meaningless");
+ }
+ return;
+ }
+
status = new OltPortStatus(
eapolStatus != null ? eapolStatus : OltFlowsStatus.NONE,
subscriberFlowsStatus != null ? subscriberFlowsStatus : OltFlowsStatus.NONE,
diff --git a/impl/src/test/java/org/opencord/olt/impl/OltFlowServiceTest.java b/impl/src/test/java/org/opencord/olt/impl/OltFlowServiceTest.java
index c9bad5f..39f764e 100644
--- a/impl/src/test/java/org/opencord/olt/impl/OltFlowServiceTest.java
+++ b/impl/src/test/java/org/opencord/olt/impl/OltFlowServiceTest.java
@@ -77,9 +77,11 @@
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.onosproject.net.AnnotationKeys.PORT_NAME;
+import static org.opencord.olt.impl.OltFlowService.OltFlowsStatus.ERROR;
import static org.opencord.olt.impl.OltFlowService.OltFlowsStatus.NONE;
import static org.opencord.olt.impl.OltFlowService.OltFlowsStatus.ADDED;
import static org.opencord.olt.impl.OltFlowService.OltFlowsStatus.PENDING_ADD;
+import static org.opencord.olt.impl.OltFlowService.OltFlowsStatus.PENDING_REMOVE;
import static org.opencord.olt.impl.OltFlowService.OltFlowsStatus.REMOVED;
import static org.opencord.olt.impl.OsgiPropertyConstants.DEFAULT_BP_ID_DEFAULT;
@@ -171,6 +173,38 @@
Assert.assertEquals(NONE, updated.dhcpStatus);
}
+ /**
+ * If the flow status is PENDING_REMOVE or ERROR and there is no
+ * previous state in the map that don't update it.
+ * In case of a device disconnection we immediately wipe out the status,
+ * but then flows might update the cpStatus map. That result
+ */
+ @Test
+ public void doNotUpdateConnectPointStatus() {
+ DeviceId deviceId = DeviceId.deviceId("test-device");
+ ProviderId pid = new ProviderId("of", "foo");
+ Device device =
+ new DefaultDevice(pid, deviceId, Device.Type.OLT, "", "", "", "", null);
+ Port port1 = new DefaultPort(device, PortNumber.portNumber(1), true,
+ DefaultAnnotations.builder().set(PORT_NAME, "port-1").build());
+
+ ServiceKey sk1 = new ServiceKey(new AccessDevicePort(port1), new UniTagInformation());
+
+ // cpStatus map for the test
+ oltFlowService.cpStatus = oltFlowService.storageService.
+ <ServiceKey, OltPortStatus>consistentMapBuilder().build().asJavaMap();
+
+ // check that an entry is not created if the only status is pending remove
+ oltFlowService.updateConnectPointStatus(sk1, null, null, PENDING_REMOVE);
+ OltPortStatus entry = oltFlowService.cpStatus.get(sk1);
+ Assert.assertNull(entry);
+
+ // check that an entry is not created if the only status is ERROR
+ oltFlowService.updateConnectPointStatus(sk1, null, null, ERROR);
+ entry = oltFlowService.cpStatus.get(sk1);
+ Assert.assertNull(entry);
+ }
+
@Test
public void testHasDefaultEapol() {
DeviceId deviceId = DeviceId.deviceId("test-device");