feat(snmp_zabbix): use WalkAll for SNMPv1 discovery #1377
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for SNMPv1 devices in the discovery process by using WalkAll (GETNEXT) instead of BulkWalkAll (GETBULK), which is not supported in SNMPv1. The change also adds a fallback mechanism for non-v1 versions when BulkWalkAll fails.
Key changes:
- Added SNMPv1 version detection to use appropriate SNMP walk method
- Implemented fallback from
BulkWalkAlltoWalkAllfor error recovery in SNMPv2/v3
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kongfei605
force-pushed
the
snmp_zbx_v1
branch
from
2031e7b to
c22ec9d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
inputs/snmp_zabbix/discovery.go
Outdated
| var pdus []gosnmp.SnmpPDU | ||
| var err error | ||
|
|
||
| // SNMPv1 do not support GETBULK，use WalkAll (GETNEXT) |
There was a problem hiding this comment.
Mixed punctuation: The comment uses a Chinese comma (，) instead of a standard comma (,). Should be: 'GETBULK, use WalkAll'.
| // SNMPv1 do not support GETBULK，use WalkAll (GETNEXT) | |
| // SNMPv1 do not support GETBULK, use WalkAll (GETNEXT) |
inputs/snmp_zabbix/discovery.go
Outdated
| if err != nil { | ||
| pdusRetry, errRetry := client.WalkAll(oid) | ||
| if errRetry == nil { | ||
| pdus = pdusRetry | ||
| err = nil | ||
| } else { | ||
| err = fmt.Errorf("bulk walk failed: %v, fallback walk failed: %w", err, errRetry) | ||
| } | ||
| } |
There was a problem hiding this comment.
This fallback logic is duplicated in three places (lines 228-236, 280-289, 392-400). Consider extracting this into a helper function to reduce code duplication and improve maintainability.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
inputs/snmp_zabbix/discovery.go
Outdated
| if client.Version == gosnmp.Version1 { | ||
| pdus, err = client.WalkAll(oid) | ||
| } else { | ||
| pdus, err = client.BulkWalkAll(oid) | ||
| if err != nil { | ||
| pdusRetry, errRetry := client.WalkAll(oid) | ||
| if errRetry == nil { | ||
| pdus = pdusRetry | ||
| err = nil | ||
| } else { | ||
| err = fmt.Errorf("bulk walk failed: %v, fallback walk failed: %w", err, errRetry) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The same version check and fallback logic is duplicated across three functions (performMultiOidWalkDiscovery, performZabbixDependentDiscovery, and performStandardDiscovery). Consider extracting this into a helper function to improve maintainability and reduce code duplication. For example, a function like performWalkWithFallback(client *gosnmp.GoSNMP, oid string) ([]gosnmp.SnmpPDU, error) would centralize this logic.
inputs/snmp_zabbix/discovery.go
Outdated
| err = nil | ||
| } else { | ||
| err = fmt.Errorf("bulk walk failed: %v, fallback walk failed: %w", err, errRetry) | ||
| log.Printf("W!: %v", err) |
There was a problem hiding this comment.
The error is logged here but not in the other two places where the same fallback logic is used (lines 234 and 398). This inconsistency makes debugging more difficult. Either remove this logging statement for consistency, or add it to the other locations as well.
| log.Printf("W!: %v", err) |
kongfei605
force-pushed
the
snmp_zbx_v1
branch
from
5dcfe67 to
22f8477
Compare
1 participant
No description provided.