prometheus / snmp_exporter

SNMP Exporter for Prometheus
Apache License 2.0
1.7k stars 630 forks source link

generator new file management #625

Open xkilian opened 3 years ago

xkilian commented 3 years ago

Problem: Each class of devices should be managed in its own directory to simplify testing, mib compatibility and shareability of configs. Merging snmp.yml files becomes a hassle and must be handled by an external script and having a single monster snmp.yml is a pain to review. Also having all generator configuration files names generator.yml and output snmp.yml is not ideal.

Solution: Create an MANDATORY --input option and an OPTIONAL --path option for the output path.

The generator MUST have an input option to specify the name of the generatorxxxx.yml file. The input file MUST have a generator prefix The input file MUST be a valid yml file. The xxx portion SHOULD represent that grouping identifier for a class of devices or a vendor

Example valid input files:

Example invalid input files:

If no output path is specified:

An output path SHOULD be specified to deploy to the directory the collector will read

The output directory for production files MUST be a common directory where all snmp.yml will be merged by the collector.

This FR is linked to a change in the collector #628 to read the snmp_xxxx.yml files for a given directory (startup flag of snmp_exporter).

As discussed with @SuperQ .

RichiH commented 3 years ago

I think all of the above could be achieved with support for a snmp.d or snmp.yml.d, no?

RichiH commented 3 years ago

https://github.com/prometheus/snmp_exporter/issues/628 should cover the essence of your intention, correct?

xkilian commented 3 years ago

628 covers it for the collector. Sorry, I missed it, I will update the issue description to refer to #628.

As for the generator, the above FR is still applicable.

RichiH commented 3 years ago

Now that I think about it again, that makes sense. I will need to think about it some more. A mapping from generator_fool.yml -> snmp_exporter_foo.yml could make sense.

lausser commented 3 years ago

I have my own script which reads from smaller yml files, where every involved mib has one: if_mib.yml rmon_mib.yml snmpv2_mib.yml,.... They all start with

modules:
  # Default IF-MIB interfaces table with ifIndex.
  if_mib:
    walk: [sysUpTime, interfaces, ifXTable]

or

modules:
  snmpv2_mib:
    walk:
      - sysDescr

and these files get merged into the generator.yml

On the other side i have a script which creates scrape configs for network devices. Depending on the model and usage every device gets a list like this: self.snmp_exporter_modules = ["if_mib", "snmpv2_mib"] or self.snmp_exporter_modules = ["if_mib", "snmpv2_mib", "rmon_mib"]

My first approach was to create two or three scrape configs for the file_sd_configs, one for every mib. But this could mean that two or even more scrapes (snmpwalks) could run against a device at the same time. Sometimes this can lead to timeouts (if you have 512 interfaces for example). So i thought to have the walks run in a serial manner. The scrape config of a device would then contain a module which is a sorted join of all the single mibs (or module) names, for example module: if_mib__rmon_mib__snmpv2_mib

The script i mentioned in the beginning would not only copy the contents of the single-mib-ymls into one generator.yml, but also create combined modules by merging these contents.

modules:
  if_mib_
    walk:
      - ifTable
      - ifXTable
  snmpv2_mib:
    walk:
      - sysDescr
  rmon_mib:
    walk:
      - etherstatsTable

and there are also

  if_mib__snmpv2_mib:
    walk:
      - ifTable
      - ifXTable
      - sysDescr
  if_mib__rmon_mib__snmpv2_mib:
    walk:
      - ifTable
      - ifXTable
      - sysDescr
      - etherstatsTable

Of course this creates a big generator.yml in the first step, but the number of typical combinations will not be too big. This would become much easier, if it was possible to allow a list of modules and the snmp_exporter would walk one after the other (skipping duplicate tables/oids) Also splitting up generator.yml in smaller portions (in generator.d) would make it easier to exchange config files for certain devices.

xkilian commented 3 years ago

This FR aims to answer:

