From 82e21897f3e735594510709fc9f0bc85f37d8f4c Mon Sep 17 00:00:00 2001 From: Andrea Campanella Date: Mon, 10 Sep 2018 14:08:16 +0200 Subject: [PATCH] Removing applyRole to avoid double mastership requests to the provider. The request roleforSync call handles the request. The store selects a role and this in turn issues a handleMastershipEvent call that deals with applying the request Adapting NETCONF device provider to mark online only after session establishment. Change-Id: Id16937ced3af31b5b65bc6e5b0794557cf900a57 --- .../net/device/impl/DeviceManager.java | 26 ----------- .../device/impl/NetconfDeviceProvider.java | 46 +++++++++++++------ .../impl/NetconfDeviceProviderTest.java | 12 +++-- 3 files changed, 41 insertions(+), 43 deletions(-) diff --git a/core/net/src/main/java/org/onosproject/net/device/impl/DeviceManager.java b/core/net/src/main/java/org/onosproject/net/device/impl/DeviceManager.java index 2161f7d99d..a5d8abccc5 100644 --- a/core/net/src/main/java/org/onosproject/net/device/impl/DeviceManager.java +++ b/core/net/src/main/java/org/onosproject/net/device/impl/DeviceManager.java @@ -519,31 +519,6 @@ public class DeviceManager super(provider); } - /** - * Apply role in reaction to provider event. - * - * @param deviceId device identifier - * @param newRole new role to apply to the device - * @return true if the request was sent to provider - */ - private boolean applyRole(DeviceId deviceId, MastershipRole newRole) { - - if (newRole.equals(MastershipRole.NONE)) { - //no-op - return true; - } - - DeviceProvider provider = provider(); - if (provider == null) { - log.warn("Provider for {} was not found. Cannot apply role {}", - deviceId, newRole); - return false; - } - provider.roleChanged(deviceId, newRole); - // not triggering probe when triggered by provider service event - return true; - } - @Override public void deviceConnected(DeviceId deviceId, DeviceDescription deviceDescription) { @@ -570,7 +545,6 @@ public class DeviceManager log.info("Local role is {} for {}", role, deviceId); DeviceEvent event = store.createOrUpdateDevice(provider().id(), deviceId, deviceDescription); - applyRole(deviceId, role); if (portConfig != null) { //updating the ports if configration exists diff --git a/providers/netconf/device/src/main/java/org/onosproject/provider/netconf/device/impl/NetconfDeviceProvider.java b/providers/netconf/device/src/main/java/org/onosproject/provider/netconf/device/impl/NetconfDeviceProvider.java index 8281f4e554..02c629ebb6 100644 --- a/providers/netconf/device/src/main/java/org/onosproject/provider/netconf/device/impl/NetconfDeviceProvider.java +++ b/providers/netconf/device/src/main/java/org/onosproject/provider/netconf/device/impl/NetconfDeviceProvider.java @@ -71,6 +71,7 @@ import org.osgi.service.component.ComponentContext; import org.slf4j.Logger; import java.io.IOException; +import java.net.InetSocketAddress; import java.net.Socket; import java.net.URI; import java.net.URISyntaxException; @@ -327,9 +328,13 @@ public class NetconfDeviceProvider extends AbstractProvider // method to test reachability. //test connection to device opening a socket to it. log.debug("Testing reachability for {}:{}", ip, port); - try (Socket socket = new Socket(ip, port)) { + Socket socket = new Socket(); + try { + socket.connect(new InetSocketAddress(ip, port), 1000); log.debug("rechability of {}, {}, {}", deviceId, socket.isConnected(), !socket.isClosed()); - return socket.isConnected() && !socket.isClosed(); + boolean isConnected = socket.isConnected() && !socket.isClosed(); + socket.close(); + return isConnected; } catch (IOException e) { log.info("Device {} is not reachable", deviceId); log.debug(" error details", e); @@ -426,6 +431,10 @@ public class NetconfDeviceProvider extends AbstractProvider log.trace("{} not my scheme, skipping", deviceId); return; } + if (!isReachable(deviceId)) { + log.warn("Can't connect to device {}", deviceId); + return; + } DeviceDescription deviceDescription = createDeviceRepresentation(deviceId, config); log.debug("Connecting NETCONF device {}, on {}:{} with username {}", deviceId, config.ip(), config.port(), config.username()); @@ -434,11 +443,6 @@ public class NetconfDeviceProvider extends AbstractProvider if (deviceService.getDevice(deviceId) == null) { providerService.deviceConnected(deviceId, deviceDescription); } - try { - checkAndUpdateDevice(deviceId, deviceDescription, true); - } catch (Exception e) { - log.error("Unhandled exception checking {}", deviceId, e); - } } private void checkAndUpdateDevice(DeviceId deviceId, DeviceDescription deviceDescription, boolean newlyConnected) { @@ -451,7 +455,7 @@ public class NetconfDeviceProvider extends AbstractProvider if (!isReachable && deviceService.isAvailable(deviceId)) { providerService.deviceDisconnected(deviceId); return; - } else if (newlyConnected) { + } else if (newlyConnected && mastershipService.isLocalMaster(deviceId)) { updateDeviceDescription(deviceId, deviceDescription, device); } if (isReachable && deviceService.isAvailable(deviceId) && @@ -478,11 +482,16 @@ public class NetconfDeviceProvider extends AbstractProvider deviceId, new DefaultDeviceDescription( updatedDeviceDescription, true, updatedDeviceDescription.annotations())); - } else if (updatedDeviceDescription == null) { + } else if (updatedDeviceDescription == null && deviceDescription != null) { providerService.deviceConnected( deviceId, new DefaultDeviceDescription( deviceDescription, true, deviceDescription.annotations())); + } else { + providerService.deviceConnected(deviceId, new DefaultDeviceDescription(deviceId.uri(), + device.type(), device.manufacturer(), device.hwVersion(), device.swVersion(), + device.serialNumber(), device.chassisId(), true, + (SparseAnnotations) device.annotations())); } } } else { @@ -540,6 +549,7 @@ public class NetconfDeviceProvider extends AbstractProvider .set(IPADDRESS, ipAddress) .set(PORT, String.valueOf(config.port())) .set(AnnotationKeys.PROTOCOL, SCHEME_NAME.toUpperCase()) + .set(AnnotationKeys.PROVIDER_MARK_ONLINE, String.valueOf(true)) .build(); return new DefaultDeviceDescription( deviceId.uri(), @@ -568,8 +578,15 @@ public class NetconfDeviceProvider extends AbstractProvider private void initiateConnection(DeviceId deviceId, MastershipRole newRole) { try { if (isReachable(deviceId)) { - controller.connectDevice(deviceId); - providerService.receivedRoleReply(deviceId, newRole, MastershipRole.MASTER); + NetconfDevice device = controller.connectDevice(deviceId); + if (device != null) { + providerService.receivedRoleReply(deviceId, newRole, MastershipRole.MASTER); + try { + checkAndUpdateDevice(deviceId, null, true); + } catch (Exception e) { + log.error("Unhandled exception checking {}", deviceId, e); + } + } } } catch (Exception e) { if (deviceService.getDevice(deviceId) != null) { @@ -645,12 +662,15 @@ public class NetconfDeviceProvider extends AbstractProvider private class InternalDeviceListener implements DeviceListener { @Override public void event(DeviceEvent event) { - executor.execute(() -> discoverPorts(event.subject().id())); + if (deviceService.isAvailable(event.subject().id())) { + executor.execute(() -> discoverPorts(event.subject().id())); + } } @Override public boolean isRelevant(DeviceEvent event) { - if (event.type() != DeviceEvent.Type.DEVICE_ADDED) { + if (event.type() != DeviceEvent.Type.DEVICE_ADDED && + event.type() != DeviceEvent.Type.DEVICE_AVAILABILITY_CHANGED) { return false; } if (mastershipService.getMasterFor(event.subject().id()) == null) { diff --git a/providers/netconf/device/src/test/java/org/onosproject/provider/netconf/device/impl/NetconfDeviceProviderTest.java b/providers/netconf/device/src/test/java/org/onosproject/provider/netconf/device/impl/NetconfDeviceProviderTest.java index 53e7838140..90763ec9d3 100644 --- a/providers/netconf/device/src/test/java/org/onosproject/provider/netconf/device/impl/NetconfDeviceProviderTest.java +++ b/providers/netconf/device/src/test/java/org/onosproject/provider/netconf/device/impl/NetconfDeviceProviderTest.java @@ -263,12 +263,11 @@ public class NetconfDeviceProviderTest { assertNotNull(providerService); assertTrue("Event should be relevant", provider.cfgListener.isRelevant(deviceAddedEvent)); available = true; + + assertFalse("Device should not be reachable" + NETCONF_DEVICE_ID_STRING, + provider.isReachable(DeviceId.deviceId(NETCONF_DEVICE_ID_STRING))); provider.cfgListener.event(deviceAddedEvent); - deviceAdded.await(); - assertEquals("Device should be added", 1, deviceStore.getDeviceCount()); - assertTrue("Device incorrectly added" + NETCONF_DEVICE_ID_STRING, - devices.containsKey(DeviceId.deviceId(NETCONF_DEVICE_ID_STRING))); devices.clear(); } @@ -340,6 +339,11 @@ public class NetconfDeviceProviderTest { private class MockDeviceService extends DeviceServiceAdapter { DeviceListener listener = null; + @Override + public boolean isAvailable(DeviceId deviceId) { + return true; + } + @Override public Device getDevice(DeviceId deviceId) { if (deviceId.toString().equals(NETCONF_DEVICE_ID_STRING)) {