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

Accessing Grains in Custom Grains #47338

Closed
BenjaminSchiborr opened this issue Apr 26, 2018 · 13 comments · Fixed by #47372
Closed

Accessing Grains in Custom Grains #47338

BenjaminSchiborr opened this issue Apr 26, 2018 · 13 comments · Fixed by #47372
Labels
Feature new functionality including changes to functionality and code refactors, etc.
Milestone

Comments

@BenjaminSchiborr
Copy link

BenjaminSchiborr commented Apr 26, 2018

This is a question and possibly a feature request.

When writing custom grains I want to be able to utilize already existing grain information (either from core grains or other custom grains) in my custom grain. This is useful for situations when one property of a minion depends on another property of a minion that is already represented by another grain.

Example:
Let's assume we have a set of servers. Each server has a hostname that follows a specific naming convention. Part of that naming convention is that a role is encoded in the name of each server. To be able to target specific roles via grains one wants to retrieve the hostname of a minion, extract the role information and make it available as a grain.

One way of retrieving the hostname is via some custom python code in the custom grain. Another way is to e.g. just import from salt.grains.core import hostname and reuse this function.

From my POV the second option is preferable; it is DRY and probably better maintained than any custom code I would write.

Thus my questions are:

  • Is accessing existing grains in a custom grain a support usecase?
  • If yes, what is the correct way of doing this? (The above example works, however, if you e.g. want to utilize the "virtual" property you run into some follow up issues)
  • If no, how should one go about this?

Versions Report

Salt Version:
           Salt: 2017.7.5

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: 0.6.4
      gitpython: 1.0.1
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.21.1
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.12 (default, Dec  4 2017, 14:50:18)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.2.0
           RAET: Not Installed
          smmap: 0.9.0
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: Ubuntu 16.04 xenial
         locale: UTF-8
        machine: x86_64
        release: 4.4.0-119-generic
         system: Linux
        version: Ubuntu 16.04 xenial
@gtmanfred
Copy link
Contributor

No dunders are injected into the grains modules except for __opts__, because we use grains for loading other modules, and you can't reliably know that grains will already be loaded in order to reference them in other grains.

Also, the problem with importing other grains, is that the best way to load them is through the loader, which would also try to load your custom grain, so you end up with the chicken and the egg scenario.

The best way to do what you want to do, is write your own function to get the hostname or whatever value that you need, and use that in your custom grain, unfortunately there is just not a better way to logically get that value with the current loader system that we have.

@gtmanfred gtmanfred added the Question The issue is more of a question rather than a bug or a feature request label Apr 27, 2018
@gtmanfred gtmanfred added this to the Blocked milestone Apr 27, 2018
@BenjaminSchiborr
Copy link
Author

@gtmanfred : Thanks for the explanation. That makes sense. I guess for my example "hostname" above it works, because the function hostname is a relatively independent core grain.

Still, according to the documentation there is a precedence to the order of how grains are loaded:

  1. Core grains.
  2. Custom grains in /etc/salt/grains.
  3. Custom grains in /etc/salt/minion.
  4. Custom grain modules in _grains directory, synced to minions.

Thus, at the point in time when 4 is evaluated 1 to 3 must have already been evaluated. This matches (in part) with what I observed in the loader:

def grains(opts, force_refresh=False, proxy=None):
[...]
    # Run core grains
    for key in funcs:
        if not key.startswith('core.'):
            continue
        log.trace('Loading %s grain', key)
        ret = funcs[key]()
[...]
    # Run the rest of the grains
    for key in funcs:
        if key.startswith('core.') or key == '_errors':
            continue
        try:
[...]
            log.trace('Loading %s grain', key)
            if funcs[key].__code__.co_argcount == 1:
                ret = funcs[key](proxy)
            else:
                ret = funcs[key]()
[...]

I think the code corresponds to 1 and 4 (I do not know how 2 and 3 are loaded). If one were to inject the existing grains into the second ret = funcs[key](), wouldn't that make existing grains available in the custom grains? I'm probably leaning myself a little bit far out of the window since I do not know the salt codebase very well, but it seems like a few code changes could significantly enhance the re-usability of existing grains.
I do not know if my assumptions here are correct, but I think generally this would make a great new feature.

@gtmanfred
Copy link
Contributor

Do you have a link to where it says that in the documentation? because what actually happens afaik is that all of the grains are loaded by the loader, including custom grain modules, then all of them are run, and then that data is overwritten by /etc/salt/grains and grains in /etc/salt/minion

All of the grain modules are loaded with this one function

https://github.com/saltstack/salt/blob/2017.7/salt/loader.py#L624

Whether they are in salt.grains, /var/cache/salt/extmods/grains, or if they have the salt.loader entry point setup in their setup.py file for pyhton modules and have grains_dir specified.

They are all loaded, and it runs the function and assigns it to ret here

https://github.com/saltstack/salt/blob/2017.7/salt/loader.py#L722

You could try to just pass in grains_data into that function, but i would still think there is going to be a race condition, because it is just running all the functions at random, at least if you are running anything besides python 3.6 which has ordered dictionaries by default, and you can't be gauranteed to always have one module run before another.

Now, it does run all the core.<function> grains in the first for loop, and then all the other grains in the second for loop, so that may be reasonable to add, but it would definitely require testing.

https://github.com/saltstack/salt/blob/2017.7/salt/loader.py#L745

and then it mixes up the way we would write grains if it is not a core, and they wouldn't all be written the same.

@gtmanfred
Copy link
Contributor

@saltstack/team-core does anyone have an opinion on passing the core grains to the rest of the grains functions, so that that data can be used for custom grains?

