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
diff --git a/core/net/src/main/java/org/onlab/onos/net/intent/impl/IntentManager.java b/core/net/src/main/java/org/onlab/onos/net/intent/impl/IntentManager.java
index d307978..56ea650 100644
--- a/core/net/src/main/java/org/onlab/onos/net/intent/impl/IntentManager.java
+++ b/core/net/src/main/java/org/onlab/onos/net/intent/impl/IntentManager.java
@@ -300,6 +300,7 @@
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 @@
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 @@
return !isComplete() ? batches.get(currentBatch) : null;
}
- void batchSuccess(BatchWrite batchWrite) {
+ void batchSuccess() {
// move on to next Batch
- if (++currentBatch == batches.size()) {
- finalizeStates(batchWrite);
- }
+ currentBatch++;
}
void batchFailed() {
@@ -684,7 +684,6 @@
// 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 @@
batchWrite.removeIntent(intent.id());
break;
case FAILED:
+ batchWrite.setState(intent, FAILED);
batchWrite.removeInstalledIntents(intent.id());
break;
@@ -801,27 +801,39 @@
batch.addAll(update.currentBatch());
}
}
- return (batch.size() > 0) ? flowRuleService.applyBatch(batch) : null;
- }
-
- private void updateBatches(CompletedBatchOperation completed) {
- if (completed.isSuccess()) {
+ if (batch.size() > 0) {
+ //FIXME apply batch might throw an exception
+ return flowRuleService.applyBatch(batch);
+ } else {
+ // 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 @@
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 @@
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 @@
// 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 @@
log.error("Error submitting batches:", e);
// FIXME incomplete Intents should be cleaned up
// (transition to FAILED, etc.)
+ abandonShip();
}
}
}
diff --git a/core/net/src/test/java/org/onlab/onos/net/intent/impl/IntentManagerTest.java b/core/net/src/test/java/org/onlab/onos/net/intent/impl/IntentManagerTest.java
index 8528228..8709295 100644
--- a/core/net/src/test/java/org/onlab/onos/net/intent/impl/IntentManagerTest.java
+++ b/core/net/src/test/java/org/onlab/onos/net/intent/impl/IntentManagerTest.java
@@ -206,6 +206,56 @@
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 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 @@
}
}
+ 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 @@
}
}
+ 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.
diff --git a/core/net/src/test/java/org/onlab/onos/net/intent/impl/MockFlowRuleService.java b/core/net/src/test/java/org/onlab/onos/net/intent/impl/MockFlowRuleService.java
index 53097a0..ba74e31 100644
--- a/core/net/src/test/java/org/onlab/onos/net/intent/impl/MockFlowRuleService.java
+++ b/core/net/src/test/java/org/onlab/onos/net/intent/impl/MockFlowRuleService.java
@@ -1,7 +1,9 @@
package org.onlab.onos.net.intent.impl;
-import com.google.common.collect.Sets;
-import com.google.common.util.concurrent.Futures;
+import java.util.Collections;
+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.FlowRuleListener;
import org.onlab.onos.net.flow.FlowRuleService;
-import java.util.Collections;
-import java.util.Set;
-import java.util.concurrent.Future;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import com.google.common.util.concurrent.Futures;
public class MockFlowRuleService implements FlowRuleService {
@@ -23,7 +25,17 @@
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