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/api/src/main/java/org/onosproject/net/AnnotationKeys.java b/core/api/src/main/java/org/onosproject/net/AnnotationKeys.java
index ce3c094..2e92c53 100644
--- a/core/api/src/main/java/org/onosproject/net/AnnotationKeys.java
+++ b/core/api/src/main/java/org/onosproject/net/AnnotationKeys.java
@@ -89,16 +89,6 @@
public static final String DRIVER = "driver";
/**
- * Annotation key for the device availability behavior. The value of this key
- * is expected to be a boolean ({@code true} or {@code false}) and it is
- * used to determine if the device can be marked online by the core when
- * deemed appropriate (value is {@code false}, default behaviour if key is
- * not present), or if marking online should be left to providers ({@code
- * true}).
- */
- public static final String PROVIDER_MARK_ONLINE = "providerMarkOnline";
-
- /**
* Annotation key for durable links.
*/
public static final String DURABLE = "durable";
diff --git a/core/api/src/main/java/org/onosproject/net/behaviour/PiPipelineProgrammable.java b/core/api/src/main/java/org/onosproject/net/behaviour/PiPipelineProgrammable.java
index 22367ba..be1c6c4 100644
--- a/core/api/src/main/java/org/onosproject/net/behaviour/PiPipelineProgrammable.java
+++ b/core/api/src/main/java/org/onosproject/net/behaviour/PiPipelineProgrammable.java
@@ -44,16 +44,17 @@
CompletableFuture<Boolean> setPipeconf(PiPipeconf pipeconf);
/**
- * Returns true if the device is configured with the given pipeconf, false
- * otherwise.
+ * Probes the device to verify that the given pipeconf is the one currently
+ * configured.
* <p>
* This method is expected to always return true after successfully calling
* {@link #setPipeconf(PiPipeconf)} with the given pipeconf.
*
* @param pipeconf pipeconf
- * @return true if the device has the given pipeconf set, false otherwise
+ * @return completable future eventually true if the device has the given
+ * pipeconf set, false otherwise
*/
- boolean isPipeconfSet(PiPipeconf pipeconf);
+ CompletableFuture<Boolean> isPipeconfSet(PiPipeconf pipeconf);
/**
* Returns the default pipeconf for this device, to be used when any other
diff --git a/core/api/src/main/java/org/onosproject/net/config/basics/BasicDeviceConfig.java b/core/api/src/main/java/org/onosproject/net/config/basics/BasicDeviceConfig.java
index 0353bf2..1a7ffd6 100644
--- a/core/api/src/main/java/org/onosproject/net/config/basics/BasicDeviceConfig.java
+++ b/core/api/src/main/java/org/onosproject/net/config/basics/BasicDeviceConfig.java
@@ -29,6 +29,7 @@
private static final String TYPE = "type";
private static final String DRIVER = "driver";
private static final String MANAGEMENT_ADDRESS = "managementAddress";
+ private static final String PIPECONF = "pipeconf";
private static final String MANUFACTURER = "manufacturer";
private static final String HW_VERSION = "hwVersion";
private static final String SW_VERSION = "swVersion";
@@ -41,6 +42,7 @@
private static final int SW_VERSION_MAX_LENGTH = 256;
private static final int SERIAL_MAX_LENGTH = 256;
private static final int MANAGEMENT_ADDRESS_MAX_LENGTH = 1024;
+ private static final int PIPECONF_MAX_LENGTH = 256;
@Override
public boolean isValid() {
@@ -52,13 +54,14 @@
&& hasOnlyFields(ALLOWED, NAME, LOC_TYPE, LATITUDE, LONGITUDE,
GRID_Y, GRID_X, UI_TYPE, RACK_ADDRESS, OWNER, TYPE, DRIVER, ROLES,
MANUFACTURER, HW_VERSION, SW_VERSION, SERIAL,
- MANAGEMENT_ADDRESS, DEVICE_KEY_ID)
+ MANAGEMENT_ADDRESS, PIPECONF, DEVICE_KEY_ID)
&& isValidLength(DRIVER, DRIVER_MAX_LENGTH)
&& isValidLength(MANUFACTURER, MANUFACTURER_MAX_LENGTH)
&& isValidLength(HW_VERSION, MANUFACTURER_MAX_LENGTH)
&& isValidLength(SW_VERSION, MANUFACTURER_MAX_LENGTH)
&& isValidLength(SERIAL, MANUFACTURER_MAX_LENGTH)
- && isValidLength(MANAGEMENT_ADDRESS, MANAGEMENT_ADDRESS_MAX_LENGTH);
+ && isValidLength(MANAGEMENT_ADDRESS, MANAGEMENT_ADDRESS_MAX_LENGTH)
+ && isValidLength(PIPECONF, PIPECONF_MAX_LENGTH);
}
/**
@@ -186,15 +189,25 @@
}
/**
- * Returns the device management ip (ip:port).
+ * Returns the device management address (e.g, "ip:port" or full URI
+ * string).
*
- * @return device management address (ip:port) or null if not set
+ * @return device management address or null if not set
*/
public String managementAddress() {
return get(MANAGEMENT_ADDRESS, null);
}
/**
+ * Returns the device pipeconf.
+ *
+ * @return device pipeconf or null if not set
+ */
+ public String pipeconf() {
+ return get(PIPECONF, null);
+ }
+
+ /**
* Sets the device management ip (ip:port).
*
* @param managementAddress new device management address (ip:port); null to clear
@@ -202,11 +215,23 @@
*/
public BasicDeviceConfig managementAddress(String managementAddress) {
checkArgument(managementAddress.length() <= MANAGEMENT_ADDRESS_MAX_LENGTH,
- "serialNumber exceeds maximum length " + MANAGEMENT_ADDRESS_MAX_LENGTH);
+ "managementAddress exceeds maximum length " + MANAGEMENT_ADDRESS_MAX_LENGTH);
return (BasicDeviceConfig) setOrClear(MANAGEMENT_ADDRESS, managementAddress);
}
/**
+ * Sets the device pipeconf.
+ *
+ * @param pipeconf new device pipeconf
+ * @return self
+ */
+ public BasicDeviceConfig pipeconf(String pipeconf) {
+ checkArgument(pipeconf.length() <= PIPECONF_MAX_LENGTH,
+ "pipeconf exceeds maximum length " + MANAGEMENT_ADDRESS_MAX_LENGTH);
+ return (BasicDeviceConfig) setOrClear(PIPECONF, pipeconf);
+ }
+
+ /**
* Returns the device key id.
*
* @return device key id or null if not set
diff --git a/core/api/src/main/java/org/onosproject/net/device/DeviceAgentEvent.java b/core/api/src/main/java/org/onosproject/net/device/DeviceAgentEvent.java
index d2520b0..b214070 100644
--- a/core/api/src/main/java/org/onosproject/net/device/DeviceAgentEvent.java
+++ b/core/api/src/main/java/org/onosproject/net/device/DeviceAgentEvent.java
@@ -54,19 +54,19 @@
ROLE_MASTER,
/**
- * Signifies that the agent has standby/slave mastership role.
+ * Signifies that the agent has acquired standby/slave mastership role.
*/
ROLE_STANDBY,
/**
- * Signifies that the agent cannot acquire any valid mastership role for
+ * Signifies that the agent doesn't have any valid mastership role for
* the device.
*/
ROLE_NONE,
/**
- * Signifies that the agent cannot perform operations on the device
- * because its role is not master.
+ * Signifies that the agent tried to perform some operations on the
+ * device that requires master role.
*/
NOT_MASTER,
@@ -81,15 +81,4 @@
public DeviceAgentEvent(Type type, DeviceId deviceId) {
super(type, deviceId);
}
-
- /**
- * Creates a new device agent event for the given type, device ID and time.
- *
- * @param type event type
- * @param deviceId device ID
- * @param time occurrence time
- */
- public DeviceAgentEvent(Type type, DeviceId deviceId, long time) {
- super(type, deviceId, time);
- }
}
diff --git a/core/api/src/main/java/org/onosproject/net/device/DeviceHandshaker.java b/core/api/src/main/java/org/onosproject/net/device/DeviceHandshaker.java
index bceaeff..8a941ca 100644
--- a/core/api/src/main/java/org/onosproject/net/device/DeviceHandshaker.java
+++ b/core/api/src/main/java/org/onosproject/net/device/DeviceHandshaker.java
@@ -31,13 +31,63 @@
public interface DeviceHandshaker extends DeviceConnect {
/**
- * Checks the reachability (connectivity) of a device. Reachability, unlike
- * availability, denotes whether THIS particular node can send messages and
- * receive replies from the specified device.
+ * Returns true if this node is presumed to be able to send messages and
+ * receive replies from the device.
+ * <p>
+ * The implementation should not make any attempt at actively probing the
+ * device over the network, as such it should not block execution. Instead,
+ * it should return a result based solely on internal state (e.g. socket
+ * state). If it returns true, then this node is expected to communicate
+ * with the server successfully. In other words, if any message would be
+ * sent to the device immediately after this method is called and returns
+ * true, then such message is expected, but NOT guaranteed, to reach the
+ * device. If false, it means communication with the device is unlikely to
+ * happen soon.
+ * <p>
+ * Some implementations might require a connection to be created via {@link
+ * #connect()} before checking for reachability. Similarly, after invoking
+ * {@link #disconnect()}, this method might always return false.
*
- * @return CompletableFuture eventually true if reachable, false otherwise
+ * @return true if the device is deemed reachable, false otherwise
*/
- CompletableFuture<Boolean> isReachable();
+ boolean isReachable();
+
+ /**
+ * Similar to {@link #isReachable()}, but performs probing of the device
+ * over the network. This method should be called if {@link #isReachable()}
+ * returns false and the caller wants to be sure this is not a transient
+ * failure state by actively probing the device.
+ *
+ * @return completable future eventually true if device responded to probe,
+ * false otherwise
+ */
+ CompletableFuture<Boolean> probeReachability();
+
+ /**
+ * Checks the availability of the device. Availability denotes whether the
+ * device is reachable and able to perform its functions as expected (e.g.,
+ * forward traffic). Similar to {@link #isReachable()}, implementations are
+ * not allowed to probe the device over the network, but the result should
+ * be based solely on internal state.
+ * <p>
+ * Implementation of this method is optional. If not supported, an exception
+ * should be thrown.
+ *
+ * @return true if the device is deemed available, false otherwise
+ * @throws UnsupportedOperationException if this method is not supported and
+ * {@link #probeAvailability()} should
+ * be used instead.
+ */
+ boolean isAvailable();
+
+ /**
+ * Similar to {@link #isAvailable()} but allows probing the device over the
+ * network. Differently from {@link #isAvailable()}, implementation of this
+ * method is mandatory.
+ *
+ * @return completable future eventually true if available, false otherwise
+ */
+ CompletableFuture<Boolean> probeAvailability();
/**
* Notifies the device a mastership role change as decided by the core. The
@@ -45,10 +95,48 @@
* signaling the mastership role accepted by the device.
*
* @param newRole new mastership role
+ * @throws UnsupportedOperationException if the device does not support
+ * mastership handling
*/
void roleChanged(MastershipRole newRole);
/**
+ * Notifies the device of a mastership role change as decided by the core.
+ * Differently from {@link #roleChanged(MastershipRole)}, the role is
+ * described by the given preference value, where {@code preference = 0}
+ * signifies {@link MastershipRole#MASTER} role and {@code preference > 0}
+ * signifies {@link MastershipRole#STANDBY}. Smaller preference values
+ * indicates higher mastership priority for different nodes.
+ * <p>
+ * This method does not permit notifying role {@link MastershipRole#NONE},
+ * in which case {@link #roleChanged(MastershipRole)} should be used
+ * instead.
+ * <p>
+ * Term is a monotonically increasing number, increased by one every time a
+ * new master is elected.
+ * <p>
+ * The implementation of this method should trigger a {@link
+ * DeviceAgentEvent} signaling the mastership role accepted by the device.
+ *
+ * @param preference preference value, where 0 signifies {@link
+ * MastershipRole#MASTER} and all other values {@link
+ * MastershipRole#STANDBY}
+ * @param term term number
+ * @throws UnsupportedOperationException if the device does not support
+ * mastership handling, or if it does
+ * not support setting preference-based
+ * mastership, and {@link #roleChanged(MastershipRole)}
+ * should be used instead
+ */
+ default void roleChanged(int preference, long term) {
+ if (preference == 0) {
+ roleChanged(MastershipRole.MASTER);
+ } else {
+ roleChanged(MastershipRole.STANDBY);
+ }
+ }
+
+ /**
* Returns the last known mastership role agreed by the device for this
* node.
*
diff --git a/core/api/src/main/java/org/onosproject/net/device/DeviceProvider.java b/core/api/src/main/java/org/onosproject/net/device/DeviceProvider.java
index ebdf7b1..771a8e4 100644
--- a/core/api/src/main/java/org/onosproject/net/device/DeviceProvider.java
+++ b/core/api/src/main/java/org/onosproject/net/device/DeviceProvider.java
@@ -41,8 +41,8 @@
* Notifies the provider of a mastership role change for the specified
* device as decided by the core.
*
- * @param deviceId device identifier
- * @param newRole newly determined mastership role
+ * @param deviceId device identifier
+ * @param newRole newly determined mastership role
*/
void roleChanged(DeviceId deviceId, MastershipRole newRole);
@@ -50,18 +50,40 @@
* Checks the reachability (connectivity) of a device from this provider.
* Reachability, unlike availability, denotes whether THIS particular node
* can send messages and receive replies from the specified device.
+ * <p>
+ * Implementations are encouraged to check for reachability by using only
+ * internal provider state, i.e., without blocking execution.
*
- * @param deviceId device identifier
+ * @param deviceId device identifier
* @return true if reachable, false otherwise
*/
boolean isReachable(DeviceId deviceId);
/**
- * Administratively enables or disables a port.
+ * Checks the availability of the device from the provider perspective.
+ * Availability denotes whether the device is reachable by
+ * this node and able to perform its functions as expected (e.g., forward
+ * traffic).
+ *
+ * <p>
+ * Implementations are encouraged to check for availability by using only
+ * internal provider state, i.e., without blocking execution.
*
* @param deviceId device identifier
+ * @return completable future eventually true if available, false otherwise
+ */
+ default boolean isAvailable(DeviceId deviceId) {
+ // For most implementations such as OpenFlow, reachability is equivalent
+ // to availability.
+ return isReachable(deviceId);
+ }
+
+ /**
+ * Administratively enables or disables a port.
+ *
+ * @param deviceId device identifier
* @param portNumber port number
- * @param enable true if port is to be enabled, false to disable
+ * @param enable true if port is to be enabled, false to disable
*/
void changePortState(DeviceId deviceId, PortNumber portNumber,
boolean enable);
diff --git a/core/api/src/main/java/org/onosproject/net/device/DeviceProviderService.java b/core/api/src/main/java/org/onosproject/net/device/DeviceProviderService.java
index 80996eb..b00fa63 100644
--- a/core/api/src/main/java/org/onosproject/net/device/DeviceProviderService.java
+++ b/core/api/src/main/java/org/onosproject/net/device/DeviceProviderService.java
@@ -74,15 +74,30 @@
void portStatusChanged(DeviceId deviceId, PortDescription portDescription);
/**
- * Notifies the core about the result of a RoleRequest sent to a device.
+ * Notifies the core about the result of a role request sent to a device.
+ * This method assumes that the provider knows the original role that was
+ * requested for a given response, if that is not the case, and only the
+ * response is known to the provider, then {@link #receivedRoleReply(DeviceId,
+ * MastershipRole)} should be used instead.
*
- * @param deviceId identity of the device
+ * @param deviceId identity of the device
* @param requested mastership role that was requested by the node
- * @param response mastership role the switch accepted
+ * @param response mastership role the switch accepted
*/
void receivedRoleReply(DeviceId deviceId, MastershipRole requested, MastershipRole response);
/**
+ * Notifies the core about a mastership role reported by the given device
+ * for this node.
+ *
+ * @param deviceId identity of the device
+ * @param response mastership role the switch accepted
+ */
+ default void receivedRoleReply(DeviceId deviceId, MastershipRole response) {
+ receivedRoleReply(deviceId, null, response);
+ }
+
+ /**
* Updates statistics about all ports of a device.
*
* @param deviceId identity of the device
diff --git a/core/api/src/main/java/org/onosproject/net/driver/DeviceConnect.java b/core/api/src/main/java/org/onosproject/net/driver/DeviceConnect.java
index f30386d..433be71 100644
--- a/core/api/src/main/java/org/onosproject/net/driver/DeviceConnect.java
+++ b/core/api/src/main/java/org/onosproject/net/driver/DeviceConnect.java
@@ -21,7 +21,9 @@
/**
* Abstraction of handler behaviour used to set-up and tear-down connections
- * with a device.
+ * with a device. A connection is intended as the presence of state (e.g. a
+ * transport session) required to carry messages between this node and the
+ * device.
*/
@Beta
public interface DeviceConnect extends HandlerBehaviour {
@@ -29,17 +31,33 @@
/**
* Connects to the device, for example by opening the transport session that
* will be later used to send control messages. Returns true if the
- * connection was initiated successfully, false otherwise.
+ * connection was initiated successfully, false otherwise. The
+ * implementation might require probing the device over the network to
+ * initiate the connection.
* <p>
- * Calling multiple times this method while a connection to the device is
- * open should result in a no-op.
+ * When calling this method while a connection to the device already exists,
+ * the behavior is not defined. For example, some implementations might
+ * require to first call {@link #disconnect()}, while other might behave as
+ * a no-op.
*
- * @return CompletableFuture with true if the operation was successful
+ * @return CompletableFuture eventually true if a connection was created
+ * successfully, false otherwise
+ * @throws IllegalStateException if a connection already exists and the
+ * implementation requires to call {@link
+ * #disconnect()} first.
*/
- CompletableFuture<Boolean> connect();
+ CompletableFuture<Boolean> connect() throws IllegalStateException;
/**
- * Returns true if a connection to the device is open, false otherwise.
+ * Returns true if a connection to the device exists, false otherwise. This
+ * method is NOT expected to send any message over the network to check for
+ * device reachability, but rather it should only give an indication if any
+ * internal connection state exists for the device. As such, it should NOT
+ * block execution.
+ * <p>
+ * In general, when called after {@link #connect()} it should always return
+ * true, while it is expected to always return false after calling {@link
+ * #disconnect()} or if {@link #connect()} was never called.
*
* @return true if the connection is open, false otherwise
*/
@@ -47,14 +65,10 @@
/**
* Disconnects from the device, for example closing the transport session
- * previously opened. Returns true if the disconnection procedure was
- * successful, false otherwise.
+ * previously opened.
* <p>
* Calling multiple times this method while a connection to the device is
- * closed should result in a no-op.
- *
- * @return CompletableFuture with true if the operation was successful
+ * already closed should result in a no-op.
*/
- CompletableFuture<Boolean> disconnect();
-
+ void disconnect();
}
diff --git a/core/api/src/main/java/org/onosproject/net/pi/service/PiPipeconfConfig.java b/core/api/src/main/java/org/onosproject/net/pi/service/PiPipeconfConfig.java
deleted file mode 100644
index e8aa731..0000000
--- a/core/api/src/main/java/org/onosproject/net/pi/service/PiPipeconfConfig.java
+++ /dev/null
@@ -1,41 +0,0 @@
-/*
- * Copyright 2017-present Open Networking Foundation
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.onosproject.net.pi.service;
-
-import com.google.common.annotations.Beta;
-import org.onosproject.net.DeviceId;
-import org.onosproject.net.config.Config;
-import org.onosproject.net.pi.model.PiPipeconfId;
-
-/**
- * Configuration for the PiPipeconf susbystem.
- */
-@Beta
-public final class PiPipeconfConfig extends Config<DeviceId> {
-
- public static final String PIPIPECONFID = "piPipeconfId";
-
- @Override
- public boolean isValid() {
- return hasOnlyFields(PIPIPECONFID);
- //TODO will reinstate after synchonization of events
- //&& !piPipeconfId().id().equals("");
- }
-
- public PiPipeconfId piPipeconfId() {
- return new PiPipeconfId(get(PIPIPECONFID, ""));
- }
-}