[BUG] __salt__ and __pillar__ not available in execution modules when function calls are chained

[binocvlar]

Description

https://docs.saltstack.com/en/latest/ref/modules/index.html seems to be rather misleading:

1. __salt__ and __pillar__ are not available in execution module functions called from another file in the module. E.g. lumberjack.is_ok() has access to __salt__ and __pillar__, but sleep.all_night() (which is within the module lumberjack, and is called by lumberjack.is_ok()) cannot access __salt__ or __pillar__

2. When calling module functions with salt-call, it seems as though modules cannot be more than one level deep (i.e. you can call foo.bar(), but not foo.bar.baz())

Setup

Directory structure of the execution module called lumberjack:

lumberjack/
├── __init__.py
├── sleep
│   └── __init__.py
└── work
    └── __init__.py

2 directories, 3 files

lumberjack/__init__.py:

from . import sleep, work

def is_ok(person):
    """ Checks whether a person is really a lumberjack """
    return sleep.all_night(person) and work.all_day(person)

lumberjack/sleep/__init__.py:

def all_night(person):
    # This is just here to show that __grains__ doesn't exist when function calls are chained
    _grains_test = __grains__["saltversion"]
    return bool(person)

lumberjack/work/__init__.py:

def all_day(person):
    return bool(person)

Steps to Reproduce the behavior

1. Add the lumberjack directory as described into _modules/
2. Run salt-call saltutil.sync_all on a minion
3. Run salt-call lumberjack.is_ok "Michael Palin" on the same minion

Note that adding all functions into the same file seems to fix the first issue (i.e. __salt__ and __pillar__ missing)

Expected behavior I expected __grains__ et al to be available from other functions within the same module. I also expected to be able to structure my module arbitrarily deep. If there's a technical reason why either of these expectations weren't met, then I'd expect the documentation to make these caveats clear.

Screenshots

