Unit tests and manager changes for failure cases

Add unit tests for intent intallation failures: - compilation error - installation exception - installation returns error future Change-Id: Idc5e19980032cc5c1b9804c7627833fb126b7a81

Unit tests and manager changes for failure cases
Add unit tests for intent intallation failures: - compilation error - installation exception - installation returns error future Change-Id: Idc5e19980032cc5c1b9804c7627833fb126b7a81
Ray Milkey · Gerrit Code Review
Commit 93508c2634e340f80950c2eefd41b48caa8396b3 93508c26 1 parent ae3e7336
Showing 3 changed files with 130 additions and 25 deletions
core/net/src/main/java/org/onlab/onos/net/intent/impl/IntentManager.java
core/net/src/test/java/org/onlab/onos/net/intent/impl/IntentManagerTest.java
core/net/src/test/java/org/onlab/onos/net/intent/impl/MockFlowRuleService.java
--- a/core/net/src/main/java/org/onlab/onos/net/intent/impl/IntentManager.java
View file @93508c2
+++ b/core/net/src/main/java/org/onlab/onos/net/intent/impl/IntentManager.java
View file @93508c2
@@ -300,6 +300,7 @@ public class IntentManager
             update.setInstallables(installables);
         } catch (PathNotFoundException e) {
             log.debug("Path not found for intent {}", intent);
+            update.setInflightState(intent, FAILED);
         } catch (IntentException e) {
             log.warn("Unable to compile intent {} due to:", intent.id(), e);
@@ -350,11 +351,12 @@ public class IntentManager
                                                installable.resources());
             try {
                 batches.addAll(getInstaller(installable).install(installable));
-            } catch (IntentException e) {
+            } catch (Exception e) { // TODO this should be IntentException
                 log.warn("Unable to install intent {} due to:", update.newIntent().id(), e);
                 trackerService.removeTrackedResources(update.newIntent().id(),
                                                       installable.resources());
-                //FIXME we failed... intent should be recompiled
+                //TODO we failed; intent should be recompiled
+                update.setInflightState(update.newIntent(), FAILED);
             }
         }
         update.addBatches(batches);
@@ -660,11 +662,9 @@ public class IntentManager
             return !isComplete() ? batches.get(currentBatch) : null;
         }
