[ONOS-7605] Unable to modify groups on BMV2
Change-Id: I797366ac152dccac75f8ed82b62abe6e5da6dd3e
diff --git a/apps/p4runtime-test/src/test/java/org/onosproject/p4runtime/test/P4RuntimeTest.java b/apps/p4runtime-test/src/test/java/org/onosproject/p4runtime/test/P4RuntimeTest.java
index 6c320ad..47381bc 100644
--- a/apps/p4runtime-test/src/test/java/org/onosproject/p4runtime/test/P4RuntimeTest.java
+++ b/apps/p4runtime-test/src/test/java/org/onosproject/p4runtime/test/P4RuntimeTest.java
@@ -269,7 +269,8 @@
.withId(groupId)
.addMembers(members)
.build();
- CompletableFuture<Boolean> success = client.writeActionGroupMembers(actionGroup,
+ CompletableFuture<Boolean> success = client.writeActionGroupMembers(actionGroup.actionProfileId(),
+ actionGroup.members(),
P4RuntimeClient.WriteOperationType.INSERT,
bmv2DefaultPipeconf);
assert (success.get());
diff --git a/core/api/src/main/java/org/onosproject/net/pi/runtime/PiActionGroup.java b/core/api/src/main/java/org/onosproject/net/pi/runtime/PiActionGroup.java
index dd90f9b..a22e039 100644
--- a/core/api/src/main/java/org/onosproject/net/pi/runtime/PiActionGroup.java
+++ b/core/api/src/main/java/org/onosproject/net/pi/runtime/PiActionGroup.java
@@ -26,7 +26,6 @@
import java.util.Collection;
import java.util.Map;
-import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
/**
@@ -179,7 +178,6 @@
*/
public PiActionGroup build() {
checkNotNull(id);
- checkArgument(!members.isEmpty(), "Members cannot be empty");
checkNotNull(piActionProfileId);
return new PiActionGroup(id, ImmutableSet.copyOf(members.values()),
piActionProfileId);
diff --git a/drivers/p4runtime/src/main/java/org/onosproject/drivers/p4runtime/P4RuntimeGroupProgrammable.java b/drivers/p4runtime/src/main/java/org/onosproject/drivers/p4runtime/P4RuntimeGroupProgrammable.java
index 31ee45b..125cd8a 100644
--- a/drivers/p4runtime/src/main/java/org/onosproject/drivers/p4runtime/P4RuntimeGroupProgrammable.java
+++ b/drivers/p4runtime/src/main/java/org/onosproject/drivers/p4runtime/P4RuntimeGroupProgrammable.java
@@ -17,6 +17,7 @@
package org.onosproject.drivers.p4runtime;
import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
import org.onosproject.drivers.p4runtime.mirror.P4RuntimeGroupMirror;
import org.onosproject.drivers.p4runtime.mirror.TimedEntry;
import org.onosproject.net.DeviceId;
@@ -30,6 +31,7 @@
import org.onosproject.net.pi.model.PiActionProfileModel;
import org.onosproject.net.pi.runtime.PiActionGroup;
import org.onosproject.net.pi.runtime.PiActionGroupHandle;
+import org.onosproject.net.pi.runtime.PiActionGroupMember;
import org.onosproject.net.pi.service.PiGroupTranslator;
import org.onosproject.net.pi.service.PiTranslatedEntity;
import org.onosproject.net.pi.service.PiTranslationException;
@@ -48,6 +50,7 @@
import static org.onosproject.p4runtime.api.P4RuntimeClient.WriteOperationType.DELETE;
import static org.onosproject.p4runtime.api.P4RuntimeClient.WriteOperationType.INSERT;
+import static org.onosproject.p4runtime.api.P4RuntimeClient.WriteOperationType.MODIFY;
import static org.slf4j.LoggerFactory.getLogger;
/**
@@ -57,14 +60,11 @@
extends AbstractP4RuntimeHandlerBehaviour
implements GroupProgrammable {
- private enum Operation {
- APPLY, REMOVE
- }
-
private static final String ACT_GRP_MEMS_STR = "action group members";
private static final String DELETE_STR = "delete";
private static final String ACT_GRP_STR = "action group";
private static final String INSERT_STR = "insert";
+ private static final String MODIFY_STR = "modify";
private static final Logger log = getLogger(P4RuntimeGroupProgrammable.class);
@@ -143,21 +143,8 @@
final Lock lock = GROUP_LOCKS.computeIfAbsent(handle, k -> new ReentrantLock());
lock.lock();
try {
- final Operation operation;
- switch (groupOp.opType()) {
- case ADD:
- case MODIFY:
- operation = Operation.APPLY;
- break;
- case DELETE:
- operation = Operation.REMOVE;
- break;
- default:
- log.warn("Group operation {} not supported", groupOp.opType());
- return;
- }
processPiGroup(handle, piGroup,
- groupOnDevice, pdGroup, operation);
+ groupOnDevice, pdGroup, groupOp.opType());
} finally {
lock.unlock();
}
@@ -166,37 +153,103 @@
private void processPiGroup(PiActionGroupHandle handle,
PiActionGroup groupToApply,
PiActionGroup groupOnDevice,
- Group pdGroup, Operation operation) {
- if (operation == Operation.APPLY) {
+ Group pdGroup, GroupOperation.Type operationType) {
+ if (operationType == GroupOperation.Type.ADD) {
if (groupOnDevice != null) {
- if (checkMirrorBeforeUpdate
- && groupOnDevice.equals(groupToApply)) {
- // Group on device has the same members, ignore operation.
- return;
- }
- // Remove before adding it.
- processPiGroup(handle, groupToApply, groupOnDevice,
- pdGroup, Operation.REMOVE);
+ log.warn("Unable to add group {} since group already on device {}",
+ groupToApply.id(), deviceId);
+ log.debug("To apply: {}", groupToApply);
+ log.debug("On device: {}", groupOnDevice);
+ return;
}
+
if (writeGroupToDevice(groupToApply)) {
groupMirror.put(handle, groupToApply);
translator.learn(handle, new PiTranslatedEntity<>(
pdGroup, groupToApply, handle));
}
+ } else if (operationType == GroupOperation.Type.MODIFY) {
+ if (groupOnDevice == null) {
+ log.warn("Group {} does not exists on device {}, can not modify it",
+ groupToApply.id(), deviceId);
+ return;
+ }
+ if (checkMirrorBeforeUpdate
+ && groupOnDevice.equals(groupToApply)) {
+ // Group on device has the same members, ignore operation.
+ return;
+ }
+ if (modifyGroupFromDevice(groupToApply, groupOnDevice)) {
+ groupMirror.put(handle, groupToApply);
+ translator.learn(handle,
+ new PiTranslatedEntity<>(pdGroup, groupToApply, handle));
+ }
} else {
- if (deleteGroupFromDevice(groupToApply)) {
+ if (groupOnDevice == null) {
+ log.warn("Unable to remove group {} from device {} since it does" +
+ "not exists on device.", groupToApply.id(), deviceId);
+ return;
+ }
+ if (deleteGroupFromDevice(groupOnDevice)) {
groupMirror.remove(handle);
translator.forget(handle);
}
}
}
+ private boolean modifyGroupFromDevice(PiActionGroup groupToApply, PiActionGroup groupOnDevice) {
+ PiActionProfileId groupProfileId = groupToApply.actionProfileId();
+ Collection<PiActionGroupMember> membersToRemove = Sets.newHashSet(groupOnDevice.members());
+ membersToRemove.removeAll(groupToApply.members());
+ Collection<PiActionGroupMember> membersToAdd = Sets.newHashSet(groupToApply.members());
+ membersToAdd.removeAll(groupOnDevice.members());
+
+ if (!membersToAdd.isEmpty() &&
+ !completeFuture(client.writeActionGroupMembers(groupProfileId, membersToAdd, INSERT, pipeconf),
+ ACT_GRP_MEMS_STR, INSERT_STR)) {
+ // remove what we added
+ completeFuture(client.writeActionGroupMembers(groupProfileId, membersToAdd, DELETE, pipeconf),
+ ACT_GRP_MEMS_STR, INSERT_STR);
+ return false;
+ }
+
+ if (!completeFuture(client.writeActionGroup(groupToApply, MODIFY, pipeconf),
+ ACT_GRP_STR, MODIFY_STR)) {
+ // recover group information
+ completeFuture(client.writeActionGroup(groupOnDevice, MODIFY, pipeconf),
+ ACT_GRP_STR, MODIFY_STR);
+ // remove what we added
+ completeFuture(client.writeActionGroupMembers(groupProfileId, membersToAdd, DELETE, pipeconf),
+ ACT_GRP_MEMS_STR, INSERT_STR);
+ return false;
+ }
+
+ if (!membersToRemove.isEmpty() &&
+ !completeFuture(client.writeActionGroupMembers(groupProfileId, membersToRemove, DELETE, pipeconf),
+ ACT_GRP_MEMS_STR, DELETE_STR)) {
+ // add what we removed
+ completeFuture(client.writeActionGroupMembers(groupProfileId, membersToRemove, INSERT, pipeconf),
+ ACT_GRP_MEMS_STR, DELETE_STR);
+ // recover group information
+ completeFuture(client.writeActionGroup(groupOnDevice, MODIFY, pipeconf),
+ ACT_GRP_STR, MODIFY_STR);
+ // remove what we added
+ completeFuture(client.writeActionGroupMembers(groupProfileId, membersToAdd, DELETE, pipeconf),
+ ACT_GRP_MEMS_STR, INSERT_STR);
+ return false;
+ }
+
+ return true;
+ }
+
private boolean writeGroupToDevice(PiActionGroup groupToApply) {
// First insert members, then group.
// The operation is deemed successful if both operations are successful.
// FIXME: add transactional semantics, i.e. remove members if group fails.
final boolean membersSuccess = completeFuture(
- client.writeActionGroupMembers(groupToApply, INSERT, pipeconf),
+ client.writeActionGroupMembers(groupToApply.actionProfileId(),
+ groupToApply.members(),
+ INSERT, pipeconf),
ACT_GRP_MEMS_STR, INSERT_STR);
return membersSuccess && completeFuture(
client.writeActionGroup(groupToApply, INSERT, pipeconf),
@@ -210,7 +263,9 @@
client.writeActionGroup(piActionGroup, DELETE, pipeconf),
ACT_GRP_STR, DELETE_STR);
return groupSuccess && completeFuture(
- client.writeActionGroupMembers(piActionGroup, DELETE, pipeconf),
+ client.writeActionGroupMembers(piActionGroup.actionProfileId(),
+ piActionGroup.members(),
+ DELETE, pipeconf),
ACT_GRP_MEMS_STR, DELETE_STR);
}
diff --git a/protocols/p4runtime/api/src/main/java/org/onosproject/p4runtime/api/P4RuntimeClient.java b/protocols/p4runtime/api/src/main/java/org/onosproject/p4runtime/api/P4RuntimeClient.java
index 67f9fcf..0bf61f2 100644
--- a/protocols/p4runtime/api/src/main/java/org/onosproject/p4runtime/api/P4RuntimeClient.java
+++ b/protocols/p4runtime/api/src/main/java/org/onosproject/p4runtime/api/P4RuntimeClient.java
@@ -23,6 +23,7 @@
import org.onosproject.net.pi.model.PiPipeconf;
import org.onosproject.net.pi.model.PiTableId;
import org.onosproject.net.pi.runtime.PiActionGroup;
+import org.onosproject.net.pi.runtime.PiActionGroupMember;
import org.onosproject.net.pi.runtime.PiCounterCellData;
import org.onosproject.net.pi.runtime.PiCounterCellId;
import org.onosproject.net.pi.runtime.PiMeterCellConfig;
@@ -123,12 +124,14 @@
/**
* Performs the given write operation for the given action group members and pipeconf.
*
- * @param group action group
+ * @param profileId action group profile ID
+ * @param members action group members
* @param opType write operation type
* @param pipeconf the pipeconf currently deployed on the device
* @return true if the operation was successful, false otherwise
*/
- CompletableFuture<Boolean> writeActionGroupMembers(PiActionGroup group,
+ CompletableFuture<Boolean> writeActionGroupMembers(PiActionProfileId profileId,
+ Collection<PiActionGroupMember> members,
WriteOperationType opType,
PiPipeconf pipeconf);
diff --git a/protocols/p4runtime/ctl/src/main/java/org/onosproject/p4runtime/ctl/ActionProfileMemberEncoder.java b/protocols/p4runtime/ctl/src/main/java/org/onosproject/p4runtime/ctl/ActionProfileMemberEncoder.java
index 2f08a59..7170eba 100644
--- a/protocols/p4runtime/ctl/src/main/java/org/onosproject/p4runtime/ctl/ActionProfileMemberEncoder.java
+++ b/protocols/p4runtime/ctl/src/main/java/org/onosproject/p4runtime/ctl/ActionProfileMemberEncoder.java
@@ -16,8 +16,8 @@
package org.onosproject.p4runtime.ctl;
+import org.onosproject.net.pi.model.PiActionProfileId;
import org.onosproject.net.pi.model.PiPipeconf;
-import org.onosproject.net.pi.runtime.PiActionGroup;
import org.onosproject.net.pi.runtime.PiActionGroupMember;
import org.onosproject.net.pi.runtime.PiActionGroupMemberId;
import p4.P4RuntimeOuterClass;
@@ -47,14 +47,14 @@
/**
* Encode a PiActionGroupMember to a ActionProfileMember.
*
- * @param group the PI action group of members
+ * @param profileId the PI action group profile ID of members
* @param member the member to encode
* @param pipeconf the pipeconf, as encode spec
* @return encoded member
* @throws P4InfoBrowser.NotFoundException can't find action profile from P4Info browser
* @throws EncodeException can't find P4Info from pipeconf
*/
- static ActionProfileMember encode(PiActionGroup group,
+ static ActionProfileMember encode(PiActionProfileId profileId,
PiActionGroupMember member,
PiPipeconf pipeconf)
throws P4InfoBrowser.NotFoundException, EncodeException {
@@ -73,7 +73,7 @@
// action profile id
P4InfoOuterClass.ActionProfile actionProfile =
- browser.actionProfiles().getByName(group.actionProfileId().id());
+ browser.actionProfiles().getByName(profileId.id());
int actionProfileId = actionProfile.getPreamble().getId();
actionProfileMemberBuilder.setActionProfileId(actionProfileId);
diff --git a/protocols/p4runtime/ctl/src/main/java/org/onosproject/p4runtime/ctl/P4RuntimeClientImpl.java b/protocols/p4runtime/ctl/src/main/java/org/onosproject/p4runtime/ctl/P4RuntimeClientImpl.java
index cd988fd..5fc153a 100644
--- a/protocols/p4runtime/ctl/src/main/java/org/onosproject/p4runtime/ctl/P4RuntimeClientImpl.java
+++ b/protocols/p4runtime/ctl/src/main/java/org/onosproject/p4runtime/ctl/P4RuntimeClientImpl.java
@@ -219,13 +219,15 @@
}
@Override
- public CompletableFuture<Boolean> writeActionGroupMembers(PiActionGroup group,
+ public CompletableFuture<Boolean> writeActionGroupMembers(PiActionProfileId profileId,
+ Collection<PiActionGroupMember> members,
WriteOperationType opType,
PiPipeconf pipeconf) {
- return supplyInContext(() -> doWriteActionGroupMembers(group, opType, pipeconf),
+ return supplyInContext(() -> doWriteActionGroupMembers(profileId, members, opType, pipeconf),
"writeActionGroupMembers-" + opType.name());
}
+
@Override
public CompletableFuture<Boolean> writeActionGroup(PiActionGroup group,
WriteOperationType opType,
@@ -548,12 +550,13 @@
return CounterEntryCodec.decodeCounterEntities(entities, pipeconf);
}
- private boolean doWriteActionGroupMembers(PiActionGroup group, WriteOperationType opType, PiPipeconf pipeconf) {
+ private boolean doWriteActionGroupMembers(PiActionProfileId profileId, Collection<PiActionGroupMember> members,
+ WriteOperationType opType, PiPipeconf pipeconf) {
final Collection<ActionProfileMember> actionProfileMembers = Lists.newArrayList();
- for (PiActionGroupMember member : group.members()) {
+ for (PiActionGroupMember member : members) {
try {
- actionProfileMembers.add(ActionProfileMemberEncoder.encode(group, member, pipeconf));
+ actionProfileMembers.add(ActionProfileMemberEncoder.encode(profileId, member, pipeconf));
} catch (EncodeException | P4InfoBrowser.NotFoundException e) {
log.warn("Unable to encode group member, aborting {} operation: {} [{}]",
opType.name(), e.getMessage(), member.toString());
@@ -585,7 +588,7 @@
blockingStub.write(writeRequestMsg);
return true;
} catch (StatusRuntimeException e) {
- logWriteErrors(group.members(), e, opType, "group member");
+ logWriteErrors(members, e, opType, "group member");
return false;
}
}
diff --git a/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/P4RuntimeGroupTest.java b/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/P4RuntimeGroupTest.java
index 6379f8b..25d0e00 100644
--- a/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/P4RuntimeGroupTest.java
+++ b/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/P4RuntimeGroupTest.java
@@ -191,7 +191,7 @@
@Test
public void testInsertPiActionMembers() throws Exception {
CompletableFuture<Void> complete = p4RuntimeServerImpl.expectRequests(1);
- client.writeActionGroupMembers(GROUP, INSERT, PIPECONF);
+ client.writeActionGroupMembers(ACT_PROF_ID, GROUP_MEMBERS, INSERT, PIPECONF);
complete.get(DEFAULT_TIMEOUT_TIME, TimeUnit.SECONDS);
WriteRequest result = p4RuntimeServerImpl.getWriteReqs().get(0);
assertEquals(1, result.getDeviceId());