Committed by
Brian O'Connor
Fixes for GossipIntentStore
* State checking to prevent state updates outrunning. * Copy IntentData on the way in and out of the store. Change-Id: Id18164d15c896c5a62376aac17b7c8c2cac420c2
Showing
3 changed files
with
102 additions
and
8 deletions
| ... | @@ -242,11 +242,13 @@ public class EventuallyConsistentMapImpl<K, V> | ... | @@ -242,11 +242,13 @@ public class EventuallyConsistentMapImpl<K, V> |
| 242 | synchronized (this) { | 242 | synchronized (this) { |
| 243 | Timestamp removed = removedItems.get(key); | 243 | Timestamp removed = removedItems.get(key); |
| 244 | if (removed != null && removed.compareTo(timestamp) > 0) { | 244 | if (removed != null && removed.compareTo(timestamp) > 0) { |
| 245 | + log.debug("ecmap - removed was newer {}", value); | ||
| 245 | return false; | 246 | return false; |
| 246 | } | 247 | } |
| 247 | 248 | ||
| 248 | Timestamped<V> existing = items.get(key); | 249 | Timestamped<V> existing = items.get(key); |
| 249 | if (existing != null && existing.isNewer(timestamp)) { | 250 | if (existing != null && existing.isNewer(timestamp)) { |
| 251 | + log.debug("ecmap - existing was newer {}", value); | ||
| 250 | return false; | 252 | return false; |
| 251 | } else { | 253 | } else { |
| 252 | items.put(key, new Timestamped<>(value, timestamp)); | 254 | items.put(key, new Timestamped<>(value, timestamp)); | ... | ... |
| ... | @@ -45,13 +45,18 @@ import org.slf4j.Logger; | ... | @@ -45,13 +45,18 @@ import org.slf4j.Logger; |
| 45 | import java.util.List; | 45 | import java.util.List; |
| 46 | import java.util.stream.Collectors; | 46 | import java.util.stream.Collectors; |
| 47 | 47 | ||
| 48 | +import static org.onosproject.net.intent.IntentState.FAILED; | ||
| 49 | +import static org.onosproject.net.intent.IntentState.INSTALLED; | ||
| 50 | +import static org.onosproject.net.intent.IntentState.INSTALLING; | ||
| 51 | +import static org.onosproject.net.intent.IntentState.WITHDRAWING; | ||
| 52 | +import static org.onosproject.net.intent.IntentState.WITHDRAWN; | ||
| 48 | import static org.slf4j.LoggerFactory.getLogger; | 53 | import static org.slf4j.LoggerFactory.getLogger; |
| 49 | 54 | ||
| 50 | /** | 55 | /** |
| 51 | * Manages inventory of Intents in a distributed data store that uses optimistic | 56 | * Manages inventory of Intents in a distributed data store that uses optimistic |
| 52 | * replication and gossip based techniques. | 57 | * replication and gossip based techniques. |
| 53 | */ | 58 | */ |
| 54 | -@Component(immediate = false, enabled = false) | 59 | +@Component(immediate = false, enabled = true) |
| 55 | @Service | 60 | @Service |
| 56 | public class GossipIntentStore | 61 | public class GossipIntentStore |
| 57 | extends AbstractStore<IntentEvent, IntentStoreDelegate> | 62 | extends AbstractStore<IntentEvent, IntentStoreDelegate> |
| ... | @@ -144,17 +149,104 @@ public class GossipIntentStore | ... | @@ -144,17 +149,104 @@ public class GossipIntentStore |
| 144 | return null; | 149 | return null; |
| 145 | } | 150 | } |
| 146 | 151 | ||
| 152 | + private IntentData copyData(IntentData original) { | ||
| 153 | + if (original == null) { | ||
| 154 | + return null; | ||
| 155 | + } | ||
| 156 | + IntentData result = | ||
| 157 | + new IntentData(original.intent(), original.state(), original.version()); | ||
| 158 | + | ||
| 159 | + if (original.installables() != null) { | ||
| 160 | + result.setInstallables(original.installables()); | ||
| 161 | + } | ||
| 162 | + return result; | ||
| 163 | + } | ||
| 164 | + | ||
| 165 | + /** | ||
| 166 | + * TODO. | ||
| 167 | + * @param currentData | ||
| 168 | + * @param newData | ||
| 169 | + * @return | ||
| 170 | + */ | ||
| 171 | + private boolean isUpdateAcceptable(IntentData currentData, IntentData newData) { | ||
| 172 | + | ||
| 173 | + if (currentData == null) { | ||
| 174 | + return true; | ||
| 175 | + } else if (currentData.version().compareTo(newData.version()) < 0) { | ||
| 176 | + return true; | ||
| 177 | + } else if (currentData.version().compareTo(newData.version()) > 0) { | ||
| 178 | + return false; | ||
| 179 | + } | ||
| 180 | + | ||
| 181 | + // current and new data versions are the same | ||
| 182 | + IntentState currentState = currentData.state(); | ||
| 183 | + IntentState newState = newData.state(); | ||
| 184 | + | ||
| 185 | + switch (newState) { | ||
| 186 | + case INSTALLING: | ||
| 187 | + if (currentState == INSTALLING) { | ||
| 188 | + return false; | ||
| 189 | + } | ||
| 190 | + // FALLTHROUGH | ||
| 191 | + case INSTALLED: | ||
| 192 | + if (currentState == INSTALLED) { | ||
| 193 | + return false; | ||
| 194 | + } else if (currentState == WITHDRAWING || currentState == WITHDRAWN) { | ||
| 195 | + log.warn("Invalid state transition from {} to {} for intent {}", | ||
| 196 | + currentState, newState, newData.key()); | ||
| 197 | + return false; | ||
| 198 | + } | ||
| 199 | + return true; | ||
| 200 | + | ||
| 201 | + case WITHDRAWING: | ||
| 202 | + if (currentState == WITHDRAWING) { | ||
| 203 | + return false; | ||
| 204 | + } | ||
| 205 | + // FALLTHROUGH | ||
| 206 | + case WITHDRAWN: | ||
| 207 | + if (currentState == WITHDRAWN) { | ||
| 208 | + return false; | ||
| 209 | + } else if (currentState == INSTALLING || currentState == INSTALLED) { | ||
| 210 | + log.warn("Invalid state transition from {} to {} for intent {}", | ||
| 211 | + currentState, newState, newData.key()); | ||
| 212 | + return false; | ||
| 213 | + } | ||
| 214 | + return true; | ||
| 215 | + | ||
| 216 | + | ||
| 217 | + case FAILED: | ||
| 218 | + if (currentState == FAILED) { | ||
| 219 | + return false; | ||
| 220 | + } | ||
| 221 | + return true; | ||
| 222 | + | ||
| 223 | + | ||
| 224 | + case COMPILING: | ||
| 225 | + case RECOMPILING: | ||
| 226 | + case INSTALL_REQ: | ||
| 227 | + case WITHDRAW_REQ: | ||
| 228 | + default: | ||
| 229 | + log.warn("Invalid state {} for intent {}", newState, newData.key()); | ||
| 230 | + return false; | ||
| 231 | + } | ||
| 232 | + } | ||
| 233 | + | ||
| 147 | @Override | 234 | @Override |
| 148 | public void write(IntentData newData) { | 235 | public void write(IntentData newData) { |
| 149 | - log.debug("writing intent {}", newData); | 236 | + //log.debug("writing intent {}", newData); |
| 150 | 237 | ||
| 238 | + IntentData currentData = currentState.get(newData.key()); | ||
| 239 | + | ||
| 240 | + if (isUpdateAcceptable(currentData, newData)) { | ||
| 151 | // Only the master is modifying the current state. Therefore assume | 241 | // Only the master is modifying the current state. Therefore assume |
| 152 | // this always succeeds | 242 | // this always succeeds |
| 153 | - currentState.put(newData.key(), newData); | 243 | + currentState.put(newData.key(), copyData(newData)); |
| 154 | 244 | ||
| 155 | // if current.put succeeded | 245 | // if current.put succeeded |
| 156 | pending.remove(newData.key(), newData); | 246 | pending.remove(newData.key(), newData); |
| 157 | - | 247 | + } else { |
| 248 | + log.debug("not writing update: {}", newData); | ||
| 249 | + } | ||
| 158 | /*try { | 250 | /*try { |
| 159 | notifyDelegate(IntentEvent.getEvent(newData)); | 251 | notifyDelegate(IntentEvent.getEvent(newData)); |
| 160 | } catch (IllegalArgumentException e) { | 252 | } catch (IllegalArgumentException e) { |
| ... | @@ -179,7 +271,7 @@ public class GossipIntentStore | ... | @@ -179,7 +271,7 @@ public class GossipIntentStore |
| 179 | 271 | ||
| 180 | @Override | 272 | @Override |
| 181 | public IntentData getIntentData(Key key) { | 273 | public IntentData getIntentData(Key key) { |
| 182 | - return currentState.get(key); | 274 | + return copyData(currentState.get(key)); |
| 183 | } | 275 | } |
| 184 | 276 | ||
| 185 | @Override | 277 | @Override |
| ... | @@ -189,7 +281,7 @@ public class GossipIntentStore | ... | @@ -189,7 +281,7 @@ public class GossipIntentStore |
| 189 | log.debug("updating timestamp"); | 281 | log.debug("updating timestamp"); |
| 190 | data.setVersion(new SystemClockTimestamp()); | 282 | data.setVersion(new SystemClockTimestamp()); |
| 191 | } | 283 | } |
| 192 | - pending.put(data.key(), data); | 284 | + pending.put(data.key(), copyData(data)); |
| 193 | } | 285 | } |
| 194 | 286 | ||
| 195 | @Override | 287 | @Override |
| ... | @@ -234,7 +326,7 @@ public class GossipIntentStore | ... | @@ -234,7 +326,7 @@ public class GossipIntentStore |
| 234 | // some work. | 326 | // some work. |
| 235 | if (isMaster(event.value().intent())) { | 327 | if (isMaster(event.value().intent())) { |
| 236 | if (delegate != null) { | 328 | if (delegate != null) { |
| 237 | - delegate.process(event.value()); | 329 | + delegate.process(copyData(event.value())); |
| 238 | } | 330 | } |
| 239 | } | 331 | } |
| 240 | 332 | ... | ... |
| ... | @@ -43,7 +43,7 @@ import static org.slf4j.LoggerFactory.getLogger; | ... | @@ -43,7 +43,7 @@ import static org.slf4j.LoggerFactory.getLogger; |
| 43 | 43 | ||
| 44 | //TODO Note: this store will be removed once the GossipIntentStore is stable | 44 | //TODO Note: this store will be removed once the GossipIntentStore is stable |
| 45 | 45 | ||
| 46 | -@Component(immediate = true) | 46 | +@Component(immediate = true, enabled = false) |
| 47 | @Service | 47 | @Service |
| 48 | //FIXME remove this | 48 | //FIXME remove this |
| 49 | public class SimpleIntentStore | 49 | public class SimpleIntentStore | ... | ... |
-
Please register or login to post a comment