root@ubuntu-focal:~# salt-call lumberjack.is_ok "Michael Palin"
[ERROR   ] An un-handled exception was caught by salt's global exception handler:
NameError: name '__grains__' is not defined
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    load_entry_point('salt==3001.1', 'console_scripts', 'salt-call')()
  File "/usr/lib/python3/dist-packages/salt/scripts.py", line 445, in salt_call
    client.run()
  File "/usr/lib/python3/dist-packages/salt/cli/call.py", line 58, in run
    caller.run()
  File "/usr/lib/python3/dist-packages/salt/cli/caller.py", line 121, in run
    ret = self.call()
  File "/usr/lib/python3/dist-packages/salt/cli/caller.py", line 228, in call
    ret["return"] = self.minion.executors[fname](
  File "/usr/lib/python3/dist-packages/salt/executors/direct_call.py", line 12, in execute
    return func(*args, **kwargs)
  File "/var/cache/salt/minion/extmods/modules/lumberjack/__init__.py", line 8, in is_ok
    return sleep.all_night(person) and work.all_day(person)
  File "/var/cache/salt/minion/extmods/modules/lumberjack/sleep/__init__.py", line 3, in all_night
    _grains_test = __grains__["saltversion"]
NameError: name '__grains__' is not defined
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    load_entry_point('salt==3001.1', 'console_scripts', 'salt-call')()
  File "/usr/lib/python3/dist-packages/salt/scripts.py", line 445, in salt_call
    client.run()
  File "/usr/lib/python3/dist-packages/salt/cli/call.py", line 58, in run
    caller.run()
  File "/usr/lib/python3/dist-packages/salt/cli/caller.py", line 121, in run
    ret = self.call()
  File "/usr/lib/python3/dist-packages/salt/cli/caller.py", line 228, in call
    ret["return"] = self.minion.executors[fname](
  File "/usr/lib/python3/dist-packages/salt/executors/direct_call.py", line 12, in execute
    return func(*args, **kwargs)
  File "/var/cache/salt/minion/extmods/modules/lumberjack/__init__.py", line 8, in is_ok
    return sleep.all_night(person) and work.all_day(person)
  File "/var/cache/salt/minion/extmods/modules/lumberjack/sleep/__init__.py", line 3, in all_night
    _grains_test = __grains__["saltversion"]
NameError: name '__grains__' is not defined

Versions Report

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) ``` Salt Version: Salt: 3001.1 Dependency Versions: cffi: Not Installed cherrypy: Not Installed dateutil: 2.7.3 docker-py: Not Installed gitdb: 2.0.6 gitpython: 3.0.7 Jinja2: 2.10.1 libgit2: Not Installed M2Crypto: Not Installed Mako: Not Installed msgpack-pure: Not Installed msgpack-python: 0.6.2 mysql-python: Not Installed pycparser: Not Installed pycrypto: 2.6.1 pycryptodome: 3.6.1 pygit2: Not Installed Python: 3.8.2 (default, Jul 16 2020, 14:00:26) python-gnupg: 0.4.5 PyYAML: 5.3.1 PyZMQ: 18.1.1 smmap: 2.0.5 timelib: Not Installed Tornado: 4.5.3 ZMQ: 4.3.2 System Versions: dist: ubuntu 20.04 focal locale: utf-8 machine: x86_64 release: 5.4.0-45-generic system: Linux version: Ubuntu 20.04 focal ```

Additional context Add any other context about the problem here.

[max-arnold]

I'd avoid using nested modules and stick with a flat module structure (like with the built-in modules). The Salt Loader has the following constraints (design decisions):

1. It can only call top-level modules (hence the module.function notation for salt-call and salt['module.function'] for Jinja)
2. It will only inject the magic dunder dictionaries (__salt__, __pillar__, __grains__, etc) into functions that are loaded through it (not imported directly), so you have to use the __salt__['module.function'] to make calls across module boundaries

So, to fix the example code you would need:

1. Flatten the module structure, e.g., lumberjack.py, lumberjack_work.py, and lumberjack_sleep.py
2. Call the module functions using the __salt__ dunder:

def is_ok(person):
    """ Checks whether a person is really a lumberjack """
    return __salt__['lumberjack_sleep.all_night'](person) and __salt__['lumberjack_work.all_day'](person)

I guess the documentation you linked is a bit misleading (the example is clearly unable to handle your use-case).

[binocvlar]

Thanks for your feedback @max-arnold : FWIW, I've already changed my code to have a flat structure, and am aware (through experimentation/reading the code) that the dunder dictionaries are only injected into the the outermost layer of the module.

The primary issues that I see are as follows:

1. The documentation doesn't convey these limitations to the user, leaving them to discover this for themselves (leading to a lot of wasted time and frustration, when considering the size of the userbase).
2. The flat structure and lack of dunder dictionaries vastly reduces the usability and power of the modules system.

With respect to point 2, an improved interface for interacting with pillar, grains etc would be a massive improvement. Given that we're in python at this point, it would be nice if we could simply import this data, or make a function call which returns it. This would be way better than dunder dictionaries which are injected at runtime (my IDE rightly hates references to undefined variables, for example), but I'm aware that this particular ship has probably sailed... Point is, I don't actually care that the dunder dictionaries aren't present - what makes life difficult is that I don't think there is a supported alternative method to obtain the data that they contain.

[garethgreenaway]

@binocvlar Thanks for the report. And thank you to @max-arnold for the explanation on the current limitation in the Salt loader. A lot of these limitations are fixed with some of the new ideas going into POP. I'm labeling this as a documentation bug as we should be calling out that the various Salt dunder functions are not available in any nested files.

[max-arnold]

I was about to leave a comment about POP, but Gareth beat me :)

POP/Idem are more Pythonic, so the magic dunders are gone. Instead, each function receives an explicit Hub argument (Explicit is better than implicit): https://pop-book.readthedocs.io/en/latest/main/hub.html This is much more IDE- and import-friendly.

Also, Salt Magnesium (3002) is going to ship with the experimental Idem support: https://github.com/saltstack/salt/pull/58119