NMS-19979: clear data collection alarms after a restart or collectd reload - #8774
NMS-19979: clear data collection alarms after a restart or collectd reload#8774marshallmassengill wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses NMS-19979 by ensuring dataCollectionFailed alarms can be cleared when collection recovers across an OpenNMS restart or a collectd reload, by seeding each newly-built CollectableService’s initial in-memory collection status from outstanding (uncleared) data collection failure alarms.
Changes:
- Add alarm-backed “outstanding collection failure” seeding in
Collectdand pass an initialCollectionStatusinto eachCollectableService. - Extend
CollectableServiceto accept aninitialStatus, preserving transition semantics sodataCollectionSucceededis emitted when appropriate. - Add/extend test coverage: unit tests for seeded status behavior and a new integration test validating the alarm query against a real DB.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| opennms-services/src/main/java/org/opennms/netmgt/collectd/Collectd.java | Seed initial collection status from uncleared dataCollectionFailed alarms and pass it into scheduled services. |
| opennms-services/src/main/java/org/opennms/netmgt/collectd/CollectableService.java | Accept an injected initial CollectionStatus instead of always starting at SUCCEEDED. |
| opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectableServiceTest.java | Add tests asserting event emission behavior for seeded FAILED vs SUCCEEDED initial status. |
| opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectdOutstandingFailuresIT.java | New IT validating the alarm query used for seeding works against a real database. |
| opennms-services/src/test/java/org/opennms/netmgt/collectd/ThresholdIT.java | Wire AlarmDao into Collectd in the test setup. |
| opennms-services/src/test/java/org/opennms/netmgt/collectd/DuplicatePrimaryAddressIT.java | Provide AlarmDao to Collectd (mocked) to satisfy new dependency. |
| opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectdMoreIT.java | Provide AlarmDao to Collectd (mocked) to satisfy new dependency. |
| opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectdIT.java | Provide AlarmDao to Collectd (mocked) to satisfy new dependency. |
| opennms-services/src/test/java/org/opennms/netmgt/collectd/CollectdExternalIT.java | Wire AlarmDao into Collectd in the test setup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Why not simplify this to clear all |
And this is exactly why I opened this as a draft. Honestly... I'm not sure I can come up with a reason not to do that other than keeping alarms through reboots somehow seems important to me and feels like a breaking change... which is also why I'm not sure if this is a bug or an enhancement since it's changing behavior that's been like this for so long. |
|
If that is the concern, you could unconditionally emit a |
That's not bad... I suspect we would want to add something to vacuumd to clear those after some amount of time if they aren't picked up by the defaults. I'd have to look at it. I'll test this one. That's definitely simple enough. |
Ok... I don't think this one makes sense when you're doing something like Chance and have collections going at scale. That could bog things down quite a bit with a lot of extra events. |
|
If you set |
|
Pushed an update to see if we can get this simplified a bit based on @dino2gnt's suggestion. |
a5270b1 to
1e0cc0c
Compare
cgorantla
left a comment
cgorantla
left a comment
There was a problem hiding this comment.
This looks clean.
On develop, we should add packageName to dataCollectionFailed and dataCollectionSucceeded events so that we could differentiate between failures on a given package ( may be because of specific parameters)
…eload 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.
1e0cc0c to
b9e31bc
Compare
…vent 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.
I'll see about getting an NMS open for that once this gets merged. It does seem like a useful addition. |
My attempt at a fix for the collectd reload bug. A dataCollectionFailed alarm never clears if collection recovers 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 is no longer a transition, no dataCollectionSucceeded is sent, and the alarm is orphaned. Seed it to UNKNOWN instead.
Tests: two cases in CollectableServiceTest, one for the transition on first success and one confirming later successful cycles stay quiet.
Assisted by Anthropic Claude Opus 5.
External References