diff --git a/opennms-services/src/main/java/org/opennms/netmgt/collectd/CollectableService.java b/opennms-services/src/main/java/org/opennms/netmgt/collectd/CollectableService.java index 271be369c3cf..bcc0ee41b8dc 100644 --- a/opennms-services/src/main/java/org/opennms/netmgt/collectd/CollectableService.java +++ b/opennms-services/src/main/java/org/opennms/netmgt/collectd/CollectableService.java @@ -156,7 +156,10 @@ protected CollectableService(OnmsIpInterface iface, IpInterfaceDao ifaceDao, Col m_persisterFactory = persisterFactory; m_nodeId = iface.getNode().getId().intValue(); - m_status = CollectionStatus.SUCCEEDED; + // Start out with no opinion rather than assuming success, so the first successful collection is a + // transition and emits the event that clears an alarm left over from a previous scheduling + // generation. A restart and a collectd reload both discard and rebuild every CollectableService. + m_status = CollectionStatus.UNKNOWN; m_updates = new CollectorUpdates(); diff --git a/opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectableServiceTest.java b/opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectableServiceTest.java index 3dcc45e188ff..407b345f2405 100644 --- a/opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectableServiceTest.java +++ b/opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectableServiceTest.java @@ -61,6 +61,8 @@ import org.opennms.netmgt.dao.api.IpInterfaceDao; import org.opennms.netmgt.dao.api.ResourceStorageDao; import org.opennms.netmgt.dao.mock.MockEventIpcManager; +import org.opennms.netmgt.events.api.EventConstants; +import org.opennms.netmgt.events.api.EventIpcManager; import org.opennms.netmgt.events.api.EventIpcManagerFactory; import org.opennms.netmgt.model.OnmsIpInterface; import org.opennms.netmgt.rrd.RrdRepository; @@ -69,6 +71,7 @@ import org.opennms.netmgt.scheduler.Scheduler; import org.opennms.netmgt.snmp.InetAddrUtils; import org.opennms.netmgt.threshd.api.ThresholdingService; +import org.opennms.netmgt.xml.event.Event; import org.opennms.test.FileAnticipator; import org.springframework.transaction.PlatformTransactionManager; @@ -245,6 +248,45 @@ public void thresholdingSessionIsCreatedWhenExplicitlyEnabled() throws Exception verify(thresholdingService, times(1)).createSession(anyInt(), any(), any(), any()); } + /** + * A CollectableService starts with no known status, so the first successful collection is a transition + * and emits dataCollectionSucceeded. That is what clears a dataCollectionFailed alarm raised before a + * restart or a collectd reload, both of which rebuild every CollectableService. See NMS-19979. + */ + @Test + public void sendsSucceededEventOnFirstSuccessfulCollection() throws CollectionInitializationException, CollectionException, IOException { + EventIpcManager eventIpcManager = mock(EventIpcManager.class); + EventIpcManagerFactory.setIpcManager(eventIpcManager); + + createCollectableService(); + when(spec.collect(any())).thenReturn(null); + + service.run(); + + ArgumentCaptor eventCaptor = ArgumentCaptor.forClass(Event.class); + verify(eventIpcManager, times(1)).sendNow(eventCaptor.capture()); + assertEquals(EventConstants.DATA_COLLECTION_SUCCEEDED_EVENT_UEI, eventCaptor.getValue().getUei()); + } + + /** + * Only the transition emits, so a service that keeps collecting successfully stays quiet after the + * first pass rather than sending an event per collection cycle. + */ + @Test + public void sendsNoFurtherEventWhileCollectionKeepsSucceeding() throws CollectionInitializationException, CollectionException, IOException { + EventIpcManager eventIpcManager = mock(EventIpcManager.class); + EventIpcManagerFactory.setIpcManager(eventIpcManager); + + createCollectableService(); + when(spec.collect(any())).thenReturn(null); + + service.run(); + service.run(); + service.run(); + + verify(eventIpcManager, times(1)).sendNow(any(Event.class)); + } + private void createCollectableService() throws CollectionInitializationException, IOException { // Disable thresholding Map paramsMap = new HashMap<>(); diff --git a/opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectdIT.java b/opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectdIT.java index fca74478ddef..dc1f46886cac 100644 --- a/opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectdIT.java +++ b/opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectdIT.java @@ -88,6 +88,7 @@ import org.opennms.netmgt.scheduler.mock.MockScheduler; import org.opennms.netmgt.threshd.api.ThresholdingService; import org.opennms.netmgt.threshd.api.ThresholdingSession; +import org.opennms.netmgt.xml.event.Event; import org.opennms.test.JUnitConfigurationEnvironment; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.core.io.ClassPathResource; @@ -336,6 +337,9 @@ public void testOneMatchingSpec() throws Exception { verify(m_eventIpcManager, times(1)).addEventListener(eq(m_collectd), (Collection)isA(Collection.class)); verify(m_eventIpcManager, times(1)).removeEventListener(m_collectd); + // This test actually collects, and the first success transitions out of UNKNOWN and emits + // dataCollectionSucceeded. Account for it here so tearDown's verifyNoMoreInteractions passes. + verify(m_eventIpcManager, times(1)).sendNow(isA(Event.class)); } /** diff --git a/opennms-services/src/test/java/org/opennms/netmgt/collectd/ThresholdIT.java b/opennms-services/src/test/java/org/opennms/netmgt/collectd/ThresholdIT.java index a88e5d8356c0..81dc710ffec4 100644 --- a/opennms-services/src/test/java/org/opennms/netmgt/collectd/ThresholdIT.java +++ b/opennms-services/src/test/java/org/opennms/netmgt/collectd/ThresholdIT.java @@ -204,6 +204,10 @@ public void canTriggerThreshold() throws Exception { EventAnticipator eventAnticipator = mockEventIpcManager.getEventAnticipator(); + // Each scheduling generation starts out UNKNOWN, so its first successful collection emits this + // once. Anticipate before the service is scheduled below, or the collection can beat us to it. + anticipateDataCollectionSucceeded(eventAnticipator); + // Let's send a nodeGainedService event EventBuilder bldr = new EventBuilder(EventConstants.NODE_GAINED_SERVICE_EVENT_UEI, "Test"); bldr.setNodeid(1); @@ -273,6 +277,9 @@ public void canTriggerThreshold() throws Exception { eventAnticipator.reset(); + // The category change above rescheduled the service, so a fresh generation reports success again. + anticipateDataCollectionSucceeded(eventAnticipator); + // Again, Assert 2 collections are performed and that Threshold is no longer triggered collector.resetLatch(2); if (!collector.getLatch().await(30, TimeUnit.SECONDS)) { @@ -284,6 +291,14 @@ public void canTriggerThreshold() throws Exception { collectd.stop(); } + private static void anticipateDataCollectionSucceeded(EventAnticipator eventAnticipator) { + EventBuilder bldr = new EventBuilder(EventConstants.DATA_COLLECTION_SUCCEEDED_EVENT_UEI, "OpenNMS.Collectd"); + bldr.setNodeid(1); + bldr.setInterface(addr("192.168.1.1")); + bldr.setService("Mock"); + eventAnticipator.anticipateEvent(bldr.getEvent()); + } + private void initThreshdFactories(String threshd, String thresholds) throws Exception { thresholdingDao.overrideConfig(getClass().getResourceAsStream(thresholds)); threshdDao.overrideConfig(getClass().getResourceAsStream(threshd));