Updating Intent Manager to deal with failures.

Added ids to Flow batch futures.
Adding some basic unit tests for IntentManger
Adding failedIds to the completedOperation in FlowRuleManager

Change-Id: I7645cead193299f70d319d254cd1e82d96909e7b

@@ -15,6 +15,9 @@
  */
 package org.onlab.onos.net.flow;
 
+
+import java.util.Collections;
+
 import java.util.Set;
 
 import com.google.common.collect.ImmutableSet;
@@ -26,6 +29,21 @@ public class CompletedBatchOperation implements BatchOperationResult<FlowRule> {
 
     private final boolean success;
     private final Set<FlowRule> failures;
+    private final Set<Long> failedIds;
+
+    /**
+     * Creates a new batch completion result.
+     *
+     * @param success  indicates whether the completion is successful.
+     * @param failures set of any failures encountered
+     * @param failedIds (optional) set of failed operation ids
+     */
+    public CompletedBatchOperation(boolean success, Set<? extends FlowRule> failures,
+                                   Set<Long> failedIds) {
+        this.success = success;
+        this.failures = ImmutableSet.copyOf(failures);
+        this.failedIds = ImmutableSet.copyOf(failedIds);
+    }
 
     /**
      * Creates a new batch completion result.
@@ -36,8 +54,11 @@ public class CompletedBatchOperation implements BatchOperationResult<FlowRule> {
     public CompletedBatchOperation(boolean success, Set<? extends FlowRule> failures) {
         this.success = success;
         this.failures = ImmutableSet.copyOf(failures);
+        this.failedIds = Collections.emptySet();
     }
 
+
+
     @Override
     public boolean isSuccess() {
         return success;
@@ -48,4 +69,8 @@ public class CompletedBatchOperation implements BatchOperationResult<FlowRule> {
         return failures;
     }
 
+    public Set<Long> failedIds() {
+        return failedIds;
+    }
+
 }

@@ -21,8 +21,20 @@ import org.onlab.onos.net.flow.FlowRuleBatchEntry.FlowRuleOperation;
 public class FlowRuleBatchEntry
         extends BatchOperationEntry<FlowRuleOperation, FlowRule> {
 
+    private final Long id; // FIXME: consider using Optional<Long>
+
     public FlowRuleBatchEntry(FlowRuleOperation operator, FlowRule target) {
         super(operator, target);
+        this.id = null;
+    }
+
+    public FlowRuleBatchEntry(FlowRuleOperation operator, FlowRule target, Long id) {
+        super(operator, target);
+        this.id = id;
+    }
+
+    public Long id() {
+        return id;
     }
 
     public enum FlowRuleOperation {

@@ -18,38 +18,52 @@ package org.onlab.onos.net.flow;
 import java.util.Collections;
 import java.util.List;
 
-import org.onlab.onos.net.flow.FlowRuleBatchEntry.FlowRuleOperation;
+import com.google.common.base.Function;
+import com.google.common.collect.FluentIterable;
+
+
 
 import com.google.common.collect.Lists;
 
 public class FlowRuleBatchRequest {
 
     private final int batchId;
-    private final List<FlowRule> toAdd;
-    private final List<FlowRule> toRemove;
+    private final List<FlowRuleBatchEntry> toAdd;
+    private final List<FlowRuleBatchEntry> toRemove;
 
-    public FlowRuleBatchRequest(int batchId, List<? extends FlowRule> toAdd, List<? extends FlowRule> toRemove) {
+    public FlowRuleBatchRequest(int batchId, List<FlowRuleBatchEntry> toAdd,
+                                List<FlowRuleBatchEntry> toRemove) {
         this.batchId = batchId;
         this.toAdd = Collections.unmodifiableList(toAdd);
         this.toRemove = Collections.unmodifiableList(toRemove);
     }
 
     public List<FlowRule> toAdd() {
-        return toAdd;
+        return FluentIterable.from(toAdd).transform(
+                new Function<FlowRuleBatchEntry, FlowRule>() {
+
+            @Override
+            public FlowRule apply(FlowRuleBatchEntry input) {
+                return input.getTarget();
+            }
+        }).toList();
     }
 
     public List<FlowRule> toRemove() {
-        return toRemove;
+        return FluentIterable.from(toRemove).transform(
+                new Function<FlowRuleBatchEntry, FlowRule>() {
+
+                    @Override
+                    public FlowRule apply(FlowRuleBatchEntry input) {
+                        return input.getTarget();
+                    }
+                }).toList();
     }
 
     public FlowRuleBatchOperation asBatchOperation() {
         List<FlowRuleBatchEntry> entries = Lists.newArrayList();
-        for (FlowRule e : toAdd) {
-            entries.add(new FlowRuleBatchEntry(FlowRuleOperation.ADD, e));
-        }
-        for (FlowRule e : toRemove) {
-            entries.add(new FlowRuleBatchEntry(FlowRuleOperation.REMOVE, e));
-        }
+        entries.addAll(toAdd);
+        entries.addAll(toRemove);
         return new FlowRuleBatchOperation(entries);
     }
 

@@ -37,10 +37,16 @@ public interface IntentBatchService {
     void removeIntentOperations(IntentOperations operations);
 
     /**
-     * Returns the set of intent batches currently being tracked.
+     * Returns the set of intent batches that are pending.
      * @return set of batches
      */
-    Set<IntentOperations> getIntentOperations();
+    Set<IntentOperations> getPendingOperations();
+
+    /**
+     * Returns the set of intent batches currently being processed.
+     * @return set of batches
+     */
+    Set<IntentOperations> getCurrentOperations();
 
     /**
      * Sets the batch service delegate.

@@ -67,6 +67,18 @@ public enum IntentState {
     RECOMPILING,
 
     /**
+     * TODO: Indicated that an intent will soon be recompiled.
+     */
+    //UPDATE,
+
+    /**
+     * TODO.
+     * Indicates that an application has requested that an intent be withdrawn.
+     * It will start withdrawing short, but not necessarily on this instance.
+     */
+    //WITHDRAW_REQ,
+
+    /**
      * Indicates that the intent is being withdrawn. This is a transitional
      * state, triggered by invocation of the
      * {@link IntentService#withdraw(Intent)} but one with only one outcome,

@@ -21,6 +21,8 @@ import java.util.List;
 import org.junit.Test;
 import org.onlab.onos.net.intent.IntentTestsMocks;
 
+import static org.onlab.onos.net.flow.FlowRuleBatchEntry.FlowRuleOperation.*;
+
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
@@ -38,10 +40,10 @@ public class FlowRuleBatchRequestTest {
     public void testConstruction() {
         final FlowRule rule1 = new IntentTestsMocks.MockFlowRule(1);
         final FlowRule rule2 = new IntentTestsMocks.MockFlowRule(2);
-        final List<FlowRule> toAdd = new LinkedList<>();
-        toAdd.add(rule1);
-        final List<FlowRule> toRemove = new LinkedList<>();
-        toRemove.add(rule2);
+        final List<FlowRuleBatchEntry> toAdd = new LinkedList<>();
+        toAdd.add(new FlowRuleBatchEntry(ADD, rule1));
+        final List<FlowRuleBatchEntry> toRemove = new LinkedList<>();
+        toRemove.add(new FlowRuleBatchEntry(REMOVE, rule2));
 
 
         final FlowRuleBatchRequest request =

@@ -26,6 +26,7 @@ import java.util.Collections;
 import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 
 import org.onlab.onos.net.DeviceId;
@@ -331,7 +332,22 @@ public class IntentTestsMocks {
             return false;
         }
 
+        @Override
+        public int hashCode() {
+            return Objects.hash(priority);
+        }
 
+        @Override
+        public boolean equals(Object obj) {
+            if (this == obj) {
+                return true;
+            }
+            if (obj == null || getClass() != obj.getClass()) {
+                return false;
+            }
+            final MockFlowRule other = (MockFlowRule) obj;
+            return Objects.equals(this.priority, other.priority);
+        }
     }
 
 

@@ -488,13 +488,14 @@ public class FlowRuleManager
 
             boolean success = true;
             Set<FlowRule> failed = Sets.newHashSet();
+            Set<Long> failedIds = Sets.newHashSet();
             CompletedBatchOperation completed;
             for (Future<CompletedBatchOperation> future : futures) {
                 completed = future.get();
-                success = validateBatchOperation(failed, completed);
+                success = validateBatchOperation(failed, failedIds, completed);
             }
 
-            return finalizeBatchOperation(success, failed);
+            return finalizeBatchOperation(success, failed, failedIds);
 
         }
 
@@ -508,15 +509,17 @@ public class FlowRuleManager
             }
             boolean success = true;
             Set<FlowRule> failed = Sets.newHashSet();
+            Set<Long> failedIds = Sets.newHashSet();
             CompletedBatchOperation completed;
             for (Future<CompletedBatchOperation> future : futures) {
                 completed = future.get(timeout, unit);
-                success = validateBatchOperation(failed, completed);
+                success = validateBatchOperation(failed, failedIds, completed);
             }
-            return finalizeBatchOperation(success, failed);
+            return finalizeBatchOperation(success, failed, failedIds);
         }
 
         private boolean validateBatchOperation(Set<FlowRule> failed,
+                                               Set<Long> failedIds,
                                                CompletedBatchOperation completed) {
 
             if (isCancelled()) {
@@ -524,6 +527,7 @@ public class FlowRuleManager
             }
             if (!completed.isSuccess()) {
                 failed.addAll(completed.failedItems());
+                failedIds.addAll(completed.failedIds());
                 cleanUpBatch();
                 cancelAllSubBatches();
                 return false;
@@ -538,7 +542,8 @@ public class FlowRuleManager
         }
 
         private CompletedBatchOperation finalizeBatchOperation(boolean success,
-                Set<FlowRule> failed) {
+                                                               Set<FlowRule> failed,
+                                                               Set<Long> failedIds) {
             synchronized (this) {
                 if (!state.compareAndSet(BatchState.STARTED, BatchState.FINISHED)) {
                     if (state.get() == BatchState.FINISHED) {
@@ -546,7 +551,7 @@ public class FlowRuleManager
                     }
                     throw new CancellationException();
                 }
-                overall = new CompletedBatchOperation(success, failed);
+                overall = new CompletedBatchOperation(success, failed, failedIds);
                 return overall;
             }
         }

@@ -101,7 +101,8 @@ public class PathIntentInstaller implements IntentInstaller<PathIntent> {
             FlowRule rule = new DefaultFlowRule(link.src().deviceId(),
                     builder.build(), treatment, 123, //FIXME 123
                     appId, (short) (intent.id().fingerprint() & 0xffff), 0, true);
-            rules.add(new FlowRuleBatchEntry(FlowRuleOperation.ADD, rule));
+            rules.add(new FlowRuleBatchEntry(FlowRuleOperation.ADD, rule,
+                                             intent.id().fingerprint()));
             prev = link.dst();
         }
         return Lists.newArrayList(new FlowRuleBatchOperation(rules));
@@ -127,7 +128,8 @@ public class PathIntentInstaller implements IntentInstaller<PathIntent> {
             FlowRule rule = new DefaultFlowRule(link.src().deviceId(),
                     builder.build(), treatment,
                     123, appId, (short) (intent.id().fingerprint() & 0xffff), 0, true);
-            rules.add(new FlowRuleBatchEntry(FlowRuleOperation.REMOVE, rule));
+            rules.add(new FlowRuleBatchEntry(FlowRuleOperation.REMOVE, rule,
+                                             intent.id().fingerprint()));
             prev = link.dst();
         }
         return Lists.newArrayList(new FlowRuleBatchOperation(rules));

+package org.onlab.onos.net.intent.impl;
+
+import com.google.common.collect.Sets;
+import com.google.common.util.concurrent.Futures;
+import org.onlab.onos.core.ApplicationId;
+import org.onlab.onos.net.DeviceId;
+import org.onlab.onos.net.flow.CompletedBatchOperation;
+import org.onlab.onos.net.flow.FlowEntry;
+import org.onlab.onos.net.flow.FlowRule;
+import org.onlab.onos.net.flow.FlowRuleBatchEntry;
+import org.onlab.onos.net.flow.FlowRuleBatchOperation;
+import org.onlab.onos.net.flow.FlowRuleListener;
+import org.onlab.onos.net.flow.FlowRuleService;
+
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.Future;
+
+
+public class MockFlowRuleService implements FlowRuleService {
+
+    private Future<CompletedBatchOperation> future;
+    final Set<FlowRule> flows = Sets.newHashSet();
+
+    public void setFuture(boolean success) {
+        future = Futures.immediateFuture(new CompletedBatchOperation(true, Collections.emptySet()));
+    }
+
+    @Override
+    public Future<CompletedBatchOperation> applyBatch(FlowRuleBatchOperation batch) {
+        for (FlowRuleBatchEntry fbe : batch.getOperations()) {
+            FlowRule fr = fbe.getTarget();
+            switch (fbe.getOperator()) {
+                case ADD:
+                    flows.add(fr);
+                    break;
+                case REMOVE:
+                    flows.remove(fr);
+                    break;
+                case MODIFY:
+                    break;
+                default:
+                    break;
+            }
+        }
+        return future;
+    }
+
+    @Override
+    public int getFlowRuleCount() {
+        return flows.size();
+    }
+
+    @Override
+    public Iterable<FlowEntry> getFlowEntries(DeviceId deviceId) {
+        return null;
+    }
+
+    @Override
+    public void applyFlowRules(FlowRule... flowRules) {
+    }
+
+    @Override
+    public void removeFlowRules(FlowRule... flowRules) {
+    }
+
+    @Override
+    public void removeFlowRulesById(ApplicationId appId) {
+    }
+
+    @Override
+    public Iterable<FlowRule> getFlowRulesById(ApplicationId id) {
+        return null;
+    }
+
+    @Override
+    public Iterable<FlowRule> getFlowRulesByGroupId(ApplicationId appId, short groupId) {
+        return null;
+    }
+
+    @Override
+    public void addListener(FlowRuleListener listener) {
+
+    }
+
+    @Override
+    public void removeListener(FlowRuleListener listener) {
+
+    }
+}
+

@@ -340,7 +340,8 @@ public class DistributedFlowRuleStore
     public Future<CompletedBatchOperation> storeBatch(FlowRuleBatchOperation operation) {
 
         if (operation.getOperations().isEmpty()) {
-            return Futures.immediateFuture(new CompletedBatchOperation(true, Collections.<FlowRule>emptySet()));
+            return Futures.immediateFuture(new CompletedBatchOperation(true,
+                                                                       Collections.<FlowRule>emptySet()));
         }
 
         DeviceId deviceId = operation.getOperations().get(0).getTarget().deviceId();
@@ -379,8 +380,8 @@ public class DistributedFlowRuleStore
     private ListenableFuture<CompletedBatchOperation>
                         storeBatchInternal(FlowRuleBatchOperation operation) {
 
-        final List<StoredFlowEntry> toRemove = new ArrayList<>();
-        final List<StoredFlowEntry> toAdd = new ArrayList<>();
+        final List<FlowRuleBatchEntry> toRemove = new ArrayList<>();
+        final List<FlowRuleBatchEntry> toAdd = new ArrayList<>();
         DeviceId did = null;
 
 
@@ -396,14 +397,14 @@ public class DistributedFlowRuleStore
                     StoredFlowEntry entry = getFlowEntryInternal(flowRule);
                     if (entry != null) {
                         entry.setState(FlowEntryState.PENDING_REMOVE);
-                        toRemove.add(entry);
+                        toRemove.add(batchEntry);
                     }
                 } else if (op.equals(FlowRuleOperation.ADD)) {
                     StoredFlowEntry flowEntry = new DefaultFlowEntry(flowRule);
                     DeviceId deviceId = flowRule.deviceId();
                     if (!flowEntries.containsEntry(deviceId, flowEntry)) {
                         flowEntries.put(deviceId, flowEntry);
-                        toAdd.add(flowEntry);
+                        toAdd.add(batchEntry);
                     }
                 }
             }
@@ -427,8 +428,8 @@ public class DistributedFlowRuleStore
     }
 
     private void updateBackup(final DeviceId deviceId,
-                              final List<StoredFlowEntry> toAdd,
-                              final List<? extends FlowRule> list) {
+                              final List<FlowRuleBatchEntry> toAdd,
+                              final List<FlowRuleBatchEntry> list) {
 
         Future<?> submit = backupExecutors.submit(new UpdateBackup(deviceId, toAdd, list));
 
@@ -442,8 +443,9 @@ public class DistributedFlowRuleStore
         }
     }
 
-    private void updateBackup(DeviceId deviceId, List<StoredFlowEntry> toAdd) {
-        updateBackup(deviceId, toAdd, Collections.<FlowEntry>emptyList());
+    private void updateBackup(DeviceId deviceId, List<FlowRuleBatchEntry> toAdd) {
+
+        updateBackup(deviceId, toAdd, Collections.<FlowRuleBatchEntry>emptyList());
     }
 
     @Override
@@ -477,8 +479,9 @@ public class DistributedFlowRuleStore
                 stored.setPackets(rule.packets());
                 if (stored.state() == FlowEntryState.PENDING_ADD) {
                     stored.setState(FlowEntryState.ADDED);
-                    // update backup.
-                    updateBackup(did, Arrays.asList(stored));
+                    FlowRuleBatchEntry entry =
+                            new FlowRuleBatchEntry(FlowRuleOperation.ADD, stored);
+                    updateBackup(did, Arrays.asList(entry));
                     return new FlowRuleEvent(Type.RULE_ADDED, rule);
                 }
                 return new FlowRuleEvent(Type.RULE_UPDATED, rule);
@@ -515,7 +518,9 @@ public class DistributedFlowRuleStore
         try {
             // This is where one could mark a rule as removed and still keep it in the store.
             final boolean removed = flowEntries.remove(deviceId, rule);
-            updateBackup(deviceId, Collections.<StoredFlowEntry>emptyList(), Arrays.asList(rule));
+            FlowRuleBatchEntry entry =
+                    new FlowRuleBatchEntry(FlowRuleOperation.REMOVE, rule);
+            updateBackup(deviceId, Collections.<FlowRuleBatchEntry>emptyList(), Arrays.asList(entry));
             if (removed) {
                 return new FlowRuleEvent(RULE_REMOVED, rule);
             } else {
@@ -687,15 +692,17 @@ public class DistributedFlowRuleStore
     }
 
     // Task to update FlowEntries in backup HZ store
+    // TODO: Should be refactored to contain only one list and not
+    //      toAdd and toRemove
     private final class UpdateBackup implements Runnable {
 
         private final DeviceId deviceId;
-        private final List<StoredFlowEntry> toAdd;
-        private final List<? extends FlowRule> toRemove;
+        private final List<FlowRuleBatchEntry> toAdd;
+        private final List<FlowRuleBatchEntry> toRemove;
 
         public UpdateBackup(DeviceId deviceId,
-                             List<StoredFlowEntry> toAdd,
-                             List<? extends FlowRule> list) {
+                             List<FlowRuleBatchEntry> toAdd,
+                             List<FlowRuleBatchEntry> list) {
             this.deviceId = checkNotNull(deviceId);
             this.toAdd = checkNotNull(toAdd);
             this.toRemove = checkNotNull(list);
@@ -707,7 +714,8 @@ public class DistributedFlowRuleStore
                 log.trace("update backup {} +{} -{}", deviceId, toAdd, toRemove);
                 final SMap<FlowId, ImmutableList<StoredFlowEntry>> backupFlowTable = smaps.get(deviceId);
                 // Following should be rewritten using async APIs
-                for (StoredFlowEntry entry : toAdd) {
+                for (FlowRuleBatchEntry bEntry : toAdd) {
+                    final FlowRule entry = bEntry.getTarget();
                     final FlowId id = entry.id();
                     ImmutableList<StoredFlowEntry> original = backupFlowTable.get(id);
                     List<StoredFlowEntry> list = new ArrayList<>();
@@ -715,8 +723,8 @@ public class DistributedFlowRuleStore
                         list.addAll(original);
                     }
 
-                    list.remove(entry);
-                    list.add(entry);
+                    list.remove(bEntry.getTarget());
+                    list.add((StoredFlowEntry) entry);
 
                     ImmutableList<StoredFlowEntry> newValue = ImmutableList.copyOf(list);
                     boolean success;
@@ -730,7 +738,8 @@ public class DistributedFlowRuleStore
                         log.error("Updating backup failed.");
                     }
                 }
-                for (FlowRule entry : toRemove) {
+                for (FlowRuleBatchEntry bEntry : toRemove) {
+                    final FlowRule entry = bEntry.getTarget();
                     final FlowId id = entry.id();
                     ImmutableList<StoredFlowEntry> original = backupFlowTable.get(id);
                     List<StoredFlowEntry> list = new ArrayList<>();
@@ -738,7 +747,7 @@ public class DistributedFlowRuleStore
                         list.addAll(original);
                     }
 
-                    list.remove(entry);
+                    list.remove(bEntry.getTarget());
 
                     ImmutableList<StoredFlowEntry> newValue = ImmutableList.copyOf(list);
                     boolean success;

@@ -25,7 +25,6 @@ import org.onlab.onos.net.intent.IntentBatchService;
 import org.onlab.onos.net.intent.IntentOperations;
 import org.slf4j.Logger;
 
-import java.util.Collection;
 import java.util.LinkedList;
 import java.util.Queue;
 import java.util.Set;
@@ -82,10 +81,17 @@ public class DistributedIntentBatchQueue implements IntentBatchService {
     }
 
     @Override
-    public Set<IntentOperations> getIntentOperations() {
-        Set<IntentOperations> set = Sets.newHashSet(currentBatches);
-        set.addAll((Collection) pendingBatches);
-        return set;
+    public Set<IntentOperations> getPendingOperations() {
+        synchronized (this) {
+            return Sets.newHashSet(pendingBatches);
+        }
+    }
+
+    @Override
+    public Set<IntentOperations> getCurrentOperations() {
+        synchronized (this) {
+            return Sets.newHashSet(currentBatches);
+        }
     }
 
     @Override

@@ -263,19 +263,19 @@ public class SimpleFlowRuleStore
     @Override
     public Future<CompletedBatchOperation> storeBatch(
             FlowRuleBatchOperation batchOperation) {
-        List<FlowRule> toAdd = new ArrayList<>();
-        List<FlowRule> toRemove = new ArrayList<>();
+        List<FlowRuleBatchEntry> toAdd = new ArrayList<>();
+        List<FlowRuleBatchEntry> toRemove = new ArrayList<>();
         for (FlowRuleBatchEntry entry : batchOperation.getOperations()) {
             final FlowRule flowRule = entry.getTarget();
             if (entry.getOperator().equals(FlowRuleOperation.ADD)) {
                 if (!getFlowEntries(flowRule.deviceId(), flowRule.id()).contains(flowRule)) {
                     storeFlowRule(flowRule);
-                    toAdd.add(flowRule);
+                    toAdd.add(entry);
                 }
             } else if (entry.getOperator().equals(FlowRuleOperation.REMOVE)) {
                 if (getFlowEntries(flowRule.deviceId(), flowRule.id()).contains(flowRule)) {
                     deleteFlowRule(flowRule);
-                    toRemove.add(flowRule);
+                    toRemove.add(entry);
                 }
             } else {
                 throw new UnsupportedOperationException("Unsupported operation type");

@@ -15,7 +15,7 @@
  */
 package org.onlab.onos.store.trivial.impl;
 
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Deactivate;
@@ -25,7 +25,8 @@ import org.onlab.onos.net.intent.IntentBatchService;
 import org.onlab.onos.net.intent.IntentOperations;
 import org.slf4j.Logger;
 
-import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.Queue;
 import java.util.Set;
 
 import static com.google.common.base.Preconditions.checkNotNull;
@@ -37,7 +38,8 @@ import static org.slf4j.LoggerFactory.getLogger;
 public class SimpleIntentBatchQueue implements IntentBatchService {
 
     private final Logger log = getLogger(getClass());
-    private final Set<IntentOperations> pendingBatches = new HashSet<>();
+    private final Queue<IntentOperations> pendingBatches = new LinkedList<>();
+    private final Set<IntentOperations> currentBatches = Sets.newHashSet();
     private IntentBatchDelegate delegate;
 
     @Activate
@@ -53,18 +55,42 @@ public class SimpleIntentBatchQueue implements IntentBatchService {
     @Override
     public void addIntentOperations(IntentOperations operations) {
         checkState(delegate != null, "No delegate set");
+        synchronized (this) {
             pendingBatches.add(operations);
-        delegate.execute(operations);
+            if (currentBatches.isEmpty()) {
+                IntentOperations work = pendingBatches.poll();
+                currentBatches.add(work);
+                delegate.execute(work);
+            }
+        }
     }
 
     @Override
     public void removeIntentOperations(IntentOperations operations) {
-        pendingBatches.remove(operations);
+        // we allow at most one outstanding batch at a time
+        synchronized (this) {
+            checkState(currentBatches.remove(operations), "Operations not found in current ops.");
+            checkState(currentBatches.isEmpty(), "More than one outstanding batch.");
+            IntentOperations work = pendingBatches.poll();
+            if (work != null) {
+                currentBatches.add(work);
+                delegate.execute(work);
+            }
+        }
     }
 
     @Override
-    public Set<IntentOperations> getIntentOperations() {
-        return ImmutableSet.copyOf(pendingBatches);
+    public Set<IntentOperations> getPendingOperations() {
+        synchronized (this) {
+            return Sets.newHashSet(pendingBatches);
+        }
+    }
+
+    @Override
+    public Set<IntentOperations> getCurrentOperations() {
+        synchronized (this) {
+            return Sets.newHashSet(currentBatches);
+        }
     }
 
     @Override

@@ -53,6 +53,8 @@ import org.projectfloodlight.openflow.types.VlanPcp;
 import org.projectfloodlight.openflow.types.VlanVid;
 import org.slf4j.Logger;
 
+import java.util.Optional;
+
 /**
  * Builder for OpenFlow flow mods based on FlowRules.
  */
@@ -63,6 +65,7 @@ public abstract class FlowModBuilder {
     private final OFFactory factory;
     private final FlowRule flowRule;
     private final TrafficSelector selector;
+    protected final Long xid;
 
     /**
      * Creates a new flow mod builder.
@@ -71,12 +74,13 @@ public abstract class FlowModBuilder {
      * @param factory the OpenFlow factory to use to build the flow mod
      * @return the new flow mod builder
      */
-    public static FlowModBuilder builder(FlowRule flowRule, OFFactory factory) {
+    public static FlowModBuilder builder(FlowRule flowRule,
+                                         OFFactory factory, Optional<Long> xid) {
         switch (factory.getVersion()) {
         case OF_10:
-            return new FlowModBuilderVer10(flowRule, factory);
+            return new FlowModBuilderVer10(flowRule, factory, xid);
         case OF_13:
-            return new FlowModBuilderVer13(flowRule, factory);
+            return new FlowModBuilderVer13(flowRule, factory, xid);
         default:
             throw new UnsupportedOperationException(
                     "No flow mod builder for protocol version " + factory.getVersion());
@@ -89,10 +93,12 @@ public abstract class FlowModBuilder {
      * @param flowRule the flow rule to transform into a flow mod
      * @param factory the OpenFlow factory to use to build the flow mod
      */
-    protected FlowModBuilder(FlowRule flowRule, OFFactory factory) {
+    protected FlowModBuilder(FlowRule flowRule, OFFactory factory, Optional<Long> xid) {
         this.factory = factory;
         this.flowRule = flowRule;
         this.selector = flowRule.selector();
+        this.xid = xid.orElse((long) 0);
+
     }
 
     /**

@@ -18,6 +18,7 @@ package org.onlab.onos.provider.of.flow.impl;
 import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Optional;
 
 import org.onlab.onos.net.flow.FlowRule;
 import org.onlab.onos.net.flow.TrafficTreatment;
@@ -62,8 +63,9 @@ public class FlowModBuilderVer10 extends FlowModBuilder {
      * @param flowRule the flow rule to transform into a flow mod
      * @param factory the OpenFlow factory to use to build the flow mod
      */
-    protected FlowModBuilderVer10(FlowRule flowRule, OFFactory factory) {
-        super(flowRule, factory);
+    protected FlowModBuilderVer10(FlowRule flowRule,
+                                  OFFactory factory, Optional<Long> xid) {
+        super(flowRule, factory, xid);
 
         this.treatment = flowRule.treatment();
     }
@@ -77,7 +79,7 @@ public class FlowModBuilderVer10 extends FlowModBuilder {
 
         //TODO: what to do without bufferid? do we assume that there will be a pktout as well?
         OFFlowAdd fm = factory().buildFlowAdd()
-                .setXid(cookie)
+                .setXid(xid)
                 .setCookie(U64.of(cookie))
                 .setBufferId(OFBufferId.NO_BUFFER)
                 .setActions(actions)
@@ -98,7 +100,7 @@ public class FlowModBuilderVer10 extends FlowModBuilder {
 
         //TODO: what to do without bufferid? do we assume that there will be a pktout as well?
         OFFlowMod fm = factory().buildFlowModify()
-                .setXid(cookie)
+                .setXid(xid)
                 .setCookie(U64.of(cookie))
                 .setBufferId(OFBufferId.NO_BUFFER)
                 .setActions(actions)
@@ -118,7 +120,7 @@ public class FlowModBuilderVer10 extends FlowModBuilder {
         long cookie = flowRule().id().value();
 
         OFFlowDelete fm = factory().buildFlowDelete()
-                .setXid(cookie)
+                .setXid(xid)
                 .setCookie(U64.of(cookie))
                 .setBufferId(OFBufferId.NO_BUFFER)
                 .setActions(actions)

@@ -18,6 +18,7 @@ package org.onlab.onos.provider.of.flow.impl;
 import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Optional;
 
 import org.onlab.onos.net.flow.FlowRule;
 import org.onlab.onos.net.flow.TrafficTreatment;
@@ -70,8 +71,8 @@ public class FlowModBuilderVer13 extends FlowModBuilder {
      * @param flowRule the flow rule to transform into a flow mod
      * @param factory the OpenFlow factory to use to build the flow mod
      */
-    protected FlowModBuilderVer13(FlowRule flowRule, OFFactory factory) {
-        super(flowRule, factory);
+    protected FlowModBuilderVer13(FlowRule flowRule, OFFactory factory, Optional<Long> xid) {
+        super(flowRule, factory, xid);
 
         this.treatment = flowRule.treatment();
     }
@@ -93,7 +94,7 @@ public class FlowModBuilderVer13 extends FlowModBuilder {
 
         //TODO: what to do without bufferid? do we assume that there will be a pktout as well?
         OFFlowAdd fm = factory().buildFlowAdd()
-                .setXid(cookie)
+                .setXid(xid)
                 .setCookie(U64.of(cookie))
                 .setBufferId(OFBufferId.NO_BUFFER)
                 .setActions(actions)
@@ -117,7 +118,7 @@ public class FlowModBuilderVer13 extends FlowModBuilder {
 
         //TODO: what to do without bufferid? do we assume that there will be a pktout as well?
         OFFlowMod fm = factory().buildFlowModify()
-                .setXid(cookie)
+                .setXid(xid)
                 .setCookie(U64.of(cookie))
                 .setBufferId(OFBufferId.NO_BUFFER)
                 .setActions(actions)
@@ -140,7 +141,7 @@ public class FlowModBuilderVer13 extends FlowModBuilder {
         long cookie = flowRule().id().value();
 
         OFFlowDelete fm = factory().buildFlowDelete()
-                .setXid(cookie)
+                .setXid(xid)
                 .setCookie(U64.of(cookie))
                 .setBufferId(OFBufferId.NO_BUFFER)
                 //.setActions(actions) //FIXME do we want to send actions in flowdel?

@@ -19,7 +19,6 @@ import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Sets;
-import com.google.common.util.concurrent.ExecutionList;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Deactivate;
@@ -76,6 +75,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CountDownLatch;
@@ -122,7 +122,7 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
 
     private final Map<Dpid, FlowStatsCollector> collectors = Maps.newHashMap();
 
-    private final AtomicLong xidCounter = new AtomicLong(0);
+    private final AtomicLong xidCounter = new AtomicLong(1);
 
     /**
      * Creates an OpenFlow host provider.
@@ -164,7 +164,8 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
 
     private void applyRule(FlowRule flowRule) {
         OpenFlowSwitch sw = controller.getSwitch(Dpid.dpid(flowRule.deviceId().uri()));
-        sw.sendMsg(FlowModBuilder.builder(flowRule, sw.factory()).buildFlowAdd());
+        sw.sendMsg(FlowModBuilder.builder(flowRule, sw.factory(),
+                                          Optional.empty()).buildFlowAdd());
     }
 
 
@@ -178,7 +179,8 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
 
     private void removeRule(FlowRule flowRule) {
         OpenFlowSwitch sw = controller.getSwitch(Dpid.dpid(flowRule.deviceId().uri()));
-        sw.sendMsg(FlowModBuilder.builder(flowRule, sw.factory()).buildFlowDel());
+        sw.sendMsg(FlowModBuilder.builder(flowRule, sw.factory(),
+                                          Optional.empty()).buildFlowDel());
     }
 
     @Override
@@ -211,7 +213,10 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
                 return failed;
             }
             sws.add(new Dpid(sw.getId()));
-            FlowModBuilder builder = FlowModBuilder.builder(flowRule, sw.factory());
+            Long flowModXid = xidCounter.getAndIncrement();
+            FlowModBuilder builder =
+                    FlowModBuilder.builder(flowRule, sw.factory(),
+                                           Optional.of(flowModXid));
             OFFlowMod mod = null;
             switch (fbe.getOperator()) {
                 case ADD:
@@ -228,7 +233,7 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
             }
             if (mod != null) {
                 mods.put(mod, sw);
-                fmXids.put(xidCounter.getAndIncrement(), fbe);
+                fmXids.put(flowModXid, fbe);
             } else {
                 log.error("Conversion of flowrule {} failed.", flowRule);
             }
@@ -237,6 +242,7 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
         for (Long xid : fmXids.keySet()) {
             pendingFMs.put(xid, installation);
         }
+
         pendingFutures.put(installation.xid(), installation);
         for (Map.Entry<OFFlowMod, OpenFlowSwitch> entry : mods.entrySet()) {
             OpenFlowSwitch sw = entry.getValue();
@@ -368,13 +374,13 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
         private final AtomicBoolean ok = new AtomicBoolean(true);
         private final Map<Long, FlowRuleBatchEntry> fms;
 
+
         private final Set<FlowEntry> offendingFlowMods = Sets.newHashSet();
+        private Long failedId;
 
         private final CountDownLatch countDownLatch;
         private BatchState state;
 
-        private final ExecutionList executionList = new ExecutionList();
-
         public InstallationFuture(Set<Dpid> sws, Map<Long, FlowRuleBatchEntry> fmXids) {
             this.xid = xidCounter.getAndIncrement();
             this.state = BatchState.STARTED;
@@ -393,6 +399,7 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
             removeRequirement(dpid);
             FlowEntry fe = null;
             FlowRuleBatchEntry fbe = fms.get(msg.getXid());
+            failedId = fbe.id();
             FlowRule offending = fbe.getTarget();
             //TODO handle specific error msgs
             switch (msg.getErrType()) {
@@ -492,8 +499,11 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
         public CompletedBatchOperation get() throws InterruptedException, ExecutionException {
             countDownLatch.await();
             this.state = BatchState.FINISHED;
-            CompletedBatchOperation result = new CompletedBatchOperation(ok.get(), offendingFlowMods);
-            //FIXME do cleanup here
+            Set<Long> failedIds = (failedId != null) ?  Sets.newHashSet(failedId) : Collections.emptySet();
+            CompletedBatchOperation result =
+                    new CompletedBatchOperation(ok.get(), offendingFlowMods, failedIds);
+            //FIXME do cleanup here (moved by BOC)
+            cleanUp();
             return result;
         }
 
@@ -503,8 +513,11 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
                 TimeoutException {
             if (countDownLatch.await(timeout, unit)) {
                 this.state = BatchState.FINISHED;
-                CompletedBatchOperation result = new CompletedBatchOperation(ok.get(), offendingFlowMods);
-                // FIXME do cleanup here
+                Set<Long> failedIds = (failedId != null) ?  Sets.newHashSet(failedId) : Collections.emptySet();
+                CompletedBatchOperation result =
+                        new CompletedBatchOperation(ok.get(), offendingFlowMods, failedIds);
+                // FIXME do cleanup here (moved by BOC)
+                cleanUp();
                 return result;
             }
             throw new TimeoutException();
@@ -522,8 +535,8 @@ public class OpenFlowRuleProvider extends AbstractProvider implements FlowRulePr
         private void removeRequirement(Dpid dpid) {
             countDownLatch.countDown();
             sws.remove(dpid);
-            //FIXME don't do cleanup here
-            cleanUp();
+            //FIXME don't do cleanup here (moved by BOC)
+            //cleanUp();
         }
     }