From b9e31bc5d3557bf07cb1d8cfca99a3bab9feee66 Mon Sep 17 00:00:00 2001 From: Marshall Massengill Date: Wed, 12 Aug 2026 16:28:14 -0400 Subject: [PATCH 1/2] NMS-19979: clear data collection alarms after a restart or collectd reload A dataCollectionFailed alarm never cleared if collection recovered across an OpenNMS restart or a collectd configuration reload. CollectableService keeps the last collection status in memory and updateStatus() only emits an event on a transition. That status was seeded to SUCCEEDED, and both a restart and rebuildScheduler() discard and rebuild every CollectableService, so the recovery was no longer a transition, no dataCollectionSucceeded was sent, and the alarm was orphaned. Seed it to UNKNOWN instead. The first collection then always transitions, whichever way it goes: a success emits dataCollectionSucceeded and clears the alarm the previous generation left behind, and a failure emits dataCollectionFailed exactly as before. Subsequent collections are unchanged, so this costs one extra event per collected service per restart or reload, not one per collection cycle. An unmatched dataCollectionSucceeded creates a Normal severity alarm of its own, which the default alarmd cleanUp rule deletes after five minutes. --- .../netmgt/collectd/CollectableService.java | 5 ++- .../collectd/CollectableServiceTest.java | 42 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) 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<>(); From 9b5337565e5a988e99504c75c840dfc741df6b26 Mon Sep 17 00:00:00 2001 From: Marshall Massengill Date: Thu, 13 Aug 2026 11:24:06 -0400 Subject: [PATCH 2/2] NMS-19979: Update collectd tests for the first-collection succeeded event Seeding m_status to UNKNOWN makes the first successful collection of every scheduling generation a transition, so it now emits dataCollectionSucceeded. Two existing tests assumed that event never fired. ThresholdIT anticipates it once per generation. A nodeCategoryMembershipChanged event unschedules and reschedules the node, building a fresh CollectableService, so the reschedule reports success again after the second category change. CollectdIT.testOneMatchingSpec performs real collections, so it accounts for the send explicitly rather than loosening tearDown's verifyNoMoreInteractions. --- .../org/opennms/netmgt/collectd/CollectdIT.java | 4 ++++ .../org/opennms/netmgt/collectd/ThresholdIT.java | 15 +++++++++++++++ 2 files changed, 19 insertions(+) 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));