Repository: saltstack/salt
Pull request: Allow for passing in previously compiled grains to custom grain modules
### What does this PR do?
If a grains module function accepts a variable named `grains` then the grains dict will be passed in.

### What issues does this PR fix or reference?
Closes #47338

### Previous Behavior
No grains are passed to custom grains modules

### New Behavior
Previously compiled grains are passed into grains functions if the function accepts a variable called `grains`

### Tests written?

Yes

### Commits signed with GPG?

Yes

Problem statement:
Accessing Grains in Custom Grains
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
```

Hints from issue discussion:
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 : 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.
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.
@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?
@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 🏎 
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
@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
```
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
Actually, I got it.

Commit message: Allow for passing in previously compiled grains to custom grain modules

Closes #47338

Patch to review:
diff --git a/doc/topics/grains/index.rst b/doc/topics/grains/index.rst
index 2b1967815214..d1fbee3b70df 100644
--- a/doc/topics/grains/index.rst
+++ b/doc/topics/grains/index.rst
@@ -269,6 +269,11 @@ grain will override that core grain. Similarly, grains from
 ``/etc/salt/minion`` override both core grains and custom grain modules, and
 grains in ``_grains`` will override *any* grains of the same name.
 
+For custom grains, if the function takes an argument ``grains``, then the
+previously rendered grains will be passed in.  Because the rest of the grains
+could be rendered in any order, the only grains that can be relied upon to be
+passed in are ``core`` grains. This was added in the Fluorine Release.
+
 
 Examples of Grains
 ==================
diff --git a/doc/topics/releases/fluorine.rst b/doc/topics/releases/fluorine.rst
index 9b73d0cfc61b..1926747ba2bb 100644
--- a/doc/topics/releases/fluorine.rst
+++ b/doc/topics/releases/fluorine.rst
@@ -5,6 +5,16 @@ Salt Release Notes - Codename Fluorine
 ======================================
 
 
+Grains Dictionary Passed into Custom Grains
+-------------------------------------------
+
+Starting in this release, if a custom grains function accepts a variable named
+``grains``, the Grains dictionary of the already compiled grains will be passed
+in.  Because of the non deterministic order that grains are rendered in, the
+only grains that can be relied upon to be passed in are ``core.py`` grains,
+since those are compiled first.
+
+
 "Virtual Package" Support Dropped for APT
 -----------------------------------------
 
diff --git a/salt/loader.py b/salt/loader.py
index 56b4b038ea78..4b07cb3afc09 100644
--- a/salt/loader.py
+++ b/salt/loader.py
@@ -751,8 +751,14 @@ def grains(opts, force_refresh=False, proxy=None):
             # proxymodule for retrieving information from the connected
             # device.
             log.trace('Loading %s grain', key)
-            if funcs[key].__code__.co_argcount == 1:
-                ret = funcs[key](proxy)
+            parameters = list(funcs[key].__code__.co_varnames)
+            if funcs[key].__code__.co_argcount > 0:
+                kwargs = {}
+                if 'proxy' in parameters:
+                    kwargs['proxy'] = proxy
+                if 'grains' in parameters:
+                    kwargs['grains'] = grains_data
+                ret = funcs[key](**kwargs)
             else:
                 ret = funcs[key]()
         except Exception:
diff --git a/tests/integration/files/file/base/_grains/test_custom_grains.py b/tests/integration/files/file/base/_grains/test_custom_grains.py
new file mode 100644
index 000000000000..639803bafcf2
--- /dev/null
+++ b/tests/integration/files/file/base/_grains/test_custom_grains.py
@@ -0,0 +1,4 @@
+def test(grains):
+    if 'os' in grains:
+        return {'custom_grain_test': 'itworked'}
+    return {'custom_grain_test': 'itdidntwork'}
diff --git a/tests/integration/grains/test_custom.py b/tests/integration/grains/test_custom.py
new file mode 100644
index 000000000000..f9c534d812f9
--- /dev/null
+++ b/tests/integration/grains/test_custom.py
@@ -0,0 +1,22 @@
+# -*- coding: utf-8 -*-
+'''
+Test the core grains
+'''
+
+# Import python libs
+from __future__ import absolute_import, unicode_literals
+
+# Import Salt Testing libs
+from tests.support.case import ModuleCase
+
+
+class TestGrainsCore(ModuleCase):
+    '''
+    Test the core grains grains
+    '''
+
+    def test_grains_passed_to_custom_grain(self):
+        '''
+        test if current grains are passed to grains module functions that have a grains argument
+        '''
+        self.assertEqual(self.run_function('grains.get', ['custom_grain_test']), 'itworked')


Code review comments:
diff_hunk: @@ -751,8 +751,14 @@ def grains(opts, force_refresh=False, proxy=None):
             # proxymodule for retrieving information from the connected
             # device.
             log.trace('Loading %s grain', key)
-            if funcs[key].__code__.co_argcount == 1:
-                ret = funcs[key](proxy)
+            parameters = list(funcs[key].__code__.co_varnames)
+            if funcs[key].__code__.co_argcount > 0:
+                kwargs = {}
+                if 'proxy' in parameters:
+                    kwargs['proxy'] = proxy
+                if 'grains' in parameters:
+                    kwargs['grains'] = grains_data
+                ret = funcs[key](**kwargs)
path: salt/loader.py
text: @user1:
No need to double-check length of the `co_varnames` as well as no need to `if/else` to call `funcs[key]` with or without `kwargs`. If `parameters` is an empty list, `kwargs` stays that way too, and so the call `funcs[key](**{})` equals to `funcs[key]()` as so:

```python
parameters = list(funcs[key].__code__.co_varnames)
kwargs = {}
if 'proxy' in parameters:
    kwargs['proxy'] = proxy
if 'grains' in parameters:
    kwargs['grains'] = grains_data
ret = funcs[key](**kwargs)
```


@author:
good point, and it looks like you can pass an empty kwargs to a function that doesn't take any arguments, TIL

diff_hunk: @@ -751,8 +751,14 @@ def grains(opts, force_refresh=False, proxy=None):
             # proxymodule for retrieving information from the connected
             # device.
             log.trace('Loading %s grain', key)
-            if funcs[key].__code__.co_argcount == 1:
-                ret = funcs[key](proxy)
+            parameters = list(funcs[key].__code__.co_varnames)
+            if funcs[key].__code__.co_argcount > 0:
path: salt/loader.py
text: @user1:
The `> 0` is not really needed. :-) Enough here `if parameters:`. But the whole check is also not needed (see below).