홍길동 / onos

Go to a project
Toggle navigation Toggle navigation pinning
Yuta HIGUCHI
b5b71b3c 1 parent 4b0ca2cf

DistributedMastershipStore 

- try to avoid unnecessary remote writes
- avoid reads from blocking if possible
- atomically read term info

Change-Id: I50badc718726261ccb14a6feefc578b420d28923
Inline Side-by-side
Showing 3 changed files with 181 additions and 101 deletions
...@@ -18,6 +18,7 @@ package org.onlab.onos.store.mastership.impl; ...@@ -18,6 +18,7 @@ package org.onlab.onos.store.mastership.impl;
18 import static org.onlab.onos.mastership.MastershipEvent.Type.MASTER_CHANGED; 18 import static org.onlab.onos.mastership.MastershipEvent.Type.MASTER_CHANGED;
19 import static org.apache.commons.lang3.concurrent.ConcurrentUtils.putIfAbsent; 19 import static org.apache.commons.lang3.concurrent.ConcurrentUtils.putIfAbsent;
20 20
21 +import java.util.HashSet;
21 import java.util.Map; 22 import java.util.Map;
22 import java.util.Set; 23 import java.util.Set;
23 24
...@@ -42,7 +43,6 @@ import org.onlab.onos.store.serializers.KryoNamespaces; ...@@ -42,7 +43,6 @@ import org.onlab.onos.store.serializers.KryoNamespaces;
42 import org.onlab.onos.store.serializers.KryoSerializer; 43 import org.onlab.onos.store.serializers.KryoSerializer;
43 import org.onlab.util.KryoNamespace; 44 import org.onlab.util.KryoNamespace;
44 45
45 -import com.google.common.collect.ImmutableSet;
46 import com.hazelcast.core.EntryEvent; 46 import com.hazelcast.core.EntryEvent;
47 import com.hazelcast.core.EntryListener; 47 import com.hazelcast.core.EntryListener;
48 import com.hazelcast.core.IAtomicLong; 48 import com.hazelcast.core.IAtomicLong;
...@@ -106,46 +106,50 @@ implements MastershipStore { ...@@ -106,46 +106,50 @@ implements MastershipStore {
106 106
107 @Override 107 @Override
108 public MastershipRole getRole(NodeId nodeId, DeviceId deviceId) { 108 public MastershipRole getRole(NodeId nodeId, DeviceId deviceId) {
109 - final RoleValue roleInfo = getRoleValue(deviceId); 109 + final RoleValue roleInfo = roleMap.get(deviceId);
110 - if (roleInfo.contains(MASTER, nodeId)) { 110 + if (roleInfo != null) {
111 - return MASTER; 111 + return roleInfo.getRole(nodeId);
112 - }
113 - if (roleInfo.contains(STANDBY, nodeId)) {
114 - return STANDBY;
115 } 112 }
116 return NONE; 113 return NONE;
117 } 114 }
118 115
119 @Override 116 @Override
120 - public MastershipEvent setMaster(NodeId nodeId, DeviceId deviceId) { 117 + public MastershipEvent setMaster(NodeId newMaster, DeviceId deviceId) {
121 118
122 - MastershipRole role = getRole(nodeId, deviceId);
123 roleMap.lock(deviceId); 119 roleMap.lock(deviceId);
124 try { 120 try {
125 - RoleValue rv = getRoleValue(deviceId); 121 + final RoleValue rv = getRoleValue(deviceId);
126 - switch (role) { 122 + final MastershipRole currentRole = rv.getRole(newMaster);
123 + switch (currentRole) {
127 case MASTER: 124 case MASTER:
128 //reinforce mastership 125 //reinforce mastership
129 - rv.reassign(nodeId, STANDBY, NONE); 126 + // RoleInfo integrity check
130 - roleMap.put(deviceId, rv); 127 + boolean modified = rv.reassign(newMaster, STANDBY, NONE);
128 + if (modified) {
129 + roleMap.put(deviceId, rv);
130 + // should never reach here.
131 + log.warn("{} was in both MASTER and STANDBY for {}", newMaster, deviceId);
132 + // trigger BACKUPS_CHANGED?
133 + }
131 return null; 134 return null;
132 case STANDBY: 135 case STANDBY:
133 case NONE: 136 case NONE:
134 - NodeId current = rv.get(MASTER); 137 + final NodeId currentMaster = rv.get(MASTER);
135 - if (current != null) { 138 + if (currentMaster != null) {
136 - //backup and replace current master 139 + // place current master in STANDBY
137 - rv.reassign(current, NONE, STANDBY); 140 + rv.reassign(currentMaster, NONE, STANDBY);
138 - rv.replace(current, nodeId, MASTER); 141 + rv.replace(currentMaster, newMaster, MASTER);
139 } else { 142 } else {
140 //no master before so just add. 143 //no master before so just add.
141 - rv.add(MASTER, nodeId); 144 + rv.add(MASTER, newMaster);
142 } 145 }
143 - rv.reassign(nodeId, STANDBY, NONE); 146 + // remove newMaster from STANDBY
144 - roleMap.put(deviceId, rv); 147 + rv.reassign(newMaster, STANDBY, NONE);
145 updateTerm(deviceId); 148 updateTerm(deviceId);
149 + roleMap.put(deviceId, rv);
146 return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo()); 150 return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo());
147 default: 151 default:
148 - log.warn("unknown Mastership Role {}", role); 152 + log.warn("unknown Mastership Role {}", currentRole);
149 return null; 153 return null;
150 } 154 }
151 } finally { 155 } finally {
...@@ -161,66 +165,83 @@ implements MastershipStore { ...@@ -161,66 +165,83 @@ implements MastershipStore {
161 165
162 @Override 166 @Override
163 public RoleInfo getNodes(DeviceId deviceId) { 167 public RoleInfo getNodes(DeviceId deviceId) {
164 - roleMap.lock(deviceId); 168 + RoleValue rv = roleMap.get(deviceId);
165 - try { 169 + if (rv != null) {
166 - RoleValue rv = getRoleValue(deviceId);
167 return rv.roleInfo(); 170 return rv.roleInfo();
168 - } finally { 171 + } else {
169 - roleMap.unlock(deviceId); 172 + return new RoleInfo();
170 } 173 }
171 } 174 }
172 175
173 @Override 176 @Override
174 public Set<DeviceId> getDevices(NodeId nodeId) { 177 public Set<DeviceId> getDevices(NodeId nodeId) {
175 - ImmutableSet.Builder<DeviceId> builder = ImmutableSet.builder(); 178 + Set<DeviceId> devices = new HashSet<>();
176 179
177 for (Map.Entry<DeviceId, RoleValue> el : roleMap.entrySet()) { 180 for (Map.Entry<DeviceId, RoleValue> el : roleMap.entrySet()) {
178 if (nodeId.equals(el.getValue().get(MASTER))) { 181 if (nodeId.equals(el.getValue().get(MASTER))) {
179 - builder.add(el.getKey()); 182 + devices.add(el.getKey());
180 } 183 }
181 } 184 }
182 185
183 - return builder.build(); 186 + return devices;
184 } 187 }
185 188
186 @Override 189 @Override
187 public MastershipRole requestRole(DeviceId deviceId) { 190 public MastershipRole requestRole(DeviceId deviceId) {
188 - NodeId local = clusterService.getLocalNode().id();
189 191
192 + // if no master => become master
193 + // if there already exists a master:
194 + // if I was the master return MASTER
195 + // else put myself in STANDBY and return STANDBY
196 +
197 + final NodeId local = clusterService.getLocalNode().id();
198 + boolean modified = false;
190 roleMap.lock(deviceId); 199 roleMap.lock(deviceId);
191 try { 200 try {
192 - RoleValue rv = getRoleValue(deviceId); 201 + final RoleValue rv = getRoleValue(deviceId);
193 - MastershipRole role = getRole(local, deviceId); 202 + if (rv.get(MASTER) == null) {
194 - switch (role) { 203 + // there's no master become one
204 + // move out from STANDBY
205 + rv.reassign(local, STANDBY, NONE);
206 + rv.add(MASTER, local);
207 +
208 + updateTerm(deviceId);
209 + roleMap.put(deviceId, rv);
210 + return MASTER;
211 + }
212 + final MastershipRole currentRole = rv.getRole(local);
213 + switch (currentRole) {
195 case MASTER: 214 case MASTER:
196 - rv.reassign(local, STANDBY, NONE); 215 + // RoleInfo integrity check
197 - terms.putIfAbsent(deviceId, INIT); 216 + modified = rv.reassign(local, STANDBY, NONE);
198 - roleMap.put(deviceId, rv); 217 + if (modified) {
199 - break; 218 + log.warn("{} was in both MASTER and STANDBY for {}", local, deviceId);
219 + // should never reach here,
220 + // but heal if we happened to be there
221 + roleMap.put(deviceId, rv);
222 + // trigger BACKUPS_CHANGED?
223 + }
224 + return currentRole;
200 case STANDBY: 225 case STANDBY:
201 - rv.reassign(local, NONE, STANDBY); 226 + // RoleInfo integrity check
202 - roleMap.put(deviceId, rv); 227 + modified = rv.reassign(local, NONE, STANDBY);
203 - terms.putIfAbsent(deviceId, INIT); 228 + if (modified) {
204 - break; 229 + log.warn("{} was in both NONE and STANDBY for {}", local, deviceId);
205 - case NONE: 230 + // should never reach here,
206 - //either we're the first standby, or first to device. 231 + // but heal if we happened to be there
207 - //for latter, claim mastership. 232 + roleMap.put(deviceId, rv);
208 - if (rv.get(MASTER) == null) { 233 + // trigger BACKUPS_CHANGED?
209 - rv.add(MASTER, local);
210 - rv.reassign(local, STANDBY, NONE);
211 - updateTerm(deviceId);
212 - role = MastershipRole.MASTER;
213 - } else {
214 - rv.add(STANDBY, local);
215 - rv.reassign(local, NONE, STANDBY);
216 - role = MastershipRole.STANDBY;
217 } 234 }
235 + return currentRole;
236 + case NONE:
237 + rv.reassign(local, NONE, STANDBY);
218 roleMap.put(deviceId, rv); 238 roleMap.put(deviceId, rv);
219 - break; 239 + // TODO: notifyDelegate BACKUPS_CHANGED
240 + return STANDBY;
220 default: 241 default:
221 - log.warn("unknown Mastership Role {}", role); 242 + log.warn("unknown Mastership Role {}", currentRole);
222 } 243 }
223 - return role; 244 + return currentRole;
224 } finally { 245 } finally {
225 roleMap.unlock(deviceId); 246 roleMap.unlock(deviceId);
226 } 247 }
...@@ -228,35 +249,58 @@ implements MastershipStore { ...@@ -228,35 +249,58 @@ implements MastershipStore {
228 249
229 @Override 250 @Override
230 public MastershipTerm getTermFor(DeviceId deviceId) { 251 public MastershipTerm getTermFor(DeviceId deviceId) {
231 - RoleValue rv = getRoleValue(deviceId); 252 + // term information and role must be read atomically
232 - if ((rv.get(MASTER) == null) || (terms.get(deviceId) == null)) { 253 + // acquiring write lock for the device
233 - return null; 254 + roleMap.lock(deviceId);
255 + try {
256 + RoleValue rv = getRoleValue(deviceId);
257 + final Integer term = terms.get(deviceId);
258 + final NodeId master = rv.get(MASTER);
259 + if ((master == null) || (term == null)) {
260 + return null;
261 + }
262 + return MastershipTerm.of(master, term);
263 + } finally {
264 + roleMap.unlock(deviceId);
234 } 265 }
235 - return MastershipTerm.of(rv.get(MASTER), terms.get(deviceId));
236 } 266 }
237 267
238 @Override 268 @Override
239 public MastershipEvent setStandby(NodeId nodeId, DeviceId deviceId) { 269 public MastershipEvent setStandby(NodeId nodeId, DeviceId deviceId) {
240 - MastershipEvent event = null; 270 + // if nodeId was MASTER, rotate STANDBY
271 + // if nodeId was STANDBY no-op
272 + // if nodeId was NONE, add to STANDBY
241 273
242 roleMap.lock(deviceId); 274 roleMap.lock(deviceId);
243 try { 275 try {
244 - RoleValue rv = getRoleValue(deviceId); 276 + final RoleValue rv = getRoleValue(deviceId);
245 - MastershipRole role = getRole(nodeId, deviceId); 277 + final MastershipRole currentRole = getRole(nodeId, deviceId);
246 - switch (role) { 278 + switch (currentRole) {
247 case MASTER: 279 case MASTER:
248 - event = reelect(nodeId, deviceId, rv); 280 + NodeId newMaster = reelect(nodeId, deviceId, rv);
249 - //fall through to reinforce role 281 + rv.reassign(nodeId, NONE, STANDBY);
282 + if (newMaster != null) {
283 + updateTerm(deviceId);
284 + roleMap.put(deviceId, rv);
285 + return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo());
286 + } else {
287 + // no master candidate
288 + roleMap.put(deviceId, rv);
289 + // FIXME: Should there be new event type?
290 + // or should we issue null Master event?
291 + return null;
292 + }
250 case STANDBY: 293 case STANDBY:
251 - //fall through to reinforce role 294 + return null;
252 case NONE: 295 case NONE:
253 rv.reassign(nodeId, NONE, STANDBY); 296 rv.reassign(nodeId, NONE, STANDBY);
254 roleMap.put(deviceId, rv); 297 roleMap.put(deviceId, rv);
255 - break; 298 + // TODO: BACKUPS_CHANGED?
299 + return null;
256 default: 300 default:
257 - log.warn("unknown Mastership Role {}", role); 301 + log.warn("unknown Mastership Role {}", currentRole);
258 } 302 }
259 - return event; 303 + return null;
260 } finally { 304 } finally {
261 roleMap.unlock(deviceId); 305 roleMap.unlock(deviceId);
262 } 306 }
...@@ -264,56 +308,71 @@ implements MastershipStore { ...@@ -264,56 +308,71 @@ implements MastershipStore {
264 308
265 @Override 309 @Override
266 public MastershipEvent relinquishRole(NodeId nodeId, DeviceId deviceId) { 310 public MastershipEvent relinquishRole(NodeId nodeId, DeviceId deviceId) {
267 - MastershipEvent event = null; 311 + // relinquishRole is basically set to None
312 +
313 + // If nodeId was master reelect next and remove nodeId
314 + // else remove from STANDBY
268 315
269 roleMap.lock(deviceId); 316 roleMap.lock(deviceId);
270 try { 317 try {
271 - RoleValue rv = getRoleValue(deviceId); 318 + final RoleValue rv = getRoleValue(deviceId);
272 - MastershipRole role = getRole(nodeId, deviceId); 319 + final MastershipRole currentRole = rv.getRole(nodeId);
273 - switch (role) { 320 + switch (currentRole) {
274 case MASTER: 321 case MASTER:
275 - event = reelect(nodeId, deviceId, rv); 322 + NodeId newMaster = reelect(nodeId, deviceId, rv);
276 - if (event != null) { 323 + if (newMaster != null) {
277 updateTerm(deviceId); 324 updateTerm(deviceId);
325 + roleMap.put(deviceId, rv);
326 + return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo());
327 + } else {
328 + // no master candidate
329 + roleMap.put(deviceId, rv);
330 + // Should there be new event type?
331 + return null;
278 } 332 }
279 - //fall through to reinforce relinquishment
280 case STANDBY: 333 case STANDBY:
281 //fall through to reinforce relinquishment 334 //fall through to reinforce relinquishment
282 case NONE: 335 case NONE:
283 - rv.reassign(nodeId, STANDBY, NONE); 336 + boolean modified = rv.reassign(nodeId, STANDBY, NONE);
284 - roleMap.put(deviceId, rv); 337 + if (modified) {
285 - break; 338 + roleMap.put(deviceId, rv);
339 + // TODO: BACKUPS_CHANGED?
340 + return null;
341 + }
342 + return null;
286 default: 343 default:
287 - log.warn("unknown Mastership Role {}", role); 344 + log.warn("unknown Mastership Role {}", currentRole);
288 } 345 }
289 - return event; 346 + return null;
290 } finally { 347 } finally {
291 roleMap.unlock(deviceId); 348 roleMap.unlock(deviceId);
292 } 349 }
293 } 350 }
294 351
352 + // TODO: Consider moving this to RoleValue method
295 //helper to fetch a new master candidate for a given device. 353 //helper to fetch a new master candidate for a given device.
296 - private MastershipEvent reelect( 354 + private NodeId reelect(
297 NodeId current, DeviceId deviceId, RoleValue rv) { 355 NodeId current, DeviceId deviceId, RoleValue rv) {
298 356
299 //if this is an queue it'd be neater. 357 //if this is an queue it'd be neater.
300 - NodeId backup = null; 358 + NodeId candidate = null;
301 for (NodeId n : rv.nodesOfRole(STANDBY)) { 359 for (NodeId n : rv.nodesOfRole(STANDBY)) {
302 if (!current.equals(n)) { 360 if (!current.equals(n)) {
303 - backup = n; 361 + candidate = n;
304 break; 362 break;
305 } 363 }
306 } 364 }
307 365
308 - if (backup == null) { 366 + if (candidate == null) {
309 log.info("{} giving up and going to NONE for {}", current, deviceId); 367 log.info("{} giving up and going to NONE for {}", current, deviceId);
310 rv.remove(MASTER, current); 368 rv.remove(MASTER, current);
369 + // master did change, but there is no master candidate.
311 return null; 370 return null;
312 } else { 371 } else {
313 - log.info("{} trying to pass mastership for {} to {}", current, deviceId, backup); 372 + log.info("{} trying to pass mastership for {} to {}", current, deviceId, candidate);
314 - rv.replace(current, backup, MASTER); 373 + rv.replace(current, candidate, MASTER);
315 - rv.reassign(backup, STANDBY, NONE); 374 + rv.reassign(candidate, STANDBY, NONE);
316 - return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo()); 375 + return candidate;
317 } 376 }
318 } 377 }
319 378
...@@ -340,6 +399,7 @@ implements MastershipStore { ...@@ -340,6 +399,7 @@ implements MastershipStore {
340 } 399 }
341 400
342 //adds or updates term information. 401 //adds or updates term information.
402 + // must be guarded by roleMap.lock(deviceId)
343 private void updateTerm(DeviceId deviceId) { 403 private void updateTerm(DeviceId deviceId) {
344 Integer term = terms.get(deviceId); 404 Integer term = terms.get(deviceId);
345 if (term == null) { 405 if (term == null) {
......
...@@ -15,6 +15,10 @@ ...@@ -15,6 +15,10 @@
15 */ 15 */
16 package org.onlab.onos.store.mastership.impl; 16 package org.onlab.onos.store.mastership.impl;
17 17
18 +import static org.onlab.onos.net.MastershipRole.MASTER;
19 +import static org.onlab.onos.net.MastershipRole.NONE;
20 +import static org.onlab.onos.net.MastershipRole.STANDBY;
21 +
18 import java.util.Collections; 22 import java.util.Collections;
19 import java.util.EnumMap; 23 import java.util.EnumMap;
20 import java.util.LinkedList; 24 import java.util.LinkedList;
...@@ -59,18 +63,30 @@ final class RoleValue { ...@@ -59,18 +63,30 @@ final class RoleValue {
59 return value.get(type).contains(nodeId); 63 return value.get(type).contains(nodeId);
60 } 64 }
61 65
66 + public MastershipRole getRole(NodeId nodeId) {
67 + if (contains(MASTER, nodeId)) {
68 + return MASTER;
69 + }
70 + if (contains(STANDBY, nodeId)) {
71 + return STANDBY;
72 + }
73 + return NONE;
74 + }
75 +
62 /** 76 /**
63 * Associates a node to a certain role. 77 * Associates a node to a certain role.
64 * 78 *
65 * @param type the role 79 * @param type the role
66 * @param nodeId the node ID of the node to associate 80 * @param nodeId the node ID of the node to associate
81 + * @return true if modified
67 */ 82 */
68 - public void add(MastershipRole type, NodeId nodeId) { 83 + public boolean add(MastershipRole type, NodeId nodeId) {
69 List<NodeId> nodes = value.get(type); 84 List<NodeId> nodes = value.get(type);
70 85
71 if (!nodes.contains(nodeId)) { 86 if (!nodes.contains(nodeId)) {
72 - nodes.add(nodeId); 87 + return nodes.add(nodeId);
73 } 88 }
89 + return false;
74 } 90 }
75 91
76 /** 92 /**
...@@ -78,7 +94,7 @@ final class RoleValue { ...@@ -78,7 +94,7 @@ final class RoleValue {
78 * 94 *
79 * @param type the role 95 * @param type the role
80 * @param nodeId the ID of the node to remove 96 * @param nodeId the ID of the node to remove
81 - * @return 97 + * @return true if modified
82 */ 98 */
83 public boolean remove(MastershipRole type, NodeId nodeId) { 99 public boolean remove(MastershipRole type, NodeId nodeId) {
84 List<NodeId> nodes = value.get(type); 100 List<NodeId> nodes = value.get(type);
...@@ -96,10 +112,12 @@ final class RoleValue { ...@@ -96,10 +112,12 @@ final class RoleValue {
96 * @param nodeId the Node ID of node changing roles 112 * @param nodeId the Node ID of node changing roles
97 * @param from the old role 113 * @param from the old role
98 * @param to the new role 114 * @param to the new role
115 + * @return true if modified
99 */ 116 */
100 - public void reassign(NodeId nodeId, MastershipRole from, MastershipRole to) { 117 + public boolean reassign(NodeId nodeId, MastershipRole from, MastershipRole to) {
101 - remove(from, nodeId); 118 + boolean modified = remove(from, nodeId);
102 - add(to, nodeId); 119 + modified |= add(to, nodeId);
120 + return modified;
103 } 121 }
104 122
105 /** 123 /**
...@@ -109,10 +127,12 @@ final class RoleValue { ...@@ -109,10 +127,12 @@ final class RoleValue {
109 * @param from the old NodeId to replace 127 * @param from the old NodeId to replace
110 * @param to the new NodeId 128 * @param to the new NodeId
111 * @param type the role associated with the old NodeId 129 * @param type the role associated with the old NodeId
130 + * @return true if modified
112 */ 131 */
113 - public void replace(NodeId from, NodeId to, MastershipRole type) { 132 + public boolean replace(NodeId from, NodeId to, MastershipRole type) {
114 - remove(type, from); 133 + boolean modified = remove(type, from);
115 - add(type, to); 134 + modified |= add(type, to);
135 + return modified;
116 } 136 }
117 137
118 /** 138 /**
......
...@@ -142,7 +142,7 @@ public class DistributedMastershipStoreTest { ...@@ -142,7 +142,7 @@ public class DistributedMastershipStoreTest {
142 testStore.setCurrent(CN1); 142 testStore.setCurrent(CN1);
143 143
144 //if already MASTER, nothing should happen 144 //if already MASTER, nothing should happen
145 - testStore.put(DID2, N1, true, false, false); 145 + testStore.put(DID2, N1, true, false, true);
146 assertEquals("wrong role for MASTER:", MASTER, dms.requestRole(DID2)); 146 assertEquals("wrong role for MASTER:", MASTER, dms.requestRole(DID2));
147 147
148 //populate maps with DID1, N1 thru NONE case 148 //populate maps with DID1, N1 thru NONE case
......