Committed by
Gerrit Code Review
Fix SpringOpenTTP thread leak
- It was creating 2 theads everytime a Device connected. Now uses thread pool shared across Devices, where threads will die out on idle. Should resolve ONOS-3579 Change-Id: I490b2ef677853677fbd151af27f6ac2be563774c
Showing
2 changed files
with
39 additions
and
10 deletions
| ... | @@ -15,6 +15,7 @@ | ... | @@ -15,6 +15,7 @@ |
| 15 | */ | 15 | */ |
| 16 | package org.onosproject.driver.pipeline; | 16 | package org.onosproject.driver.pipeline; |
| 17 | 17 | ||
| 18 | +import static java.util.concurrent.Executors.newScheduledThreadPool; | ||
| 18 | import static org.onlab.util.Tools.groupedThreads; | 19 | import static org.onlab.util.Tools.groupedThreads; |
| 19 | import static org.slf4j.LoggerFactory.getLogger; | 20 | import static org.slf4j.LoggerFactory.getLogger; |
| 20 | 21 | ||
| ... | @@ -86,8 +87,8 @@ import java.util.Collections; | ... | @@ -86,8 +87,8 @@ import java.util.Collections; |
| 86 | import java.util.List; | 87 | import java.util.List; |
| 87 | import java.util.Objects; | 88 | import java.util.Objects; |
| 88 | import java.util.Set; | 89 | import java.util.Set; |
| 89 | -import java.util.concurrent.Executors; | ||
| 90 | import java.util.concurrent.ScheduledExecutorService; | 90 | import java.util.concurrent.ScheduledExecutorService; |
| 91 | +import java.util.concurrent.ScheduledThreadPoolExecutor; | ||
| 91 | import java.util.concurrent.TimeUnit; | 92 | import java.util.concurrent.TimeUnit; |
| 92 | import java.util.stream.Collectors; | 93 | import java.util.stream.Collectors; |
| 93 | 94 | ||
| ... | @@ -97,6 +98,11 @@ import java.util.stream.Collectors; | ... | @@ -97,6 +98,11 @@ import java.util.stream.Collectors; |
| 97 | public class SpringOpenTTP extends AbstractHandlerBehaviour | 98 | public class SpringOpenTTP extends AbstractHandlerBehaviour |
| 98 | implements Pipeliner { | 99 | implements Pipeliner { |
| 99 | 100 | ||
| 101 | + /** | ||
| 102 | + * GroupCheck delay. | ||
| 103 | + */ | ||
| 104 | + private static final int CHECK_DELAY = 500; | ||
| 105 | + | ||
| 100 | // Default table ID - compatible with CpqD switch | 106 | // Default table ID - compatible with CpqD switch |
| 101 | private static final int TABLE_VLAN = 0; | 107 | private static final int TABLE_VLAN = 0; |
| 102 | private static final int TABLE_TMAC = 1; | 108 | private static final int TABLE_TMAC = 1; |
| ... | @@ -118,7 +124,7 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour | ... | @@ -118,7 +124,7 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour |
| 118 | protected int aclTableId = TABLE_ACL; | 124 | protected int aclTableId = TABLE_ACL; |
| 119 | protected int srcMacTableId = TABLE_SMAC; | 125 | protected int srcMacTableId = TABLE_SMAC; |
| 120 | 126 | ||
| 121 | - protected final Logger log = getLogger(getClass()); | 127 | + private static final Logger log = getLogger(SpringOpenTTP.class); |
| 122 | 128 | ||
| 123 | private ServiceDirectory serviceDirectory; | 129 | private ServiceDirectory serviceDirectory; |
| 124 | private FlowRuleService flowRuleService; | 130 | private FlowRuleService flowRuleService; |
| ... | @@ -130,11 +136,19 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour | ... | @@ -130,11 +136,19 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour |
| 130 | 136 | ||
| 131 | private Cache<GroupKey, NextObjective> pendingGroups; | 137 | private Cache<GroupKey, NextObjective> pendingGroups; |
| 132 | 138 | ||
| 133 | - private ScheduledExecutorService groupChecker = Executors | 139 | + private static final ScheduledExecutorService GROUP_CHECKER |
| 134 | - .newScheduledThreadPool(2, | 140 | + = newScheduledThreadPool(2, |
| 135 | - groupedThreads("onos/pipeliner", | 141 | + groupedThreads("onos/pipeliner", |
| 136 | - "spring-open-%d", | 142 | + "spring-open-%d", log)); |
| 137 | - log)); | 143 | + static { |
| 144 | + // ONOS-3579 workaround, let core threads die out on idle | ||
| 145 | + if (GROUP_CHECKER instanceof ScheduledThreadPoolExecutor) { | ||
| 146 | + ScheduledThreadPoolExecutor executor = (ScheduledThreadPoolExecutor) GROUP_CHECKER; | ||
| 147 | + executor.setKeepAliveTime(CHECK_DELAY * 2, TimeUnit.MILLISECONDS); | ||
| 148 | + executor.allowCoreThreadTimeOut(true); | ||
| 149 | + } | ||
| 150 | + } | ||
| 151 | + | ||
| 138 | protected KryoNamespace appKryo = new KryoNamespace.Builder() | 152 | protected KryoNamespace appKryo = new KryoNamespace.Builder() |
| 139 | .register(KryoNamespaces.API) | 153 | .register(KryoNamespaces.API) |
| 140 | .register(GroupKey.class) | 154 | .register(GroupKey.class) |
| ... | @@ -158,9 +172,6 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour | ... | @@ -158,9 +172,6 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour |
| 158 | } | 172 | } |
| 159 | }).build(); | 173 | }).build(); |
| 160 | 174 | ||
| 161 | - groupChecker.scheduleAtFixedRate(new GroupChecker(), 0, 500, | ||
| 162 | - TimeUnit.MILLISECONDS); | ||
| 163 | - | ||
| 164 | coreService = serviceDirectory.get(CoreService.class); | 175 | coreService = serviceDirectory.get(CoreService.class); |
| 165 | flowRuleService = serviceDirectory.get(FlowRuleService.class); | 176 | flowRuleService = serviceDirectory.get(FlowRuleService.class); |
| 166 | groupService = serviceDirectory.get(GroupService.class); | 177 | groupService = serviceDirectory.get(GroupService.class); |
| ... | @@ -342,6 +353,7 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour | ... | @@ -342,6 +353,7 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour |
| 342 | + " in dev:{}", nextObjective.id(), deviceId); | 353 | + " in dev:{}", nextObjective.id(), deviceId); |
| 343 | pendingGroups.put(key, nextObjective); | 354 | pendingGroups.put(key, nextObjective); |
| 344 | groupService.addGroup(groupDescription); | 355 | groupService.addGroup(groupDescription); |
| 356 | + verifyPendingGroupLater(); | ||
| 345 | } | 357 | } |
| 346 | } | 358 | } |
| 347 | break; | 359 | break; |
| ... | @@ -367,6 +379,7 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour | ... | @@ -367,6 +379,7 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour |
| 367 | + "in device {}", nextObjective.id(), deviceId); | 379 | + "in device {}", nextObjective.id(), deviceId); |
| 368 | pendingGroups.put(key, nextObjective); | 380 | pendingGroups.put(key, nextObjective); |
| 369 | groupService.addGroup(groupDescription); | 381 | groupService.addGroup(groupDescription); |
| 382 | + verifyPendingGroupLater(); | ||
| 370 | } | 383 | } |
| 371 | break; | 384 | break; |
| 372 | case FAILOVER: | 385 | case FAILOVER: |
| ... | @@ -1130,6 +1143,10 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour | ... | @@ -1130,6 +1143,10 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour |
| 1130 | } | 1143 | } |
| 1131 | } | 1144 | } |
| 1132 | 1145 | ||
| 1146 | + private void verifyPendingGroupLater() { | ||
| 1147 | + GROUP_CHECKER.schedule(new GroupChecker(), CHECK_DELAY, TimeUnit.MILLISECONDS); | ||
| 1148 | + } | ||
| 1149 | + | ||
| 1133 | private class GroupChecker implements Runnable { | 1150 | private class GroupChecker implements Runnable { |
| 1134 | 1151 | ||
| 1135 | @Override | 1152 | @Override |
| ... | @@ -1158,6 +1175,13 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour | ... | @@ -1158,6 +1175,13 @@ public class SpringOpenTTP extends AbstractHandlerBehaviour |
| 1158 | obj.id(), | 1175 | obj.id(), |
| 1159 | new SpringOpenGroup(key, null)); | 1176 | new SpringOpenGroup(key, null)); |
| 1160 | }); | 1177 | }); |
| 1178 | + | ||
| 1179 | + if (!pendingGroups.asMap().isEmpty()) { | ||
| 1180 | + // Periodically execute only if entry remains in pendingGroups. | ||
| 1181 | + // Iterating pendingGroups trigger cleanUp and expiration, | ||
| 1182 | + // which will eventually empty the pendingGroups. | ||
| 1183 | + verifyPendingGroupLater(); | ||
| 1184 | + } | ||
| 1161 | } | 1185 | } |
| 1162 | } | 1186 | } |
| 1163 | 1187 | ... | ... |
| ... | @@ -15,6 +15,8 @@ | ... | @@ -15,6 +15,8 @@ |
| 15 | */ | 15 | */ |
| 16 | package org.onosproject.driver.pipeline; | 16 | package org.onosproject.driver.pipeline; |
| 17 | 17 | ||
| 18 | +import static org.slf4j.LoggerFactory.getLogger; | ||
| 19 | + | ||
| 18 | import java.util.Collection; | 20 | import java.util.Collection; |
| 19 | import java.util.Collections; | 21 | import java.util.Collections; |
| 20 | import java.util.List; | 22 | import java.util.List; |
| ... | @@ -42,6 +44,7 @@ import org.onosproject.net.flowobjective.FilteringObjective; | ... | @@ -42,6 +44,7 @@ import org.onosproject.net.flowobjective.FilteringObjective; |
| 42 | import org.onosproject.net.flowobjective.ForwardingObjective; | 44 | import org.onosproject.net.flowobjective.ForwardingObjective; |
| 43 | import org.onosproject.net.flowobjective.ObjectiveError; | 45 | import org.onosproject.net.flowobjective.ObjectiveError; |
| 44 | import org.onosproject.net.group.Group; | 46 | import org.onosproject.net.group.Group; |
| 47 | +import org.slf4j.Logger; | ||
| 45 | 48 | ||
| 46 | /** | 49 | /** |
| 47 | * Spring-open driver implementation for Dell hardware switches. | 50 | * Spring-open driver implementation for Dell hardware switches. |
| ... | @@ -55,6 +58,8 @@ public class SpringOpenTTPDell extends SpringOpenTTP { | ... | @@ -55,6 +58,8 @@ public class SpringOpenTTPDell extends SpringOpenTTP { |
| 55 | private static final int DELL_TABLE_MPLS = 25; | 58 | private static final int DELL_TABLE_MPLS = 25; |
| 56 | private static final int DELL_TABLE_ACL = 40; | 59 | private static final int DELL_TABLE_ACL = 40; |
| 57 | 60 | ||
| 61 | + private final Logger log = getLogger(getClass()); | ||
| 62 | + | ||
| 58 | //TODO: Store this info in the distributed store. | 63 | //TODO: Store this info in the distributed store. |
| 59 | private MacAddress deviceTMac = null; | 64 | private MacAddress deviceTMac = null; |
| 60 | 65 | ... | ... |
-
Please register or login to post a comment