Fixed NIC name to port number inconsistencies in server driver
This patch fixes some weak assumptions regarding the way the
server device driver maps NIC names and port numbers.
Also, some necessary refactoring and stdout message fixes are
committed.
Addressed comments from ONOS developers
Change-Id: I6730d971ddf8d4fe97c2d3bef75f1432e7a4592e
Signed-off-by: Georgios Katsikas <katsikas.gp@gmail.com>
diff --git a/drivers/server/src/main/java/org/onosproject/drivers/server/BasicServerDriver.java b/drivers/server/src/main/java/org/onosproject/drivers/server/BasicServerDriver.java
index 3cc18e0..046578a 100644
--- a/drivers/server/src/main/java/org/onosproject/drivers/server/BasicServerDriver.java
+++ b/drivers/server/src/main/java/org/onosproject/drivers/server/BasicServerDriver.java
@@ -15,6 +15,9 @@
*/
package org.onosproject.drivers.server;
+import org.onosproject.drivers.server.devices.RestServerSBDevice;
+
+import org.onosproject.net.DeviceId;
import org.onosproject.net.driver.AbstractHandlerBehaviour;
import org.onosproject.net.driver.DriverHandler;
import org.onosproject.protocol.rest.RestSBController;
@@ -22,6 +25,7 @@
import org.onlab.osgi.ServiceNotFoundException;
import org.slf4j.Logger;
+
import com.fasterxml.jackson.databind.JsonNode;
import java.util.EnumSet;
@@ -136,6 +140,25 @@
}
/**
+ * Finds the NIC name that corresponds to a device's port number.
+ *
+ * @param deviceId a device ID
+ * @param port a NIC port number
+ * @return device's NIC name
+ */
+ public static String findNicInterfaceWithPort(DeviceId deviceId, long port) {
+ RestServerSBDevice device = null;
+ try {
+ device = (RestServerSBDevice) controller.getDevice(deviceId);
+ } catch (ClassCastException ccEx) {
+ return null;
+ }
+ checkNotNull(device, DEVICE_NULL);
+
+ return device.portNameFromNumber(port);
+ }
+
+ /**
* Return all the enumeration's types in a space-separated string.
*
* @param <E> the expected class of the enum
diff --git a/drivers/server/src/main/java/org/onosproject/drivers/server/FlowRuleProgrammableServerImpl.java b/drivers/server/src/main/java/org/onosproject/drivers/server/FlowRuleProgrammableServerImpl.java
index 7177dc0..efc7bd6 100644
--- a/drivers/server/src/main/java/org/onosproject/drivers/server/FlowRuleProgrammableServerImpl.java
+++ b/drivers/server/src/main/java/org/onosproject/drivers/server/FlowRuleProgrammableServerImpl.java
@@ -43,6 +43,8 @@
import java.util.concurrent.ConcurrentHashMap;
import javax.ws.rs.ProcessingException;
+import com.google.common.base.Strings;
+
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static org.slf4j.LoggerFactory.getLogger;
@@ -66,7 +68,6 @@
* Parameters to be exchanged with the server's agent.
*/
private static final String PARAM_RULES = "rules";
- private static final String PARAM_NIC_ID = "nicId";
private static final String PARAM_CPU_ID = "cpuId";
private static final String PARAM_CPU_RULES = "cpuRules";
private static final String PARAM_RULE_ID = "ruleId";
@@ -127,7 +128,6 @@
// Each device might have multiple NICs
for (JsonNode nicNode : scNode.path(PARAM_NICS)) {
- String nicId = get(nicNode, PARAM_NIC_ID);
JsonNode cpusNode = nicNode.path(PARAM_CPUS);
// Each NIC can dispatch to multiple CPU cores
@@ -281,20 +281,18 @@
Map<Long, ArrayNode> cpuObjSet =
new ConcurrentHashMap<Long, ArrayNode>();
- String nicId = null;
+ String nic = null;
for (FlowRule rule : rules) {
NicFlowRule nicRule = (NicFlowRule) rule;
long coreIndex = nicRule.cpuCoreIndex();
// Keep the ID of the target NIC
- if (nicId == null) {
- long nicIfaceNb = nicRule.interfaceNumber();
- checkArgument(nicIfaceNb > 0,
- "Attempted to install NIC rules on an invalid NIC ID: "
- + nicIfaceNb);
- // NIC IDs in the dataplane start from 0
- nicId = Long.toString(nicIfaceNb - 1);
+ if (nic == null) {
+ nic = findNicInterfaceWithPort(deviceId, nicRule.interfaceNumber());
+ checkArgument(
+ !Strings.isNullOrEmpty(nic),
+ "Attempted to install rules on an invalid NIC");
}
// Create a JSON array for this CPU core
@@ -314,8 +312,7 @@
}
ObjectNode nicObjNode = mapper.createObjectNode();
- // TODO: Fix this as it might cause issues
- nicObjNode.put("nicId", "fd" + nicId);
+ nicObjNode.put("nicName", nic);
ArrayNode cpusArrayNode = nicObjNode.putArray(PARAM_CPUS);
diff --git a/drivers/server/src/main/java/org/onosproject/drivers/server/ServerDevicesDiscovery.java b/drivers/server/src/main/java/org/onosproject/drivers/server/ServerDevicesDiscovery.java
index 808c1d4..0755bcd 100644
--- a/drivers/server/src/main/java/org/onosproject/drivers/server/ServerDevicesDiscovery.java
+++ b/drivers/server/src/main/java/org/onosproject/drivers/server/ServerDevicesDiscovery.java
@@ -62,6 +62,7 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ObjectNode;
+import com.google.common.base.Strings;
import com.google.common.collect.Lists;
import com.google.common.collect.ImmutableList;
@@ -80,6 +81,7 @@
import java.util.Set;
import java.util.TreeSet;
+import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static org.slf4j.LoggerFactory.getLogger;
@@ -111,7 +113,8 @@
private static final String PARAM_TIMING_STATS = "timing_stats";
private static final String PARAM_TIMING_AUTOSCALE = "autoscale_timing_stats";
- private static final String NIC_PARAM_ID = "id";
+ private static final String NIC_PARAM_NAME = "name";
+ private static final String NIC_PARAM_PORT_INDEX = "index";
private static final String NIC_PARAM_PORT_TYPE = "portType";
private static final String NIC_PARAM_PORT_TYPE_FIBER = "fiber";
private static final String NIC_PARAM_PORT_TYPE_COPPER = "copper";
@@ -284,26 +287,28 @@
Set<NicDevice> nicSet = new HashSet<NicDevice>();
JsonNode nicNode = objNode.path(PARAM_NICS);
+ DefaultAnnotations.Builder annotations = DefaultAnnotations.builder();
+
// Construct NIC objects
for (JsonNode nn : nicNode) {
ObjectNode nicObjNode = (ObjectNode) nn;
// All the NIC attributes
- String nicId = get(nn, NIC_PARAM_ID);
- int port = Integer.parseInt(nicId.replaceAll("\\D+", ""));
+ String nicName = get(nn, NIC_PARAM_NAME);
+ long nicIndex = nicObjNode.path(NIC_PARAM_PORT_INDEX).asLong();
long speed = nicObjNode.path(NIC_PARAM_SPEED).asLong();
String portTypeStr = get(nn, NIC_PARAM_PORT_TYPE);
Port.Type portType = PORT_TYPE_MAP.get(portTypeStr);
if (portType == null) {
throw new IllegalArgumentException(
- portTypeStr + " is not a valid port type for NIC " + nicId);
+ portTypeStr + " is not a valid port type for NIC " + nicName);
}
boolean status = nicObjNode.path(NIC_PARAM_STATUS).asInt() > 0;
String hwAddr = get(nn, NIC_PARAM_HW_ADDR);
JsonNode tagNode = nicObjNode.path(BasicServerDriver.NIC_PARAM_RX_FILTER);
if (tagNode == null) {
throw new IllegalArgumentException(
- "The Rx filters of NIC " + nicId + " are not reported");
+ "The Rx filters of NIC " + nicName + " are not reported");
}
// Convert the JSON list into an array of strings
@@ -325,10 +330,12 @@
rxFilterMechanism.addRxFilter(rf);
}
+ // Store NIC name to number mapping as an annotation
+ annotations.set(nicName, Long.toString(nicIndex));
+
// Construct a NIC device for this server
NicDevice nic = new DefaultNicDevice(
- nicId, port, portType, speed, status, hwAddr, rxFilterMechanism
- );
+ nicName, nicIndex, portType, speed, status, hwAddr, rxFilterMechanism);
// Add it to the set
nicSet.add(nic);
@@ -354,7 +361,7 @@
desc = new DefaultServerDeviceDescription(
new URI(id), Device.Type.SERVER, vendor,
hw, sw, serial, new ChassisId(),
- cpuSet, nicSet, DefaultAnnotations.EMPTY
+ cpuSet, nicSet, annotations.build()
);
} catch (URISyntaxException uEx) {
log.error("Failed to create a server device description for: {}",
@@ -404,26 +411,22 @@
if ((device == null) || (device.nics() == null)) {
log.error("No ports available on {}", deviceId);
- return ImmutableList.copyOf(portDescriptions);
+ return Collections.EMPTY_LIST;
}
// Sorted list of NIC ports
Set<NicDevice> nics = new TreeSet(device.nics());
// Iterate through the NICs of this device to populate the list
- long portCounter = 0;
for (NicDevice nic : nics) {
- // The port number of this NIC
- PortNumber portNumber = PortNumber.portNumber(++portCounter);
-
// Include the name of this device as an annotation
DefaultAnnotations.Builder annotations = DefaultAnnotations.builder()
- .set(AnnotationKeys.PORT_NAME, nic.id());
+ .set(AnnotationKeys.PORT_NAME, nic.name());
// Create a port description and add it to the list
portDescriptions.add(
DefaultPortDescription.builder()
- .withPortNumber(portNumber)
+ .withPortNumber(PortNumber.portNumber(nic.portNumber(), nic.name()))
.isEnabled(nic.status())
.type(nic.portType())
.portSpeed(nic.speed())
@@ -431,7 +434,7 @@
.build());
log.info("Port discovery on device {}: NIC {} is {} at {} Mbps",
- deviceId, nic.port(), nic.status() ? "up" : "down",
+ deviceId, nic.portNumber(), nic.status() ? "up" : "down",
nic.speed());
}
@@ -455,9 +458,6 @@
* @return list of (per port) PortStatistics
*/
private Collection<PortStatistics> getPortStatistics(DeviceId deviceId) {
- // List of port statistics to return
- Collection<PortStatistics> portStats = null;
-
// Get global monitoring statistics
MonitoringStatistics monStats = getGlobalMonitoringStatistics(deviceId);
if (monStats == null) {
@@ -465,7 +465,7 @@
}
// Filter out the NIC statistics
- portStats = monStats.nicStatisticsAll();
+ Collection<PortStatistics> portStats = monStats.nicStatisticsAll();
if (portStats == null) {
return Collections.EMPTY_LIST;
}
@@ -492,9 +492,6 @@
* @return list of (per core) CpuStatistics
*/
public Collection<CpuStatistics> getCpuStatistics(DeviceId deviceId) {
- // List of port statistics to return
- Collection<CpuStatistics> cpuStats = null;
-
// Get global monitoring statistics
MonitoringStatistics monStats = getGlobalMonitoringStatistics(deviceId);
if (monStats == null) {
@@ -502,7 +499,7 @@
}
// Filter out the CPU statistics
- cpuStats = monStats.cpuStatisticsAll();
+ Collection<CpuStatistics> cpuStats = monStats.cpuStatisticsAll();
if (cpuStats == null) {
return Collections.EMPTY_LIST;
}
@@ -716,12 +713,12 @@
*/
private Collection<CpuStatistics> parseCpuStatistics(
DeviceId deviceId, JsonNode objNode) {
- Collection<CpuStatistics> cpuStats = Lists.newArrayList();
-
if (objNode == null) {
- return cpuStats;
+ return Collections.EMPTY_LIST;
}
+ Collection<CpuStatistics> cpuStats = Lists.newArrayList();
+
JsonNode cpuNode = objNode.path(BasicServerDriver.PARAM_CPUS);
for (JsonNode cn : cpuNode) {
@@ -760,20 +757,31 @@
*/
private Collection<PortStatistics> parseNicStatistics(
DeviceId deviceId, JsonNode objNode) {
- Collection<PortStatistics> nicStats = Lists.newArrayList();
-
if (objNode == null) {
- return nicStats;
+ return Collections.EMPTY_LIST;
}
+ RestServerSBDevice device = null;
+ try {
+ device = (RestServerSBDevice) getController().getDevice(deviceId);
+ } catch (ClassCastException ccEx) {
+ return Collections.EMPTY_LIST;
+ }
+ checkNotNull(device, DEVICE_NULL);
+
+ Collection<PortStatistics> nicStats = Lists.newArrayList();
+
JsonNode nicNode = objNode.path(PARAM_NICS);
for (JsonNode nn : nicNode) {
ObjectNode nicObjNode = (ObjectNode) nn;
// All the NIC attributes
- String nicId = get(nn, NIC_PARAM_ID);
- int port = Integer.parseInt(nicId.replaceAll("\\D+", ""));
+ String nicName = get(nn, NIC_PARAM_NAME);
+ checkArgument(!Strings.isNullOrEmpty(nicName), "NIC name is empty or NULL");
+
+ long portNumber = device.portNumberFromName(nicName);
+ checkArgument(portNumber >= 0, "Unknown port ID " + portNumber + " for NIC " + nicName);
long rxCount = nicObjNode.path(NIC_STATS_RX_COUNT).asLong();
long rxBytes = nicObjNode.path(NIC_STATS_RX_BYTES).asLong();
@@ -789,7 +797,7 @@
DefaultPortStatistics.builder();
nicBuilder.setDeviceId(deviceId)
- .setPort(port)
+ .setPort((int) portNumber)
.setPacketsReceived(rxCount)
.setPacketsSent(txCount)
.setBytesReceived(rxBytes)
diff --git a/drivers/server/src/main/java/org/onosproject/drivers/server/devices/RestServerSBDevice.java b/drivers/server/src/main/java/org/onosproject/drivers/server/devices/RestServerSBDevice.java
index f57e734..0d64248 100644
--- a/drivers/server/src/main/java/org/onosproject/drivers/server/devices/RestServerSBDevice.java
+++ b/drivers/server/src/main/java/org/onosproject/drivers/server/devices/RestServerSBDevice.java
@@ -53,4 +53,20 @@
*/
int numberOfNics();
+ /**
+ * Returns the port number of a specific NIC.
+ *
+ * @param portName name of the NIC
+ * @return NIC port number
+ */
+ long portNumberFromName(String portName);
+
+ /**
+ * Returns the port name of a specific NIC.
+ *
+ * @param portNumber NIC port number
+ * @return NIC name
+ */
+ String portNameFromNumber(long portNumber);
+
}
diff --git a/drivers/server/src/main/java/org/onosproject/drivers/server/devices/nic/NicDevice.java b/drivers/server/src/main/java/org/onosproject/drivers/server/devices/nic/NicDevice.java
index 91cd7f9..c3de14e 100644
--- a/drivers/server/src/main/java/org/onosproject/drivers/server/devices/nic/NicDevice.java
+++ b/drivers/server/src/main/java/org/onosproject/drivers/server/devices/nic/NicDevice.java
@@ -27,37 +27,37 @@
public interface NicDevice extends Comparable {
/**
- * Returns the ID of this NIC.
+ * Returns the name of this NIC.
*
- * @return NIC ID
+ * @return NIC name
*/
- String id();
+ String name();
/**
* Returns the port number of this NIC.
*
- * @return integer port number for the NIC
+ * @return NIC port number
*/
- int port();
+ long portNumber();
/**
* Returns the type of the port of this NIC.
*
- * @return port type
+ * @return NIC port type
*/
Type portType();
/**
* Returns the speed of the NIC in Mbps.
*
- * @return integer NIC speed in Mbps
+ * @return NIC speed in Mbps
*/
long speed();
/**
* Returns the current status of the NIC.
*
- * @return boolean NIC status (up=true, down=false)
+ * @return NIC status (up=true, down=false)
*/
boolean status();
@@ -71,7 +71,7 @@
/**
* Returns the MAC address of the NIC.
*
- * @return MacAddress hardware address of the NIC
+ * @return hardware address of the NIC
*/
MacAddress macAddress();
diff --git a/drivers/server/src/main/java/org/onosproject/drivers/server/impl/devices/DefaultNicDevice.java b/drivers/server/src/main/java/org/onosproject/drivers/server/impl/devices/DefaultNicDevice.java
index 3ff5e65..9f782d8 100644
--- a/drivers/server/src/main/java/org/onosproject/drivers/server/impl/devices/DefaultNicDevice.java
+++ b/drivers/server/src/main/java/org/onosproject/drivers/server/impl/devices/DefaultNicDevice.java
@@ -22,6 +22,7 @@
import org.onlab.packet.MacAddress;
import com.google.common.base.MoreObjects;
+import com.google.common.base.Strings;
import java.util.Objects;
@@ -34,8 +35,8 @@
*/
public class DefaultNicDevice implements NicDevice, Comparable {
- private final String id;
- private final int port;
+ private final String name;
+ private final long portNumber;
private final long speed;
private final Type portType;
private boolean status;
@@ -46,21 +47,18 @@
public static final long MAX_SPEED = 200000;
public DefaultNicDevice(
- String id,
- int port,
+ String name,
+ long portNumber,
Type portType,
long speed,
boolean status,
String macStr,
NicRxFilter rxFilterMechanisms) {
- checkNotNull(id, "NIC ID cannot be null");
- checkArgument(!id.isEmpty(), "NIC ID cannot be empty");
- checkArgument(port >= 0, "NIC port number must be non-negative");
+ checkArgument(!Strings.isNullOrEmpty(name), "NIC name cannot be empty or NULL");
+ checkArgument(portNumber >= 0, "NIC port number must be non-negative");
checkNotNull(portType, "NIC port type cannot be null");
- checkArgument(
- (speed >= 0) && (speed <= MAX_SPEED),
- "NIC speed must be positive and less or equal than " + MAX_SPEED + " Mbps"
- );
+ checkArgument((speed >= 0) && (speed <= MAX_SPEED),
+ "NIC speed must be positive and less or equal than " + MAX_SPEED + " Mbps");
checkNotNull(macStr, "NIC MAC address cannot be null");
checkNotNull(rxFilterMechanisms, "NIC Rx filter mechanisms cannot be null");
@@ -69,8 +67,8 @@
status = false;
}
- this.id = id;
- this.port = port;
+ this.name = name;
+ this.portNumber = portNumber;
this.speed = speed;
this.portType = portType;
this.status = status;
@@ -79,13 +77,13 @@
}
@Override
- public String id() {
- return this.id;
+ public String name() {
+ return this.name;
}
@Override
- public int port() {
- return this.port;
+ public long portNumber() {
+ return this.portNumber;
}
@Override
@@ -133,8 +131,8 @@
public String toString() {
return MoreObjects.toStringHelper(this)
.omitNullValues()
- .add("id", id())
- .add("port", port())
+ .add("name", name())
+ .add("port", portNumber())
.add("mac", macAddress.toString())
.add("portType", portType())
.add("speed", speed())
@@ -152,15 +150,15 @@
return false;
}
NicDevice device = (NicDevice) obj;
- return this.id().equals(device.id()) &&
- this.port() == device.port() &&
+ return this.name().equals(device.name()) &&
+ this.portNumber() == device.portNumber() &&
this.speed == device.speed() &&
this.macAddress.equals(device.macAddress());
}
@Override
public int hashCode() {
- return Objects.hash(id, port, speed, macAddress);
+ return Objects.hash(name, portNumber, speed, macAddress);
}
@Override
@@ -176,9 +174,9 @@
if (other instanceof NicDevice) {
NicDevice otherNic = (NicDevice) other;
- if (this.port() == otherNic.port()) {
+ if (this.portNumber() == otherNic.portNumber()) {
return 0;
- } else if (this.port() > otherNic.port()) {
+ } else if (this.portNumber() > otherNic.portNumber()) {
return 1;
} else {
return -1;
diff --git a/drivers/server/src/main/java/org/onosproject/drivers/server/impl/devices/DefaultRestServerSBDevice.java b/drivers/server/src/main/java/org/onosproject/drivers/server/impl/devices/DefaultRestServerSBDevice.java
index 410a5b4..cc2e541 100644
--- a/drivers/server/src/main/java/org/onosproject/drivers/server/impl/devices/DefaultRestServerSBDevice.java
+++ b/drivers/server/src/main/java/org/onosproject/drivers/server/impl/devices/DefaultRestServerSBDevice.java
@@ -25,6 +25,7 @@
import org.onlab.packet.IpAddress;
import com.google.common.base.MoreObjects;
+import com.google.common.base.Strings;
import com.google.common.collect.Lists;
import java.util.Objects;
@@ -91,6 +92,36 @@
}
@Override
+ public long portNumberFromName(String portName) {
+ if (Strings.isNullOrEmpty(portName)) {
+ return -1;
+ }
+
+ for (NicDevice nic : this.nics) {
+ if (nic.name().equals(portName)) {
+ return nic.portNumber();
+ }
+ }
+
+ return -1;
+ }
+
+ @Override
+ public String portNameFromNumber(long portNumber) {
+ if (portNumber < 0) {
+ return "";
+ }
+
+ for (NicDevice nic : this.nics) {
+ if (nic.portNumber() == portNumber) {
+ return nic.name();
+ }
+ }
+
+ return "";
+ }
+
+ @Override
public String toString() {
return MoreObjects.toStringHelper(this)
.omitNullValues()
diff --git a/drivers/server/src/main/java/org/onosproject/drivers/server/impl/stats/DefaultCpuStatistics.java b/drivers/server/src/main/java/org/onosproject/drivers/server/impl/stats/DefaultCpuStatistics.java
index 43b7273..f9c1989 100644
--- a/drivers/server/src/main/java/org/onosproject/drivers/server/impl/stats/DefaultCpuStatistics.java
+++ b/drivers/server/src/main/java/org/onosproject/drivers/server/impl/stats/DefaultCpuStatistics.java
@@ -49,11 +49,11 @@
checkNotNull(deviceId, "Device ID is NULL");
checkArgument(
(id >= 0) && (id < MAX_CPU_NB),
- "CPU core ID must be in [0, " + String.valueOf(MAX_CPU_NB - 1) + "]"
+ "Invalid CPU core ID " + String.valueOf(id) + ", not in [0, " + String.valueOf(MAX_CPU_NB - 1) + "]"
);
checkArgument(
(load >= MIN_CPU_LOAD) && (load <= MAX_CPU_LOAD),
- "CPU load must be in [" + MIN_CPU_LOAD + ", " + MAX_CPU_LOAD + "]"
+ "Invalid CPU load " + Float.toString(load) + ", not in [" + MIN_CPU_LOAD + ", " + MAX_CPU_LOAD + "]"
);
this.deviceId = deviceId;
diff --git a/protocols/rest/api/src/main/java/org/onosproject/protocol/rest/RestSBDevice.java b/protocols/rest/api/src/main/java/org/onosproject/protocol/rest/RestSBDevice.java
index 6767b2d..0a22764 100644
--- a/protocols/rest/api/src/main/java/org/onosproject/protocol/rest/RestSBDevice.java
+++ b/protocols/rest/api/src/main/java/org/onosproject/protocol/rest/RestSBDevice.java
@@ -43,7 +43,7 @@
IpAddress ip();
/**
- * Returns the password of this device.
+ * Returns the port of this device.
*
* @return port
*/