Refactor channel and mastership handling in P4Runtime
This (big) change aims at solving the issue observed with mastership flapping
and device connection/disconnection with P4Runtime.
Channel handling is now based on the underlying gRPC channel state. Before,
channel events (open/close/error) were generated as a consequence of P4Runtime
StreamChannel events, making device availability dependent on mastership. Now
Stream Channel events only affect mastership (MASTER/STANDBY or NONE when the
SteamChannel RPC is not active).
Mastership handling has been refactored to generate P4Runtime election IDs that
are compatible with the mastership preference decided by the MastershipService.
GeneralDeviceProvider has been re-implemented to support in-order
device event processing and to reduce implementation complexity. Stats polling
has been moved to a separate component, and netcfg handling updated to only
depend on BasicDeviceConfig, augmented with a pipeconf field, and re-using the
managementAddress field to set the gRPC server endpoints (e.g.
grpc://myswitch.local:50051). Before it was depending on 3 different config
classes, making hard to detect changes.
Finally, this change affects some core interfaces:
- Adds a method to DeviceProvider and DeviceHandshaker to check for device
availability, making the meaning of availability device-specific. This is needed
in cases where the device manager needs to change the availability state of a
device (as in change #20842)
- Support device providers not capable of reconciling mastership role responses
with requests (like P4Runtime).
- Clarify the meaning of "connection" in the DeviceConnect behavior.
- Allows driver-based providers to check devices for reachability and
availability without probing the device via the network.
Change-Id: I7ff30d29f5d02ad938e3171536e54ae2916629a2
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 6ce7c0c..0ffe615 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
@@ -23,13 +23,11 @@
import org.onlab.util.Tools;
import org.onosproject.cluster.ClusterService;
import org.onosproject.cluster.NodeId;
-import org.onosproject.net.config.basics.PortDescriptionsConfig;
import org.onosproject.mastership.MastershipEvent;
import org.onosproject.mastership.MastershipListener;
import org.onosproject.mastership.MastershipService;
import org.onosproject.mastership.MastershipTerm;
import org.onosproject.mastership.MastershipTermService;
-import org.onosproject.net.AnnotationKeys;
import org.onosproject.net.ConnectPoint;
import org.onosproject.net.Device;
import org.onosproject.net.Device.Type;
@@ -46,6 +44,7 @@
import org.onosproject.net.config.basics.BasicDeviceConfig;
import org.onosproject.net.config.basics.DeviceAnnotationConfig;
import org.onosproject.net.config.basics.PortAnnotationConfig;
+import org.onosproject.net.config.basics.PortDescriptionsConfig;
import org.onosproject.net.device.DefaultPortDescription;
import org.onosproject.net.device.DeviceAdminService;
import org.onosproject.net.device.DeviceDescription;
@@ -499,7 +498,7 @@
}
// If this node is the master, ensure the device is marked online.
- if (myRole == MASTER) {
+ if (myRole == MASTER && canMarkOnline(device)) {
post(store.markOnline(deviceId));
}
@@ -788,15 +787,26 @@
return;
}
+ final MastershipRole expected = mastershipService.getLocalRole(deviceId);
+
+ if (requested == null) {
+ // Provider is not able to reconcile role responses with
+ // requests. We assume what was requested is what we expect.
+ // This will work only if mastership doesn't change too often,
+ // and devices are left enough time to provide responses before
+ // a different role is requested.
+ requested = expected;
+ }
+
if (Objects.equals(requested, response)) {
- if (Objects.equals(requested, mastershipService.getLocalRole(deviceId))) {
+ if (Objects.equals(requested, expected)) {
return;
} else {
- log.warn("Role mismatch on {}. set to {}, but store demands {}",
- deviceId, response, mastershipService.getLocalRole(deviceId));
+ log.warn("Role mismatch on {}. Set to {}, but store demands {}",
+ deviceId, response, expected);
// roleManager got the device to comply, but doesn't agree with
// the store; use the store's view, then try to reassert.
- backgroundService.execute(() -> reassertRole(deviceId, mastershipService.getLocalRole(deviceId)));
+ backgroundService.execute(() -> reassertRole(deviceId, expected));
return;
}
} else {
@@ -830,9 +840,12 @@
}
private boolean canMarkOnline(Device device) {
- final boolean providerMarkOnline = Boolean.parseBoolean(
- device.annotations().value(AnnotationKeys.PROVIDER_MARK_ONLINE));
- return !providerMarkOnline;
+ DeviceProvider provider = getProvider(device.id());
+ if (provider == null) {
+ log.warn("Provider for {} was not found. Cannot evaluate availability", device.id());
+ return false;
+ }
+ return provider.isAvailable(device.id());
}
// Applies the specified role to the device; ignores NONE
diff --git a/core/net/src/main/java/org/onosproject/net/pi/impl/PiPipeconfManager.java b/core/net/src/main/java/org/onosproject/net/pi/impl/PiPipeconfManager.java
index 27f6e4d..9653091 100644
--- a/core/net/src/main/java/org/onosproject/net/pi/impl/PiPipeconfManager.java
+++ b/core/net/src/main/java/org/onosproject/net/pi/impl/PiPipeconfManager.java
@@ -23,10 +23,8 @@
import org.onlab.util.ItemNotFoundException;
import org.onlab.util.SharedExecutors;
import org.onosproject.net.DeviceId;
-import org.onosproject.net.config.ConfigFactory;
import org.onosproject.net.config.NetworkConfigRegistry;
import org.onosproject.net.config.basics.BasicDeviceConfig;
-import org.onosproject.net.config.basics.SubjectFactories;
import org.onosproject.net.driver.Behaviour;
import org.onosproject.net.driver.DefaultDriver;
import org.onosproject.net.driver.Driver;
@@ -36,7 +34,6 @@
import org.onosproject.net.driver.DriverProvider;
import org.onosproject.net.pi.model.PiPipeconf;
import org.onosproject.net.pi.model.PiPipeconfId;
-import org.onosproject.net.pi.service.PiPipeconfConfig;
import org.onosproject.net.pi.service.PiPipeconfMappingStore;
import org.onosproject.net.pi.service.PiPipeconfService;
import org.osgi.service.component.annotations.Activate;
@@ -73,7 +70,6 @@
private final Logger log = getLogger(getClass());
private static final String MERGED_DRIVER_SEPARATOR = ":";
- private static final String CFG_SCHEME = "piPipeconf";
private static final int MISSING_DRIVER_WATCHDOG_INTERVAL = 5; // Seconds.
@@ -98,19 +94,8 @@
protected ExecutorService executor = Executors.newFixedThreadPool(
10, groupedThreads("onos/pipeconf-manager", "%d", log));
- protected final ConfigFactory configFactory =
- new ConfigFactory<DeviceId, PiPipeconfConfig>(
- SubjectFactories.DEVICE_SUBJECT_FACTORY,
- PiPipeconfConfig.class, CFG_SCHEME) {
- @Override
- public PiPipeconfConfig createConfig() {
- return new PiPipeconfConfig();
- }
- };
-
@Activate
public void activate() {
- cfgService.registerConfigFactory(configFactory);
driverAdminService.addListener(driverListener);
checkMissingMergedDrivers();
if (!missingMergedDrivers.isEmpty()) {
@@ -127,7 +112,6 @@
@Deactivate
public void deactivate() {
executor.shutdown();
- cfgService.unregisterConfigFactory(configFactory);
driverAdminService.removeListener(driverListener);
pipeconfs.clear();
missingMergedDrivers.clear();
@@ -231,7 +215,7 @@
if (getDriver(newDriverName) != null) {
return newDriverName;
}
- log.info("Creating merged driver {}...", newDriverName);
+ log.debug("Creating merged driver {}...", newDriverName);
final Driver mergedDriver = buildMergedDriver(
pipeconfId, baseDriverName, newDriverName);
if (mergedDriver == null) {
diff --git a/core/net/src/main/java/org/onosproject/net/pi/impl/PiPipeconfWatchdogManager.java b/core/net/src/main/java/org/onosproject/net/pi/impl/PiPipeconfWatchdogManager.java
index 62fb157..eafb778 100644
--- a/core/net/src/main/java/org/onosproject/net/pi/impl/PiPipeconfWatchdogManager.java
+++ b/core/net/src/main/java/org/onosproject/net/pi/impl/PiPipeconfWatchdogManager.java
@@ -17,6 +17,7 @@
package org.onosproject.net.pi.impl;
import com.google.common.collect.Maps;
+import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.Striped;
import org.onlab.util.KryoNamespace;
import org.onlab.util.Tools;
@@ -250,7 +251,7 @@
if (!handshaker.isConnected()) {
return false;
}
- if (pipelineProg.isPipeconfSet(pipeconf)) {
+ if (Futures.getUnchecked(pipelineProg.isPipeconfSet(pipeconf))) {
log.debug("Pipeconf {} already configured on {}",
pipeconf.id(), device.id());
return true;
diff --git a/core/net/src/test/java/org/onosproject/net/pi/impl/PiPipeconfManagerTest.java b/core/net/src/test/java/org/onosproject/net/pi/impl/PiPipeconfManagerTest.java
index 20ded6a..de778a0 100644
--- a/core/net/src/test/java/org/onosproject/net/pi/impl/PiPipeconfManagerTest.java
+++ b/core/net/src/test/java/org/onosproject/net/pi/impl/PiPipeconfManagerTest.java
@@ -44,7 +44,6 @@
import org.onosproject.net.driver.DriverProvider;
import org.onosproject.net.pi.model.PiPipeconf;
import org.onosproject.net.pi.model.PiPipeconfId;
-import org.onosproject.net.pi.service.PiPipeconfConfig;
import java.io.IOException;
import java.io.InputStream;
@@ -55,7 +54,6 @@
import java.util.Set;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.onosproject.pipelines.basic.PipeconfLoader.BASIC_PIPECONF;
@@ -77,9 +75,6 @@
private final Set<NetworkConfigListener> netCfgListeners = new HashSet<>();
private final Set<DriverProvider> providers = new HashSet<>();
- private final PiPipeconfConfig piPipeconfConfig = new PiPipeconfConfig();
- private final InputStream jsonStream = PiPipeconfManagerTest.class
- .getResourceAsStream("/org/onosproject/net/pi/impl/piPipeconfId.json");
private final BasicDeviceConfig basicDeviceConfig = new BasicDeviceConfig();
private final InputStream jsonStreamBasic = PiPipeconfManagerTest.class
.getResourceAsStream("/org/onosproject/net/pi/impl/basic.json");
@@ -95,11 +90,8 @@
piPipeconf = BASIC_PIPECONF;
piPipeconfService.cfgService = cfgService;
piPipeconfService.driverAdminService = driverAdminService;
- String key = "piPipeconf";
ObjectMapper mapper = new ObjectMapper();
- JsonNode jsonNode = mapper.readTree(jsonStream);
ConfigApplyDelegate delegate = new MockDelegate();
- piPipeconfConfig.init(DEVICE_ID, key, jsonNode, mapper, delegate);
String keyBasic = "basic";
JsonNode jsonNodeBasic = mapper.readTree(jsonStreamBasic);
basicDeviceConfig.init(DEVICE_ID, keyBasic, jsonNodeBasic, mapper, delegate);
@@ -111,7 +103,6 @@
assertEquals("Incorrect driver admin service", driverAdminService, piPipeconfService.driverAdminService);
assertEquals("Incorrect driverAdminService service", driverAdminService, piPipeconfService.driverAdminService);
assertEquals("Incorrect configuration service", cfgService, piPipeconfService.cfgService);
- assertTrue("Incorrect config factory", cfgFactories.contains(piPipeconfService.configFactory));
}
@Test
@@ -120,7 +111,6 @@
assertEquals("Incorrect driver admin service", null, piPipeconfService.driverAdminService);
assertEquals("Incorrect driverAdminService service", null, piPipeconfService.driverAdminService);
assertEquals("Incorrect configuration service", null, piPipeconfService.cfgService);
- assertFalse("Config factory should be unregistered", cfgFactories.contains(piPipeconfService.configFactory));
}
@Test
@@ -139,7 +129,8 @@
@Test
public void mergeDriver() {
- PiPipeconfId piPipeconfId = cfgService.getConfig(DEVICE_ID, PiPipeconfConfig.class).piPipeconfId();
+ PiPipeconfId piPipeconfId = new PiPipeconfId(cfgService.getConfig(
+ DEVICE_ID, BasicDeviceConfig.class).pipeconf());
assertEquals(piPipeconf.id(), piPipeconfId);
String baseDriverName = cfgService.getConfig(DEVICE_ID, BasicDeviceConfig.class).driver();
@@ -196,10 +187,7 @@
@Override
public <S, C extends Config<S>> C getConfig(S subject, Class<C> configClass) {
DeviceId did = (DeviceId) subject;
- if (configClass.equals(PiPipeconfConfig.class)
- && did.equals(DEVICE_ID)) {
- return (C) piPipeconfConfig;
- } else if (configClass.equals(BasicDeviceConfig.class)
+ if (configClass.equals(BasicDeviceConfig.class)
&& did.equals(DEVICE_ID)) {
return (C) basicDeviceConfig;
}
diff --git a/core/net/src/test/resources/org/onosproject/net/pi/impl/basic.json b/core/net/src/test/resources/org/onosproject/net/pi/impl/basic.json
index 42de9ad..ad2a205 100644
--- a/core/net/src/test/resources/org/onosproject/net/pi/impl/basic.json
+++ b/core/net/src/test/resources/org/onosproject/net/pi/impl/basic.json
@@ -1,3 +1,4 @@
{
- "driver": "baseDriver"
-}
\ No newline at end of file
+ "driver": "baseDriver",
+ "pipeconf": "org.onosproject.pipelines.basic"
+}
diff --git a/core/net/src/test/resources/org/onosproject/net/pi/impl/piPipeconfId.json b/core/net/src/test/resources/org/onosproject/net/pi/impl/piPipeconfId.json
deleted file mode 100644
index 7f42958..0000000
--- a/core/net/src/test/resources/org/onosproject/net/pi/impl/piPipeconfId.json
+++ /dev/null
@@ -1,3 +0,0 @@
-{
- "piPipeconfId": "org.onosproject.pipelines.basic"
-}
\ No newline at end of file