Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

generator new file management #625

Open
xkilian opened this issue Mar 5, 2021 · 20 comments
Open

generator new file management #625

xkilian opened this issue Mar 5, 2021 · 20 comments

Comments

@xkilian
Copy link

xkilian commented Mar 5, 2021

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 .

@RichiH
Copy link
Member

RichiH commented Mar 9, 2021

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

@RichiH
Copy link
Member

RichiH commented Mar 9, 2021

#628 should cover the essence of your intention, correct?

@xkilian
Copy link
Author

xkilian commented Mar 9, 2021

#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
Copy link
Member

RichiH commented Mar 9, 2021

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.

@xkilian xkilian changed the title generator new file management and collector read from directory snmp_xxx.yml files generator new file management Mar 9, 2021
@lausser
Copy link

lausser commented Mar 9, 2021

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
Copy link
Author

xkilian commented Mar 9, 2021

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
Copy link
Author

xkilian commented Mar 23, 2021

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

@RichiH
Copy link
Member

RichiH commented Mar 29, 2021

@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
Copy link
Member

SuperQ commented Mar 29, 2021

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
Copy link
Author

xkilian commented Mar 29, 2021

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
Copy link
Member

RichiH commented Mar 29, 2021

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 added a commit to Azoplee/snmp_exporter that referenced this issue Apr 16, 2021
@sebastien-coavoux
Copy link
Contributor

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 added a commit to Azoplee/snmp_exporter that referenced this issue May 3, 2021
@xkilian
Copy link
Author

xkilian commented Aug 9, 2021

@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
Copy link
Member

RichiH commented Aug 10, 2021

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
Copy link
Author

xkilian commented Aug 20, 2021

@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
Copy link
Member

RichiH commented Aug 20, 2021

@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
Copy link
Author

xkilian commented Aug 20, 2021

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
Copy link
Member

RichiH commented Aug 28, 2023

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

@SuperQ
Copy link
Member

SuperQ commented Aug 28, 2023

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
Copy link
Member

SuperQ commented May 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants