Fixing an issue where intents fail to re-route after a node is restarted; caused…
… by failure to register intent resources correctly. Change-Id: I239e3b538d5b9134422fa629514e095e4914bb0c
Showing
10 changed files
with
112 additions
and
23 deletions
| ... | @@ -66,6 +66,13 @@ public interface IntentService { | ... | @@ -66,6 +66,13 @@ public interface IntentService { |
| 66 | Iterable<Intent> getIntents(); | 66 | Iterable<Intent> getIntents(); |
| 67 | 67 | ||
| 68 | /** | 68 | /** |
| 69 | + * Returns an iterable of intent data objects currently in the system. | ||
| 70 | + * | ||
| 71 | + * @return set of intent data objects | ||
| 72 | + */ | ||
| 73 | + Iterable<IntentData> getIntentData(); | ||
| 74 | + | ||
| 75 | + /** | ||
| 69 | * Returns the number of intents currently in the system. | 76 | * Returns the number of intents currently in the system. |
| 70 | * | 77 | * |
| 71 | * @return number of intents | 78 | * @return number of intents | ... | ... |
| ... | @@ -29,4 +29,12 @@ public interface IntentStoreDelegate extends StoreDelegate<IntentEvent> { | ... | @@ -29,4 +29,12 @@ public interface IntentStoreDelegate extends StoreDelegate<IntentEvent> { |
| 29 | * @param intentData intent data object | 29 | * @param intentData intent data object |
| 30 | */ | 30 | */ |
| 31 | void process(IntentData intentData); | 31 | void process(IntentData intentData); |
| 32 | + | ||
| 33 | + /** | ||
| 34 | + * Called when a new intent has been updated for which this node is the master. | ||
| 35 | + * | ||
| 36 | + * @param intentData intent data object | ||
| 37 | + */ | ||
| 38 | + default void onUpdate(IntentData intentData) { | ||
| 39 | + } | ||
| 32 | } | 40 | } | ... | ... |
| ... | @@ -183,6 +183,11 @@ public class FakeIntentManager implements TestableIntentService { | ... | @@ -183,6 +183,11 @@ public class FakeIntentManager implements TestableIntentService { |
| 183 | } | 183 | } |
| 184 | 184 | ||
| 185 | @Override | 185 | @Override |
| 186 | + public Iterable<IntentData> getIntentData() { | ||
| 187 | + throw new UnsupportedOperationException(); | ||
| 188 | + } | ||
| 189 | + | ||
| 190 | + @Override | ||
| 186 | public long getIntentCount() { | 191 | public long getIntentCount() { |
| 187 | return intents.size(); | 192 | return intents.size(); |
| 188 | } | 193 | } | ... | ... |
| ... | @@ -43,6 +43,11 @@ public class IntentServiceAdapter implements IntentService { | ... | @@ -43,6 +43,11 @@ public class IntentServiceAdapter implements IntentService { |
| 43 | } | 43 | } |
| 44 | 44 | ||
| 45 | @Override | 45 | @Override |
| 46 | + public Iterable<IntentData> getIntentData() { | ||
| 47 | + return null; | ||
| 48 | + } | ||
| 49 | + | ||
| 50 | + @Override | ||
| 46 | public long getIntentCount() { | 51 | public long getIntentCount() { |
| 47 | return 0; | 52 | return 0; |
| 48 | } | 53 | } | ... | ... |
| ... | @@ -182,6 +182,12 @@ public class IntentManager | ... | @@ -182,6 +182,12 @@ public class IntentManager |
| 182 | } | 182 | } |
| 183 | 183 | ||
| 184 | @Override | 184 | @Override |
| 185 | + public Iterable<IntentData> getIntentData() { | ||
| 186 | + checkPermission(Permission.INTENT_READ); | ||
| 187 | + return store.getIntentData(false, 0); | ||
| 188 | + } | ||
| 189 | + | ||
| 190 | + @Override | ||
| 185 | public long getIntentCount() { | 191 | public long getIntentCount() { |
| 186 | checkPermission(Permission.INTENT_READ); | 192 | checkPermission(Permission.INTENT_READ); |
| 187 | 193 | ||
| ... | @@ -258,6 +264,11 @@ public class IntentManager | ... | @@ -258,6 +264,11 @@ public class IntentManager |
| 258 | public void process(IntentData data) { | 264 | public void process(IntentData data) { |
| 259 | accumulator.add(data); | 265 | accumulator.add(data); |
| 260 | } | 266 | } |
| 267 | + | ||
| 268 | + @Override | ||
| 269 | + public void onUpdate(IntentData intentData) { | ||
| 270 | + trackerService.trackIntent(intentData); | ||
| 271 | + } | ||
| 261 | } | 272 | } |
| 262 | 273 | ||
| 263 | private void buildAndSubmitBatches(Iterable<Key> intentKeys, | 274 | private void buildAndSubmitBatches(Iterable<Key> intentKeys, | ... | ... |
| ... | @@ -17,6 +17,7 @@ package org.onosproject.net.intent.impl; | ... | @@ -17,6 +17,7 @@ package org.onosproject.net.intent.impl; |
| 17 | 17 | ||
| 18 | import com.google.common.collect.HashMultimap; | 18 | import com.google.common.collect.HashMultimap; |
| 19 | import com.google.common.collect.Lists; | 19 | import com.google.common.collect.Lists; |
| 20 | +import com.google.common.collect.Maps; | ||
| 20 | import com.google.common.collect.SetMultimap; | 21 | import com.google.common.collect.SetMultimap; |
| 21 | import org.apache.felix.scr.annotations.Activate; | 22 | import org.apache.felix.scr.annotations.Activate; |
| 22 | import org.apache.felix.scr.annotations.Component; | 23 | import org.apache.felix.scr.annotations.Component; |
| ... | @@ -40,6 +41,7 @@ import org.onosproject.net.host.HostEvent; | ... | @@ -40,6 +41,7 @@ import org.onosproject.net.host.HostEvent; |
| 40 | import org.onosproject.net.host.HostListener; | 41 | import org.onosproject.net.host.HostListener; |
| 41 | import org.onosproject.net.host.HostService; | 42 | import org.onosproject.net.host.HostService; |
| 42 | import org.onosproject.net.intent.Intent; | 43 | import org.onosproject.net.intent.Intent; |
| 44 | +import org.onosproject.net.intent.IntentData; | ||
| 43 | import org.onosproject.net.intent.IntentService; | 45 | import org.onosproject.net.intent.IntentService; |
| 44 | import org.onosproject.net.intent.Key; | 46 | import org.onosproject.net.intent.Key; |
| 45 | import org.onosproject.net.intent.PartitionEvent; | 47 | import org.onosproject.net.intent.PartitionEvent; |
| ... | @@ -57,7 +59,9 @@ import org.slf4j.Logger; | ... | @@ -57,7 +59,9 @@ import org.slf4j.Logger; |
| 57 | import java.util.Collection; | 59 | import java.util.Collection; |
| 58 | import java.util.Collections; | 60 | import java.util.Collections; |
| 59 | import java.util.HashSet; | 61 | import java.util.HashSet; |
| 62 | +import java.util.List; | ||
| 60 | import java.util.Set; | 63 | import java.util.Set; |
| 64 | +import java.util.concurrent.ConcurrentMap; | ||
| 61 | import java.util.concurrent.ExecutorService; | 65 | import java.util.concurrent.ExecutorService; |
| 62 | import java.util.concurrent.Executors; | 66 | import java.util.concurrent.Executors; |
| 63 | import java.util.concurrent.ScheduledExecutorService; | 67 | import java.util.concurrent.ScheduledExecutorService; |
| ... | @@ -69,7 +73,9 @@ import static com.google.common.base.Preconditions.checkNotNull; | ... | @@ -69,7 +73,9 @@ import static com.google.common.base.Preconditions.checkNotNull; |
| 69 | import static com.google.common.collect.Multimaps.synchronizedSetMultimap; | 73 | import static com.google.common.collect.Multimaps.synchronizedSetMultimap; |
| 70 | import static java.util.concurrent.Executors.newSingleThreadExecutor; | 74 | import static java.util.concurrent.Executors.newSingleThreadExecutor; |
| 71 | import static org.onlab.util.Tools.groupedThreads; | 75 | import static org.onlab.util.Tools.groupedThreads; |
| 76 | +import static org.onlab.util.Tools.isNullOrEmpty; | ||
| 72 | import static org.onosproject.net.LinkKey.linkKey; | 77 | import static org.onosproject.net.LinkKey.linkKey; |
| 78 | +import static org.onosproject.net.intent.IntentState.INSTALLED; | ||
| 73 | import static org.onosproject.net.link.LinkEvent.Type.LINK_REMOVED; | 79 | import static org.onosproject.net.link.LinkEvent.Type.LINK_REMOVED; |
| 74 | import static org.onosproject.net.link.LinkEvent.Type.LINK_UPDATED; | 80 | import static org.onosproject.net.link.LinkEvent.Type.LINK_UPDATED; |
| 75 | import static org.slf4j.LoggerFactory.getLogger; | 81 | import static org.slf4j.LoggerFactory.getLogger; |
| ... | @@ -84,6 +90,8 @@ public class ObjectiveTracker implements ObjectiveTrackerService { | ... | @@ -84,6 +90,8 @@ public class ObjectiveTracker implements ObjectiveTrackerService { |
| 84 | 90 | ||
| 85 | private final Logger log = getLogger(getClass()); | 91 | private final Logger log = getLogger(getClass()); |
| 86 | 92 | ||
| 93 | + private final ConcurrentMap<Key, Intent> intents = Maps.newConcurrentMap(); | ||
| 94 | + | ||
| 87 | private final SetMultimap<LinkKey, Key> intentsByLink = | 95 | private final SetMultimap<LinkKey, Key> intentsByLink = |
| 88 | //TODO this could be slow as a point of synchronization | 96 | //TODO this could be slow as a point of synchronization |
| 89 | synchronizedSetMultimap(HashMultimap.<LinkKey, Key>create()); | 97 | synchronizedSetMultimap(HashMultimap.<LinkKey, Key>create()); |
| ... | @@ -195,6 +203,46 @@ public class ObjectiveTracker implements ObjectiveTrackerService { | ... | @@ -195,6 +203,46 @@ public class ObjectiveTracker implements ObjectiveTrackerService { |
| 195 | } | 203 | } |
| 196 | } | 204 | } |
| 197 | 205 | ||
| 206 | + @Override | ||
| 207 | + public void trackIntent(IntentData intentData) { | ||
| 208 | + | ||
| 209 | + //NOTE: This will be called for intents that are being added to the store | ||
| 210 | + // locally (i.e. every intent update) | ||
| 211 | + | ||
| 212 | + Key key = intentData.key(); | ||
| 213 | + Intent intent = intentData.intent(); | ||
| 214 | + boolean isLocal = intentService.isLocal(key); | ||
| 215 | + List<Intent> installables = intentData.installables(); | ||
| 216 | + | ||
| 217 | + if (log.isTraceEnabled()) { | ||
| 218 | + log.trace("intent {}, old: {}, new: {}, installableCount: {}, resourceCount: {}", | ||
| 219 | + key, | ||
| 220 | + intentsByDevice.values().contains(key), | ||
| 221 | + isLocal, | ||
| 222 | + installables.size(), | ||
| 223 | + intent.resources().size() + | ||
| 224 | + installables.stream() | ||
| 225 | + .mapToLong(i -> i.resources().size()).sum()); | ||
| 226 | + } | ||
| 227 | + | ||
| 228 | + if (isNullOrEmpty(installables) && intentData.state() == INSTALLED) { | ||
| 229 | + log.warn("Intent {} is INSTALLED with no installables", key); | ||
| 230 | + } | ||
| 231 | + | ||
| 232 | + if (isLocal) { | ||
| 233 | + addTrackedResources(key, intent.resources()); | ||
| 234 | + for (Intent installable : installables) { | ||
| 235 | + addTrackedResources(key, installable.resources()); | ||
| 236 | + } | ||
| 237 | + // FIXME check all resources against current topo service(s); recompile if necessary | ||
| 238 | + } else { | ||
| 239 | + removeTrackedResources(key, intent.resources()); | ||
| 240 | + for (Intent installable : installables) { | ||
| 241 | + removeTrackedResources(key, installable.resources()); | ||
| 242 | + } | ||
| 243 | + } | ||
| 244 | + } | ||
| 245 | + | ||
| 198 | // Internal re-actor to topology change events. | 246 | // Internal re-actor to topology change events. |
| 199 | private class InternalTopologyListener implements TopologyListener { | 247 | private class InternalTopologyListener implements TopologyListener { |
| 200 | @Override | 248 | @Override |
| ... | @@ -371,25 +419,11 @@ public class ObjectiveTracker implements ObjectiveTrackerService { | ... | @@ -371,25 +419,11 @@ public class ObjectiveTracker implements ObjectiveTrackerService { |
| 371 | } | 419 | } |
| 372 | try { | 420 | try { |
| 373 | //FIXME very inefficient | 421 | //FIXME very inefficient |
| 374 | - for (Intent intent : intentService.getIntents()) { | 422 | + for (IntentData intentData : intentService.getIntentData()) { |
| 375 | try { | 423 | try { |
| 376 | - if (intentService.isLocal(intent.key())) { | 424 | + trackIntent(intentData); |
| 377 | - log.trace("intent {}, old: {}, new: {}", | ||
| 378 | - intent.key(), intentsByDevice.values().contains(intent.key()), true); | ||
| 379 | - addTrackedResources(intent.key(), intent.resources()); | ||
| 380 | - intentService.getInstallableIntents(intent.key()).stream() | ||
| 381 | - .forEach(installable -> | ||
| 382 | - addTrackedResources(intent.key(), installable.resources())); | ||
| 383 | - } else { | ||
| 384 | - log.trace("intent {}, old: {}, new: {}", | ||
| 385 | - intent.key(), intentsByDevice.values().contains(intent.key()), false); | ||
| 386 | - removeTrackedResources(intent.key(), intent.resources()); | ||
| 387 | - intentService.getInstallableIntents(intent.key()).stream() | ||
| 388 | - .forEach(installable -> | ||
| 389 | - removeTrackedResources(intent.key(), installable.resources())); | ||
| 390 | - } | ||
| 391 | } catch (NullPointerException npe) { | 425 | } catch (NullPointerException npe) { |
| 392 | - log.warn("intent error {}", intent.key(), npe); | 426 | + log.warn("intent error {}", intentData.key(), npe); |
| 393 | } | 427 | } |
| 394 | } | 428 | } |
| 395 | } catch (Exception e) { | 429 | } catch (Exception e) { | ... | ... |
| ... | @@ -15,11 +15,12 @@ | ... | @@ -15,11 +15,12 @@ |
| 15 | */ | 15 | */ |
| 16 | package org.onosproject.net.intent.impl; | 16 | package org.onosproject.net.intent.impl; |
| 17 | 17 | ||
| 18 | -import java.util.Collection; | ||
| 19 | - | ||
| 20 | import org.onosproject.net.NetworkResource; | 18 | import org.onosproject.net.NetworkResource; |
| 19 | +import org.onosproject.net.intent.IntentData; | ||
| 21 | import org.onosproject.net.intent.Key; | 20 | import org.onosproject.net.intent.Key; |
| 22 | 21 | ||
| 22 | +import java.util.Collection; | ||
| 23 | + | ||
| 23 | /** | 24 | /** |
| 24 | * Auxiliary service for tracking intent path flows and for notifying the | 25 | * Auxiliary service for tracking intent path flows and for notifying the |
| 25 | * intent service of environment changes via topology change delegate. | 26 | * intent service of environment changes via topology change delegate. |
| ... | @@ -59,4 +60,10 @@ public interface ObjectiveTrackerService { | ... | @@ -59,4 +60,10 @@ public interface ObjectiveTrackerService { |
| 59 | void removeTrackedResources(Key intentKey, | 60 | void removeTrackedResources(Key intentKey, |
| 60 | Collection<NetworkResource> resources); | 61 | Collection<NetworkResource> resources); |
| 61 | 62 | ||
| 63 | + /** | ||
| 64 | + * Submits the specified intent data to be tracked. | ||
| 65 | + * | ||
| 66 | + * @param intentData intent data object to be tracked | ||
| 67 | + */ | ||
| 68 | + void trackIntent(IntentData intentData); | ||
| 62 | } | 69 | } | ... | ... |
| ... | @@ -28,13 +28,14 @@ import org.junit.Ignore; | ... | @@ -28,13 +28,14 @@ import org.junit.Ignore; |
| 28 | import org.junit.Test; | 28 | import org.junit.Test; |
| 29 | import org.onosproject.TestApplicationId; | 29 | import org.onosproject.TestApplicationId; |
| 30 | import org.onosproject.cfg.ComponentConfigAdapter; | 30 | import org.onosproject.cfg.ComponentConfigAdapter; |
| 31 | +import org.onosproject.common.event.impl.TestEventDispatcher; | ||
| 31 | import org.onosproject.core.ApplicationId; | 32 | import org.onosproject.core.ApplicationId; |
| 32 | import org.onosproject.core.impl.TestCoreManager; | 33 | import org.onosproject.core.impl.TestCoreManager; |
| 33 | -import org.onosproject.common.event.impl.TestEventDispatcher; | ||
| 34 | import org.onosproject.net.NetworkResource; | 34 | import org.onosproject.net.NetworkResource; |
| 35 | import org.onosproject.net.intent.FlowRuleIntent; | 35 | import org.onosproject.net.intent.FlowRuleIntent; |
| 36 | import org.onosproject.net.intent.Intent; | 36 | import org.onosproject.net.intent.Intent; |
| 37 | import org.onosproject.net.intent.IntentCompiler; | 37 | import org.onosproject.net.intent.IntentCompiler; |
| 38 | +import org.onosproject.net.intent.IntentData; | ||
| 38 | import org.onosproject.net.intent.IntentEvent; | 39 | import org.onosproject.net.intent.IntentEvent; |
| 39 | import org.onosproject.net.intent.IntentEvent.Type; | 40 | import org.onosproject.net.intent.IntentEvent.Type; |
| 40 | import org.onosproject.net.intent.IntentExtensionService; | 41 | import org.onosproject.net.intent.IntentExtensionService; |
| ... | @@ -145,6 +146,11 @@ public class IntentManagerTest { | ... | @@ -145,6 +146,11 @@ public class IntentManagerTest { |
| 145 | public void removeTrackedResources(Key key, Collection<NetworkResource> resources) { | 146 | public void removeTrackedResources(Key key, Collection<NetworkResource> resources) { |
| 146 | //TODO | 147 | //TODO |
| 147 | } | 148 | } |
| 149 | + | ||
| 150 | + @Override | ||
| 151 | + public void trackIntent(IntentData intentData) { | ||
| 152 | + //TODO | ||
| 153 | + } | ||
| 148 | } | 154 | } |
| 149 | 155 | ||
| 150 | private static class MockInstallableIntent extends FlowRuleIntent { | 156 | private static class MockInstallableIntent extends FlowRuleIntent { | ... | ... |
| ... | @@ -288,11 +288,16 @@ public class GossipIntentStore | ... | @@ -288,11 +288,16 @@ public class GossipIntentStore |
| 288 | private final class InternalCurrentListener implements | 288 | private final class InternalCurrentListener implements |
| 289 | EventuallyConsistentMapListener<Key, IntentData> { | 289 | EventuallyConsistentMapListener<Key, IntentData> { |
| 290 | @Override | 290 | @Override |
| 291 | - public void event( | 291 | + public void event(EventuallyConsistentMapEvent<Key, IntentData> event) { |
| 292 | - EventuallyConsistentMapEvent<Key, IntentData> event) { | ||
| 293 | - if (event.type() == EventuallyConsistentMapEvent.Type.PUT) { | ||
| 294 | IntentData intentData = event.value(); | 292 | IntentData intentData = event.value(); |
| 295 | 293 | ||
| 294 | + if (event.type() == EventuallyConsistentMapEvent.Type.PUT) { | ||
| 295 | + // The current intents map has been updated. If we are master for | ||
| 296 | + // this intent's partition, notify the Manager that it should | ||
| 297 | + // emit notifications about updated tracked resources. | ||
| 298 | + if (delegate != null && isMaster(event.value().intent().key())) { | ||
| 299 | + delegate.onUpdate(new IntentData(intentData)); // copy for safety, likely unnecessary | ||
| 300 | + } | ||
| 296 | notifyDelegateIfNotNull(IntentEvent.getEvent(intentData)); | 301 | notifyDelegateIfNotNull(IntentEvent.getEvent(intentData)); |
| 297 | } | 302 | } |
| 298 | } | 303 | } | ... | ... |
-
Please register or login to post a comment