Control the collection of lvm grains via config

[agraul]

What does this PR do?

Refactor options to control grain computation into a single disabled_grains list and make lvm aware of it.

lvm grain collection can take a long time on systems with a lot of volumes and volume groups. On one server we measured ~3 minutes, which is way too long for grains.

This change is backwards-compatible, leaving the lvm grain collection enabled by default.

What issues does this PR fix or reference?

Fixes: #63114

Previous Behavior

lvm grains are always loaded

New Behavior

lvm grains are not loaded when disabled in minion configuration

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

• Docs: needs changes, e.g. announce the move to disabled_grains
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated

Does this need a grains/lvm.conf specific test, or is this covered by the __virtual__() / Salt loader tests?

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[Ch3LL]

I know we have another similar setting enable_fqdns_grains, but I'm wondering if we should have a more global setting instead of creating a new config for each grain that might want to be disabled.

How about a config similar to disable_grains which is a list of grains to disable?

[agraul]

We could use a global config instead, that would make things a bit easier for the user. There already is the grains_blacklist which is similar in concept but works on a different granularity. grains_blacklist only filters the grains after they are computed, which does not help when grains computation is slow. But it allows, afaics, to filter out single grains out of a grain function that computes multiple grains. If every grain function only ever returned a single grain, we could just skip the computation for those functions based on the blacklist.

What do you think about the following:

1. we add disable_grains
2. we add a "config translation layer" that automatically populates disable_grains from all existing "grains opt-out settings"
3. all grain modules that use these settings are changed to only check the single disable_grains list
4. we mark existing "grains opt-out settings" deprecated, telling users to use the disable_grains list instead

In an unrelated project, I just experienced how changing the configuration can go badly when old config keys are not accepted anymore and I'd keep 2. around for a long time to not break existing configs.

Depending on how we implement 2., enable_lvm_grains: False could still work without explicitly allowing it. That it's of little interest to you, but could be useful in our downstream Salt package

[Ch3LL]

Can you elaborate on (we add a "config translation layer" that automatically populates disable_grains from all existing "grains opt-out settings")

I just want to be clear what your proposing on this one.

There other points sound solid to me.

[agraul]

Sure, I have two ways to do this in mind, either using a regex to extract the grains module from the existing "grains opt-out settings", or with a hard-coded list of known "opt-out settings". The overall approach is the same for both, if it's possible with a simple regex I'd prefer that approach since it's more open. Anyway, the idea is this:

At the end of apply_minion_config in salt/config/__init__.py, the existing config keys are checked (using the list or regex). For each match that's set to False, the grain module name is added to disable_grains.

[agraul]

I started implementing this and hit a bit of a blocker: some grains are currently disabled by default. I'm not sure how to handle this. If I add them to disabled_grains per default (in DEFAULT_(MASTER|MINION)_OPTS, the user can't take them out using a config file.

One way to solve this is to only add the default disabled_grains in conf/master and conf/minion, but that's not a nice solution. Another way would be to also add an enabled_grains (or similarly named) config, with the sole purpose of taking grains out of disabled_grains.

How do you feel about this @Ch3LL ?

[agraul]

Please disregard my previous comment, I didn't see the forest for the trees, the user can simply provide disabled_grains with the default(s) removed.

[agraul]

I did a first implementation of disabled_grains. I'm not totally happy yet because each grain module / function needs to be updated. If we change the structure of disabled_grains to differentiate between grain modules and grain functions, the loader could take care of skipping disabled grains. I'll work on that next.

[agraul]

Given that the schema can have extra options, is it ok to remove the settings from it?

[Ch3LL]

I think its okay to put them on a deprecation path in favor of the new disabled_grains.

Can you make sure you add a comment:
# salt.utils.versions.warn_until(Potassium, "enabled_fqdns_grains is deprecated in favor of.......)
The only reason being is we grep the cod for warn_until and version name each time we work on removing deprecations.

Also add changelog files for each setting you are removing with the filename deprecated stating the details on what users can now use instead.

[Ch3LL]

Looks like htere are some doc and lint test failures. When those are resolved i'll give this a full review. Thanks for implementing this.

[agraul]

I still have to do the requested changes for the deprecation. If possible, it would be great to have a review for the code changes. I'm a bit conflicted on having the grains do the "disabling" in __virtual__ vs putting it in the loader code. Any opinions on what would be a better fit?

[Ch3LL]

ping @s0undt3ch any thoughts here?

I think I tend to agree it would be nice if this logic was in the loader instead of having to add it to each __virtual__ function but wanted to get your opinion.

[dwoz]

Closing this due to inactivity. Anyone should feel free to re-open it if they want to see it through to the end in one release cycle.

[agraul]

Closing this due to inactivity. Anyone should feel free to re-open it if they want to see it through to the end in one release cycle.

I would like to get this in, I think a multiple minutes grain collection time is pretty bad. I still don't know if my current approach is fine by you or if you'd rather do this in the loader.

[dwoz]

Great, and thanks for the reply. Please re-base and fix the conflicts so we can see if all tests pass.