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 2f29645..a4af213 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;
}
@@ -261,6 +433,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();
@@ -353,6 +531,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;
+ }
}
/**