snmp_exporter icon indicating copy to clipboard operation
snmp_exporter copied to clipboard

generator new file management

Open xkilian opened this issue 4 years ago • 20 comments

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 generator_xxxx.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:

  • generator_cisco_switches.yml
  • generator_cisco_fw.yml
  • generator_paloalto.yml

Example invalid input files:

  • cisco_routers.yml
  • netscreen_generator.yml

If no output path is specified:

  • name the output file becomes: snmp_xxx.yml and is placed in the current directory (for validation)

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

  • valid example: /opt/prometheus/etc/snmp_exporter/snmp.d/
  • valid example: /etc/snmp_exporter/snmp.d

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 .

xkilian avatar Mar 05 '21 22:03 xkilian

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

RichiH avatar Mar 09 '21 10:03 RichiH

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

RichiH avatar Mar 09 '21 10:03 RichiH

#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.

xkilian avatar Mar 09 '21 11:03 xkilian

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.

RichiH avatar Mar 09 '21 11:03 RichiH

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.

lausser avatar Mar 09 '21 14:03 lausser

This FR aims to answer:

  • splitting generator config in manageable groups of configurations
  • manage naming of input and output files
  • Adding a generator.d would be used to house the each generator directory configuration kit (mibs and generator.yml) (thanks for the hint on using generator.d)

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 avatar Mar 09 '21 15:03 xkilian

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

xkilian avatar Mar 23 '21 22:03 xkilian

@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?

RichiH avatar Mar 29 '21 09:03 RichiH

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.

SuperQ avatar Mar 29 '21 09:03 SuperQ

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.

xkilian avatar Mar 29 '21 11:03 xkilian

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.

RichiH avatar Mar 29 '21 14:03 RichiH

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.

sebastien-coavoux avatar Apr 17 '21 00:04 sebastien-coavoux

@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.

xkilian avatar Aug 09 '21 14:08 xkilian

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

RichiH avatar Aug 10 '21 08:08 RichiH

@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.

xkilian avatar Aug 20 '21 14:08 xkilian

@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

RichiH avatar Aug 20 '21 14:08 RichiH

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).

xkilian avatar Aug 20 '21 15:08 xkilian

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

RichiH avatar Aug 28 '23 14:08 RichiH

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 avatar Aug 28 '23 14:08 SuperQ