Committed by
Gerrit Code Review
Improvement: Ensure configurations options are current and valid in NewDistributedFlowRuleStore
Bug fix: Only accept backups for devices that the local node does not manage. Change-Id: If7b1e8c3b0339e5d756e250c38fe53dc191084d1
Showing
1 changed file
with
48 additions
and
4 deletions
... | @@ -81,6 +81,7 @@ import java.util.concurrent.ConcurrentMap; | ... | @@ -81,6 +81,7 @@ import java.util.concurrent.ConcurrentMap; |
81 | import java.util.concurrent.ExecutorService; | 81 | import java.util.concurrent.ExecutorService; |
82 | import java.util.concurrent.Executors; | 82 | import java.util.concurrent.Executors; |
83 | import java.util.concurrent.ScheduledExecutorService; | 83 | import java.util.concurrent.ScheduledExecutorService; |
84 | +import java.util.concurrent.ScheduledFuture; | ||
84 | import java.util.concurrent.TimeUnit; | 85 | import java.util.concurrent.TimeUnit; |
85 | import java.util.concurrent.atomic.AtomicInteger; | 86 | import java.util.concurrent.atomic.AtomicInteger; |
86 | import java.util.stream.Collectors; | 87 | import java.util.stream.Collectors; |
... | @@ -106,6 +107,7 @@ public class NewDistributedFlowRuleStore | ... | @@ -106,6 +107,7 @@ public class NewDistributedFlowRuleStore |
106 | 107 | ||
107 | private static final int MESSAGE_HANDLER_THREAD_POOL_SIZE = 8; | 108 | private static final int MESSAGE_HANDLER_THREAD_POOL_SIZE = 8; |
108 | private static final boolean DEFAULT_BACKUP_ENABLED = true; | 109 | private static final boolean DEFAULT_BACKUP_ENABLED = true; |
110 | + private static final int DEFAULT_BACKUP_PERIOD_MILLIS = 2000; | ||
109 | private static final long FLOW_RULE_STORE_TIMEOUT_MILLIS = 5000; | 111 | private static final long FLOW_RULE_STORE_TIMEOUT_MILLIS = 5000; |
110 | 112 | ||
111 | @Property(name = "msgHandlerPoolSize", intValue = MESSAGE_HANDLER_THREAD_POOL_SIZE, | 113 | @Property(name = "msgHandlerPoolSize", intValue = MESSAGE_HANDLER_THREAD_POOL_SIZE, |
... | @@ -116,6 +118,10 @@ public class NewDistributedFlowRuleStore | ... | @@ -116,6 +118,10 @@ public class NewDistributedFlowRuleStore |
116 | label = "Indicates whether backups are enabled or not") | 118 | label = "Indicates whether backups are enabled or not") |
117 | private boolean backupEnabled = DEFAULT_BACKUP_ENABLED; | 119 | private boolean backupEnabled = DEFAULT_BACKUP_ENABLED; |
118 | 120 | ||
121 | + @Property(name = "backupPeriod", intValue = DEFAULT_BACKUP_PERIOD_MILLIS, | ||
122 | + label = "Delay in ms between successive backup runs") | ||
123 | + private int backupPeriod = DEFAULT_BACKUP_PERIOD_MILLIS; | ||
124 | + | ||
119 | private InternalFlowTable flowTable = new InternalFlowTable(); | 125 | private InternalFlowTable flowTable = new InternalFlowTable(); |
120 | 126 | ||
121 | @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) | 127 | @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) |
... | @@ -142,6 +148,7 @@ public class NewDistributedFlowRuleStore | ... | @@ -142,6 +148,7 @@ public class NewDistributedFlowRuleStore |
142 | private Map<Long, NodeId> pendingResponses = Maps.newConcurrentMap(); | 148 | private Map<Long, NodeId> pendingResponses = Maps.newConcurrentMap(); |
143 | private ExecutorService messageHandlingExecutor; | 149 | private ExecutorService messageHandlingExecutor; |
144 | 150 | ||
151 | + private ScheduledFuture<?> backupTask; | ||
145 | private final ScheduledExecutorService backupSenderExecutor = | 152 | private final ScheduledExecutorService backupSenderExecutor = |
146 | Executors.newSingleThreadScheduledExecutor(groupedThreads("onos/flow", "backup-sender")); | 153 | Executors.newSingleThreadScheduledExecutor(groupedThreads("onos/flow", "backup-sender")); |
147 | 154 | ||
... | @@ -173,7 +180,13 @@ public class NewDistributedFlowRuleStore | ... | @@ -173,7 +180,13 @@ public class NewDistributedFlowRuleStore |
173 | 180 | ||
174 | registerMessageHandlers(messageHandlingExecutor); | 181 | registerMessageHandlers(messageHandlingExecutor); |
175 | 182 | ||
176 | - backupSenderExecutor.scheduleWithFixedDelay(() -> flowTable.backup(), 0, 2000, TimeUnit.MILLISECONDS); | 183 | + if (backupEnabled) { |
184 | + backupTask = backupSenderExecutor.scheduleWithFixedDelay( | ||
185 | + flowTable::backup, | ||
186 | + 0, | ||
187 | + backupPeriod, | ||
188 | + TimeUnit.MILLISECONDS); | ||
189 | + } | ||
177 | 190 | ||
178 | logConfig("Started"); | 191 | logConfig("Started"); |
179 | } | 192 | } |
... | @@ -199,6 +212,7 @@ public class NewDistributedFlowRuleStore | ... | @@ -199,6 +212,7 @@ public class NewDistributedFlowRuleStore |
199 | Dictionary properties = context.getProperties(); | 212 | Dictionary properties = context.getProperties(); |
200 | int newPoolSize; | 213 | int newPoolSize; |
201 | boolean newBackupEnabled; | 214 | boolean newBackupEnabled; |
215 | + int newBackupPeriod; | ||
202 | try { | 216 | try { |
203 | String s = get(properties, "msgHandlerPoolSize"); | 217 | String s = get(properties, "msgHandlerPoolSize"); |
204 | newPoolSize = isNullOrEmpty(s) ? msgHandlerPoolSize : Integer.parseInt(s.trim()); | 218 | newPoolSize = isNullOrEmpty(s) ? msgHandlerPoolSize : Integer.parseInt(s.trim()); |
... | @@ -206,13 +220,38 @@ public class NewDistributedFlowRuleStore | ... | @@ -206,13 +220,38 @@ public class NewDistributedFlowRuleStore |
206 | s = get(properties, "backupEnabled"); | 220 | s = get(properties, "backupEnabled"); |
207 | newBackupEnabled = isNullOrEmpty(s) ? backupEnabled : Boolean.parseBoolean(s.trim()); | 221 | newBackupEnabled = isNullOrEmpty(s) ? backupEnabled : Boolean.parseBoolean(s.trim()); |
208 | 222 | ||
223 | + s = get(properties, "backupPeriod"); | ||
224 | + newBackupPeriod = isNullOrEmpty(s) ? backupPeriod : Integer.parseInt(s.trim()); | ||
225 | + | ||
209 | } catch (NumberFormatException | ClassCastException e) { | 226 | } catch (NumberFormatException | ClassCastException e) { |
210 | newPoolSize = MESSAGE_HANDLER_THREAD_POOL_SIZE; | 227 | newPoolSize = MESSAGE_HANDLER_THREAD_POOL_SIZE; |
211 | newBackupEnabled = DEFAULT_BACKUP_ENABLED; | 228 | newBackupEnabled = DEFAULT_BACKUP_ENABLED; |
229 | + newBackupPeriod = DEFAULT_BACKUP_PERIOD_MILLIS; | ||
212 | } | 230 | } |
213 | 231 | ||
232 | + boolean restartBackupTask = false; | ||
214 | if (newBackupEnabled != backupEnabled) { | 233 | if (newBackupEnabled != backupEnabled) { |
215 | backupEnabled = newBackupEnabled; | 234 | backupEnabled = newBackupEnabled; |
235 | + if (!backupEnabled && backupTask != null) { | ||
236 | + backupTask.cancel(false); | ||
237 | + backupTask = null; | ||
238 | + } | ||
239 | + restartBackupTask = backupEnabled; | ||
240 | + } | ||
241 | + if (newBackupPeriod != backupPeriod) { | ||
242 | + backupPeriod = newBackupPeriod; | ||
243 | + restartBackupTask = backupEnabled; | ||
244 | + } | ||
245 | + if (restartBackupTask) { | ||
246 | + if (backupTask != null) { | ||
247 | + // cancel previously running task | ||
248 | + backupTask.cancel(false); | ||
249 | + } | ||
250 | + backupTask = backupSenderExecutor.scheduleWithFixedDelay( | ||
251 | + flowTable::backup, | ||
252 | + 0, | ||
253 | + backupPeriod, | ||
254 | + TimeUnit.MILLISECONDS); | ||
216 | } | 255 | } |
217 | if (newPoolSize != msgHandlerPoolSize) { | 256 | if (newPoolSize != msgHandlerPoolSize) { |
218 | msgHandlerPoolSize = newPoolSize; | 257 | msgHandlerPoolSize = newPoolSize; |
... | @@ -254,8 +293,8 @@ public class NewDistributedFlowRuleStore | ... | @@ -254,8 +293,8 @@ public class NewDistributedFlowRuleStore |
254 | } | 293 | } |
255 | 294 | ||
256 | private void logConfig(String prefix) { | 295 | private void logConfig(String prefix) { |
257 | - log.info("{} with msgHandlerPoolSize = {}; backupEnabled = {}", | 296 | + log.info("{} with msgHandlerPoolSize = {}; backupEnabled = {}, backupPeriod = {}", |
258 | - prefix, msgHandlerPoolSize, backupEnabled); | 297 | + prefix, msgHandlerPoolSize, backupEnabled, backupPeriod); |
259 | } | 298 | } |
260 | 299 | ||
261 | // This is not a efficient operation on a distributed sharded | 300 | // This is not a efficient operation on a distributed sharded |
... | @@ -620,6 +659,9 @@ public class NewDistributedFlowRuleStore | ... | @@ -620,6 +659,9 @@ public class NewDistributedFlowRuleStore |
620 | } | 659 | } |
621 | 660 | ||
622 | private void backup() { | 661 | private void backup() { |
662 | + if (!backupEnabled) { | ||
663 | + return; | ||
664 | + } | ||
623 | //TODO: Force backup when backups change. | 665 | //TODO: Force backup when backups change. |
624 | try { | 666 | try { |
625 | // determine the set of devices that we need to backup during this run. | 667 | // determine the set of devices that we need to backup during this run. |
... | @@ -674,7 +716,9 @@ public class NewDistributedFlowRuleStore | ... | @@ -674,7 +716,9 @@ public class NewDistributedFlowRuleStore |
674 | 716 | ||
675 | private void onBackupReceipt(Map<DeviceId, Map<FlowId, Set<StoredFlowEntry>>> flowTables) { | 717 | private void onBackupReceipt(Map<DeviceId, Map<FlowId, Set<StoredFlowEntry>>> flowTables) { |
676 | Set<DeviceId> managedDevices = mastershipService.getDevicesOf(local); | 718 | Set<DeviceId> managedDevices = mastershipService.getDevicesOf(local); |
677 | - Maps.filterKeys(flowTables, managedDevices::contains).forEach((deviceId, flowTable) -> { | 719 | + // Only process those devices are that not managed by the local node. |
720 | + Maps.filterKeys(flowTables, deviceId -> !managedDevices.contains(deviceId)) | ||
721 | + .forEach((deviceId, flowTable) -> { | ||
678 | Map<FlowId, Set<StoredFlowEntry>> deviceFlowTable = getFlowTable(deviceId); | 722 | Map<FlowId, Set<StoredFlowEntry>> deviceFlowTable = getFlowTable(deviceId); |
679 | deviceFlowTable.clear(); | 723 | deviceFlowTable.clear(); |
680 | deviceFlowTable.putAll(flowTable); | 724 | deviceFlowTable.putAll(flowTable); | ... | ... |
-
Please register or login to post a comment