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

Add parameter named grains_blacklist #49481

Merged
merged 6 commits into from
Oct 1, 2018

Conversation

rongzeng54
Copy link
Contributor

What does this PR do?

Add parameter named grains_blacklist to block the specific grains.

What issues does this PR fix or reference?

No

Previous Behavior

The default grains list, seems getting longer and uncontrolled by the user.

New Behavior

The user finally has a chance to say no.
Of course, only for advanced users because of the dependencies of module.

Tests written?

No

Commits signed with GPG?

No

@rongzeng54 rongzeng54 requested a review from a team as a code owner September 1, 2018 18:36
@ghost ghost self-requested a review September 1, 2018 18:36
@cachedout
Copy link
Contributor

Overall, I like the idea of this feature but I'm not certain about this implementation.

Would it not be more efficient to just filter at the point that the grains themselves are generated versus doing this on the instantiation of the LazyLoader? I wonder if we should move this logic to the grains() function in the loader. Let me know what you think, @rongzeng54

@rongzeng54
Copy link
Contributor Author

@cachedout
Thank you for your reminder. Please check the new commit.

@cachedout
Copy link
Contributor

This is much better. I like this a lot.

@cachedout
Copy link
Contributor

@terminalmage You feel good about this?

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to have documentation added in doc/ref/configuration/minion.rst.

In addition, all other whitelists/blacklists that we support in salt use salt.utils.stringutils.expr_match() to support exact matches, fnmatch.fnmatch() matches, and regex matches. We should be using it here as well for consistency.

Lastly, rather than doing type checking, we should be setting a default value for this in DEFAULT_MINION_OPTS in salt/config/__init__.py. For example:

'grains_blacklist': [],

There should also be an entry for this option in VALID_OPTS as well, with the valid type being list.

@rongzeng54
Copy link
Contributor Author

Hi @terminalmage
Thanks for your reminder. Already fixed.

doc/ref/configuration/minion.rst Outdated Show resolved Hide resolved
doc/ref/configuration/minion.rst Outdated Show resolved Hide resolved
doc/ref/configuration/minion.rst Outdated Show resolved Hide resolved
doc/ref/configuration/minion.rst Outdated Show resolved Hide resolved
salt/loader.py Outdated Show resolved Hide resolved
doc/ref/configuration/minion.rst Outdated Show resolved Hide resolved
doc/ref/configuration/minion.rst Show resolved Hide resolved
salt/loader.py Outdated Show resolved Hide resolved
@rongzeng54
Copy link
Contributor Author

Hi @terminalmage
Please check again.

Copy link
Contributor

@isbm isbm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is even needed at all? If there is something in the default list, then it needs to be there anyway. If you want only specific keys, then you can already get them. But once you need something that you normally do not need and this "muter" exist, then you have a trouble.

I find this useless, sorry.

@rongzeng54
Copy link
Contributor Author

@isbm
Useless data wastes server-side storage space, bandwidth caused by data synchronization, even speed.
As described, It is not for everyone.

@isbm
Copy link
Contributor

isbm commented Sep 12, 2018

@isbm
Useless data wastes server-side storage space, bandwidth caused by data synchronization, even speed.
As described, It is not for everyone.

Then simply don't ask for all data. Ask only for what you need. But once you do need it but at the same time cannot get it, then I find this a very bad idea.

@rongzeng54
Copy link
Contributor Author

@isbm
Useless data wastes server-side storage space, bandwidth caused by data synchronization, even speed.
As described, It is not for everyone.

Then simply don't ask for all data. Ask only for what you need. But once you do need it but at the same time cannot get it, then I find this a very bad idea.

The following is the configuration of our production environment, we believe that we cannot need them, but we also believe that others may need.

grains_blacklist:
  - 'lsb_distrib_codename'
  - 'lsb_distrib_id'
  - 'lsb_distrib_release'
  - 'cpu_flags'
  - 'fqdn_ip4'
  - 'fqdn_ip6'
  - 'num_gpus'
  - 'gpus'
  - 'locale_info'
  - 'machine_id'
  - 'pythonpath'
  - 'saltversioninfo'
  - 'pythonversion'
  - 'server_id'
  - 'saltpath'
  - 'domain'
  - 'localhost'
  - 'ip4_interfaces'
  - 'ip6_interfaces'
  - 'zmqversion'
  - 'SSDs'
  - 'disks'
  - 'mdadm'
  - 'username'
  - 'groupname'
  - 'uid'
  - 'gid'
  - 'systemd'

@rongzeng54
Copy link
Contributor Author

Hi @terminalmage
It seems that all the problems have been solved.

@rallytime
Copy link
Contributor

Can you write some tests for this new functionality? We wouldn't want this to regress in the future. :)

zr added 6 commits September 22, 2018 14:37
- Update the documentation of minion configuration
- Use salt.utils.stringutils.expr_match() to support exact matches
- Setting a default value in DEFAULT_MINION_OPTS & VALID_OPTS
- fix dict.keys() for python3
- fix the document error
@rongzeng54
Copy link
Contributor Author

Can you write some tests for this new functionality? We wouldn't want this to regress in the future. :)

Hi @rallytime . It is done.

@rallytime
Copy link
Contributor

Thanks @rongzeng54!

@rallytime rallytime merged commit ba7436e into saltstack:develop Oct 1, 2018
@rongzeng54 rongzeng54 deleted the zr.add_grains_blacklist branch October 15, 2018 17:26
@max-arnold
Copy link
Contributor

This option seems to partially duplicate the existing disable_grains parameter (briefly mentioned in #42567).

IMO, having two different ways to do this is confusing.

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

Successfully merging this pull request may close these issues.

6 participants