@BenjaminSchiborr
Copy link
Author

@gtmanfred, for sure: https://docs.saltstack.com/en/latest/topics/grains/#precedence

Thanks for looking into this. I agree that this would require some testing. But looking at just the plain code it seems like the custom grains could easily get access to the core grains. Maybe I'll give it a shot locally for fun 🏎

@gtmanfred
Copy link
Contributor

we will definitely need to do some inspection into the grains modules, and make sure they accept arguments like we do for the proxy() modules, but if something accepts a grains variable, i can't think of a reason to pass it, but I do think this needs to be updated, because from looking at the code, all of the grains modules are run first, before merging in /etc/salt/grains

@BenjaminSchiborr
Copy link
Author

BenjaminSchiborr commented Apr 27, 2018

@gtmanfred , I wrote a little spike to demonstrate how this could work:

    # Run the rest of the grains
    for key in funcs:
        if key.startswith('core.') or key == '_errors':
            continue
        try:
            # Grains are loaded too early to take advantage of the injected
            # __proxy__ variable.  Pass an instance of that LazyLoader
            # here instead to grains functions if the grains functions take
            # one parameter.  Then the grains can have access to the
            # proxymodule for retrieving information from the connected
            # device.
            parameters = list(funcs[key].__code__.co_varnames)
            if funcs[key].__code__.co_argcount == 1:
                if 'proxy' in parameters:
                  ret = funcs[key](proxy)
                else:
                  ret = funcs[key](grains_data)
            elif funcs[key].__code__.co_argcount == 2:
                  ret = funcs[key](grains_data, proxy)
            else:
                ret = funcs[key]()

I have not worked with salt proxies. The assumption is that if one parameter is passed we check if its proxy and then pass proxy. For any other parameter we would always pass the grains. If two parameters are passed we pass grain and proxy (assuming that grain would always be the first parameter). Otherwise we don't pass anything.

Using this one could write the following custom grain:

#!/usr/bin/env python
import logging
log = logging.getLogger(__name__)

def reuse_core_grain(grains): 
    return {'reuse_core_grain': grains['virtual']}

Querying the grain leads to to this:

root@salt:/home/vagrant# salt minion grains.get reuse_core_grain
minion:
    VirtualBox

@gtmanfred
Copy link
Contributor

that looks great, but I would still say we should check if grains is in the parameters, and pass grains=grains_data since you will also possibly need to pass proxy to that for proxy minion grains.

If you wouldn't mind opening a PR for this, and a test case, that would be greatly appreciated.

Thanks,
Daniel

@gtmanfred gtmanfred added Feature new functionality including changes to functionality and code refactors, etc. and removed Question The issue is more of a question rather than a bug or a feature request labels Apr 27, 2018
@gtmanfred gtmanfred modified the milestones: Blocked, Approved Apr 27, 2018
@gtmanfred
Copy link
Contributor

Actually, I got it.

@gtmanfred
Copy link
Contributor

Test included #47372 . Awesome idea!

Thanks,
Daniel

rares-pop pushed a commit to ni/salt that referenced this issue Jul 4, 2018
@theonlyjohnny
Copy link

Is this feature released? It does not seem to be working for me, on salt version 2018.3.0

 % sudo salt-call -V                                    
Salt Version:                                           
           Salt: 2018.3.0                               
                                                        
Dependency Versions:                                    
           cffi: Not Installed                          
       cherrypy: Not Installed                          
       dateutil: 2.5.3                                  
      docker-py: 1.10.6                                 
          gitdb: 2.0.0                                  
      gitpython: 2.1.1                                  
          ioflo: Not Installed                          
         Jinja2: 2.9.4                                  
        libgit2: Not Installed                          
        libnacl: Not Installed                          
       M2Crypto: Not Installed                          
           Mako: Not Installed                          
   msgpack-pure: Not Installed                          
 msgpack-python: 0.4.8                                  
   mysql-python: Not Installed                          
      pycparser: Not Installed                          
       pycrypto: 2.6.1                                  
   pycryptodome: Not Installed                          
         pygit2: Not Installed                          
         Python: 2.7.13 (default, Nov 24 2017, 17:33:09)
   python-gnupg: Not Installed                          
         PyYAML: 3.12                                   
          PyZMQ: 16.0.2                                 
           RAET: Not Installed                          
          smmap: 2.0.1                                  
        timelib: Not Installed                          
        Tornado: 4.4.3                                  
            ZMQ: 4.2.1                                  
                                                        
System Versions:                                        
           dist: debian 9.4                             
         locale: UTF-8                                  
        machine: x86_64                                 
        release: 4.9.0-4-amd64                          
         system: Linux                                  
        version: debian 9.4                             
% cat ~/aws-saltstack/_grains/upstream.py
def test(grains):
    if 'os' in grains:
        return {'custom_grain_test': 'itworked'}
    return {'custom_grain_test': 'itdidntwork'}
% sudo salt-call --local --file-root ~/aws-saltstack saltutil.sync_grains
[CRITICAL] Failed to load grains defined in grain file upstream.test in function <function test at 0x7f36cedd2c80>, error:
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/salt/loader.py", line 754, in grains
    ret = funcs[key](proxy)
  File "/var/cache/salt/minion/extmods/grains/upstream.py", line 2, in test
    if 'os' in grains:
TypeError: argument of type 'NoneType' is not iterable

@theonlyjohnny
Copy link

This grain is obviously copied from #47372 -- the exact test case specified there -- though it occurs with other function names/logic

@gtmanfred
Copy link
Contributor

gtmanfred commented Aug 11, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature new functionality including changes to functionality and code refactors, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants