Move "FQDNs" grains calculation to "network" module and allow disabled them

[meaksh]

What does this PR do?

This PR fixes an issue that is currently blocking the "core" grains execution for several seconds when the some "FQDNs" cannot be calculated.

Currently, in order to calculate the fqdns grain, all the IP addresses from the minion are processed sequentially. The problem here comes from the underlying calls to socket.gethostbyaddr which can takes 5 seconds until it's released (after reaching the socket.timeout) when there is no defined "fqdn" for that IP.

In some scenarios, depending on the minion network configuration, this situation makes the fqdn grain calculation to take even more than 15 seconds. This completely breaks minion execution not only at startup time but any time the minion refreshes its grains.

This PR makes the fqdn_lookup to be executed in parallel (using threads) so, in those cases we would only wait until "socket.timeout" is reached once and prevent from blocking the minion execution for such a long time.

Besides of that, the whole "fqdn" calculation logic has been moved to the network execution module. Since the process of calculating the "fqdns" can take too much in case of networking issues, it makes more sense to move if away from the "core" grains and also introduce a new configuration parameters called enable_fqdns_grains(default True) to allow not to calculate "fqdns" as part of the "core" grains rendering.

Tests written?

Yes

Commits signed with GPG?

Yes

[brejoc]

People from gevent claim that this is indeed thread safe, even though the underlying C code isn't: https://github.com/gevent/gevent/blob/b01bcf66c9229d78100e93fe2bf57c3b6afcfedd/src/gevent/resolver/thread.py#L55-L59

lgtm 👍

[meaksh]

Do we have any feedback here?

[meaksh]

We've pushed some extra changes to this PR that move the "fqdns" calculation away from the "core" grains to the network execution module. The "fqdns" grains is calculated by default but can be omitted by setting enable_fqdns_grains configuration parameter to False.

[max-arnold]

@meaksh Maybe it makes sense to use disable_grains (#48773 (comment)) or blacklist_grains (#49481) option to disable the grain?

Then we can avoid introducing yet another config option.

[dincamihai]

@max-arnold i can see the same approach was taken here

salt/salt/grains/core.py

Line 194 in 331eda0

if __opts__.get('enable_gpu_grains', True) is False:

It looks like blacklist_grains is just filtering the grains, not preventing from rendering. In this PR we want to prevent computing the grains because it is expensive and not reliable when there's a wrongly configured dns.

I see that disable_grains is not documented. Would that prevent computing the fqdns grains by not calling the fqdns function?

[max-arnold]

It looks like blacklist_grains is just filtering the grains, not preventing from rendering.

Then maybe it makes sense to improve on that PR (or contact the author) and actually skip rendering blacklisted grains.

I see that disable_grains is not documented. Would that prevent computing the fqdns grains by not calling the fqdns function?

It should, but it works on module-level (i.e. will disable the whole network grain module)

[dincamihai]

It looks like blacklist_grains is just filtering the grains, not preventing from rendering.

Then maybe it makes sense to improve on that PR (or contact the author) and actually skip rendering blacklisted grains.

I see that disable_grains is not documented. Would that prevent computing the fqdns grains by not calling the fqdns function?

It should, but it works on module-level (i.e. will disable the whole network grain module)

fqdns is a core grain (not in a separate module) this means we can't use it here, right?

[max-arnold]

Yes, unless it is moved to a separate module.

[dincamihai]

Yes, unless it is moved to a separate module.

If we move it to a separate module we would change the way salt works at the moment.
I would avoid changing the grains structure.
With this PR, one doesn't have to add anything to existing configuration. Everything should work as before. Only when one wants to disable fqdns grain the new config param is needed.
Does it make sense? Have I manage to convince you? :)

[max-arnold]

Everything you said makes complete sense :)

Still, there is an apparent need for a generic way to disable grains, and introducing another one-off option seems suboptimal.

[dincamihai]

Everything you said makes complete sense :)

Still, there is an apparent need for a generic way to disable grains, and introducing another one-off option seems suboptimal.

You are right about the generic way of disabling grains. I think it deserves a good and consistent implementation in its own PR.
I've removed the parameter to disable the fqdns grain from this PR. It contains now only the implementation with threads and the refactoring to allow fqdns retrieval both via grains and from network.fqdns module.

We will keep the disabling of this grain as a patch in our own salt package until the generic way to disabling grains will be available.

Is there anything else we can do to get this one merged?

[meaksh]

Closing in favor of #55581