Closed candlerb closed 1 year ago
SuperQ
commented
1 year ago I noticed this too, but haven't had time to dig into it.
Changing the generator config doesn't affect parse errors. Only changing the contents of the MIBs do. The parse errors happen before the config is used.
candlerb
commented
1 year ago Possibly to do with #508 ?
candlerb
commented
1 year ago I think it's partly to do with #508. I made this patch:
diff --git a/generator/main.go b/generator/main.go
index 113dde0..15d7fe7 100644
--- a/generator/main.go
+++ b/generator/main.go
@@ -116,7 +116,7 @@ func main() {
parseOutput = strings.TrimSpace(parseOutput)
parseErrors := len(parseOutput) != 0
if parseErrors {
- level.Warn(logger).Log("msg", "NetSNMP reported parse error(s)", "errors", len(strings.Split(parseOutput, "\n")))
+ level.Warn(logger).Log("msg", "NetSNMP reported parse error(s)", "errors", len(strings.Split(parseOutput, "\n")), "text", parseOutput)
}
nodes := getMIBTree()And now I get this output:
ts=2023-04-14T15:36:35.850Z caller=main.go:119 level=warn msg="NetSNMP reported parse error(s)" errors=2 text="No log handling enabled - using stderr logging\nDid not find 'DisplayString' in module #-1 (mibs/CyberPower.MIB)"So the message "No log handling enabled - using stderr logging" is a spurious warning and can be ignored, but the one about DisplayString is real.
Also, I can replicate that using snmptranslate:
$ MIBDIRS=mibs MIBS=: snmptranslate -On CPS-MIB::upsBaseIdentModel
No log handling enabled - using stderr logging
Did not find 'DisplayString' in module #-1 (mibs/CyberPower.MIB)
.1.3.6.1.4.1.3808.1.1.1.1.1.1OK, I think I found the problem:
$ grep -R RFC1213 mibs
mibs/ISDN-MIB: FROM RFC1213-MIB;
mibs/apc-powernet-mib: DisplayString FROM RFC1213-MIB
mibs/CyberPower.MIB: DisplayString FROM RFC1213-MIBAll the other MIBs import DisplayString from SNMPv2-TC. (In fact, ISDN-MIB does too. It's importing transmission from RFC1213-MIB).
The straightforward solution, then, is to fetch this file from https://raw.githubusercontent.com/net-snmp/net-snmp/v5.9/mibs/RFC1213-MIB.txt
I can do a PR for that. It makes the generator problem go away (no error is returned).
I see two other things which would be nice to do:
candlerb
commented
1 year ago Hmm, there's a problem adding RFC1213-MIB; it overrides some of the things in IF-MIB. For example, comparing the original ../snmp.yml to the generated snmp.yml:
@@ -51,16 +52,16 @@
1: other
2: regular1822
3: hdh1822
- 4: ddnX25
- 5: rfc877x25
- 6: ethernetCsmacd
- 7: iso88023Csmacd
- 8: iso88024TokenBus
- 9: iso88025TokenRing
- 10: iso88026Man
+ 4: ddn-x25
+ 5: rfc877-x25
+ 6: ethernet-csmacd
+ 7: iso88023-csmacd
+ 8: iso88024-tokenBus
+ 9: iso88025-tokenRing
+ 10: iso88026-man
11: starLan
- 12: proteon10Mbit
- 13: proteon80Mbit
+ 12: proteon-10Mbit
+ 13: proteon-80Mbit
14: hyperchannel
15: fddi
16: lapband lots of comment differences too, e.g.
@@ -407,15 +138,14 @@
oid: 1.3.6.1.2.1.2.2.1.10
type: counter
help: The total number of octets received on the interface, including framing
- characters - 1.3.6.1.2.1.2.2.1.10
+ characters. - 1.3.6.1.2.1.2.2.1.10
indexes:
- labelname: ifIndex
type: gauge
- name: ifInUcastPkts
oid: 1.3.6.1.2.1.2.2.1.11
type: counter
- help: The number of packets, delivered by this sub-layer to a higher (sub-)layer,
- which were not addressed to a multicast or broadcast address at this sub-layer
+ help: The number of subnetwork-unicast packets delivered to a higher-layer protocol.
- 1.3.6.1.2.1.2.2.1.11
indexes:
- labelname: ifIndexSo the alternative is to patch those two MIB files to import DisplayString from SNMPv2-TC instead. What do you think about that??
sed -i '' -e 's/\(DisplayString[[:space:]]*FROM \)RFC1213-MIB/\1SNMPv2-TC/' mibs/CyberPower.MIB mibs/apc-powernet-mib"make generate" is also happy after that. (ISDN-MIB importing "transmission" from RFC1213-MIB doesn't cause any problem)
With this change, the only difference between ../snmp.yml and snmp.yml is a number of removed "fixed_size: 6" lines for PhysAddress48 objects: e.g.
--- ../snmp.yml 2023-03-28 07:51:08.000000000 +0100
+++ snmp.yml 2023-04-14 17:01:57.000000000 +0100
@@ -4776,7 +4776,6 @@
indexes:
- labelname: bsnAPDot3MacAddress
type: PhysAddress48
- fixed_size: 6
- labelname: bsnAPIfSlotId
type: gauge
lookups:(74 instances in total)
SuperQ
commented
1 year ago Ahh, interesting. I guess we need to file some upstream issues with those MIBs?
I'm not a big fan of patching MIBs, but, this seems easy enough.
candlerb
commented
1 year ago I guess we need to file some upstream issues with those MIBs?
The trouble is, I don't think there's anything technically wrong with those MIBs, even if they're outdated. They're free to import DisplayString from an older MIB if they like. Arguably, the problem is that net-snmp cannot keep track of multiple definitions of the same OID depending on the context they are referred from; if multiple MIBs define the same OID, then one overwrites the other.
If we could make the newer MIB take precedence, that would probably be OK. But I don't how to do that deterministically.
Another option might be to provide a dummy version of the RFC1213-MIB which defines DisplayString but nothing else - that feels wrong though.
Anyway, I've got a PR ready for patching the mibs at download time, you can see what you think.
dswarbrick
commented
1 year ago FWIW, SNMPv2-TC is derived from RFC 2579. Whilst that RFC doesn't specifically obsolete RFC 1213 or its descendants, it is of course more recent.
Stuff like this is why I'm in favour of moving the MIB minefield out of this repo. Just run smilint on pretty much any third party MIB and bask in the glory of all the syntax errors and standards violations. Sometimes it's a wonder that they successfully parse at all.
SuperQ
commented
1 year ago Yea, true. It's valid, but annoying.
The generator.yml was never meant to be more than a simple example config to show off what's possible. Not be a universal config for all use cases.
Some ideas:
I setup https://github.com/prometheus-community/snmp a while back to be a place to move the MIB minefield to. Something structured more like LibreNMS's MIB repo. Where we have various core MIBs available in dirs, and the vendor-specific stuff in their own dirs. Then at build time we select the collection of MIBDIRS for each generator.yml.
Thinking about this more, I'm starting to like the idea of having MIBDIRS per module. This will make build slower since we need to iterate over the net-snmp code several times. But, it would allow for better handling of these conflicts.
candlerb
commented
1 year ago Some ideas:
- We could improve on the generator by allowing for MIBDIRS to be specified per module.
- We move to more iterative build where fragments of snmp.yml are generated and assembled.
Yes - although I wonder if we'd also have to download each MIB into its own directory to prevent them stomping on each other. I'm not sure if specifying MIBS helps here, or whether net-snmp always reads in every file in every MIBDIRS,
Alternatively in git we could build a custom MIBDIR for each module, containing symlinks to the modules to import.
On top of this, I think it would be great if snmp_exporter itself accepted multiple snmp.yml files, e.g. it could read in snmp.d/*.yml. Then you could drop in newly built modules easily without worrying about whether your chosen build affected other working modules.
candlerb
commented
1 year ago Cross-ref: #872 for long-term solution
Host operating system: output of
uname -a(macOS 12.6.5, Intel)
snmp_exporter version: output of
snmp_exporter -versionI installed generator using
go install github.com/prometheus/snmp_exporter/generator@lateston 2023-04-14generatedoesn't have a "version" or "--version" subcommand.What device/snmpwalk OID are you using?
N/A - using supplied generator.yml and
make generateto fetch the MIBsIf this is a new device, please link to the MIB(s).
N/A
What did you do that produced an error?
What did you expect to see?
Completion without errors
What did you see instead?
"NetSNMP reported parse error(s)" errors=2followed by make terminating. Full log:I note that snmptranslate seems happy when given a simple test:
It's unclear which MIBs have the parse errors in them. snmp.yml is created, but a bit smaller than the official one:
Diffing them, I see that some entries from CPS-MIB are different, but commenting out CyberPower from generator.yml doesn't make a difference.