[SDFAB-753] Improve ONOS cluster event
Main idea of this change is to add an additional parameter
in the event that carries information about the failed instance.
Additionally, prevents several NPE by using hostname as id
when controller hostname cannot be resolved into an ip.
Change-Id: Id9886afe3f1e5ecee0f1414b2722c340680a813e
(cherry picked from commit fa5dc3c137a4deaa020a669388470b511c2b6a8e)
diff --git a/core/api/src/main/java/org/onosproject/cluster/ClusterEvent.java b/core/api/src/main/java/org/onosproject/cluster/ClusterEvent.java
index 6d905b2..d4ec678 100644
--- a/core/api/src/main/java/org/onosproject/cluster/ClusterEvent.java
+++ b/core/api/src/main/java/org/onosproject/cluster/ClusterEvent.java
@@ -55,6 +55,25 @@
INSTANCE_DEACTIVATED
}
+ public enum InstanceType {
+ /**
+ * Signifies that the event refers to an ONOS instance.
+ */
+ ONOS,
+
+ /**
+ * Signifies that the event refers to an Atomix instance.
+ */
+ STORAGE,
+
+ /**
+ * Signifies that the event refers to an Unknown instance.
+ */
+ UNKNOWN
+ }
+
+ private final InstanceType instanceType;
+
/**
* Creates an event of a given type and for the specified instance and the
* current time.
@@ -63,7 +82,7 @@
* @param instance cluster device subject
*/
public ClusterEvent(Type type, ControllerNode instance) {
- super(type, instance);
+ this(type, instance, InstanceType.UNKNOWN);
}
/**
@@ -74,13 +93,47 @@
* @param time occurrence time
*/
public ClusterEvent(Type type, ControllerNode instance, long time) {
- super(type, instance, time);
+ this(type, instance, time, InstanceType.UNKNOWN);
}
+ /**
+ * Creates an event of a given type and for the specified instance and the
+ * current time.
+ *
+ * @param type cluster event type
+ * @param instance cluster device subject
+ * @param instanceType instance type
+ */
+ public ClusterEvent(Type type, ControllerNode instance, InstanceType instanceType) {
+ super(type, instance);
+ this.instanceType = instanceType;
+ }
+
+ /**
+ * Creates an event of a given type and for the specified device and time.
+ *
+ * @param type device event type
+ * @param instance event device subject
+ * @param time occurrence time
+ * @param instanceType instance type
+ */
+ public ClusterEvent(Type type, ControllerNode instance, long time, InstanceType instanceType) {
+ super(type, instance, time);
+ this.instanceType = instanceType;
+ }
+
+ /**
+ * Returns the instance type subject.
+ *
+ * @return instance type subject or UNKNOWN if the event is not instance type specific.
+ */
+ public InstanceType instanceType() {
+ return instanceType;
+ }
@Override
public int hashCode() {
- return Objects.hash(type(), subject(), time());
+ return Objects.hash(type(), subject(), time(), instanceType());
}
@Override
@@ -92,7 +145,8 @@
final ClusterEvent other = (ClusterEvent) obj;
return Objects.equals(this.type(), other.type()) &&
Objects.equals(this.subject(), other.subject()) &&
- Objects.equals(this.time(), other.time());
+ Objects.equals(this.time(), other.time()) &&
+ Objects.equals(this.instanceType(), other.instanceType());
}
return false;
}
@@ -103,6 +157,7 @@
.add("type", type())
.add("subject", subject())
.add("time", time())
+ .add("instanceType", instanceType())
.toString();
}
diff --git a/core/api/src/test/java/org/onosproject/cluster/ClusterEventTest.java b/core/api/src/test/java/org/onosproject/cluster/ClusterEventTest.java
index 8995062..f5e1f1e 100644
--- a/core/api/src/test/java/org/onosproject/cluster/ClusterEventTest.java
+++ b/core/api/src/test/java/org/onosproject/cluster/ClusterEventTest.java
@@ -49,6 +49,26 @@
new ClusterEvent(ClusterEvent.Type.INSTANCE_READY, cNode2, time);
private final ClusterEvent sameAsEvent7 =
new ClusterEvent(ClusterEvent.Type.INSTANCE_READY, cNode2, time);
+ private final ClusterEvent event8 =
+ new ClusterEvent(ClusterEvent.Type.INSTANCE_ADDED, cNode2, ClusterEvent.InstanceType.ONOS);
+ private final ClusterEvent event9 =
+ new ClusterEvent(ClusterEvent.Type.INSTANCE_ADDED, cNode2, ClusterEvent.InstanceType.STORAGE);
+ private final ClusterEvent event10 =
+ new ClusterEvent(ClusterEvent.Type.INSTANCE_REMOVED, cNode2, ClusterEvent.InstanceType.ONOS);
+ private final ClusterEvent event11 =
+ new ClusterEvent(ClusterEvent.Type.INSTANCE_REMOVED, cNode2, ClusterEvent.InstanceType.STORAGE);
+ private final ClusterEvent event12 =
+ new ClusterEvent(ClusterEvent.Type.INSTANCE_ACTIVATED, cNode1, ClusterEvent.InstanceType.ONOS);
+ private final ClusterEvent event13 =
+ new ClusterEvent(ClusterEvent.Type.INSTANCE_ACTIVATED, cNode1, ClusterEvent.InstanceType.STORAGE);
+ private final ClusterEvent event14 =
+ new ClusterEvent(ClusterEvent.Type.INSTANCE_READY, cNode1, ClusterEvent.InstanceType.ONOS);
+ private final ClusterEvent event15 =
+ new ClusterEvent(ClusterEvent.Type.INSTANCE_READY, cNode1, ClusterEvent.InstanceType.STORAGE);
+ private final ClusterEvent event16 =
+ new ClusterEvent(ClusterEvent.Type.INSTANCE_DEACTIVATED, cNode1, ClusterEvent.InstanceType.ONOS);
+ private final ClusterEvent event17 =
+ new ClusterEvent(ClusterEvent.Type.INSTANCE_DEACTIVATED, cNode1, ClusterEvent.InstanceType.STORAGE);
/**
* Tests for proper operation of equals(), hashCode() and toString() methods.
@@ -63,6 +83,16 @@
.addEqualityGroup(event5)
.addEqualityGroup(event6)
.addEqualityGroup(event7, sameAsEvent7)
+ .addEqualityGroup(event8)
+ .addEqualityGroup(event9)
+ .addEqualityGroup(event10)
+ .addEqualityGroup(event11)
+ .addEqualityGroup(event12)
+ .addEqualityGroup(event13)
+ .addEqualityGroup(event14)
+ .addEqualityGroup(event15)
+ .addEqualityGroup(event16)
+ .addEqualityGroup(event17)
.testEquals();
}
@@ -73,10 +103,52 @@
public void checkConstruction() {
assertThat(event1.type(), is(ClusterEvent.Type.INSTANCE_ADDED));
assertThat(event1.subject(), is(cNode1));
+ assertThat(event1.instanceType(), is(ClusterEvent.InstanceType.UNKNOWN));
assertThat(event7.time(), is(time));
assertThat(event7.type(), is(ClusterEvent.Type.INSTANCE_READY));
assertThat(event7.subject(), is(cNode2));
+ assertThat(event7.instanceType(), is(ClusterEvent.InstanceType.UNKNOWN));
+
+ assertThat(event8.type(), is(ClusterEvent.Type.INSTANCE_ADDED));
+ assertThat(event8.subject(), is(cNode2));
+ assertThat(event8.instanceType(), is(ClusterEvent.InstanceType.ONOS));
+
+ assertThat(event9.type(), is(ClusterEvent.Type.INSTANCE_ADDED));
+ assertThat(event9.subject(), is(cNode2));
+ assertThat(event9.instanceType(), is(ClusterEvent.InstanceType.STORAGE));
+
+ assertThat(event10.type(), is(ClusterEvent.Type.INSTANCE_REMOVED));
+ assertThat(event10.subject(), is(cNode2));
+ assertThat(event10.instanceType(), is(ClusterEvent.InstanceType.ONOS));
+
+ assertThat(event11.type(), is(ClusterEvent.Type.INSTANCE_REMOVED));
+ assertThat(event11.subject(), is(cNode2));
+ assertThat(event11.instanceType(), is(ClusterEvent.InstanceType.STORAGE));
+
+ assertThat(event12.type(), is(ClusterEvent.Type.INSTANCE_ACTIVATED));
+ assertThat(event12.subject(), is(cNode1));
+ assertThat(event12.instanceType(), is(ClusterEvent.InstanceType.ONOS));
+
+ assertThat(event13.type(), is(ClusterEvent.Type.INSTANCE_ACTIVATED));
+ assertThat(event13.subject(), is(cNode1));
+ assertThat(event13.instanceType(), is(ClusterEvent.InstanceType.STORAGE));
+
+ assertThat(event14.type(), is(ClusterEvent.Type.INSTANCE_READY));
+ assertThat(event14.subject(), is(cNode1));
+ assertThat(event14.instanceType(), is(ClusterEvent.InstanceType.ONOS));
+
+ assertThat(event15.type(), is(ClusterEvent.Type.INSTANCE_READY));
+ assertThat(event15.subject(), is(cNode1));
+ assertThat(event15.instanceType(), is(ClusterEvent.InstanceType.STORAGE));
+
+ assertThat(event16.type(), is(ClusterEvent.Type.INSTANCE_DEACTIVATED));
+ assertThat(event16.subject(), is(cNode1));
+ assertThat(event16.instanceType(), is(ClusterEvent.InstanceType.ONOS));
+
+ assertThat(event17.type(), is(ClusterEvent.Type.INSTANCE_DEACTIVATED));
+ assertThat(event17.subject(), is(cNode1));
+ assertThat(event17.instanceType(), is(ClusterEvent.InstanceType.STORAGE));
}
}
diff --git a/core/common/src/main/java/org/onosproject/codec/impl/ControllerNodeCodec.java b/core/common/src/main/java/org/onosproject/codec/impl/ControllerNodeCodec.java
index 07d8e45..e59de95 100644
--- a/core/common/src/main/java/org/onosproject/codec/impl/ControllerNodeCodec.java
+++ b/core/common/src/main/java/org/onosproject/codec/impl/ControllerNodeCodec.java
@@ -36,9 +36,10 @@
public ObjectNode encode(ControllerNode node, CodecContext context) {
checkNotNull(node, "Controller node cannot be null");
ClusterService service = context.getService(ClusterService.class);
+ IpAddress nodeIp = node.ip();
return context.mapper().createObjectNode()
.put("id", node.id().toString())
- .put("ip", node.ip().toString())
+ .put("ip", nodeIp != null ? nodeIp.toString() : node.host())
.put("tcpPort", node.tcpPort())
.put("status", service.getState(node.id()).toString())
.put("lastUpdate", Long.toString(service.getLastUpdatedInstant(node.id()).toEpochMilli()))
diff --git a/core/store/primitives/src/main/java/org/onosproject/store/atomix/cluster/impl/AtomixClusterStore.java b/core/store/primitives/src/main/java/org/onosproject/store/atomix/cluster/impl/AtomixClusterStore.java
index ca82891..f531746 100644
--- a/core/store/primitives/src/main/java/org/onosproject/store/atomix/cluster/impl/AtomixClusterStore.java
+++ b/core/store/primitives/src/main/java/org/onosproject/store/atomix/cluster/impl/AtomixClusterStore.java
@@ -47,6 +47,7 @@
import java.util.stream.Collectors;
import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Strings.isNullOrEmpty;
/**
* Atomix cluster store.
@@ -57,6 +58,8 @@
private static final String STATE_KEY = "state";
private static final String VERSION_KEY = "version";
+ private static final String TYPE_KEY = "type";
+ private static final String TYPE_ONOS = "onos";
private final Logger log = LoggerFactory.getLogger(getClass());
@@ -100,13 +103,14 @@
private void changeMembership(ClusterMembershipEvent event) {
ControllerNode node = nodes.get(NodeId.nodeId(event.subject().id().id()));
+ log.debug("Received a membership event {}", event);
switch (event.type()) {
case MEMBER_ADDED:
case METADATA_CHANGED:
if (node == null) {
node = toControllerNode(event.subject());
nodes.put(node.id(), node);
- notifyDelegate(new ClusterEvent(ClusterEvent.Type.INSTANCE_ADDED, node));
+ notifyDelegate(clusterEvent(ClusterEvent.Type.INSTANCE_ADDED, event.subject(), node));
}
updateVersion(node, event.subject());
updateState(node, event.subject());
@@ -114,8 +118,8 @@
case MEMBER_REMOVED:
if (node != null
&& states.put(node.id(), ControllerNode.State.INACTIVE) != ControllerNode.State.INACTIVE) {
- notifyDelegate(new ClusterEvent(ClusterEvent.Type.INSTANCE_DEACTIVATED, node));
- notifyDelegate(new ClusterEvent(ClusterEvent.Type.INSTANCE_REMOVED, node));
+ notifyDelegate(clusterEvent(ClusterEvent.Type.INSTANCE_DEACTIVATED, event.subject(), node));
+ notifyDelegate(clusterEvent(ClusterEvent.Type.INSTANCE_REMOVED, event.subject(), node));
}
break;
default:
@@ -129,13 +133,13 @@
if (states.put(node.id(), ControllerNode.State.ACTIVE) != ControllerNode.State.ACTIVE) {
log.info("Updated node {} state to {}", node.id(), ControllerNode.State.ACTIVE);
markUpdated(node.id());
- notifyDelegate(new ClusterEvent(ClusterEvent.Type.INSTANCE_ACTIVATED, node));
+ notifyDelegate(clusterEvent(ClusterEvent.Type.INSTANCE_ACTIVATED, member, node));
}
} else {
if (states.put(node.id(), ControllerNode.State.READY) != ControllerNode.State.READY) {
log.info("Updated node {} state to {}", node.id(), ControllerNode.State.READY);
markUpdated(node.id());
- notifyDelegate(new ClusterEvent(ClusterEvent.Type.INSTANCE_READY, node));
+ notifyDelegate(clusterEvent(ClusterEvent.Type.INSTANCE_READY, member, node));
}
}
}
@@ -170,7 +174,7 @@
public Set<Node> getStorageNodes() {
return membershipService.getMembers()
.stream()
- .filter(member -> !Objects.equals(member.properties().getProperty("type"), "onos"))
+ .filter(member -> !Objects.equals(member.properties().getProperty(TYPE_KEY), TYPE_ONOS))
.map(this::toControllerNode)
.collect(Collectors.toSet());
}
@@ -179,7 +183,7 @@
public Set<ControllerNode> getNodes() {
return membershipService.getMembers()
.stream()
- .filter(member -> Objects.equals(member.properties().getProperty("type"), "onos"))
+ .filter(member -> Objects.equals(member.properties().getProperty(TYPE_KEY), TYPE_ONOS))
.map(this::toControllerNode)
.collect(Collectors.toSet());
}
@@ -221,8 +225,9 @@
nodes.put(node.id(), node);
ControllerNode.State state = node.equals(localNode)
? ControllerNode.State.ACTIVE : ControllerNode.State.INACTIVE;
- membershipService.getMember(node.id().id()).properties().setProperty(STATE_KEY, state.name());
- notifyDelegate(new ClusterEvent(ClusterEvent.Type.INSTANCE_ADDED, node));
+ Member member = membershipService.getMember(node.id().id());
+ member.properties().setProperty(STATE_KEY, state.name());
+ notifyDelegate(clusterEvent(ClusterEvent.Type.INSTANCE_ADDED, member, node));
return node;
}
@@ -232,7 +237,20 @@
ControllerNode node = nodes.remove(nodeId);
if (node != null) {
states.remove(nodeId);
- notifyDelegate(new ClusterEvent(ClusterEvent.Type.INSTANCE_REMOVED, node));
+ notifyDelegate(clusterEvent(ClusterEvent.Type.INSTANCE_REMOVED,
+ membershipService.getMember(node.id().id()), node));
}
}
+
+ private ClusterEvent clusterEvent(ClusterEvent.Type type, Member member, ControllerNode node) {
+ // Atomix nodes do not set the property TYPE. Nowadays, the internal else is not used.
+ if (member != null && !isNullOrEmpty(member.properties().getProperty(TYPE_KEY))) {
+ if (Objects.equals(member.properties().getProperty(TYPE_KEY), TYPE_ONOS)) {
+ return new ClusterEvent(type, node, ClusterEvent.InstanceType.ONOS);
+ } else {
+ return new ClusterEvent(type, node, ClusterEvent.InstanceType.STORAGE);
+ }
+ }
+ return new ClusterEvent(type, node, ClusterEvent.InstanceType.STORAGE);
+ }
}