prevent host ignore on NoSuchObject for system OIDs - #553
Conversation
|
Thanks for this — the intent is right, the system MIB group should not mark a host ignored on The current diff has a doubled operator so it does not compile: CI has not run on this PR, so nothing has told you that until now. Here is the corrected form. I built it on if (vars->type == SNMP_NOSUCHOBJECT) {
if (!strstr(snmp_oid, ".1.3.6.1.2.1.1.1.0") && !strstr(snmp_oid, ".1.3.6.1.2.1.1.2.0") &&
!strstr(snmp_oid, ".1.3.6.1.2.1.1.3.0") && !strstr(snmp_oid, ".1.3.6.1.2.1.1.4.0") &&
!strstr(snmp_oid, ".1.3.6.1.2.1.1.5.0") && !strstr(snmp_oid, ".1.3.6.1.2.1.1.6.0")) {That covers sysDescr, sysObjectID, sysUpTime, sysContact, sysName and sysLocation. Push that to your branch and the build job should go green. One question while you are here: is this the spine-side counterpart of Cacti/cacti#7510 and #7557? If the underlying problem is the same one resolved in Cacti/cacti#7559, it is worth saying so on this PR so the two get looked at together. |
|
Thanks for this, the diagnosis is solid and the log trace makes the failure easy to follow. Three things before it can go in. It does not compile. There is a doubled operator in the new condition: … ".1.3.6.1.2.1.1.3.0") && && !strstr(snmp_oid, ".1.3.6.1.2.1.1.2.0") …
^^^^Building develop with the patch applied gives The branch you patched may not be the one you hit.
The line in your log is the second one: That message only exists at line 543, in the branch this patch does not touch. So either the second branch needs the same treatment, or the reproduction is worth re-checking. There may be a smaller fix that covers both. Two smaller notes if you keep the list approach: Happy to help get it over the line, or to pick it up with credit to you if you would rather not carry it. |
somethingwithproof
left a comment
There was a problem hiding this comment.
Thanks for tracking this down, and sorry it sat this long.
The diagnosis matches what the code does. get_system_information() queries the whole system group, but snmp_get_base() only exempts sysDescr and sysUpTime from STAT_ERROR on SNMP_NOSUCHOBJECT. A device that does not implement sysObjectID, sysContact, sysName or sysLocation therefore fails availability and gets ignored for everything else as well, which is why the resource OIDs stop being polled. With Poller Threads at 1 there is no second thread to mask it.
Three things before this can go in.
CI has never run on this PR. Fork pull requests need a maintainer to approve the workflow run, and I only have triage here, so @netniV or @TheWitness will need to do that. Nothing else is worth acting on until there is a green build.
The debug message immediately below the change still reads SNMP_NOSUCHOBJECT not sysDesc or sysUptime, which is now wrong for four of the six OIDs.
strstr() is a substring match rather than an exact one, so any OID that merely contains .1.3.6.1.2.1.1.5.0 also takes the exempt path. That is pre-existing and not something you introduced, but going from two OIDs to six is a good moment to fix it. A static array of the six system OIDs compared exactly would also read better than six chained !strstr calls on one line, and it would sit consistently with snmp_varbind_is_exception() a few lines above.
develop now carries a cmocka harness under tests/unit if you want to pin the predicate down with a test. Happy to help wire that up.
|
Thank you for the detailed review and for taking the time to analyze the different code paths. I really appreciate the feedback and suggestions. Regarding Cacti/cacti#7510, yes, this PR was intended to address the Spine-side aspect of the same overall issue. I encountered several production devices where the issue not only affected system information collection, but also completely prevented resource polling because the host became ignored after a NoSuchObject response from one of the system OIDs. In our environment, there are devices that do not implement one or more of sysObjectID, sysContact, sysName, or sysLocation. When that happens, the device can end up with "0 to N sources" polled even though resource-related OIDs are available and respond correctly. In other words, this issue can result in a device becoming effectively unmonitorable even though its CPU, memory, interface, and other resource OIDs are fully accessible via SNMP. That operational impact was the primary reason for opening this PR. Based on your comments, I believe my original approach may have been addressing the symptom inside snmp_get_base(), while your suggestion points toward a cleaner and more maintainable design. Would the following approach be preferable? In get_system_information(), change the system OID queries from: to: The intention is to explicitly treat all system information OIDs as non-fatal during system information collection, allowing polling to continue even when a device returns SNMP_NOSUCHOBJECT for one of them. If this approach is preferred, I would revert the additional OID exception conditions that I added in snmp_get_base() and keep that function free from a growing list of optional OIDs. This seems more consistent with the purpose of the existing should_fail parameter. My goal is to address the issue without maintaining a growing list of optional OIDs inside snmp_get_base(). Would this be a better direction for the fix? |
somethingwithproof
left a comment
There was a problem hiding this comment.
I traced what this fixes, since the one-line diff does not show it.
get_system_information() polls all six OIDs of the system group:
.1.3.6.1.2.1.1.1.0 sysDescr
.1.3.6.1.2.1.1.2.0 sysObjectID
.1.3.6.1.2.1.1.3.0 sysUpTime
.1.3.6.1.2.1.1.4.0 sysContact
.1.3.6.1.2.1.1.5.0 sysName
.1.3.6.1.2.1.1.6.0 sysLocation
The exemption being widened here covered only the first and third. For the other four the consequence is not a missing inventory field, it is the whole device:
/* snmp.c, snmp_get_base() */
if (vars->type == SNMP_NOSUCHOBJECT) {
if (!strstr(snmp_oid, ".1.3.6.1.2.1.1.1.0") && !strstr(snmp_oid, ".1.3.6.1.2.1.1.3.0")) {
SET_UNDEFINED(result_string);
status = STAT_ERROR;and every one of those polls arrives through snmp_get(), which is snmp_get_base(current_host, snmp_oid, true), so:
if (status != STAT_SUCCESS && should_fail) {
current_host->ignore_host = TRUE;
}A device that simply does not implement sysObjectID, sysContact, sysName or sysLocation is therefore marked ignored and skipped for the rest of the cycle. Those four are optional in practice and plenty of embedded agents omit them. Exempting the whole system group is the consistent behaviour, and it matches what the two already-exempt OIDs get.
Confirmed against develop: all six are still polled by get_system_information(), snmp_get() still passes should_fail = true, and STAT_ERROR still reaches ignore_host. The reasoning holds on current code.
Two notes for whoever merges
The condition uses strstr, so it matches an OID that merely contains one of these strings rather than equals it. That is pre-existing style in this branch and this change follows it, so it is not a reason to hold the PR; worth a separate tidy if anyone cares.
There are no CI checks on this PR. It predates the workflows, and pushing an empty commit or rebasing it onto current develop would give it a run. It does not conflict with anything currently open, including #597, which I checked with git merge-tree.
Not a maintainer, so this is not an approving review. The change reads correct to me and the impact is larger than the diff suggests.
There was a problem hiding this comment.
🟡 Changes recommended
snmp_get_allow_fail() currently passes should_fail=true (same as snmp_get()), so it does not prevent ignore_host behavior and can regress the previously non-fatal modern-uptime query path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aims to prevent Spine from setting ignore_host=TRUE (and thereby skipping all subsequent polling) when certain system OID lookups in get_system_information() return NoSuchObject, so resource/data-source polling can still proceed even if optional system fields are missing.
Changes:
- Added a new SNMP helper API (
snmp_get_allow_fail) intended for “non-fatal” SNMP GETs. - Switched system OID queries in
get_system_information()to usesnmp_get_allow_fail(including sysObjectID/sysContact/sysName/sysLocation). - Adjusted debug log messages to reflect the new helper name.
File summaries
| File | Description |
|---|---|
| snmp.h | Declares the new snmp_get_allow_fail() API. |
| snmp.c | Defines snmp_get_allow_fail() (currently identical to snmp_get()). |
| poller.c | Routes system OID collection through snmp_get_allow_fail() to avoid aborting polling when system OIDs are unavailable. |
Review details
Suppressed comments (1)
poller.c:2186
- Log prefix typo: "DEVDGB" should be "DEVDBG" for consistency and greppability with other debug messages.
SPINE_LOG_DEVDBG(("DEVDGB: Device[%d] poll_result = snmp_get_allow_fail(host, '.1.3.6.1.6.3.10.2.1.3.0'); [complete]", host->id));
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| char *snmp_get_allow_fail(host_t *current_host, const char *snmp_oid) { | ||
| return snmp_get_base(current_host, snmp_oid, true); | ||
| } |
| SPINE_LOG_DEVDBG(("DEVDGB: Device[%d] poll_result = snmp_get(host, '.1.3.6.1.2.1.1.3.0'); [complete]", host->id)); | ||
| SPINE_LOG_DEVDBG(("DEVDBG: Device[%d] poll_result = snmp_get_allow_fail(host, '.1.3.6.1.2.1.1.3.0');", host->id)); | ||
| poll_result = snmp_get_allow_fail(host, ".1.3.6.1.2.1.1.3.0"); | ||
| SPINE_LOG_DEVDBG(("DEVDGB: Device[%d] poll_result = snmp_get_allow_fail(host, '.1.3.6.1.2.1.1.3.0'); [complete]", host->id)); |
Problem Description
When a device does not support one or more of the system OIDs queried by get_system_information(), not only system information collection but also resource-related SNMP polling may fail.
This issue is especially noticeable when the device's Poller Thread setting is 1, where resource-related SNMP polling does not occur at all.
During analysis, I found that if one of the OIDs queried in get_system_information() returns NoSuchObject, the host is marked as ignored through the following call path:
snmp_get()
-> snmp_get_base(current_host, snmp_oid, true)
-> NoSuchObject returned
-> current_host->ignore_host = TRUE
The OIDs queried by get_system_information() are:
Issue #366 already introduced special handling for the following OIDs:
Reference:
Issue #366: Failing to actually poll a device that returns "NoSuchObject"
However, the same handling was not applied to:
As a result, devices that do not implement one or more of these OIDs can still be marked as ignored, causing polling to stop unexpectedly.
Observed Behavior
The following log shows the issue occurring when the target device returns NoSuchObject for sysContact (.1.3.6.1.2.1.1.4.0).
After the error is detected, ignore_host becomes active and subsequent system OID requests are skipped. The device polling thread then completes without polling any of the scheduled data sources.
In this example, the device contains 17 polling items, but polling completes with:
INFO: Device[72] Thread complete and 0 to 17 sources
indicating that none of the scheduled data sources were successfully polled.
Fix
This change adds the same exception handling for the remaining four OIDs:
If any of these OIDs return NoSuchObject, the host will no longer be marked as ignored, allowing normal resource-related polling to continue.
Expected Result
Devices that do not implement one or more optional system OIDs should still be polled successfully for resource-related metrics.
Only the unavailable system information fields should be skipped, while normal SNMP data collection continues without interruption.