From 2ff044ed6e4c13c8df8f95fd3b695d14f056afd1 Mon Sep 17 00:00:00 2001 From: Tomas Slusny Date: Thu, 14 Jul 2016 16:01:54 +0200 Subject: [PATCH] Fix DeviceFlowRegistry filling - Fixed double filling of DeviceFlowRegistry - Moved nodeInstanceIdentifier parameter from DeviceFlowRegistry.fill to it's constructor Change-Id: I11be668a80b98f40e0b9aaa863d616fe669c0921 Signed-off-by: Tomas Slusny --- .../registry/flow/DeviceFlowRegistry.java | 2 +- .../impl/LifecycleConductorImpl.java | 81 +++++++++---------- .../impl/device/DeviceContextImpl.java | 2 +- .../registry/flow/DeviceFlowRegistryImpl.java | 6 +- .../impl/LifecycleConductorImplTest.java | 7 +- .../flow/DeviceFlowRegistryImplTest.java | 7 +- .../impl/services/ServiceMocking.java | 2 +- .../statistics/StatisticsManagerImplTest.java | 2 +- 8 files changed, 56 insertions(+), 53 deletions(-) diff --git a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/registry/flow/DeviceFlowRegistry.java b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/registry/flow/DeviceFlowRegistry.java index 4b8314a212..1e574f6459 100644 --- a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/registry/flow/DeviceFlowRegistry.java +++ b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/registry/flow/DeviceFlowRegistry.java @@ -24,7 +24,7 @@ import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier; */ public interface DeviceFlowRegistry extends AutoCloseable { - ListenableFuture>> fill(KeyedInstanceIdentifier instanceIdentifier); + ListenableFuture>> fill(); FlowDescriptor retrieveIdForFlow(FlowRegistryKey flowRegistryKey); diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/LifecycleConductorImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/LifecycleConductorImpl.java index c0ea15a01b..af2efaf21f 100644 --- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/LifecycleConductorImpl.java +++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/LifecycleConductorImpl.java @@ -34,6 +34,7 @@ import org.opendaylight.openflowplugin.api.openflow.lifecycle.DeviceContextChang import org.opendaylight.openflowplugin.api.openflow.lifecycle.LifecycleConductor; import org.opendaylight.openflowplugin.api.openflow.lifecycle.RoleChangeListener; import org.opendaylight.openflowplugin.api.openflow.lifecycle.ServiceChangeListener; +import org.opendaylight.openflowplugin.api.openflow.registry.flow.DeviceFlowRegistry; import org.opendaylight.openflowplugin.api.openflow.rpc.RpcContext; import org.opendaylight.openflowplugin.api.openflow.rpc.RpcManager; import org.opendaylight.openflowplugin.api.openflow.statistics.StatisticsContext; @@ -164,44 +165,7 @@ final class LifecycleConductorImpl implements LifecycleConductor, RoleChangeList if (OfpRole.BECOMEMASTER.equals(newRole)) { logText = "Start"; - - // Fill device flow registry with flows from datastore - final ListenableFuture>> deviceFlowRegistryFill = - deviceContext.getDeviceFlowRegistry().fill(deviceInfo.getNodeInstanceIdentifier()); - - // Start statistics scheduling only after we finished initializing device flow registry - Futures.addCallback(deviceFlowRegistryFill, new FutureCallback>>() { - @Override - public void onSuccess(@Nullable List> result) { - if (LOG.isDebugEnabled()) { - // Count all flows we read from datastore for debugging purposes. - // This number do not always represent how many flows were actually added - // to DeviceFlowRegistry, because of possible duplicates. - long flowCount = Optional.fromNullable(result).asSet().stream() - .flatMap(Collection::stream) - .flatMap(flowCapableNodeOptional -> flowCapableNodeOptional.asSet().stream()) - .flatMap(flowCapableNode -> flowCapableNode.getTable().stream()) - .flatMap(table -> table.getFlow().stream()) - .count(); - - LOG.debug("Finished filling flow registry with {} flows for node: {}", flowCount, deviceInfo.getNodeId()); - } - - statisticsManager.startScheduling(deviceInfo); - } - - @Override - public void onFailure(Throwable t) { - // If we manually cancelled this future, do not start scheduling of statistics - if (deviceFlowRegistryFill.isCancelled()) { - LOG.debug("Cancelled filling flow registry with flows for node: {}", deviceInfo.getNodeId()); - } else { - LOG.warn("Failed filling flow registry with flows for node: {} with exception: {}", deviceInfo.getNodeId(), t); - statisticsManager.startScheduling(deviceInfo); - } - } - }); - + fillDeviceFlowRegistry(deviceInfo, deviceContext.getDeviceFlowRegistry()); MdSalRegistrationUtils.registerServices(rpcContext, deviceContext, this.extensionConverterProvider); if (rpcContext.isStatisticsRpcEnabled()) { @@ -210,9 +174,6 @@ final class LifecycleConductorImpl implements LifecycleConductor, RoleChangeList deviceContext, notificationPublishService); } - - // Fill flow registry with flows found in operational and config datastore - deviceContext.getDeviceFlowRegistry().fill(deviceInfo.getNodeInstanceIdentifier()); } else { logText = "Stopp"; statisticsManager.stopScheduling(deviceInfo); @@ -243,6 +204,44 @@ final class LifecycleConductorImpl implements LifecycleConductor, RoleChangeList }); } + private void fillDeviceFlowRegistry(final DeviceInfo deviceInfo, final DeviceFlowRegistry deviceFlowRegistry) { + // Fill device flow registry with flows from datastore + final ListenableFuture>> deviceFlowRegistryFill = deviceFlowRegistry.fill(); + + // Start statistics scheduling only after we finished initializing device flow registry + Futures.addCallback(deviceFlowRegistryFill, new FutureCallback>>() { + @Override + public void onSuccess(@Nullable List> result) { + if (LOG.isDebugEnabled()) { + // Count all flows we read from datastore for debugging purposes. + // This number do not always represent how many flows were actually added + // to DeviceFlowRegistry, because of possible duplicates. + long flowCount = Optional.fromNullable(result).asSet().stream() + .flatMap(Collection::stream) + .flatMap(flowCapableNodeOptional -> flowCapableNodeOptional.asSet().stream()) + .flatMap(flowCapableNode -> flowCapableNode.getTable().stream()) + .flatMap(table -> table.getFlow().stream()) + .count(); + + LOG.debug("Finished filling flow registry with {} flows for node: {}", flowCount, deviceInfo.getNodeId()); + } + + statisticsManager.startScheduling(deviceInfo); + } + + @Override + public void onFailure(Throwable t) { + // If we manually cancelled this future, do not start scheduling of statistics + if (deviceFlowRegistryFill.isCancelled()) { + LOG.debug("Cancelled filling flow registry with flows for node: {}", deviceInfo.getNodeId()); + } else { + LOG.warn("Failed filling flow registry with flows for node: {} with exception: {}", deviceInfo.getNodeId(), t); + statisticsManager.startScheduling(deviceInfo); + } + } + }); + } + public MessageIntelligenceAgency getMessageIntelligenceAgency() { return messageIntelligenceAgency; } diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java index d15216cd0e..e5f360cab8 100644 --- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java +++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/device/DeviceContextImpl.java @@ -149,7 +149,7 @@ public class DeviceContextImpl implements DeviceContext, ExtensionConverterProvi deviceInfo = primaryConnectionContext.getDeviceInfo(); this.transactionChainManager = new TransactionChainManager(dataBroker, deviceInfo, conductor); auxiliaryConnectionContexts = new HashMap<>(); - deviceFlowRegistry = new DeviceFlowRegistryImpl(dataBroker); + deviceFlowRegistry = new DeviceFlowRegistryImpl(dataBroker, deviceInfo.getNodeInstanceIdentifier()); deviceGroupRegistry = new DeviceGroupRegistryImpl(); deviceMeterRegistry = new DeviceMeterRegistryImpl(); messageSpy = conductor.getMessageIntelligenceAgency(); diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImpl.java index e744812c58..0f262fde40 100644 --- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImpl.java +++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImpl.java @@ -52,6 +52,7 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry { @GuardedBy("marks") private final Collection marks = new HashSet<>(); private final DataBroker dataBroker; + private final KeyedInstanceIdentifier instanceIdentifier; private final List>>> lastFillFutures = new ArrayList<>(); // Specifies what to do with flow read from datastore @@ -68,12 +69,13 @@ public class DeviceFlowRegistryImpl implements DeviceFlowRegistry { }; - public DeviceFlowRegistryImpl(final DataBroker dataBroker) { + public DeviceFlowRegistryImpl(final DataBroker dataBroker, final KeyedInstanceIdentifier instanceIdentifier) { this.dataBroker = dataBroker; + this.instanceIdentifier = instanceIdentifier; } @Override - public ListenableFuture>> fill(final KeyedInstanceIdentifier instanceIdentifier) { + public ListenableFuture>> fill() { LOG.debug("Filling flow registry with flows for node: {}", instanceIdentifier); // Prepare path for read transaction diff --git a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/LifecycleConductorImplTest.java b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/LifecycleConductorImplTest.java index 4d0e6dceb3..c1ba409b27 100644 --- a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/LifecycleConductorImplTest.java +++ b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/LifecycleConductorImplTest.java @@ -87,10 +87,11 @@ public class LifecycleConductorImplTest { private NodeId nodeId = new NodeId("openflow-junit:1"); private OfpRole ofpRole = OfpRole.NOCHANGE; + private KeyedInstanceIdentifier nodeInstanceIdentifier = InstanceIdentifier.create(Nodes.class).child(Node.class, new NodeKey(nodeId)); @Before public void setUp() { - final KeyedInstanceIdentifier nodeInstanceIdentifier = InstanceIdentifier.create(Nodes.class).child(Node.class, new NodeKey(nodeId)); + nodeInstanceIdentifier = InstanceIdentifier.create(Nodes.class).child(Node.class, new NodeKey(nodeId)); lifecycleConductor = new LifecycleConductorImpl(messageIntelligenceAgency); lifecycleConductor.setSafelyManager(deviceManager); @@ -177,7 +178,7 @@ public class LifecycleConductorImplTest { final DataBroker dataBroker = mock(DataBroker.class); when(deviceContext.getDeviceState()).thenReturn(deviceState); - when(deviceContext.getDeviceFlowRegistry()).thenReturn(new DeviceFlowRegistryImpl(dataBroker)); + when(deviceContext.getDeviceFlowRegistry()).thenReturn(new DeviceFlowRegistryImpl(dataBroker, nodeInstanceIdentifier)); when(deviceManager.gainContext(deviceInfo)).thenReturn(deviceContext); when(deviceManager.onClusterRoleChange(deviceInfo, OfpRole.BECOMEMASTER)).thenReturn(listenableFuture); lifecycleConductor.roleChangeOnDevice(deviceInfo,OfpRole.BECOMEMASTER); @@ -192,7 +193,7 @@ public class LifecycleConductorImplTest { final DataBroker dataBroker = mock(DataBroker.class); when(deviceContext.getDeviceState()).thenReturn(deviceState); - when(deviceContext.getDeviceFlowRegistry()).thenReturn(new DeviceFlowRegistryImpl(dataBroker)); + when(deviceContext.getDeviceFlowRegistry()).thenReturn(new DeviceFlowRegistryImpl(dataBroker, nodeInstanceIdentifier)); when(deviceManager.gainContext(deviceInfo)).thenReturn(deviceContext); when(deviceManager.onClusterRoleChange(deviceInfo, OfpRole.BECOMESLAVE)).thenReturn(listenableFuture); diff --git a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImplTest.java b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImplTest.java index e80ce3e55e..6ee4a3c4ce 100644 --- a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImplTest.java +++ b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/DeviceFlowRegistryImplTest.java @@ -45,6 +45,7 @@ public class DeviceFlowRegistryImplTest { private DeviceFlowRegistryImpl deviceFlowRegistry; private FlowRegistryKey key; private FlowDescriptor descriptor; + private KeyedInstanceIdentifier nodeInstanceIdentifier; @Mock private DataBroker dataBroker; @Mock @@ -52,9 +53,10 @@ public class DeviceFlowRegistryImplTest { @Before public void setUp() throws Exception { + nodeInstanceIdentifier = InstanceIdentifier.create(Nodes.class).child(Node.class, new NodeKey(new NodeId(NODE_ID))); when(dataBroker.newReadOnlyTransaction()).thenReturn(readOnlyTransaction); when(readOnlyTransaction.read(any(), any())).thenReturn(Futures.immediateCheckedFuture(Optional.absent())); - deviceFlowRegistry = new DeviceFlowRegistryImpl(dataBroker); + deviceFlowRegistry = new DeviceFlowRegistryImpl(dataBroker, nodeInstanceIdentifier); final FlowAndStatisticsMapList flowStats = TestFlowHelper.createFlowAndStatisticsMapListBuilder(1).build(); key = FlowRegistryKeyFactory.create(flowStats); descriptor = FlowDescriptorFactory.create(key.getTableId(), new FlowId("ut:1")); @@ -66,10 +68,9 @@ public class DeviceFlowRegistryImplTest { @Test public void testFill() throws Exception { - final KeyedInstanceIdentifier nodeInstanceIdentifier = InstanceIdentifier.create(Nodes.class).child(Node.class, new NodeKey(new NodeId(NODE_ID))); final InstanceIdentifier path = nodeInstanceIdentifier.augmentation(FlowCapableNode.class); - deviceFlowRegistry.fill(nodeInstanceIdentifier).get(); + deviceFlowRegistry.fill().get(); verify(dataBroker, times(2)).newReadOnlyTransaction(); verify(readOnlyTransaction).read(LogicalDatastoreType.CONFIGURATION, path); diff --git a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/services/ServiceMocking.java b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/services/ServiceMocking.java index c3e68af0f0..85124809af 100644 --- a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/services/ServiceMocking.java +++ b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/services/ServiceMocking.java @@ -98,7 +98,7 @@ public abstract class ServiceMocking { when(mockedDeviceContext.getPrimaryConnectionContext()).thenReturn(mockedPrimConnectionContext); when(mockedDeviceContext.getMessageSpy()).thenReturn(mockedMessagSpy); - when(mockedDeviceContext.getDeviceFlowRegistry()).thenReturn(new DeviceFlowRegistryImpl(dataBroker)); + when(mockedDeviceContext.getDeviceFlowRegistry()).thenReturn(new DeviceFlowRegistryImpl(dataBroker, NODE_II)); when(mockedDeviceContext.getDeviceState()).thenReturn(mockedDeviceState); when(mockedDeviceContext.getDeviceInfo()).thenReturn(mockedDeviceInfo); when(mockedDeviceContext.getMultiMsgCollector(Matchers.any())).thenReturn(multiMessageCollector); diff --git a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/statistics/StatisticsManagerImplTest.java b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/statistics/StatisticsManagerImplTest.java index 44c3f3a864..2da9dc99ad 100644 --- a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/statistics/StatisticsManagerImplTest.java +++ b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/statistics/StatisticsManagerImplTest.java @@ -166,7 +166,7 @@ public class StatisticsManagerImplTest { when(mockedDeviceContext.getDeviceInfo()).thenReturn(mockedDeviceInfo); when(mockedDeviceContext.getPrimaryConnectionContext()).thenReturn(mockedPrimConnectionContext); when(mockedDeviceContext.getMessageSpy()).thenReturn(mockedMessagSpy); - when(mockedDeviceContext.getDeviceFlowRegistry()).thenReturn(new DeviceFlowRegistryImpl(dataBroker)); + when(mockedDeviceContext.getDeviceFlowRegistry()).thenReturn(new DeviceFlowRegistryImpl(dataBroker, nodePath)); when(mockedDeviceContext.getDeviceState()).thenReturn(mockedDeviceState); when(mockedDeviceContext.getMultiMsgCollector( Matchers.>>any())).thenAnswer( -- 2.36.6