Committed by
Gerrit Code Review
Intents are now removed after being withdrawn.
Change-Id: I7574fe94add00abf58c71c6122bb3dc5aafa0f79
Showing
7 changed files
with
45 additions
and
45 deletions
| ... | @@ -15,6 +15,7 @@ | ... | @@ -15,6 +15,7 @@ |
| 15 | */ | 15 | */ |
| 16 | package org.onlab.onos.cli.net; | 16 | package org.onlab.onos.cli.net; |
| 17 | 17 | ||
| 18 | +import com.google.common.collect.Lists; | ||
| 18 | import org.apache.karaf.shell.commands.Argument; | 19 | import org.apache.karaf.shell.commands.Argument; |
| 19 | import org.apache.karaf.shell.commands.Command; | 20 | import org.apache.karaf.shell.commands.Command; |
| 20 | import org.onlab.onos.cli.AbstractShellCommand; | 21 | import org.onlab.onos.cli.AbstractShellCommand; |
| ... | @@ -35,6 +36,7 @@ import org.onlab.onos.net.intent.PointToPointIntent; | ... | @@ -35,6 +36,7 @@ import org.onlab.onos.net.intent.PointToPointIntent; |
| 35 | import org.onlab.packet.Ethernet; | 36 | import org.onlab.packet.Ethernet; |
| 36 | import org.onlab.packet.MacAddress; | 37 | import org.onlab.packet.MacAddress; |
| 37 | 38 | ||
| 39 | +import java.util.List; | ||
| 38 | import java.util.concurrent.CountDownLatch; | 40 | import java.util.concurrent.CountDownLatch; |
| 39 | import java.util.concurrent.TimeUnit; | 41 | import java.util.concurrent.TimeUnit; |
| 40 | 42 | ||
| ... | @@ -89,39 +91,44 @@ public class IntentPushTestCommand extends AbstractShellCommand | ... | @@ -89,39 +91,44 @@ public class IntentPushTestCommand extends AbstractShellCommand |
| 89 | 91 | ||
| 90 | add = true; | 92 | add = true; |
| 91 | latch = new CountDownLatch(count); | 93 | latch = new CountDownLatch(count); |
| 92 | - IntentOperations operations = generateIntents(ingress, egress); | 94 | + List<Intent> operations = generateIntents(ingress, egress); |
| 93 | submitIntents(operations); | 95 | submitIntents(operations); |
| 94 | 96 | ||
| 95 | add = false; | 97 | add = false; |
| 96 | latch = new CountDownLatch(count); | 98 | latch = new CountDownLatch(count); |
| 97 | - operations = generateIntents(ingress, egress); | ||
| 98 | submitIntents(operations); | 99 | submitIntents(operations); |
| 99 | 100 | ||
| 100 | service.removeListener(this); | 101 | service.removeListener(this); |
| 101 | } | 102 | } |
| 102 | 103 | ||
| 103 | - private IntentOperations generateIntents(ConnectPoint ingress, ConnectPoint egress) { | 104 | + private List<Intent> generateIntents(ConnectPoint ingress, ConnectPoint egress) { |
| 104 | TrafficSelector.Builder selector = DefaultTrafficSelector.builder() | 105 | TrafficSelector.Builder selector = DefaultTrafficSelector.builder() |
| 105 | .matchEthType(Ethernet.TYPE_IPV4); | 106 | .matchEthType(Ethernet.TYPE_IPV4); |
| 106 | TrafficTreatment treatment = DefaultTrafficTreatment.builder().build(); | 107 | TrafficTreatment treatment = DefaultTrafficTreatment.builder().build(); |
| 107 | 108 | ||
| 108 | - IntentOperations.Builder ops = IntentOperations.builder(); | 109 | + List<Intent> intents = Lists.newArrayList(); |
| 109 | for (int i = 1; i <= count; i++) { | 110 | for (int i = 1; i <= count; i++) { |
| 110 | TrafficSelector s = selector | 111 | TrafficSelector s = selector |
| 111 | .matchEthSrc(MacAddress.valueOf(i)) | 112 | .matchEthSrc(MacAddress.valueOf(i)) |
| 112 | .build(); | 113 | .build(); |
| 113 | - Intent intent = new PointToPointIntent(appId(), s, treatment, | 114 | + intents.add(new PointToPointIntent(appId(), s, treatment, |
| 114 | - ingress, egress); | 115 | + ingress, egress)); |
| 116 | + | ||
| 117 | + } | ||
| 118 | + return intents; | ||
| 119 | + } | ||
| 120 | + | ||
| 121 | + private void submitIntents(List<Intent> intents) { | ||
| 122 | + IntentOperations.Builder builder = IntentOperations.builder(); | ||
| 123 | + for (Intent intent : intents) { | ||
| 115 | if (add) { | 124 | if (add) { |
| 116 | - ops.addSubmitOperation(intent); | 125 | + builder.addSubmitOperation(intent); |
| 117 | } else { | 126 | } else { |
| 118 | - ops.addWithdrawOperation(intent.id()); | 127 | + builder.addWithdrawOperation(intent.id()); |
| 119 | - } | ||
| 120 | } | 128 | } |
| 121 | - return ops.build(); | ||
| 122 | } | 129 | } |
| 130 | + IntentOperations ops = builder.build(); | ||
| 123 | 131 | ||
| 124 | - private void submitIntents(IntentOperations ops) { | ||
| 125 | start = System.currentTimeMillis(); | 132 | start = System.currentTimeMillis(); |
| 126 | service.execute(ops); | 133 | service.execute(ops); |
| 127 | try { | 134 | try { | ... | ... |
| ... | @@ -42,9 +42,8 @@ public interface IntentStore extends Store<IntentEvent, IntentStoreDelegate> { | ... | @@ -42,9 +42,8 @@ public interface IntentStore extends Store<IntentEvent, IntentStoreDelegate> { |
| 42 | * Removes the specified intent from the inventory. | 42 | * Removes the specified intent from the inventory. |
| 43 | * | 43 | * |
| 44 | * @param intentId intent identification | 44 | * @param intentId intent identification |
| 45 | - * @return removed state transition event or null if intent was not found | ||
| 46 | */ | 45 | */ |
| 47 | - IntentEvent removeIntent(IntentId intentId); | 46 | + void removeIntent(IntentId intentId); |
| 48 | 47 | ||
| 49 | /** | 48 | /** |
| 50 | * Returns the number of intents in the store. | 49 | * Returns the number of intents in the store. |
| ... | @@ -103,8 +102,6 @@ public interface IntentStore extends Store<IntentEvent, IntentStoreDelegate> { | ... | @@ -103,8 +102,6 @@ public interface IntentStore extends Store<IntentEvent, IntentStoreDelegate> { |
| 103 | */ | 102 | */ |
| 104 | List<Intent> getInstallableIntents(IntentId intentId); | 103 | List<Intent> getInstallableIntents(IntentId intentId); |
| 105 | 104 | ||
| 106 | - // TODO: this should be triggered from with the store as a result of removeIntent call | ||
| 107 | - | ||
| 108 | /** | 105 | /** |
| 109 | * Removes any installable intents which resulted from compilation of the | 106 | * Removes any installable intents which resulted from compilation of the |
| 110 | * specified original intent. | 107 | * specified original intent. | ... | ... |
| ... | @@ -694,6 +694,10 @@ public class IntentManager | ... | @@ -694,6 +694,10 @@ public class IntentManager |
| 694 | if (event != null) { | 694 | if (event != null) { |
| 695 | eventDispatcher.post(event); | 695 | eventDispatcher.post(event); |
| 696 | } | 696 | } |
| 697 | + | ||
| 698 | + if (newState == WITHDRAWN) { | ||
| 699 | + store.removeIntent(intent.id()); | ||
| 700 | + } | ||
| 697 | } | 701 | } |
| 698 | 702 | ||
| 699 | Map<Intent, IntentState> stateMap() { | 703 | Map<Intent, IntentState> stateMap() { | ... | ... |
| ... | @@ -155,7 +155,8 @@ public class IntentManagerTest { | ... | @@ -155,7 +155,8 @@ public class IntentManagerTest { |
| 155 | listener.setLatch(1, Type.WITHDRAWN); | 155 | listener.setLatch(1, Type.WITHDRAWN); |
| 156 | service.withdraw(intent); | 156 | service.withdraw(intent); |
| 157 | listener.await(Type.WITHDRAWN); | 157 | listener.await(Type.WITHDRAWN); |
| 158 | - assertEquals(1L, service.getIntentCount()); | 158 | + delay(10); //FIXME this is a race |
| 159 | + assertEquals(0L, service.getIntentCount()); | ||
| 159 | assertEquals(0L, flowRuleService.getFlowRuleCount()); | 160 | assertEquals(0L, flowRuleService.getFlowRuleCount()); |
| 160 | } | 161 | } |
| 161 | 162 | ||
| ... | @@ -176,7 +177,8 @@ public class IntentManagerTest { | ... | @@ -176,7 +177,8 @@ public class IntentManagerTest { |
| 176 | 177 | ||
| 177 | listener.await(Type.INSTALLED); | 178 | listener.await(Type.INSTALLED); |
| 178 | listener.await(Type.WITHDRAWN); | 179 | listener.await(Type.WITHDRAWN); |
| 179 | - assertEquals(1L, service.getIntentCount()); | 180 | + delay(10); //FIXME this is a race |
| 181 | + assertEquals(0L, service.getIntentCount()); | ||
| 180 | assertEquals(0L, flowRuleService.getFlowRuleCount()); | 182 | assertEquals(0L, flowRuleService.getFlowRuleCount()); |
| 181 | } | 183 | } |
| 182 | 184 | ||
| ... | @@ -198,7 +200,8 @@ public class IntentManagerTest { | ... | @@ -198,7 +200,8 @@ public class IntentManagerTest { |
| 198 | service.replace(intent.id(), intent2); | 200 | service.replace(intent.id(), intent2); |
| 199 | listener.await(Type.WITHDRAWN); | 201 | listener.await(Type.WITHDRAWN); |
| 200 | listener.await(Type.INSTALLED); | 202 | listener.await(Type.INSTALLED); |
| 201 | - assertEquals(2L, service.getIntentCount()); | 203 | + delay(10); //FIXME this is a race |
| 204 | + assertEquals(1L, service.getIntentCount()); | ||
| 202 | assertEquals(1L, manager.flowRuleService.getFlowRuleCount()); | 205 | assertEquals(1L, manager.flowRuleService.getFlowRuleCount()); |
| 203 | assertEquals(intent2.number().intValue(), | 206 | assertEquals(intent2.number().intValue(), |
| 204 | flowRuleService.flows.iterator().next().priority()); | 207 | flowRuleService.flows.iterator().next().priority()); | ... | ... |
| ... | @@ -50,6 +50,7 @@ import java.util.Map; | ... | @@ -50,6 +50,7 @@ import java.util.Map; |
| 50 | import java.util.Set; | 50 | import java.util.Set; |
| 51 | import java.util.concurrent.ConcurrentHashMap; | 51 | import java.util.concurrent.ConcurrentHashMap; |
| 52 | 52 | ||
| 53 | +import static com.google.common.base.Preconditions.checkState; | ||
| 53 | import static org.onlab.onos.net.intent.IntentState.*; | 54 | import static org.onlab.onos.net.intent.IntentState.*; |
| 54 | import static org.slf4j.LoggerFactory.getLogger; | 55 | import static org.slf4j.LoggerFactory.getLogger; |
| 55 | import static org.onlab.metrics.MetricsUtil.*; | 56 | import static org.onlab.metrics.MetricsUtil.*; |
| ... | @@ -174,20 +175,15 @@ public class DistributedIntentStore | ... | @@ -174,20 +175,15 @@ public class DistributedIntentStore |
| 174 | } | 175 | } |
| 175 | 176 | ||
| 176 | @Override | 177 | @Override |
| 177 | - public IntentEvent removeIntent(IntentId intentId) { | 178 | + public void removeIntent(IntentId intentId) { |
| 178 | Context timer = startTimer(removeIntentTimer); | 179 | Context timer = startTimer(removeIntentTimer); |
| 180 | + checkState(getIntentState(intentId) == WITHDRAWN, | ||
| 181 | + "Intent state for {} is not WITHDRAWN.", intentId); | ||
| 179 | try { | 182 | try { |
| 180 | - Intent intent = intents.remove(intentId); | 183 | + intents.remove(intentId); |
| 181 | - installable.remove(intentId); | ||
| 182 | - if (intent == null) { | ||
| 183 | - // was already removed | ||
| 184 | - return null; | ||
| 185 | - } | ||
| 186 | - IntentEvent event = this.setState(intent, WITHDRAWN); | ||
| 187 | states.remove(intentId); | 184 | states.remove(intentId); |
| 188 | transientStates.remove(intentId); | 185 | transientStates.remove(intentId); |
| 189 | - // TODO: Should we callremoveInstalledIntents if this Intent was | 186 | + installable.remove(intentId); |
| 190 | - return event; | ||
| 191 | } finally { | 187 | } finally { |
| 192 | stopTimer(timer); | 188 | stopTimer(timer); |
| 193 | } | 189 | } | ... | ... |
| ... | @@ -52,6 +52,7 @@ import java.util.Map; | ... | @@ -52,6 +52,7 @@ import java.util.Map; |
| 52 | import java.util.Set; | 52 | import java.util.Set; |
| 53 | import java.util.concurrent.ConcurrentHashMap; | 53 | import java.util.concurrent.ConcurrentHashMap; |
| 54 | 54 | ||
| 55 | +import static com.google.common.base.Preconditions.checkState; | ||
| 55 | import static org.onlab.onos.net.intent.IntentState.*; | 56 | import static org.onlab.onos.net.intent.IntentState.*; |
| 56 | import static org.slf4j.LoggerFactory.getLogger; | 57 | import static org.slf4j.LoggerFactory.getLogger; |
| 57 | import static org.onlab.metrics.MetricsUtil.*; | 58 | import static org.onlab.metrics.MetricsUtil.*; |
| ... | @@ -177,20 +178,15 @@ public class HazelcastIntentStore | ... | @@ -177,20 +178,15 @@ public class HazelcastIntentStore |
| 177 | } | 178 | } |
| 178 | 179 | ||
| 179 | @Override | 180 | @Override |
| 180 | - public IntentEvent removeIntent(IntentId intentId) { | 181 | + public void removeIntent(IntentId intentId) { |
| 181 | Context timer = startTimer(removeIntentTimer); | 182 | Context timer = startTimer(removeIntentTimer); |
| 183 | + checkState(getIntentState(intentId) == WITHDRAWN, | ||
| 184 | + "Intent state for {} is not WITHDRAWN.", intentId); | ||
| 182 | try { | 185 | try { |
| 183 | - Intent intent = intents.remove(intentId); | 186 | + intents.remove(intentId); |
| 184 | installable.remove(intentId); | 187 | installable.remove(intentId); |
| 185 | - if (intent == null) { | ||
| 186 | - // was already removed | ||
| 187 | - return null; | ||
| 188 | - } | ||
| 189 | - IntentEvent event = this.setState(intent, WITHDRAWN); | ||
| 190 | states.remove(intentId); | 188 | states.remove(intentId); |
| 191 | transientStates.remove(intentId); | 189 | transientStates.remove(intentId); |
| 192 | - // TODO: Should we callremoveInstalledIntents if this Intent was | ||
| 193 | - return event; | ||
| 194 | } finally { | 190 | } finally { |
| 195 | stopTimer(timer); | 191 | stopTimer(timer); |
| 196 | } | 192 | } | ... | ... |
| ... | @@ -33,6 +33,7 @@ import java.util.List; | ... | @@ -33,6 +33,7 @@ import java.util.List; |
| 33 | import java.util.Map; | 33 | import java.util.Map; |
| 34 | import java.util.concurrent.ConcurrentHashMap; | 34 | import java.util.concurrent.ConcurrentHashMap; |
| 35 | 35 | ||
| 36 | +import static com.google.common.base.Preconditions.checkState; | ||
| 36 | import static org.onlab.onos.net.intent.IntentState.WITHDRAWN; | 37 | import static org.onlab.onos.net.intent.IntentState.WITHDRAWN; |
| 37 | import static org.slf4j.LoggerFactory.getLogger; | 38 | import static org.slf4j.LoggerFactory.getLogger; |
| 38 | 39 | ||
| ... | @@ -68,16 +69,12 @@ public class SimpleIntentStore | ... | @@ -68,16 +69,12 @@ public class SimpleIntentStore |
| 68 | } | 69 | } |
| 69 | 70 | ||
| 70 | @Override | 71 | @Override |
| 71 | - public IntentEvent removeIntent(IntentId intentId) { | 72 | + public void removeIntent(IntentId intentId) { |
| 72 | - Intent intent = intents.remove(intentId); | 73 | + checkState(getIntentState(intentId) == WITHDRAWN, |
| 74 | + "Intent state for {} is not WITHDRAWN.", intentId); | ||
| 75 | + intents.remove(intentId); | ||
| 73 | installable.remove(intentId); | 76 | installable.remove(intentId); |
| 74 | - if (intent == null) { | ||
| 75 | - // was already removed | ||
| 76 | - return null; | ||
| 77 | - } | ||
| 78 | - IntentEvent event = this.setState(intent, WITHDRAWN); | ||
| 79 | states.remove(intentId); | 77 | states.remove(intentId); |
| 80 | - return event; | ||
| 81 | } | 78 | } |
| 82 | 79 | ||
| 83 | @Override | 80 | @Override | ... | ... |
-
Please register or login to post a comment