Trigger MastershipEvent on no more master case
Change-Id: Iaac7b7d021802e7470df061dad719dcdf0e4b73e
diff --git a/apps/foo/src/main/java/org/onlab/onos/foo/FooComponent.java b/apps/foo/src/main/java/org/onlab/onos/foo/FooComponent.java
index f7f6478..117a9a0 100644
--- a/apps/foo/src/main/java/org/onlab/onos/foo/FooComponent.java
+++ b/apps/foo/src/main/java/org/onlab/onos/foo/FooComponent.java
@@ -23,6 +23,10 @@
import org.onlab.onos.cluster.ClusterEvent;
import org.onlab.onos.cluster.ClusterEventListener;
import org.onlab.onos.cluster.ClusterService;
+import org.onlab.onos.cluster.NodeId;
+import org.onlab.onos.mastership.MastershipEvent;
+import org.onlab.onos.mastership.MastershipListener;
+import org.onlab.onos.mastership.MastershipService;
import org.onlab.onos.net.device.DeviceEvent;
import org.onlab.onos.net.device.DeviceListener;
import org.onlab.onos.net.device.DeviceService;
@@ -50,15 +54,20 @@
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
protected IntentService intentService;
+ @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
+ protected MastershipService mastershipService;
+
private final ClusterEventListener clusterListener = new InnerClusterListener();
private final DeviceListener deviceListener = new InnerDeviceListener();
private final IntentListener intentListener = new InnerIntentListener();
+ private final MastershipListener mastershipListener = new InnerMastershipListener();
@Activate
public void activate() {
clusterService.addListener(clusterListener);
deviceService.addListener(deviceListener);
intentService.addListener(intentListener);
+ mastershipService.addListener(mastershipListener);
log.info("Started");
}
@@ -67,6 +76,7 @@
clusterService.removeListener(clusterListener);
deviceService.removeListener(deviceListener);
intentService.removeListener(intentListener);
+ mastershipService.removeListener(mastershipListener);
log.info("Stopped");
}
@@ -100,6 +110,18 @@
log.info(message, event.subject());
}
}
+
+ private class InnerMastershipListener implements MastershipListener {
+ @Override
+ public void event(MastershipEvent event) {
+ final NodeId myId = clusterService.getLocalNode().id();
+ if (myId.equals(event.roleInfo().master())) {
+ log.info("I have control/I wish you luck {}", event);
+ } else {
+ log.info("you have control {}", event);
+ }
+ }
+ }
}
diff --git a/core/net/src/test/java/org/onlab/onos/cluster/impl/MastershipManagerTest.java b/core/net/src/test/java/org/onlab/onos/cluster/impl/MastershipManagerTest.java
index ddd805f..33961e4 100644
--- a/core/net/src/test/java/org/onlab/onos/cluster/impl/MastershipManagerTest.java
+++ b/core/net/src/test/java/org/onlab/onos/cluster/impl/MastershipManagerTest.java
@@ -28,6 +28,7 @@
import org.onlab.onos.cluster.NodeId;
import org.onlab.onos.event.impl.TestEventDispatcher;
import org.onlab.onos.mastership.MastershipService;
+import org.onlab.onos.mastership.MastershipStore;
import org.onlab.onos.mastership.MastershipTermService;
import org.onlab.onos.net.DeviceId;
import org.onlab.onos.store.trivial.impl.SimpleMastershipStore;
@@ -57,9 +58,9 @@
public void setUp() {
mgr = new MastershipManager();
service = mgr;
- mgr.store = new SimpleMastershipStore();
mgr.eventDispatcher = new TestEventDispatcher();
mgr.clusterService = new TestClusterService();
+ mgr.store = new TestSimpleMastershipStore(mgr.clusterService);
mgr.activate();
}
@@ -74,7 +75,8 @@
@Test
public void setRole() {
mgr.setRole(NID_OTHER, DEV_MASTER, MASTER);
- assertEquals("wrong local role:", STANDBY, mgr.getLocalRole(DEV_MASTER));
+ assertEquals("wrong local role:", NONE, mgr.getLocalRole(DEV_MASTER));
+ assertEquals("wrong obtained role:", STANDBY, mgr.requestRoleFor(DEV_MASTER));
//set to master
mgr.setRole(NID_LOCAL, DEV_MASTER, MASTER);
@@ -182,4 +184,12 @@
}
}
+
+ private final class TestSimpleMastershipStore extends SimpleMastershipStore
+ implements MastershipStore {
+
+ public TestSimpleMastershipStore(ClusterService clusterService) {
+ super.clusterService = clusterService;
+ }
+ }
}
diff --git a/core/store/dist/src/main/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStore.java b/core/store/dist/src/main/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStore.java
index 42e0799..6c2ad6a 100644
--- a/core/store/dist/src/main/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStore.java
+++ b/core/store/dist/src/main/java/org/onlab/onos/store/mastership/impl/DistributedMastershipStore.java
@@ -283,16 +283,15 @@
case MASTER:
NodeId newMaster = reelect(nodeId, deviceId, rv);
rv.reassign(nodeId, NONE, STANDBY);
+ updateTerm(deviceId);
if (newMaster != null) {
- updateTerm(deviceId);
roleMap.put(deviceId, rv);
return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo());
} else {
// no master candidate
roleMap.put(deviceId, rv);
- // FIXME: Should there be new event type?
- // or should we issue null Master event?
- return null;
+ // TODO: Should there be new event type for no MASTER?
+ return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo());
}
case STANDBY:
return null;
diff --git a/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleMastershipStore.java b/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleMastershipStore.java
index 0f36393..62c084e 100644
--- a/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleMastershipStore.java
+++ b/core/store/trivial/src/main/java/org/onlab/onos/store/trivial/impl/SimpleMastershipStore.java
@@ -29,8 +29,13 @@
import org.apache.felix.scr.annotations.Activate;
import org.apache.felix.scr.annotations.Component;
import org.apache.felix.scr.annotations.Deactivate;
+import org.apache.felix.scr.annotations.Reference;
+import org.apache.felix.scr.annotations.ReferenceCardinality;
import org.apache.felix.scr.annotations.Service;
+import org.onlab.onos.cluster.ClusterEventListener;
+import org.onlab.onos.cluster.ClusterService;
import org.onlab.onos.cluster.ControllerNode;
+import org.onlab.onos.cluster.ControllerNode.State;
import org.onlab.onos.cluster.DefaultControllerNode;
import org.onlab.onos.cluster.NodeId;
import org.onlab.onos.cluster.RoleInfo;
@@ -44,7 +49,8 @@
import org.onlab.packet.IpAddress;
import org.slf4j.Logger;
-import com.google.common.collect.Lists;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import static org.onlab.onos.mastership.MastershipEvent.Type.*;
@@ -60,23 +66,65 @@
private final Logger log = getLogger(getClass());
- public static final IpAddress LOCALHOST = IpAddress.valueOf("127.0.0.1");
-
private static final int NOTHING = 0;
private static final int INIT = 1;
- private ControllerNode instance =
- new DefaultControllerNode(new NodeId("local"), LOCALHOST);
+ @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
+ protected ClusterService clusterService;
//devices mapped to their masters, to emulate multiple nodes
protected final Map<DeviceId, NodeId> masterMap = new HashMap<>();
//emulate backups with pile of nodes
- protected final Set<NodeId> backups = new HashSet<>();
+ protected final Map<DeviceId, List<NodeId>> backups = new HashMap<>();
//terms
protected final Map<DeviceId, AtomicInteger> termMap = new HashMap<>();
@Activate
public void activate() {
+ if (clusterService == null) {
+ // just for ease of unit test
+ final ControllerNode instance =
+ new DefaultControllerNode(new NodeId("local"),
+ IpAddress.valueOf("127.0.0.1"));
+
+ clusterService = new ClusterService() {
+
+ @Override
+ public ControllerNode getLocalNode() {
+ return instance;
+ }
+
+ @Override
+ public Set<ControllerNode> getNodes() {
+ return ImmutableSet.of(instance);
+ }
+
+ @Override
+ public ControllerNode getNode(NodeId nodeId) {
+ if (instance.id().equals(nodeId)) {
+ return instance;
+ }
+ return null;
+ }
+
+ @Override
+ public State getState(NodeId nodeId) {
+ if (instance.id().equals(nodeId)) {
+ return State.ACTIVE;
+ } else {
+ return State.INACTIVE;
+ }
+ }
+
+ @Override
+ public void addListener(ClusterEventListener listener) {
+ }
+
+ @Override
+ public void removeListener(ClusterEventListener listener) {
+ }
+ };
+ }
log.info("Started");
}
@@ -86,31 +134,27 @@
}
@Override
- public MastershipEvent setMaster(NodeId nodeId, DeviceId deviceId) {
- MastershipRole role = getRole(nodeId, deviceId);
+ public synchronized MastershipEvent setMaster(NodeId nodeId, DeviceId deviceId) {
- synchronized (this) {
- switch (role) {
- case MASTER:
- return null;
- case STANDBY:
- masterMap.put(deviceId, nodeId);
- termMap.get(deviceId).incrementAndGet();
- backups.add(nodeId);
- break;
- case NONE:
- masterMap.put(deviceId, nodeId);
- termMap.put(deviceId, new AtomicInteger(INIT));
- backups.add(nodeId);
- break;
- default:
- log.warn("unknown Mastership Role {}", role);
- return null;
- }
+ MastershipRole role = getRole(nodeId, deviceId);
+ switch (role) {
+ case MASTER:
+ // no-op
+ return null;
+ case STANDBY:
+ case NONE:
+ NodeId prevMaster = masterMap.put(deviceId, nodeId);
+ incrementTerm(deviceId);
+ removeFromBackups(deviceId, nodeId);
+ addToBackup(deviceId, prevMaster);
+ break;
+ default:
+ log.warn("unknown Mastership Role {}", role);
+ return null;
}
return new MastershipEvent(MASTER_CHANGED, deviceId,
- new RoleInfo(nodeId, Lists.newLinkedList(backups)));
+ getNodes(deviceId));
}
@Override
@@ -118,12 +162,11 @@
return masterMap.get(deviceId);
}
+ // synchronized for atomic read
@Override
- public RoleInfo getNodes(DeviceId deviceId) {
- List<NodeId> nodes = new ArrayList<>();
- nodes.addAll(backups);
-
- return new RoleInfo(masterMap.get(deviceId), nodes);
+ public synchronized RoleInfo getNodes(DeviceId deviceId) {
+ return new RoleInfo(masterMap.get(deviceId),
+ backups.getOrDefault(deviceId, ImmutableList.of()));
}
@Override
@@ -134,69 +177,97 @@
ids.add(d.getKey());
}
}
- return Collections.unmodifiableSet(ids);
+ return ids;
}
@Override
- public MastershipRole requestRole(DeviceId deviceId) {
+ public synchronized MastershipRole requestRole(DeviceId deviceId) {
//query+possible reelection
- NodeId node = instance.id();
+ NodeId node = clusterService.getLocalNode().id();
MastershipRole role = getRole(node, deviceId);
switch (role) {
case MASTER:
- break;
+ return MastershipRole.MASTER;
case STANDBY:
- synchronized (this) {
- //try to "re-elect", since we're really not distributed
- NodeId rel = reelect(node);
- if (rel == null) {
- masterMap.put(deviceId, node);
- termMap.put(deviceId, new AtomicInteger(INIT));
- role = MastershipRole.MASTER;
- }
- backups.add(node);
- }
- break;
- case NONE:
- //first to get to it, say we are master
- synchronized (this) {
+ if (getMaster(deviceId) == null) {
+ // no master => become master
masterMap.put(deviceId, node);
- termMap.put(deviceId, new AtomicInteger(INIT));
- backups.add(node);
- role = MastershipRole.MASTER;
+ incrementTerm(deviceId);
+ // remove from backup list
+ removeFromBackups(deviceId, node);
+ notifyDelegate(new MastershipEvent(MASTER_CHANGED, deviceId,
+ getNodes(deviceId)));
+ return MastershipRole.MASTER;
}
- break;
+ return MastershipRole.STANDBY;
+ case NONE:
+ if (getMaster(deviceId) == null) {
+ // no master => become master
+ masterMap.put(deviceId, node);
+ incrementTerm(deviceId);
+ notifyDelegate(new MastershipEvent(MASTER_CHANGED, deviceId,
+ getNodes(deviceId)));
+ return MastershipRole.MASTER;
+ }
+ // add to backup list
+ if (addToBackup(deviceId, node)) {
+ notifyDelegate(new MastershipEvent(BACKUPS_CHANGED, deviceId,
+ getNodes(deviceId)));
+ }
+ return MastershipRole.STANDBY;
default:
log.warn("unknown Mastership Role {}", role);
}
return role;
}
+ // add to backup if not there already, silently ignores null node
+ private synchronized boolean addToBackup(DeviceId deviceId, NodeId nodeId) {
+ boolean modified = false;
+ List<NodeId> stbys = backups.getOrDefault(deviceId, new ArrayList<>());
+ if (nodeId != null && !stbys.contains(nodeId)) {
+ stbys.add(nodeId);
+ modified = true;
+ }
+ backups.put(deviceId, stbys);
+ return modified;
+ }
+
+ private synchronized boolean removeFromBackups(DeviceId deviceId, NodeId node) {
+ List<NodeId> stbys = backups.getOrDefault(deviceId, new ArrayList<>());
+ boolean modified = stbys.remove(node);
+ backups.put(deviceId, stbys);
+ return modified;
+ }
+
+ private synchronized void incrementTerm(DeviceId deviceId) {
+ AtomicInteger term = termMap.getOrDefault(deviceId, new AtomicInteger(NOTHING));
+ term.incrementAndGet();
+ termMap.put(deviceId, term);
+ }
+
@Override
public MastershipRole getRole(NodeId nodeId, DeviceId deviceId) {
//just query
NodeId current = masterMap.get(deviceId);
MastershipRole role;
- if (current == null) {
- if (backups.contains(nodeId)) {
- role = MastershipRole.STANDBY;
- } else {
- role = MastershipRole.NONE;
- }
+ if (current != null && current.equals(nodeId)) {
+ return MastershipRole.MASTER;
+ }
+
+ if (backups.getOrDefault(deviceId, Collections.emptyList()).contains(nodeId)) {
+ role = MastershipRole.STANDBY;
} else {
- if (current.equals(nodeId)) {
- role = MastershipRole.MASTER;
- } else {
- role = MastershipRole.STANDBY;
- }
+ role = MastershipRole.NONE;
}
return role;
}
+ // synchronized for atomic read
@Override
- public MastershipTerm getTermFor(DeviceId deviceId) {
+ public synchronized MastershipTerm getTermFor(DeviceId deviceId) {
if ((termMap.get(deviceId) == null)) {
return MastershipTerm.of(masterMap.get(deviceId), NOTHING);
}
@@ -205,72 +276,71 @@
}
@Override
- public MastershipEvent setStandby(NodeId nodeId, DeviceId deviceId) {
+ public synchronized MastershipEvent setStandby(NodeId nodeId, DeviceId deviceId) {
MastershipRole role = getRole(nodeId, deviceId);
- synchronized (this) {
- switch (role) {
- case MASTER:
- NodeId backup = reelect(nodeId);
- if (backup == null) {
- masterMap.remove(deviceId);
- } else {
- masterMap.put(deviceId, backup);
- termMap.get(deviceId).incrementAndGet();
- return new MastershipEvent(MASTER_CHANGED, deviceId,
- new RoleInfo(backup, Lists.newLinkedList(backups)));
- }
- case STANDBY:
- case NONE:
- if (!termMap.containsKey(deviceId)) {
- termMap.put(deviceId, new AtomicInteger(INIT));
- }
- backups.add(nodeId);
- break;
- default:
- log.warn("unknown Mastership Role {}", role);
+ switch (role) {
+ case MASTER:
+ NodeId backup = reelect(deviceId, nodeId);
+ if (backup == null) {
+ // no master alternative
+ masterMap.remove(deviceId);
+ // TODO: Should there be new event type for no MASTER?
+ return new MastershipEvent(MASTER_CHANGED, deviceId,
+ getNodes(deviceId));
+ } else {
+ NodeId prevMaster = masterMap.put(deviceId, backup);
+ incrementTerm(deviceId);
+ addToBackup(deviceId, prevMaster);
+ return new MastershipEvent(MASTER_CHANGED, deviceId,
+ getNodes(deviceId));
}
+ case STANDBY:
+ case NONE:
+ boolean modified = addToBackup(deviceId, nodeId);
+ if (modified) {
+ return new MastershipEvent(BACKUPS_CHANGED, deviceId,
+ getNodes(deviceId));
+ }
+ default:
+ log.warn("unknown Mastership Role {}", role);
}
return null;
}
//dumbly selects next-available node that's not the current one
//emulate leader election
- private NodeId reelect(NodeId nodeId) {
+ private synchronized NodeId reelect(DeviceId did, NodeId nodeId) {
+ List<NodeId> stbys = backups.getOrDefault(did, Collections.emptyList());
NodeId backup = null;
- for (NodeId n : backups) {
+ for (NodeId n : stbys) {
if (!n.equals(nodeId)) {
backup = n;
break;
}
}
- backups.remove(backup);
+ stbys.remove(backup);
return backup;
}
@Override
- public MastershipEvent relinquishRole(NodeId nodeId, DeviceId deviceId) {
+ public synchronized MastershipEvent relinquishRole(NodeId nodeId, DeviceId deviceId) {
MastershipRole role = getRole(nodeId, deviceId);
- synchronized (this) {
- switch (role) {
- case MASTER:
- NodeId backup = reelect(nodeId);
- backups.remove(nodeId);
- if (backup == null) {
- masterMap.remove(deviceId);
- } else {
- masterMap.put(deviceId, backup);
- termMap.get(deviceId).incrementAndGet();
- return new MastershipEvent(MASTER_CHANGED, deviceId,
- new RoleInfo(backup, Lists.newLinkedList(backups)));
- }
- case STANDBY:
- backups.remove(nodeId);
- case NONE:
- default:
- log.warn("unknown Mastership Role {}", role);
+ switch (role) {
+ case MASTER:
+ NodeId backup = reelect(deviceId, nodeId);
+ masterMap.put(deviceId, backup);
+ incrementTerm(deviceId);
+ return new MastershipEvent(MASTER_CHANGED, deviceId,
+ getNodes(deviceId));
+ case STANDBY:
+ if (removeFromBackups(deviceId, nodeId)) {
+ return new MastershipEvent(BACKUPS_CHANGED, deviceId,
+ getNodes(deviceId));
}
+ case NONE:
+ default:
+ log.warn("unknown Mastership Role {}", role);
}
return null;
}
-
}
diff --git a/core/store/trivial/src/test/java/org/onlab/onos/store/trivial/impl/SimpleMastershipStoreTest.java b/core/store/trivial/src/test/java/org/onlab/onos/store/trivial/impl/SimpleMastershipStoreTest.java
index 711e366..0998e0a 100644
--- a/core/store/trivial/src/test/java/org/onlab/onos/store/trivial/impl/SimpleMastershipStoreTest.java
+++ b/core/store/trivial/src/test/java/org/onlab/onos/store/trivial/impl/SimpleMastershipStoreTest.java
@@ -15,6 +15,8 @@
*/
package org.onlab.onos.store.trivial.impl;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;
@@ -22,6 +24,7 @@
import org.junit.Before;
import org.junit.Test;
import org.onlab.onos.cluster.NodeId;
+import org.onlab.onos.mastership.MastershipEvent;
import org.onlab.onos.mastership.MastershipTerm;
import org.onlab.onos.net.DeviceId;
@@ -74,6 +77,7 @@
assertEquals("wrong role", MASTER, sms.getRole(N2, DID3));
//N2 is master but N1 is only in backups set
+ put(DID4, N1, false, true);
put(DID4, N2, true, false);
assertEquals("wrong role", STANDBY, sms.getRole(N1, DID4));
}
@@ -127,12 +131,12 @@
put(DID1, N1, false, false);
assertEquals("wrong role", MASTER, sms.requestRole(DID1));
- //STANDBY without backup - become MASTER
+ //was STANDBY - become MASTER
put(DID2, N1, false, true);
assertEquals("wrong role", MASTER, sms.requestRole(DID2));
- //STANDBY with backup - stay STANDBY
- put(DID3, N2, false, true);
+ //other MASTER - stay STANDBY
+ put(DID3, N2, true, false);
assertEquals("wrong role", STANDBY, sms.requestRole(DID3));
//local (N1) is MASTER - stay MASTER
@@ -145,30 +149,34 @@
//NONE - record backup but take no other action
put(DID1, N1, false, false);
sms.setStandby(N1, DID1);
- assertTrue("not backed up", sms.backups.contains(N1));
- sms.termMap.clear();
+ assertTrue("not backed up", sms.backups.get(DID1).contains(N1));
+ int prev = sms.termMap.get(DID1).get();
sms.setStandby(N1, DID1);
- assertTrue("term not set", sms.termMap.containsKey(DID1));
+ assertEquals("term should not change", prev, sms.termMap.get(DID1).get());
//no backup, MASTER
- put(DID1, N1, true, true);
- assertNull("wrong event", sms.setStandby(N1, DID1));
+ put(DID1, N1, true, false);
+ assertNull("expect no MASTER event", sms.setStandby(N1, DID1).roleInfo().master());
assertNull("wrong node", sms.masterMap.get(DID1));
//backup, switch
sms.masterMap.clear();
put(DID1, N1, true, true);
+ put(DID1, N2, false, true);
put(DID2, N2, true, true);
- assertEquals("wrong event", MASTER_CHANGED, sms.setStandby(N1, DID1).type());
+ MastershipEvent event = sms.setStandby(N1, DID1);
+ assertEquals("wrong event", MASTER_CHANGED, event.type());
+ assertEquals("wrong master", N2, event.roleInfo().master());
}
//helper to populate master/backup structures
- private void put(DeviceId dev, NodeId node, boolean store, boolean backup) {
- if (store) {
+ private void put(DeviceId dev, NodeId node, boolean master, boolean backup) {
+ if (master) {
sms.masterMap.put(dev, node);
- }
- if (backup) {
- sms.backups.add(node);
+ } else if (backup) {
+ List<NodeId> stbys = sms.backups.getOrDefault(dev, new ArrayList<>());
+ stbys.add(node);
+ sms.backups.put(dev, stbys);
}
sms.termMap.put(dev, new AtomicInteger());
}