-        void batchSuccess(BatchWrite batchWrite) {
+        void batchSuccess() {
             // move on to next Batch
-            if (++currentBatch == batches.size()) {
+            currentBatch++;
-                finalizeStates(batchWrite);
-            }
         }
         void batchFailed() {
@@ -684,7 +684,6 @@ public class IntentManager
             // TODO we might want to try to recompile the new intent
         }
-        // make sure this is called!!!
         private void finalizeStates(BatchWrite batchWrite) {
             // events to be triggered on successful write
             for (Intent intent : stateMap.keySet()) {
@@ -699,6 +698,7 @@ public class IntentManager
                         batchWrite.removeIntent(intent.id());
                         break;
                     case FAILED:
+                        batchWrite.setState(intent, FAILED);
                         batchWrite.removeInstalledIntents(intent.id());
                         break;
@@ -801,27 +801,39 @@ public class IntentManager
                     batch.addAll(update.currentBatch());
                 }
             }
-            return (batch.size() > 0) ? flowRuleService.applyBatch(batch) : null;
+            if (batch.size() > 0) {
-        }
+                //FIXME apply batch might throw an exception
-
+                return flowRuleService.applyBatch(batch);
-        private void updateBatches(CompletedBatchOperation completed) {
+            } else {
-            if (completed.isSuccess()) {
+                // there are no flow rule batches; finalize the intent update
                 BatchWrite batchWrite = store.newBatchWrite();
                 for (IntentUpdate update : intentUpdates) {
-                    update.batchSuccess(batchWrite);
+                    update.finalizeStates(batchWrite);
                 }
                 if (!batchWrite.isEmpty()) {
                     store.batchWrite(batchWrite);
                 }
+                return null;
+            }
+        }
+
+        private void updateBatches(CompletedBatchOperation completed) {
+            if (completed.isSuccess()) {
+                for (IntentUpdate update : intentUpdates) {
+                    update.batchSuccess();
+                }
             } else {
                 // entire batch has been reverted...
-                log.warn("Failed items: {}", completed.failedItems());
+                log.debug("Failed items: {}", completed.failedItems());
+                log.debug("Failed ids: {}",  completed.failedIds());
                 for (Long id : completed.failedIds()) {
                     IntentId targetId = IntentId.valueOf(id);
                     for (IntentUpdate update : intentUpdates) {
                         List<Intent> installables = Lists.newArrayList(update.newInstallables());
-                        installables.addAll(update.oldInstallables());
+                        if (update.oldInstallables() != null) {
+                            installables.addAll(update.oldInstallables());
+                        }
                         for (Intent intent : installables) {
                             if (intent.id().equals(targetId)) {
                                 update.batchFailed();
@@ -837,8 +849,9 @@ public class IntentManager
         private void abandonShip() {
             // the batch has failed
             // TODO: maybe we should do more?
-            future = null;
             log.error("Walk the plank, matey...");
+            future = null;
+            batchService.removeIntentOperations(ops);
         }
         /**
@@ -866,11 +879,15 @@ public class IntentManager
             if (future.cancel(true)) { // cancel success; batch is reverted
                 // reset the timer
                 resetTimeoutLimit();
-                if (installAttempt++ >= MAX_ATTEMPTS) {
+                installAttempt++;
+                if (installAttempt == MAX_ATTEMPTS) {
                     log.warn("Install request timed out: {}", ops);
                     for (IntentUpdate update : intentUpdates) {
                         update.batchFailed();
                     }
+                } else if (installAttempt > MAX_ATTEMPTS) {
+                    abandonShip();
+                    return;
                 } // else just resubmit the work
                 future = applyNextBatch();
                 executor.submit(this);
@@ -880,7 +897,7 @@ public class IntentManager
                 // cancel failed... batch is broken; shouldn't happen!
                 // we could manually reverse everything
                 // ... or just core dump and send email to Ali
-                batchService.removeIntentOperations(ops);
+                abandonShip();
             }
         }
@@ -925,6 +942,7 @@ public class IntentManager
                 log.error("Error submitting batches:", e);
                 // FIXME incomplete Intents should be cleaned up
                 //       (transition to FAILED, etc.)
+                abandonShip();
             }
         }
     }
--- a/core/net/src/test/java/org/onlab/onos/net/intent/impl/IntentManagerTest.java
View file @93508c2
+++ b/core/net/src/test/java/org/onlab/onos/net/intent/impl/IntentManagerTest.java
View file @93508c2
@@ -206,6 +206,56 @@ public class IntentManagerTest {
                      flowRuleService.flows.iterator().next().priority());
     }
+    /**
+     * Tests for proper behavior of installation of an intent that triggers
+     * a compilation error.
+     */
+    @Test
+    public void errorIntentCompile() {
+        final TestIntentCompilerError errorCompiler = new TestIntentCompilerError();
+        extensionService.registerCompiler(MockIntent.class, errorCompiler);
+        MockIntent intent = new MockIntent(MockIntent.nextId());
+        listener.setLatch(1, Type.INSTALL_REQ);
+        listener.setLatch(1, Type.FAILED);
+        service.submit(intent);
+        listener.await(Type.INSTALL_REQ);
+        listener.await(Type.FAILED);
+    }
+
+    /**
+     * Tests handling a future that contains an error as a result of
+     * installing an intent.
+     */
+    @Test
+    public void errorIntentInstallFromFlows() {
+        final Long id = MockIntent.nextId();
+        flowRuleService.setFuture(false, 1);
+        MockIntent intent = new MockIntent(id);
+        listener.setLatch(1, Type.FAILED);
+        listener.setLatch(1, Type.INSTALL_REQ);
+        service.submit(intent);
+        listener.await(Type.INSTALL_REQ);
+        delay(10); // need to make sure we have some failed futures returned first
+        flowRuleService.setFuture(true, 0);
+        listener.await(Type.FAILED);
+    }
+
+    /**
+     * Tests handling of an error that is generated by the intent installer.
+     */
+    @Test
+    public void errorIntentInstallFromInstaller() {
+        final TestIntentErrorInstaller errorInstaller = new TestIntentErrorInstaller();
+        extensionService.registerInstaller(MockInstallableIntent.class, errorInstaller);
+        MockIntent intent = new MockIntent(MockIntent.nextId());
+        listener.setLatch(1, Type.INSTALL_REQ);
+        listener.setLatch(1, Type.FAILED);
+        service.submit(intent);
+        listener.await(Type.INSTALL_REQ);
+        listener.await(Type.FAILED);
+
+    }
+
     private static class TestListener implements IntentListener {
         final Multimap<IntentEvent.Type, IntentEvent> events = HashMultimap.create();
         Map<IntentEvent.Type, CountDownLatch> latchMap = Maps.newHashMap();
@@ -229,7 +279,7 @@ public class IntentManagerTest {
         public void await(IntentEvent.Type type) {
             try {
                 assertTrue("Timed out waiting for: " + type,
-                           latchMap.get(type).await(5, TimeUnit.SECONDS));
+                        latchMap.get(type).await(5, TimeUnit.SECONDS));
             } catch (InterruptedException e) {
                 e.printStackTrace();
             }
@@ -299,6 +349,14 @@ public class IntentManagerTest {
         }
     }
+    private static class TestIntentCompilerError implements IntentCompiler<MockIntent> {
+        @Override
+        public List<Intent> compile(MockIntent intent, List<Intent> installable,
+                                    Set<LinkResourceAllocations> resources) {
+            throw new IntentCompilationException("Compilation always fails");
+        }
+    }
+
     private static class TestIntentInstaller implements IntentInstaller<MockInstallableIntent> {
         @Override
         public List<FlowRuleBatchOperation> install(MockInstallableIntent intent) {
@@ -327,6 +385,23 @@ public class IntentManagerTest {
         }
     }
+    private static class TestIntentErrorInstaller implements IntentInstaller<MockInstallableIntent> {
+        @Override
+        public List<FlowRuleBatchOperation> install(MockInstallableIntent intent) {
+            throw new IntentInstallationException("install() always fails");
+        }
+
+        @Override
+        public List<FlowRuleBatchOperation> uninstall(MockInstallableIntent intent) {
+            throw new IntentRemovalException("uninstall() always fails");
+        }
+
+        @Override
+        public List<FlowRuleBatchOperation> replace(MockInstallableIntent oldIntent, MockInstallableIntent newIntent) {
+            throw new IntentInstallationException("replace() always fails");
+        }
+    }
+
     /**
      * Hamcrest matcher to check that a conllection of Intents contains an
      * Intent with the specified Intent Id.
--- a/core/net/src/test/java/org/onlab/onos/net/intent/impl/MockFlowRuleService.java
View file @93508c2
+++ b/core/net/src/test/java/org/onlab/onos/net/intent/impl/MockFlowRuleService.java
View file @93508c2
 package org.onlab.onos.net.intent.impl;
-import com.google.common.collect.Sets;
+import java.util.Collections;
-import com.google.common.util.concurrent.Futures;
+import java.util.Set;
+import java.util.concurrent.Future;
+
 import org.onlab.onos.core.ApplicationId;
 import org.onlab.onos.net.DeviceId;
 import org.onlab.onos.net.flow.CompletedBatchOperation;
@@ -12,9 +14,9 @@ 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 com.google.common.collect.ImmutableSet;
-import java.util.Set;
+import com.google.common.collect.Sets;
-import java.util.concurrent.Future;
+import com.google.common.util.concurrent.Futures;
 public class MockFlowRuleService implements FlowRuleService {
@@ -23,7 +25,17 @@ public class MockFlowRuleService implements FlowRuleService {
     final Set<FlowRule> flows = Sets.newHashSet();
     public void setFuture(boolean success) {
-        future = Futures.immediateFuture(new CompletedBatchOperation(true, Collections.emptySet()));
+        setFuture(success, 0);
+    }
+
+    public void setFuture(boolean success, long intentId) {
+        if (success) {
+            future = Futures.immediateFuture(new CompletedBatchOperation(true, Collections.emptySet()));
+        } else {
+            final Set<Long> failedIds = ImmutableSet.of(intentId);
+            future = Futures.immediateFuture(
+                    new CompletedBatchOperation(false, flows, failedIds));
+        }
     }
     @Override