ONOS-6468 Fix for race condition between compilation and installation complete state.
- Fix for a bug where intent compilation complete state,
can overwrites intent installation complete state,
if intent installation completes before compilation complete state is written to the store.
- Introduced internalState version on IntentData,
which is effectively mutation count, in order to avoid
batch write of compilation result overwriting installation result
Change-Id: I5d77dfbe496e690ebdf2b4f9643d2b64c4233182
diff --git a/core/api/src/main/java/org/onosproject/net/intent/IntentData.java b/core/api/src/main/java/org/onosproject/net/intent/IntentData.java
index c30993e..23cc319 100644
--- a/core/api/src/main/java/org/onosproject/net/intent/IntentData.java
+++ b/core/api/src/main/java/org/onosproject/net/intent/IntentData.java
@@ -44,19 +44,163 @@
private final IntentState request; //TODO perhaps we want a full fledged object for requests
private IntentState state;
+ /**
+ * Intent's user request version.
+ * <p>
+ * version is assigned when an Intent was picked up by batch worker
+ * and added to pending map.
+ */
private final Timestamp version;
+ /**
+ * Intent's internal state version.
+ */
+ // ~= mutation count
+ private int internalStateVersion;
private NodeId origin;
private int errorCount;
private List<Intent> installables;
/**
+ * Creates IntentData for Intent submit request.
+ *
+ * @param intent to request
+ * @return IntentData
+ */
+ public static IntentData submit(Intent intent) {
+ return new IntentData(checkNotNull(intent), INSTALL_REQ);
+ }
+
+ /**
+ * Creates IntentData for Intent withdraw request.
+ *
+ * @param intent to request
+ * @return IntentData
+ */
+ public static IntentData withdraw(Intent intent) {
+ return new IntentData(checkNotNull(intent), WITHDRAW_REQ);
+ }
+
+ /**
+ * Creates IntentData for Intent purge request.
+ *
+ * @param intent to request
+ * @return IntentData
+ */
+ public static IntentData purge(Intent intent) {
+ return new IntentData(checkNotNull(intent), PURGE_REQ);
+ }
+
+ /**
+ * Creates updated IntentData after assigning task to a node.
+ *
+ * @param data IntentData to update work assignment
+ * @param timestamp to assign to current request
+ * @param node node which was assigned to handle this request (local node id)
+ * @return updated IntentData object
+ */
+ public static IntentData assign(IntentData data, Timestamp timestamp, NodeId node) {
+ IntentData assigned = new IntentData(data, checkNotNull(timestamp));
+ assigned.origin = checkNotNull(node);
+ assigned.internalStateVersion++;
+ return assigned;
+ }
+
+ /**
+ * Creates a copy of given IntentData.
+ *
+ * @param data intent data to copy
+ * @return copy
+ */
+ public static IntentData copy(IntentData data) {
+ return new IntentData(data);
+ }
+
+ /**
+ * Create a copy of IntentData in next state.
+ *
+ * @param data intent data to copy
+ * @param nextState to transition to
+ * @return next state
+ */
+ public static IntentData nextState(IntentData data, IntentState nextState) {
+ IntentData next = new IntentData(data);
+ // TODO state machine sanity check
+ next.setState(checkNotNull(nextState));
+ return next;
+ }
+
+ // TODO Should this be method of it's own, or
+ // should nextState(*, CORRUPT) call increment error count?
+ /**
+ * Creates a copy of IntentData in corrupt state,
+ * incrementing error count.
+ *
+ * @param data intent data to copy
+ * @return next state
+ */
+ public static IntentData corrupt(IntentData data) {
+ IntentData next = new IntentData(data);
+ next.setState(IntentState.CORRUPT);
+ next.incrementErrorCount();
+ return next;
+ }
+
+ /**
+ * Creates updated IntentData with compilation result.
+ *
+ * @param data IntentData to update
+ * @param installables compilation result
+ * @return updated IntentData object
+ */
+ public static IntentData compiled(IntentData data, List<Intent> installables) {
+ return new IntentData(data, checkNotNull(installables));
+ }
+
+
+ /**
+ * Constructor for creating IntentData representing user request.
+ *
+ * @param intent this metadata references
+ * @param reqState request state
+ */
+ private IntentData(Intent intent,
+ IntentState reqState) {
+ this.intent = checkNotNull(intent);
+ this.request = checkNotNull(reqState);
+ this.version = null;
+ this.state = reqState;
+ this.installables = ImmutableList.of();
+ }
+
+ /**
+ * Constructor for creating updated IntentData.
+ *
+ * @param original IntentData to copy from
+ * @param newReqVersion new request version
+ */
+ private IntentData(IntentData original, Timestamp newReqVersion) {
+ intent = original.intent;
+ state = original.state;
+ request = original.request;
+ version = newReqVersion;
+ internalStateVersion = original.internalStateVersion;
+ origin = original.origin;
+ installables = original.installables;
+ errorCount = original.errorCount;
+ }
+
+ /**
* Creates a new intent data object.
*
* @param intent intent this metadata references
* @param state intent state
* @param version version of the intent for this key
+ *
+ * @deprecated in 1.11.0
*/
+ // used to create initial IntentData (version = null)
+ @Deprecated
public IntentData(Intent intent, IntentState state, Timestamp version) {
checkNotNull(intent);
checkNotNull(state);
@@ -74,7 +218,11 @@
* @param state intent state
* @param version version of the intent for this key
* @param origin ID of the node where the data was originally created
+ *
+ * @deprecated in 1.11.0
*/
+ // No longer used in the code base anywhere
+ @Deprecated
public IntentData(Intent intent, IntentState state, Timestamp version, NodeId origin) {
checkNotNull(intent);
checkNotNull(state);
@@ -96,7 +244,12 @@
* @param request intent request
* @param version version of the intent for this key
* @param origin ID of the node where the data was originally created
+ *
+ * @deprecated in 1.11.0
*/
+ // No longer used in the code base anywhere
+ // was used when IntentData is picked up by some of the node and was assigned with a version
+ @Deprecated
public IntentData(Intent intent, IntentState state, IntentState request, Timestamp version, NodeId origin) {
checkNotNull(intent);
checkNotNull(state);
@@ -115,7 +268,12 @@
* Copy constructor.
*
* @param intentData intent data to copy
+ *
+ * @deprecated in 1.11.0 use {@link #copy(IntentData)} instead
*/
+ // used to create a defensive copy
+ // to be made private
+ @Deprecated
public IntentData(IntentData intentData) {
checkNotNull(intentData);
@@ -123,6 +281,7 @@
state = intentData.state;
request = intentData.request;
version = intentData.version;
+ internalStateVersion = intentData.internalStateVersion;
origin = intentData.origin;
installables = intentData.installables;
errorCount = intentData.errorCount;
@@ -133,12 +292,19 @@
*
* @param original original data
* @param installables new installable intents to set
+ *
+ * @deprecated in 1.11.0 use {@link #compiled(IntentData, List)} instead
*/
+ // used to create an instance who reached stable state
+ // note that state is mutable field, so it gets altered else where
+ // (probably that design is mother of all intent bugs)
+ @Deprecated
public IntentData(IntentData original, List<Intent> installables) {
this(original);
+ this.internalStateVersion++;
this.installables = checkNotNull(installables).isEmpty() ?
- Collections.emptyList() : ImmutableList.copyOf(installables);
+ ImmutableList.of() : ImmutableList.copyOf(installables);
}
// kryo constructor
@@ -180,7 +346,7 @@
}
/**
- * Returns the version of the intent for this key.
+ * Returns the request version of the intent for this key.
*
* @return intent version
*/
@@ -188,6 +354,11 @@
return version;
}
+ // had to be made public for the store timestamp provider
+ public int internalStateVersion() {
+ return internalStateVersion;
+ }
+
/**
* Returns the origin node that created this intent.
*
@@ -203,6 +374,7 @@
* @param newState new state of the intent
*/
public void setState(IntentState newState) {
+ this.internalStateVersion++;
this.state = newState;
}
@@ -260,6 +432,12 @@
return false;
}
+ assert (currentData.version().equals(newData.version()));
+ if (currentData.internalStateVersion >= newData.internalStateVersion) {
+ log.trace("{} update not acceptable: current is newer internally", newData.key());
+ return false;
+ }
+
// current and new data versions are the same
IntentState currentState = currentData.state();
IntentState newState = newData.state();
@@ -347,6 +525,7 @@
.add("key", key())
.add("state", state())
.add("version", version())
+ .add("internalStateVersion", internalStateVersion)
.add("intent", intent())
.add("origin", origin())
.add("installables", installables())
diff --git a/core/api/src/main/java/org/onosproject/store/service/WallClockTimestamp.java b/core/api/src/main/java/org/onosproject/store/service/WallClockTimestamp.java
index eb634da..e96649c 100644
--- a/core/api/src/main/java/org/onosproject/store/service/WallClockTimestamp.java
+++ b/core/api/src/main/java/org/onosproject/store/service/WallClockTimestamp.java
@@ -19,9 +19,9 @@
import java.util.Objects;
+import org.joda.time.DateTime;
import org.onosproject.store.Timestamp;
-import com.google.common.base.MoreObjects;
import com.google.common.collect.ComparisonChain;
/**
@@ -69,9 +69,7 @@
@Override
public String toString() {
- return MoreObjects.toStringHelper(getClass())
- .add("unixTimestamp", unixTimestamp)
- .toString();
+ return new DateTime(unixTimestamp).toString();
}
/**
diff --git a/core/api/src/test/java/org/onosproject/net/intent/IntentDataTest.java b/core/api/src/test/java/org/onosproject/net/intent/IntentDataTest.java
index f7bb7a3..62ad289 100644
--- a/core/api/src/test/java/org/onosproject/net/intent/IntentDataTest.java
+++ b/core/api/src/test/java/org/onosproject/net/intent/IntentDataTest.java
@@ -25,6 +25,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertTrue;
+import static org.onosproject.net.intent.IntentState.FAILED;
import static org.onosproject.net.intent.IntentTestsMocks.MockIntent;
import static org.onosproject.net.intent.IntentTestsMocks.MockTimestamp;
@@ -50,6 +51,7 @@
IdGenerator idGenerator;
+ @Override
@Before
public void setUp() {
super.setUp();
@@ -102,17 +104,18 @@
assertFalse(IntentData.isUpdateAcceptable(data2, data1));
IntentData installing = new IntentData(intent1, IntentState.INSTALLING, timestamp1);
- IntentData installed = new IntentData(intent1, IntentState.INSTALLED, timestamp1);
+ IntentData installed = IntentData.nextState(installing, IntentState.INSTALLED);
IntentData withdrawing = new IntentData(intent1, IntentState.WITHDRAWING, timestamp1);
- IntentData withdrawn = new IntentData(intent1, IntentState.WITHDRAWN, timestamp1);
+ IntentData withdrawn = IntentData.nextState(withdrawing, IntentState.WITHDRAWN);
IntentData failed = new IntentData(intent1, IntentState.FAILED, timestamp1);
- IntentData purgeReq = new IntentData(intent1, IntentState.PURGE_REQ, timestamp1);
IntentData compiling = new IntentData(intent1, IntentState.COMPILING, timestamp1);
IntentData recompiling = new IntentData(intent1, IntentState.RECOMPILING, timestamp1);
- IntentData installReq = new IntentData(intent1, IntentState.INSTALL_REQ, timestamp1);
- IntentData withdrawReq = new IntentData(intent1, IntentState.WITHDRAW_REQ, timestamp1);
+
+ IntentData installReq = new IntentData(intent1, IntentState.INSTALL_REQ, timestamp2);
+ IntentData withdrawReq = new IntentData(intent1, IntentState.WITHDRAW_REQ, timestamp2);
+ IntentData purgeReq = new IntentData(intent1, IntentState.PURGE_REQ, timestamp2);
// We can't change to the same state
assertFalse(IntentData.isUpdateAcceptable(installing, installing));
@@ -120,6 +123,8 @@
// From installing we can change to installed
assertTrue(IntentData.isUpdateAcceptable(installing, installed));
+ // transition in reverse should be rejected
+ assertFalse(IntentData.isUpdateAcceptable(installed, installing));
// Sanity checks in case the manager submits bogus state transitions
assertFalse(IntentData.isUpdateAcceptable(installing, withdrawing));
@@ -144,10 +149,10 @@
assertFalse(IntentData.isUpdateAcceptable(failed, failed));
// But we can go from any install* or withdraw* state to failed
- assertTrue(IntentData.isUpdateAcceptable(installing, failed));
- assertTrue(IntentData.isUpdateAcceptable(installed, failed));
- assertTrue(IntentData.isUpdateAcceptable(withdrawing, failed));
- assertTrue(IntentData.isUpdateAcceptable(withdrawn, failed));
+ assertTrue(IntentData.isUpdateAcceptable(installing, IntentData.nextState(installing, FAILED)));
+ assertTrue(IntentData.isUpdateAcceptable(installed, IntentData.nextState(installed, FAILED)));
+ assertTrue(IntentData.isUpdateAcceptable(withdrawing, IntentData.nextState(withdrawing, FAILED)));
+ assertTrue(IntentData.isUpdateAcceptable(withdrawn, IntentData.nextState(withdrawn, FAILED)));
// We can go from anything to purgeReq
assertTrue(IntentData.isUpdateAcceptable(installing, purgeReq));
@@ -165,7 +170,7 @@
// We're never allowed to store transient states
assertFalse(IntentData.isUpdateAcceptable(installing, compiling));
assertFalse(IntentData.isUpdateAcceptable(installing, recompiling));
- assertFalse(IntentData.isUpdateAcceptable(installing, installReq));
- assertFalse(IntentData.isUpdateAcceptable(installing, withdrawReq));
+ assertFalse(IntentData.isUpdateAcceptable(installing, installing));
+ assertFalse(IntentData.isUpdateAcceptable(installing, withdrawing));
}
}
diff --git a/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java b/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java
index d812941..fbd134f 100644
--- a/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java
+++ b/core/api/src/test/java/org/onosproject/net/intent/IntentTestsMocks.java
@@ -471,6 +471,19 @@
MockTimestamp that = (MockTimestamp) o;
return this.value - that.value;
}
+
+ @Override
+ public int hashCode() {
+ return value;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (obj instanceof MockTimestamp) {
+ return this.compareTo((MockTimestamp) obj) == 0;
+ }
+ return false;
+ }
}
/**
diff --git a/core/common/src/test/java/org/onosproject/store/trivial/SimpleIntentStore.java b/core/common/src/test/java/org/onosproject/store/trivial/SimpleIntentStore.java
index 02d7eb2..9d237af 100644
--- a/core/common/src/test/java/org/onosproject/store/trivial/SimpleIntentStore.java
+++ b/core/common/src/test/java/org/onosproject/store/trivial/SimpleIntentStore.java
@@ -117,7 +117,7 @@
if (pendingData.state() == PURGE_REQ) {
current.remove(newData.key(), newData);
} else {
- current.put(newData.key(), new IntentData(newData));
+ current.put(newData.key(), IntentData.copy(newData));
}
if (pendingData.version().compareTo(newData.version()) <= 0) {
@@ -150,7 +150,7 @@
if (currentData == null) {
return null;
}
- return new IntentData(currentData);
+ return IntentData.copy(currentData);
}
@Override
@@ -167,7 +167,7 @@
existingData.version().compareTo(data.version()) < 0) {
pending.put(data.key(), data);
checkNotNull(delegate, "Store delegate is not set")
- .process(new IntentData(data));
+ .process(IntentData.copy(data));
IntentEvent.getEvent(data).ifPresent(this::notifyDelegate);
} else {
log.debug("IntentData {} is older than existing: {}",
diff --git a/core/net/src/main/java/org/onosproject/net/intent/impl/InstallCoordinator.java b/core/net/src/main/java/org/onosproject/net/intent/impl/InstallCoordinator.java
index c21f3f7..c4529bc 100644
--- a/core/net/src/main/java/org/onosproject/net/intent/impl/InstallCoordinator.java
+++ b/core/net/src/main/java/org/onosproject/net/intent/impl/InstallCoordinator.java
@@ -178,18 +178,21 @@
if (errCtxs == null || errCtxs.isEmpty()) {
if (toInstall.isPresent()) {
IntentData installData = toInstall.get();
- log.debug("Completed installing: {}", installData.key());
+ log.debug("Completed installing: {}:{}",
+ installData.key(),
+ installData.intent().id());
installData = new IntentData(installData, installData.installables());
installData.setState(INSTALLED);
intentStore.write(installData);
} else if (toUninstall.isPresent()) {
IntentData uninstallData = toUninstall.get();
uninstallData = new IntentData(uninstallData, Collections.emptyList());
- log.debug("Completed withdrawing: {}", uninstallData.key());
+ log.debug("Completed withdrawing: {}:{}",
+ uninstallData.key(),
+ uninstallData.intent().id());
switch (uninstallData.request()) {
case INSTALL_REQ:
- log.warn("{} was requested to withdraw during installation?",
- uninstallData.intent());
+ // INSTALLED intent was damaged & clean up is now complete
uninstallData.setState(FAILED);
break;
case WITHDRAW_REQ:
@@ -204,16 +207,12 @@
// if toInstall was cause of error, then recompile (manage/increment counter, when exceeded -> CORRUPT)
if (toInstall.isPresent()) {
IntentData installData = toInstall.get();
- installData.setState(CORRUPT);
- installData.incrementErrorCount();
- intentStore.write(installData);
+ intentStore.write(IntentData.corrupt(installData));
}
// if toUninstall was cause of error, then CORRUPT (another job will clean this up)
if (toUninstall.isPresent()) {
IntentData uninstallData = toUninstall.get();
- uninstallData.setState(CORRUPT);
- uninstallData.incrementErrorCount();
- intentStore.write(uninstallData);
+ intentStore.write(IntentData.corrupt(uninstallData));
}
}
}
diff --git a/core/net/src/main/java/org/onosproject/net/intent/impl/IntentManager.java b/core/net/src/main/java/org/onosproject/net/intent/impl/IntentManager.java
index 7b09a1a..0cae8da 100644
--- a/core/net/src/main/java/org/onosproject/net/intent/impl/IntentManager.java
+++ b/core/net/src/main/java/org/onosproject/net/intent/impl/IntentManager.java
@@ -239,7 +239,7 @@
public void submit(Intent intent) {
checkPermission(INTENT_WRITE);
checkNotNull(intent, INTENT_NULL);
- IntentData data = new IntentData(intent, IntentState.INSTALL_REQ, null);
+ IntentData data = IntentData.submit(intent);
store.addPending(data);
}
@@ -247,7 +247,7 @@
public void withdraw(Intent intent) {
checkPermission(INTENT_WRITE);
checkNotNull(intent, INTENT_NULL);
- IntentData data = new IntentData(intent, IntentState.WITHDRAW_REQ, null);
+ IntentData data = IntentData.withdraw(intent);
store.addPending(data);
}
@@ -255,7 +255,7 @@
public void purge(Intent intent) {
checkPermission(INTENT_WRITE);
checkNotNull(intent, INTENT_NULL);
- IntentData data = new IntentData(intent, IntentState.PURGE_REQ, null);
+ IntentData data = IntentData.purge(intent);
store.addPending(data);
// remove associated group if there is one
@@ -495,6 +495,8 @@
@Override
public void triggerCompile(Iterable<Key> intentKeys,
boolean compileAllFailed) {
+ // TODO figure out who is making excessive calls?
+ log.trace("submitting {} + all?:{}", intentKeys, compileAllFailed);
buildAndSubmitBatches(intentKeys, compileAllFailed);
}
}
@@ -509,6 +511,10 @@
CompletableFuture.runAsync(() -> {
// process intent until the phase reaches one of the final phases
List<CompletableFuture<IntentData>> futures = operations.stream()
+ .map(data -> {
+ log.debug("Start processing of {} {}@{}", data.request(), data.key(), data.version());
+ return data;
+ })
.map(x -> CompletableFuture.completedFuture(x)
.thenApply(IntentManager.this::createInitialPhase)
.thenApplyAsync(IntentProcessPhase::process, workerExecutor)
@@ -525,9 +531,9 @@
case INSTALLING:
case WITHDRAW_REQ:
case WITHDRAWING:
- x.setState(FAILED);
+ // TODO should we swtich based on current
IntentData current = store.getIntentData(x.key());
- return new IntentData(x, current.installables());
+ return IntentData.nextState(current, FAILED);
default:
return null;
}
diff --git a/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Compiling.java b/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Compiling.java
index bb82dfc..124d3fb 100644
--- a/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Compiling.java
+++ b/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Compiling.java
@@ -57,7 +57,7 @@
List<Intent> compiled = processor.compile(data.intent(),
//TODO consider passing an optional here in the future
stored.map(IntentData::installables).orElse(null));
- return Optional.of(new Installing(processor, new IntentData(data, compiled), stored));
+ return Optional.of(new Installing(processor, IntentData.compiled(data, compiled), stored));
} catch (IntentException e) {
log.warn("Unable to compile intent {} due to:", data.intent(), e);
if (stored.filter(x -> !x.installables().isEmpty()).isPresent()) {
diff --git a/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Failed.java b/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Failed.java
index 0a05e4f..26ba858 100644
--- a/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Failed.java
+++ b/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Failed.java
@@ -33,8 +33,7 @@
* @param data intentData
*/
Failed(IntentData data) {
- this.data = checkNotNull(data);
- this.data.setState(FAILED);
+ this.data = IntentData.nextState(checkNotNull(data), FAILED);
}
@Override
diff --git a/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Withdrawn.java b/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Withdrawn.java
index b51f1f7..2e8e67a 100644
--- a/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Withdrawn.java
+++ b/core/net/src/main/java/org/onosproject/net/intent/impl/phase/Withdrawn.java
@@ -33,8 +33,7 @@
* @param data intent data containing an intent to be withdrawn
*/
Withdrawn(IntentData data) {
- this.data = checkNotNull(data);
- this.data.setState(WITHDRAWN);
+ this.data = IntentData.nextState(checkNotNull(data), WITHDRAWN);
}
@Override
diff --git a/core/store/dist/src/main/java/org/onosproject/store/intent/impl/GossipIntentStore.java b/core/store/dist/src/main/java/org/onosproject/store/intent/impl/GossipIntentStore.java
index 6ccb629..5d0d54c 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/intent/impl/GossipIntentStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/intent/impl/GossipIntentStore.java
@@ -25,6 +25,7 @@
import org.apache.felix.scr.annotations.Reference;
import org.apache.felix.scr.annotations.ReferenceCardinality;
import org.apache.felix.scr.annotations.Service;
+import org.onlab.util.Backtrace;
import org.onlab.util.KryoNamespace;
import org.onosproject.cfg.ComponentConfigService;
import org.onosproject.cluster.ClusterService;
@@ -41,6 +42,7 @@
import org.onosproject.net.intent.Key;
import org.onosproject.net.intent.WorkPartitionService;
import org.onosproject.store.AbstractStore;
+import org.onosproject.store.Timestamp;
import org.onosproject.store.serializers.KryoNamespaces;
import org.onosproject.store.service.EventuallyConsistentMap;
import org.onosproject.store.service.EventuallyConsistentMapBuilder;
@@ -56,6 +58,7 @@
import java.util.Dictionary;
import java.util.List;
import java.util.Objects;
+import java.util.Optional;
import java.util.Properties;
import java.util.concurrent.atomic.AtomicLong;
import java.util.stream.Collectors;
@@ -120,6 +123,26 @@
private boolean persistenceEnabled;
+ /**
+ * TimestampProvieder for currentMap.
+ *
+ * @param key Intent key
+ * @param data Intent data
+ * @return generated time stamp
+ */
+ private Timestamp currentTimestampProvider(Key key, IntentData data) {
+ // vector timestamp consisting from 3 components
+ // (request timestamp, internal state, sequence #)
+
+ // 2nd component required to avoid compilation result overwriting installation state
+ // note: above is likely to be a sign of design issue in transition to installation phase
+ // 3rd component required for generating new timestamp for removal..
+ return new MultiValuedTimestamp<>(
+ Optional.ofNullable(data.version()).orElseGet(WallClockTimestamp::new),
+ new MultiValuedTimestamp<>(data.internalStateVersion(),
+ sequenceNumber.incrementAndGet()));
+ }
+
@Activate
public void activate(ComponentContext context) {
configService.registerProperties(getClass());
@@ -139,10 +162,7 @@
storageService.<Key, IntentData>eventuallyConsistentMapBuilder()
.withName("intent-current")
.withSerializer(intentSerializer)
- .withTimestampProvider((key, intentData) ->
- new MultiValuedTimestamp<>(intentData == null ?
- new WallClockTimestamp() : intentData.version(),
- sequenceNumber.getAndIncrement()))
+ .withTimestampProvider(this::currentTimestampProvider)
.withPeerUpdateFunction((key, intentData) -> getPeerNodes(key, intentData));
EventuallyConsistentMapBuilder pendingECMapBuilder =
@@ -284,13 +304,26 @@
// this always succeeds
if (newData.state() == PURGE_REQ) {
if (currentData != null) {
+ if (log.isTraceEnabled()) {
+ log.trace("Purging {} in currentMap. {}@{}",
+ newData.key(), newData.state(), newData.version(),
+ new Backtrace());
+ }
currentMap.remove(newData.key(), currentData);
} else {
log.info("Gratuitous purge request for intent: {}", newData.key());
}
} else {
- currentMap.put(newData.key(), new IntentData(newData));
+ if (log.isTraceEnabled()) {
+ log.trace("Putting {} in currentMap. {}@{}",
+ newData.key(), newData.state(), newData.version(),
+ new Backtrace());
+ }
+ currentMap.put(newData.key(), IntentData.copy(newData));
}
+ } else {
+ log.debug("Update for {} not acceptable from:\n{}\nto:\n{}",
+ newData.key(), currentData, newData);
}
// Remove the intent data from the pending map if the newData is more
// recent or equal to the existing entry. No matter if it is an acceptable
@@ -362,7 +395,7 @@
if (current == null) {
return null;
}
- return new IntentData(current);
+ return IntentData.copy(current);
}
@Override
@@ -373,13 +406,13 @@
// avoid the creation of Intents with state == request, which can
// be problematic if the Intent state is different from *REQ
// {INSTALL_, WITHDRAW_ and PURGE_}.
- pendingMap.put(data.key(), new IntentData(data.intent(), data.state(), data.request(),
- new WallClockTimestamp(), clusterService.getLocalNode().id()));
+ pendingMap.put(data.key(), IntentData.assign(data,
+ new WallClockTimestamp(),
+ clusterService.getLocalNode().id()));
} else {
pendingMap.compute(data.key(), (key, existingValue) -> {
if (existingValue == null || existingValue.version().isOlderThan(data.version())) {
- return new IntentData(data.intent(), data.state(), data.request(),
- data.version(), clusterService.getLocalNode().id());
+ return IntentData.assign(data, data.version(), clusterService.getLocalNode().id());
} else {
return existingValue;
}
@@ -430,7 +463,7 @@
// this intent's partition, notify the Manager that it should
// emit notifications about updated tracked resources.
if (delegate != null && isMaster(event.value().intent().key())) {
- delegate.onUpdate(new IntentData(intentData)); // copy for safety, likely unnecessary
+ delegate.onUpdate(IntentData.copy(intentData)); // copy for safety, likely unnecessary
}
IntentEvent.getEvent(intentData).ifPresent(e -> notifyDelegate(e));
}
@@ -448,7 +481,7 @@
// some work.
if (isMaster(event.value().intent().key())) {
if (delegate != null) {
- delegate.process(new IntentData(event.value()));
+ delegate.process(IntentData.copy(event.value()));
}
}
diff --git a/core/store/dist/src/test/java/org/onosproject/store/intent/impl/GossipIntentStoreTest.java b/core/store/dist/src/test/java/org/onosproject/store/intent/impl/GossipIntentStoreTest.java
index 95dd0cc..9c64691 100644
--- a/core/store/dist/src/test/java/org/onosproject/store/intent/impl/GossipIntentStoreTest.java
+++ b/core/store/dist/src/test/java/org/onosproject/store/intent/impl/GossipIntentStoreTest.java
@@ -21,6 +21,7 @@
import org.onosproject.cfg.ComponentConfigService;
import org.onosproject.cfg.ConfigProperty;
import org.onosproject.cluster.ClusterServiceAdapter;
+import org.onosproject.cluster.NodeId;
import org.onosproject.net.intent.AbstractIntentTest;
import org.onosproject.net.intent.HostToHostIntent;
import org.onosproject.net.intent.Intent;
@@ -49,6 +50,7 @@
private GossipIntentStore intentStore;
private HostToHostIntent.Builder builder1;
+ @Override
@Before
public void setUp() {
intentStore = new GossipIntentStore();
@@ -65,6 +67,7 @@
intentStore.activate(null);
}
+ @Override
@After
public void tearDown() {
intentStore.deactivate();
@@ -171,11 +174,11 @@
);
// now purge the intent that was created
- IntentData purge = new IntentData(
- intent,
- IntentState.PURGE_REQ,
- new IntentTestsMocks.MockTimestamp(12));
- intentStore.write(purge);
+ IntentData purgeAssigned =
+ IntentData.assign(IntentData.purge(intent),
+ new IntentTestsMocks.MockTimestamp(12),
+ new NodeId("node-id"));
+ intentStore.write(purgeAssigned);
// check that no intents are left
assertThat(intentStore.getIntentCount(), is(0L));
diff --git a/utils/misc/src/main/java/org/onlab/util/Backtrace.java b/utils/misc/src/main/java/org/onlab/util/Backtrace.java
new file mode 100644
index 0000000..6c79cee
--- /dev/null
+++ b/utils/misc/src/main/java/org/onlab/util/Backtrace.java
@@ -0,0 +1,27 @@
+/*
+ * Copyright 2017-present Open Networking Laboratory
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.onlab.util;
+
+/**
+ * RuntimeException object intended to used to log execution back trace.
+ *
+ * {@code log.info("Where was I called from?", new Backtrace());}
+ */
+public class Backtrace extends RuntimeException {
+
+ private static final long serialVersionUID = 6123220442887565200L;
+
+}
diff --git a/web/api/src/main/java/org/onosproject/rest/resources/IntentsWebResource.java b/web/api/src/main/java/org/onosproject/rest/resources/IntentsWebResource.java
index b3b7752..20b59f7 100644
--- a/web/api/src/main/java/org/onosproject/rest/resources/IntentsWebResource.java
+++ b/web/api/src/main/java/org/onosproject/rest/resources/IntentsWebResource.java
@@ -293,6 +293,7 @@
latch.await(WITHDRAW_EVENT_TIMEOUT_SECONDS, TimeUnit.SECONDS);
} catch (InterruptedException e) {
log.info("REST Delete operation timed out waiting for intent {}", k);
+ Thread.currentThread().interrupt();
}
// double check the state
IntentState state = service.getIntentState(k);