There is another issue, #620, which is exploring the splitting of configuration of the generator to limit repeated configurations (imports, multiple modules, etc.). You can suggest a course to take based on your experience and how you would see it. Of course, splitting configurations in classes of equipement would reduce the scope of imports to the device class. I personally do not mind this. I like having access in a self-contained generator.yml including the common stuff.

I think we all agree that a jumbo generator.yml is something to move away from.

xkilian commented 3 years ago

We are ready to work on preparing a PR for this. @RichiH Are we good to go?

RichiH commented 3 years ago

@xkilian We're discussing details on and off; the overview:

generator should follow how snmp_exporter does it. snmp_exporter will follow how Prometheus does it. Prometheus will most likely use standard Go YAML parsing.

So "last module name wins; no merging"

To make all of this easier to use and reason about, it must also be obvious from logs (and web UI in non-generator cases) which thing won when and why.

Is that enough to get started on your end?

SuperQ commented 3 years ago

I'd like to be consistent with how we handle multi-file yaml config parsing. I'm going to ping some other Prometheus devs this week to see where we're at in conf.d/*.yml parsing for Prometheus itself.

xkilian commented 3 years ago

In any case, work on the generator part is indépendant from the collector merge part.

We will start work on the pr for the generator.

RichiH commented 3 years ago

Our current thinking is at https://docs.google.com/document/d/1BK_Gc3ixoWyxr9F5qGC07HEcfDPtb6z96mfqoGyz52Y and I will open a thread on prometheus-developers to make people aware of it, as well.

Agreed that getting started on orthonal work makes sense.

sebastien-coavoux commented 3 years ago

Hey, I've just pushed a commit to start work on this item. Very basic, no test added whatsoever. You can have a first look and let me know .

It adds a required parameter to generate, the input file that must have "generator_" prefix. Also add the -o option to the output directory.

xkilian commented 3 years ago

@sebastien-coavoux @SuperQ works like a charm. If we are still going in the right direction can you confirm so that tests and documentation are updated and a pull request created. Thanks.

RichiH commented 3 years ago

I am biased, but from what I can tell this approach will be followed: https://docs.google.com/document/d/1BK_Gc3ixoWyxr9F5qGC07HEcfDPtb6z96mfqoGyz52Y/edit#heading=h.8rwanuxqa14m

CC @rfratto in case that overlaps with agent work

xkilian commented 3 years ago

@RichiH Remember that the generator portion is independant of most of the discussion that you referenced which concern more the collector. I do not see any impact on the generator itself. I do not think we should be blocking the generator changes for something so simple by an initiative that may take a long time to come to fruition.

RichiH commented 3 years ago

@xkilian My point was that you can keep this in mind when generating output; for now it's the old format, but it will come at some point.

Applying the patterns to input files, this would result in

snmp_generator.d/snmp_generator_cisco_switches.yml
snmp_generator.d/snmp_generator_cisco_fw.yml
snmp_generator.d/snmp_generator_paloalto.yml

as the default layout for files. So if the generator is run and does not find a generator.yml in $PWD, it would simply operate on $PWD/snmp_generator.d/snmp_generator*.yml

xkilian commented 3 years ago

Hmm. I would have to strongly disagree with this for the generator. The whole point of the generator changes is to facilitate seperate "domains" for generating the resulting outpout yml files that would be combined in a snmp.d/ directory. We want to have grouped together mibs, the generator file and potentially including a common base config file (passwords, etc.) from somewhere else. Still having a big combined generator files that is built using a bunch of includes is still as bad IMO as the current method. MIB conflicts, not knowing what bundle of MIBS is really required for a devices, etc. Erck. All the gains for the includes and whatnot should be applied to the collector and snmp_*.yml files, not the structure of the generator config files which need to be split up (this issue).

RichiH commented 1 year ago

@SuperQ is it worth to think this one through for the current sprint as well?

SuperQ commented 1 year ago

I'm not sure if this is something we want to do in the generator. This sounds like more of a job for a Makefile that maps a number of input files to output files.

SuperQ commented 6 months ago

While we haven't changed the generator to support multiple input files, the exporter itself now supports multiple files.

So it's no longer necessary to have a single output snmp.yml file.