ONOS-4380 Refactor AlarmId and Alarm construction and update
Change-Id: I0117afda723ba27aadb1db306f7ce15b666f102d
diff --git a/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/api/AlarmStore.java b/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/api/AlarmStore.java
index 716449b..0b857f3 100644
--- a/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/api/AlarmStore.java
+++ b/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/api/AlarmStore.java
@@ -53,11 +53,12 @@
Collection<Alarm> getAlarms(DeviceId deviceId);
/**
- * Stores an alarm.
+ * Stores or updates an alarm.
*
* @param alarm alarm
*/
- void setAlarm(Alarm alarm);
+
+ void createOrUpdateAlarm(Alarm alarm);
/**
* Removes an alarm.
diff --git a/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/impl/AlarmManager.java b/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/impl/AlarmManager.java
index 7f78cda..bfe7627 100644
--- a/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/impl/AlarmManager.java
+++ b/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/impl/AlarmManager.java
@@ -16,7 +16,6 @@
package org.onosproject.faultmanagement.impl;
import com.google.common.collect.ImmutableSet;
-import org.apache.commons.collections.CollectionUtils;
import org.apache.felix.scr.annotations.Activate;
import org.apache.felix.scr.annotations.Component;
import org.apache.felix.scr.annotations.Deactivate;
@@ -95,33 +94,52 @@
return true;
}
+ //TODO maybe map for field to update ?
@Override
public Alarm updateBookkeepingFields(AlarmId id, boolean isAcknowledged, String assignedUser) {
-
Alarm found = store.getAlarm(id);
if (found == null) {
throw new ItemNotFoundException("Alarm with id " + id + " found");
}
-
Alarm updated = new DefaultAlarm.Builder(found)
.withId(found.id())
.withAcknowledged(isAcknowledged)
.withAssignedUser(assignedUser).build();
- store.setAlarm(updated);
+ store.createOrUpdateAlarm(updated);
return updated;
}
- public Alarm clear(AlarmId id) {
+ @Override
+ public Alarm updateBookkeepingFields(AlarmId id, boolean clear, boolean isAcknowledged,
+ String assignedUser) {
+ checkNotNull(id, "Alarm id is null");
Alarm found = store.getAlarm(id);
if (found == null) {
- log.warn("Alarm {} is not present", id);
- return null;
+ throw new ItemNotFoundException("Alarm with id " + id + " found");
}
- Alarm updated = new DefaultAlarm.Builder(found).withId(id).clear().build();
- store.setAlarm(updated);
+ long now = System.currentTimeMillis();
+ DefaultAlarm.Builder alarmBuilder = new DefaultAlarm.Builder(found).withTimeUpdated(now);
+ if (found.cleared() != clear) {
+ alarmBuilder.clear().withTimeCleared(now);
+ }
+ if (found.acknowledged() != isAcknowledged) {
+ alarmBuilder.withAcknowledged(isAcknowledged);
+ }
+ if (assignedUser != null && !found.assignedUser().equals(assignedUser)) {
+ alarmBuilder.withAssignedUser(assignedUser);
+ }
+ DefaultAlarm updated = alarmBuilder.build();
+ store.createOrUpdateAlarm(updated);
return updated;
}
+ //TODO move to AlarmAdminService
+ @Override
+ public void remove(AlarmId id) {
+ checkNotNull(id, "Alarm id is null");
+ store.removeAlarm(id);
+ }
+
@Override
public Map<Alarm.SeverityLevel, Long> getAlarmCounts(DeviceId deviceId) {
return getAlarms(deviceId).stream().collect(
@@ -185,35 +203,6 @@
return new InternalAlarmProviderService(provider);
}
- // Synchronised to prevent duplicate NE alarms being raised
- protected synchronized void updateAlarms(DeviceId deviceId, Set<Alarm> discoveredSet) {
- Set<Alarm> storedSet = getActiveAlarms(deviceId);
- log.debug("CurrentNeAlarms={}. DiscoveredAlarms={}", storedSet, discoveredSet);
-
- if (CollectionUtils.isEqualCollection(storedSet, discoveredSet)) {
- log.debug("No update for {}.", deviceId);
- return;
- }
- //TODO implement distinction between UPDATED and CLEARED ALARMS
- storedSet.stream().filter(
- (stored) -> (!discoveredSet.contains(stored))).forEach((stored) -> {
- log.debug("Alarm will be Cleared as it is not on the device. Cleared alarm: {}.", stored);
- clear(stored.id());
- });
-
- discoveredSet.stream().filter(
- (discovered) -> (!storedSet.contains(discovered))).forEach((discovered) -> {
- log.info("New alarm raised {}", discovered);
- AlarmId id = generateAlarmId();
- store.setAlarm(new DefaultAlarm.Builder(discovered).withId(id).build());
- });
- }
-
- //TODO improve implementation of AlarmId
- private AlarmId generateAlarmId() {
- return AlarmId.alarmId(alarmIdGenerator.incrementAndGet());
- }
-
private class InternalAlarmProviderService extends AbstractProviderService<AlarmProvider>
implements AlarmProviderService {
@@ -223,7 +212,7 @@
@Override
public void updateAlarmList(DeviceId deviceId, Collection<Alarm> alarms) {
- updateAlarms(deviceId, ImmutableSet.copyOf(alarms));
+ alarms.forEach(alarm -> store.createOrUpdateAlarm(alarm));
}
}
}
diff --git a/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/impl/DistributedAlarmStore.java b/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/impl/DistributedAlarmStore.java
index caea52b..db1052d 100644
--- a/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/impl/DistributedAlarmStore.java
+++ b/apps/faultmanagement/fmmgr/src/main/java/org/onosproject/faultmanagement/impl/DistributedAlarmStore.java
@@ -43,6 +43,7 @@
import java.util.Collection;
import java.util.Map;
+import java.util.Objects;
import java.util.stream.Collectors;
import static org.slf4j.LoggerFactory.getLogger;
@@ -109,13 +110,18 @@
public Collection<Alarm> getAlarms(DeviceId deviceId) {
//FIXME: this is expensive, need refactoring when core maps provide different indexes.
return ImmutableSet.copyOf(alarmsMap.values().stream()
- .filter(alarm -> alarm.deviceId().equals(deviceId))
- .collect(Collectors.toSet()));
+ .filter(alarm -> alarm.deviceId().equals(deviceId))
+ .collect(Collectors.toSet()));
}
@Override
- public void setAlarm(Alarm alarm) {
- alarms.put(alarm.id(), alarm);
+ public void createOrUpdateAlarm(Alarm alarm) {
+ Alarm existing = alarmsMap.get(alarm.id());
+ if (Objects.equals(existing, alarm)) {
+ log.info("Received identical alarm, no operation needed on {}", alarm.id());
+ } else {
+ alarms.put(alarm.id(), alarm);
+ }
}
@Override
@@ -136,7 +142,7 @@
alarm = mapEvent.newValue().value();
break;
case UPDATE:
- type = AlarmEvent.Type.CREATED;
+ type = AlarmEvent.Type.UPDATED;
alarm = mapEvent.newValue().value();
break;
case REMOVE:
diff --git a/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/AlarmManagerTest.java b/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/AlarmManagerTest.java
index 87cf28e..a7a2573 100644
--- a/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/AlarmManagerTest.java
+++ b/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/AlarmManagerTest.java
@@ -31,9 +31,14 @@
import org.onosproject.incubator.net.faultmanagement.alarm.AlarmEvent;
import org.onosproject.incubator.net.faultmanagement.alarm.AlarmId;
import org.onosproject.incubator.net.faultmanagement.alarm.AlarmListener;
+import org.onosproject.incubator.net.faultmanagement.alarm.AlarmProvider;
+import org.onosproject.incubator.net.faultmanagement.alarm.AlarmProviderRegistry;
+import org.onosproject.incubator.net.faultmanagement.alarm.AlarmProviderService;
import org.onosproject.incubator.net.faultmanagement.alarm.DefaultAlarm;
import org.onosproject.net.DeviceId;
+import org.onosproject.net.MastershipRole;
import org.onosproject.net.NetTestTools;
+import org.onosproject.net.provider.AbstractProvider;
import org.onosproject.store.service.TestStorageService;
import java.util.Collections;
@@ -46,6 +51,7 @@
import static org.junit.Assert.assertTrue;
import static org.onosproject.incubator.net.faultmanagement.alarm.Alarm.SeverityLevel.CLEARED;
import static org.onosproject.incubator.net.faultmanagement.alarm.Alarm.SeverityLevel.CRITICAL;
+import static org.onosproject.net.NetTestTools.PID;
/**
* Alarm manager test suite.
@@ -53,17 +59,28 @@
public class AlarmManagerTest {
private static final DeviceId DEVICE_ID = DeviceId.deviceId("foo:bar");
- private static final DefaultAlarm ALARM_A = new DefaultAlarm.Builder(
+ private static final String UNIQUE_ID_1 = "unique_id_1";
+ private static final String UNIQUE_ID_2 = "unique_id_2";
+ private static final AlarmId A_ID = AlarmId.alarmId(DEVICE_ID, UNIQUE_ID_1);
+ private static final AlarmId B_ID = AlarmId.alarmId(DEVICE_ID, UNIQUE_ID_2);
+ private static final DefaultAlarm ALARM_A = new DefaultAlarm.Builder(A_ID,
DEVICE_ID, "aaa", Alarm.SeverityLevel.CRITICAL, 0).build();
+ private static final DefaultAlarm ALARM_A_CLEARED = new DefaultAlarm.Builder(ALARM_A)
+ .clear().build();
+
private static final DefaultAlarm ALARM_A_WITHSRC = new DefaultAlarm.Builder(
ALARM_A).forSource(AlarmEntityId.alarmEntityId("port:foo")).build();
- private static final DefaultAlarm ALARM_B = new DefaultAlarm.Builder(
+ private static final DefaultAlarm ALARM_B = new DefaultAlarm.Builder(B_ID,
DEVICE_ID, "bbb", Alarm.SeverityLevel.CRITICAL, 0).build();
+
private AlarmManager manager;
private DistributedAlarmStore alarmStore;
+ private AlarmProviderService providerService;
+ private TestProvider provider;
+ protected AlarmProviderRegistry registry;
protected TestListener listener = new TestListener();
@Rule
@@ -75,15 +92,18 @@
TestUtils.setField(alarmStore, "storageService", new TestStorageService());
alarmStore.activate();
manager = new AlarmManager();
+ registry = manager;
manager.addListener(listener);
NetTestTools.injectEventDispatcher(manager, new TestEventDispatcher());
manager.store = alarmStore;
manager.activate();
+ provider = new TestProvider();
+ providerService = registry.register(provider);
}
@Test
public void deactivate() throws Exception {
- manager.updateAlarms(DEVICE_ID, ImmutableSet.of(ALARM_B, ALARM_A));
+ providerService.updateAlarmList(DEVICE_ID, ImmutableSet.of(ALARM_B, ALARM_A));
verifyGettingSetsOfAlarms(manager, 2, 2);
alarmStore.deactivate();
manager.removeListener(listener);
@@ -110,29 +130,30 @@
manager.getAlarm(null);
exception.expect(ItemNotFoundException.class);
- manager.getAlarm(AlarmId.alarmId(1));
+ manager.getAlarm(AlarmId.alarmId(DEVICE_ID, "unique_3"));
}
@Test
public void testAlarmUpdates() throws InterruptedException {
assertTrue("No alarms should be present", manager.getAlarms().isEmpty());
- manager.updateAlarms(DEVICE_ID, ImmutableSet.of());
+ providerService.updateAlarmList(DEVICE_ID, ImmutableSet.of());
assertTrue("No alarms should be present", manager.getAlarms().isEmpty());
Map<Alarm.SeverityLevel, Long> zeroAlarms = new CountsMapBuilder().create();
assertEquals("No alarms count should be present", zeroAlarms, manager.getAlarmCounts());
assertEquals("No alarms count should be present", zeroAlarms, manager.getAlarmCounts(DEVICE_ID));
- manager.updateAlarms(DEVICE_ID, ImmutableSet.of(ALARM_B, ALARM_A));
+ providerService.updateAlarmList(DEVICE_ID, ImmutableSet.of(ALARM_B, ALARM_A));
verifyGettingSetsOfAlarms(manager, 2, 2);
validateEvents(AlarmEvent.Type.CREATED, AlarmEvent.Type.CREATED);
Map<Alarm.SeverityLevel, Long> critical2 = new CountsMapBuilder().with(CRITICAL, 2L).create();
assertEquals("A critical should be present", critical2, manager.getAlarmCounts());
assertEquals("A critical should be present", critical2, manager.getAlarmCounts(DEVICE_ID));
- manager.updateAlarms(DEVICE_ID, ImmutableSet.of(ALARM_A));
+ Alarm updated = manager.updateBookkeepingFields(ALARM_A.id(), true, false, null);
+// providerService.updateAlarmList(DEVICE_ID, ImmutableSet.of(ALARM_A));
verifyGettingSetsOfAlarms(manager, 2, 1);
- validateEvents(AlarmEvent.Type.CREATED);
+ validateEvents(AlarmEvent.Type.UPDATED);
Map<Alarm.SeverityLevel, Long> critical1cleared1 =
new CountsMapBuilder().with(CRITICAL, 1L).with(CLEARED, 1L).create();
assertEquals("A critical should be present and cleared", critical1cleared1,
@@ -141,7 +162,7 @@
manager.getAlarmCounts(DEVICE_ID));
// No change map when same alarms sent
- manager.updateAlarms(DEVICE_ID, ImmutableSet.of(ALARM_A));
+ providerService.updateAlarmList(DEVICE_ID, ImmutableSet.of(updated));
verifyGettingSetsOfAlarms(manager, 2, 1);
validateEvents();
assertEquals("Map should not be changed for same alarm", critical1cleared1,
@@ -149,18 +170,19 @@
assertEquals("Map should not be changed for same alarm", critical1cleared1,
manager.getAlarmCounts(DEVICE_ID));
- manager.updateAlarms(DEVICE_ID, ImmutableSet.of(ALARM_A, ALARM_A_WITHSRC));
- verifyGettingSetsOfAlarms(manager, 3, 2);
- validateEvents(AlarmEvent.Type.CREATED);
+ providerService.updateAlarmList(DEVICE_ID, ImmutableSet.of(updated, ALARM_A_WITHSRC));
+ verifyGettingSetsOfAlarms(manager, 2, 2);
+ validateEvents(AlarmEvent.Type.UPDATED);
Map<Alarm.SeverityLevel, Long> critical2cleared1 =
- new CountsMapBuilder().with(CRITICAL, 2L).with(CLEARED, 1L).create();
+ new CountsMapBuilder().with(CRITICAL, 2L).create();
assertEquals("A critical should be present", critical2cleared1, manager.getAlarmCounts());
assertEquals("A critical should be present", critical2cleared1, manager.getAlarmCounts(DEVICE_ID));
- manager.updateAlarms(DEVICE_ID, ImmutableSet.of());
- verifyGettingSetsOfAlarms(manager, 3, 0);
- validateEvents(AlarmEvent.Type.CREATED, AlarmEvent.Type.CREATED);
- assertEquals(new CountsMapBuilder().with(CLEARED, 3L).create(), manager.getAlarmCounts(DEVICE_ID));
+ providerService.updateAlarmList(DEVICE_ID, ImmutableSet.of());
+ verifyGettingSetsOfAlarms(manager, 2, 2);
+ validateEvents();
+ assertEquals(new CountsMapBuilder().with(CRITICAL, 2L).create(),
+ manager.getAlarmCounts(DEVICE_ID));
assertEquals("The counts should be empty for unknown devices", zeroAlarms,
manager.getAlarmCounts(DeviceId.NONE));
@@ -207,6 +229,19 @@
}
+ private class TestProvider extends AbstractProvider implements AlarmProvider {
+ private DeviceId deviceReceived;
+ private MastershipRole roleReceived;
+
+ public TestProvider() {
+ super(PID);
+ }
+
+ @Override
+ public void triggerProbe(DeviceId deviceId) {
+ }
+ }
+
/**
* Test listener class to receive alarm events.
*/
diff --git a/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/DistributedAlarmStoreTest.java b/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/DistributedAlarmStoreTest.java
index 34fb320..1aa8a56 100644
--- a/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/DistributedAlarmStoreTest.java
+++ b/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/DistributedAlarmStoreTest.java
@@ -20,6 +20,7 @@
import org.junit.Before;
import org.junit.Test;
import org.onosproject.incubator.net.faultmanagement.alarm.Alarm;
+import org.onosproject.incubator.net.faultmanagement.alarm.AlarmId;
import org.onosproject.incubator.net.faultmanagement.alarm.DefaultAlarm;
import org.onosproject.net.DeviceId;
import org.onosproject.store.service.TestStorageService;
@@ -33,7 +34,9 @@
public class DistributedAlarmStoreTest {
private DistributedAlarmStore alarmStore;
private static final DeviceId DEVICE_ID = DeviceId.deviceId("foo:bar");
- private static final DefaultAlarm ALARM_A = new DefaultAlarm.Builder(
+ private static final String UNIQUE_ID_1 = "unique_id_1";
+ private static final AlarmId A_ID = AlarmId.alarmId(DEVICE_ID, UNIQUE_ID_1);
+ private static final DefaultAlarm ALARM_A = new DefaultAlarm.Builder(A_ID,
DEVICE_ID, "aaa", Alarm.SeverityLevel.CRITICAL, 0).build();
/**
@@ -61,7 +64,7 @@
*/
@Test
public void basics() {
- alarmStore.setAlarm(ALARM_A);
+ alarmStore.createOrUpdateAlarm(ALARM_A);
assertTrue("There should be one alarm in the set.",
alarmStore.getAlarms().contains(ALARM_A));
assertTrue("The same alarm should be returned.",
diff --git a/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/PollingAlarmProviderTest.java b/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/PollingAlarmProviderTest.java
index b96af16..72d8c3c 100644
--- a/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/PollingAlarmProviderTest.java
+++ b/apps/faultmanagement/fmmgr/src/test/java/org/onosproject/faultmanagement/impl/PollingAlarmProviderTest.java
@@ -26,6 +26,7 @@
import org.onosproject.cluster.RoleInfo;
import org.onosproject.incubator.net.faultmanagement.alarm.Alarm;
import org.onosproject.incubator.net.faultmanagement.alarm.AlarmConsumer;
+import org.onosproject.incubator.net.faultmanagement.alarm.AlarmId;
import org.onosproject.incubator.net.faultmanagement.alarm.AlarmProvider;
import org.onosproject.incubator.net.faultmanagement.alarm.AlarmProviderRegistry;
import org.onosproject.incubator.net.faultmanagement.alarm.AlarmProviderRegistryAdapter;
@@ -96,8 +97,9 @@
private final DeviceEvent deviceEvent =
new DeviceEvent(DeviceEvent.Type.DEVICE_AVAILABILITY_CHANGED, device);
-
- private static final DefaultAlarm ALARM = new DefaultAlarm.Builder(
+ private static final String UNIQUE_ID_1 = "unique_id_1";
+ private static final AlarmId A_ID= AlarmId.alarmId(DEVICE_ID, UNIQUE_ID_1);
+ private static final DefaultAlarm ALARM = new DefaultAlarm.Builder(A_ID,
DEVICE_ID, "aaa", Alarm.SeverityLevel.CRITICAL, 0).build();
private final Driver driver = new MockDriver();