Address comments in gerrit 20327
Change-Id: Icadd58a401d32362a826b5ac33bdffec15ca8169
diff --git a/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbAdminService.java b/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbAdminService.java
index b429fe1..d5042d0 100644
--- a/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbAdminService.java
+++ b/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbAdminService.java
@@ -15,33 +15,30 @@
*/
package org.onosproject.l2lb.api;
-import org.onosproject.net.DeviceId;
import org.onosproject.net.PortNumber;
import java.util.Set;
/**
- * LACP admin service.
+ * L2 load balancer admin service.
*/
public interface L2LbAdminService {
/**
* Creates or updates a L2 load balancer.
*
- * @param deviceId Device ID
- * @param key L2 load balancer key
+ * @param l2LbId L2 load balancer id
* @param ports physical ports in the L2 load balancer
* @param mode L2 load balancer mode
* @return L2 load balancer that is created or updated
*/
- L2Lb createOrUpdate(DeviceId deviceId, int key, Set<PortNumber> ports, L2LbMode mode);
+ L2Lb createOrUpdate(L2LbId l2LbId, Set<PortNumber> ports, L2LbMode mode);
/**
* Removes a L2 load balancer.
*
- * @param deviceId Device ID
- * @param key L2 load balancer key
+ * @param l2LbId L2 load balancer id
* @return L2 load balancer that is removed or null if it was not possible
*/
- L2Lb remove(DeviceId deviceId, int key);
+ L2Lb remove(L2LbId l2LbId);
}
diff --git a/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbEvent.java b/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbEvent.java
index a0ce9ce..96cb3b8 100644
--- a/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbEvent.java
+++ b/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbEvent.java
@@ -22,6 +22,9 @@
import static com.google.common.base.MoreObjects.toStringHelper;
+/**
+ * L2 load balancer event.
+ */
public class L2LbEvent extends AbstractEvent<L2LbEvent.Type, L2LbData> {
private L2LbData prevSubject;
diff --git a/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbId.java b/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbId.java
index 65aecef..68bcc47 100644
--- a/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbId.java
+++ b/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbId.java
@@ -19,8 +19,17 @@
import java.util.Objects;
+/**
+ * L2 load balancer identifier.
+ * It is used to identify a load balancer across the entire system and therefore has to be unique system-wide.
+ */
public class L2LbId {
private final DeviceId deviceId;
+
+ /**
+ * L2 load balancer key.
+ * It is used to identify a load balancer on a specific device and therefore has to be unique device-wide.
+ */
private final int key;
/**
@@ -73,6 +82,6 @@
@Override
public String toString() {
- return deviceId.toString() + " (" + key + ")";
+ return deviceId.toString() + ":" + key;
}
}
diff --git a/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbMode.java b/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbMode.java
index 9c38bd6..cec6168 100644
--- a/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbMode.java
+++ b/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbMode.java
@@ -21,11 +21,16 @@
public enum L2LbMode {
/**
* Static L2 load balancer.
+ *
+ * In STATIC mode, all member ports of this load balancer will be used to transmit data as long as it is enabled.
*/
STATIC,
/**
* L2 load balancer based on LACP.
+ *
+ * In LACP mode, we decide whether to use a member port to transmit data or not according to
+ * the LACP negotiation result.
*/
LACP
}
diff --git a/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbService.java b/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbService.java
index ce396ac..50a6c79 100644
--- a/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbService.java
+++ b/apps/l2lb/src/main/java/org/onosproject/l2lb/api/L2LbService.java
@@ -18,7 +18,6 @@
import org.onosproject.core.ApplicationId;
import org.onosproject.event.ListenerService;
-import org.onosproject.net.DeviceId;
import java.util.Map;
@@ -36,14 +35,14 @@
/**
* Gets L2 load balancer that matches given device ID and key, or null if not found.
*
- * @param deviceId Device ID
- * @param key L2 load balancer key
+ * @param l2LbId L2 load balancer id
* @return L2 load balancer information
*/
- L2Lb getL2Lb(DeviceId deviceId, int key);
+ L2Lb getL2Lb(L2LbId l2LbId);
/**
- * Gets L2 load balancer next ids from the store.
+ * Gets the mapping between L2 load balancer id and
+ * the next objective id that represents the L2 load balancer.
*
* @return L2 load balancer id and next id mapping
*/
@@ -52,11 +51,10 @@
/**
* Gets L2 load balancer next id that matches given device Id and key, or null if not found.
*
- * @param deviceId Device ID
- * @param key L2 load balancer key
+ * @param l2LbId L2 load balancer id
* @return next ID
*/
- int getL2LbNext(DeviceId deviceId, int key);
+ int getL2LbNext(L2LbId l2LbId);
/**
* Reserves a l2 load balancer. Only one application
diff --git a/apps/l2lb/src/main/java/org/onosproject/l2lb/app/L2LbManager.java b/apps/l2lb/src/main/java/org/onosproject/l2lb/app/L2LbManager.java
index 8de2b7d..603684f 100644
--- a/apps/l2lb/src/main/java/org/onosproject/l2lb/app/L2LbManager.java
+++ b/apps/l2lb/src/main/java/org/onosproject/l2lb/app/L2LbManager.java
@@ -16,6 +16,7 @@
package org.onosproject.l2lb.app;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;
import org.onlab.util.KryoNamespace;
import org.onosproject.cluster.ClusterService;
@@ -44,13 +45,12 @@
import org.onosproject.net.flow.instructions.Instruction;
import org.onosproject.net.flow.instructions.Instructions;
import org.onosproject.net.flowobjective.DefaultNextObjective;
+import org.onosproject.net.flowobjective.DefaultNextTreatment;
import org.onosproject.net.flowobjective.FlowObjectiveService;
import org.onosproject.net.flowobjective.NextObjective;
import org.onosproject.net.flowobjective.Objective;
import org.onosproject.net.flowobjective.ObjectiveContext;
import org.onosproject.net.flowobjective.ObjectiveError;
-import org.onosproject.net.intf.InterfaceService;
-import org.onosproject.net.packet.PacketService;
import org.onosproject.store.serializers.KryoNamespaces;
import org.onosproject.store.service.ConsistentMap;
import org.onosproject.store.service.MapEvent;
@@ -88,12 +88,6 @@
private CoreService coreService;
@Reference(cardinality = ReferenceCardinality.MANDATORY)
- private PacketService packetService;
-
- @Reference(cardinality = ReferenceCardinality.MANDATORY)
- private InterfaceService interfaceService;
-
- @Reference(cardinality = ReferenceCardinality.MANDATORY)
private StorageService storageService;
@Reference(cardinality = ReferenceCardinality.MANDATORY)
@@ -198,15 +192,13 @@
}
@Override
- public L2Lb createOrUpdate(DeviceId deviceId, int key, Set<PortNumber> ports, L2LbMode mode) {
- L2LbId l2LbId = new L2LbId(deviceId, key);
+ public L2Lb createOrUpdate(L2LbId l2LbId, Set<PortNumber> ports, L2LbMode mode) {
log.debug("Putting {} -> {} {} into L2 load balancer store", l2LbId, mode, ports);
return Versioned.valueOrNull(l2LbStore.put(l2LbId, new L2Lb(l2LbId, ports, mode)));
}
@Override
- public L2Lb remove(DeviceId deviceId, int key) {
- L2LbId l2LbId = new L2LbId(deviceId, key);
+ public L2Lb remove(L2LbId l2LbId) {
ApplicationId reservation = Versioned.valueOrNull(l2LbResStore.get(l2LbId));
// Remove only if it is not used - otherwise it is necessary to release first
if (reservation == null) {
@@ -220,22 +212,22 @@
@Override
public Map<L2LbId, L2Lb> getL2Lbs() {
- return l2LbStore.asJavaMap();
+ return ImmutableMap.copyOf(l2LbStore.asJavaMap());
}
@Override
- public L2Lb getL2Lb(DeviceId deviceId, int key) {
- return Versioned.valueOrNull(l2LbStore.get(new L2LbId(deviceId, key)));
+ public L2Lb getL2Lb(L2LbId l2LbId) {
+ return Versioned.valueOrNull(l2LbStore.get(l2LbId));
}
@Override
public Map<L2LbId, Integer> getL2LbNexts() {
- return l2LbNextStore.asJavaMap();
+ return ImmutableMap.copyOf(l2LbNextStore.asJavaMap());
}
@Override
- public int getL2LbNext(DeviceId deviceId, int key) {
- return Versioned.valueOrNull(l2LbNextStore.get(new L2LbId(deviceId, key)));
+ public int getL2LbNext(L2LbId l2LbId) {
+ return Versioned.valueOrNull(l2LbNextStore.get(l2LbId));
}
@Override
@@ -473,7 +465,10 @@
if (nextId == null) {
nextId = flowObjService.allocateNextId();
}
- // TODO replace logical l2lb port
+ // This metadata is used to pass the key to the driver.
+ // Some driver, e.g. OF-DPA, will use that information while creating load balancing group.
+ // TODO This is not an actual LAG port. In the future, we should extend metadata structure to carry
+ // generic information. We should avoid using in_port in the metadata once generic metadata is available.
TrafficSelector meta = DefaultTrafficSelector.builder()
.matchInPort(PortNumber.portNumber(l2LbId.key())).build();
NextObjective.Builder nextObjBuilder = DefaultNextObjective.builder()
@@ -483,7 +478,7 @@
.fromApp(appId);
ports.forEach(port -> {
TrafficTreatment treatment = DefaultTrafficTreatment.builder().setOutput(port).build();
- nextObjBuilder.addTreatment(treatment);
+ nextObjBuilder.addTreatment(DefaultNextTreatment.of(treatment));
});
return nextObjBuilder;
}
@@ -514,15 +509,16 @@
@Override
public void onSuccess(Objective objective) {
NextObjective nextObj = (NextObjective) objective;
- log.debug("Success {} nextobj {} for L2 load balancer {}", nextObj.op(), nextObj, l2LbId);
+ log.debug("Successfully {} nextobj {} for L2 load balancer {}. NextObj={}",
+ nextObj.op(), nextObj.id(), l2LbId, nextObj);
l2LbProvExecutor.execute(() -> onSuccessHandler(nextObj, l2LbId));
}
@Override
public void onError(Objective objective, ObjectiveError error) {
NextObjective nextObj = (NextObjective) objective;
- log.debug("Failed {} nextobj {} for L2 load balancer {} due to {}", nextObj.op(), nextObj,
- l2LbId, error);
+ log.warn("Failed to {} nextobj {} for L2 load balancer {} due to {}. NextObj={}",
+ nextObj.op(), nextObj.id(), l2LbId, error, nextObj);
l2LbProvExecutor.execute(() -> onErrorHandler(nextObj, l2LbId));
}
}
diff --git a/apps/l2lb/src/main/java/org/onosproject/l2lb/cli/L2LbAddCommand.java b/apps/l2lb/src/main/java/org/onosproject/l2lb/cli/L2LbAddCommand.java
index f3a593c..98f6886 100644
--- a/apps/l2lb/src/main/java/org/onosproject/l2lb/cli/L2LbAddCommand.java
+++ b/apps/l2lb/src/main/java/org/onosproject/l2lb/cli/L2LbAddCommand.java
@@ -63,15 +63,16 @@
@Override
protected void doExecute() {
DeviceId deviceId = DeviceId.deviceId(deviceIdStr);
- int l2LbPort = Integer.parseInt(keyStr);
+ int l2LbKey = Integer.parseInt(keyStr);
L2LbMode mode = L2LbMode.valueOf(modeStr.toUpperCase());
Set<PortNumber> ports = Sets.newHashSet(portsStr).stream()
.map(PortNumber::fromString).collect(Collectors.toSet());
L2LbAdminService l2LbAdminService = get(L2LbAdminService.class);
- L2Lb l2Lb = l2LbAdminService.createOrUpdate(deviceId, l2LbPort, ports, mode);
- print("%s of %s executed", l2Lb == null ? CREATE : UPDATE, new L2LbId(deviceId, l2LbPort));
+ L2LbId l2LbId = new L2LbId(deviceId, l2LbKey);
+ L2Lb l2Lb = l2LbAdminService.createOrUpdate(l2LbId, ports, mode);
+ print("%s of %s executed", l2Lb == null ? CREATE : UPDATE, l2LbId);
}
}
diff --git a/apps/l2lb/src/main/java/org/onosproject/l2lb/cli/L2LbRemoveCommand.java b/apps/l2lb/src/main/java/org/onosproject/l2lb/cli/L2LbRemoveCommand.java
index 1f7b5c5..70920d9 100644
--- a/apps/l2lb/src/main/java/org/onosproject/l2lb/cli/L2LbRemoveCommand.java
+++ b/apps/l2lb/src/main/java/org/onosproject/l2lb/cli/L2LbRemoveCommand.java
@@ -47,10 +47,11 @@
@Override
protected void doExecute() {
DeviceId deviceId = DeviceId.deviceId(deviceIdStr);
- int l2LbPort = Integer.parseInt(keyStr);
+ int l2LbKey = Integer.parseInt(keyStr);
L2LbAdminService l2LbAdminService = get(L2LbAdminService.class);
- L2Lb l2Lb = l2LbAdminService.remove(deviceId, l2LbPort);
- print("Removal of %s %s", new L2LbId(deviceId, l2LbPort), l2Lb != null ? EXECUTED : FAILED);
+ L2LbId l2LbId = new L2LbId(deviceId, l2LbKey);
+ L2Lb l2Lb = l2LbAdminService.remove(l2LbId);
+ print("Removal of %s %s", l2LbId, l2Lb != null ? EXECUTED : FAILED);
}
}
diff --git a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/xconnect/impl/XconnectManager.java b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/xconnect/impl/XconnectManager.java
index e135f04..d8f03aa 100644
--- a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/xconnect/impl/XconnectManager.java
+++ b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/xconnect/impl/XconnectManager.java
@@ -1239,8 +1239,9 @@
}
String l2LbKey = port.substring("L2LB(".length(), port.length() - 1);
+ L2LbId l2LbId = new L2LbId(deviceId, Integer.parseInt(l2LbKey));
try {
- return Sets.newHashSet(l2LbService.getL2Lb(deviceId, Integer.parseInt(l2LbKey)).ports());
+ return Sets.newHashSet(l2LbService.getL2Lb(l2LbId).ports());
} catch (NumberFormatException e) {
log.debug("Port {} is not load balancer key either. Ignore", port);
} catch (NullPointerException e) {
@@ -1262,11 +1263,11 @@
String l2LbKey = port.substring("L2LB(".length(), port.length() - 1);
try {
+ L2LbId l2LbId = new L2LbId(deviceId, Integer.parseInt(l2LbKey));
NextTreatment idNextTreatment = IdNextTreatment.of(
- l2LbService.getL2LbNext(deviceId, Integer.parseInt(l2LbKey)));
+ l2LbService.getL2LbNext(l2LbId));
// Reserve only one time during next objective creation
if (reserve) {
- L2LbId l2LbId = new L2LbId(deviceId, Integer.parseInt(l2LbKey));
if (!l2LbService.reserve(new L2LbId(deviceId, Integer.parseInt(l2LbKey)), appId)) {
log.warn("Reservation failed for {}", l2LbId);
idNextTreatment = null;