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/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 31adf1f..a1ae27b 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,15 +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", newData.key());
+ 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
@@ -366,7 +397,7 @@
if (current == null) {
return null;
}
- return new IntentData(current);
+ return IntentData.copy(current);
}
@Override
@@ -377,13 +408,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;
}
@@ -434,7 +465,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));
}
@@ -452,7 +483,7 @@
// some work.
if (isMaster(event.value().intent().key())) {
if (delegate != null) {
- delegate.process(new IntentData(event.value()));
+ delegate.process(IntentData.copy(event.value()));
}
}