Salt: "Writing Execution Modules" is incorrect about __virtual__()

[arnoschuring]

Description of Issue/Question

The documentation on the__virtual__ function is inconsistent, both with itself and with the actual saltstack code.

Steps to Reproduce Issue

See Writing Execution Modules.

The second paragraph under "__virtual__ Function" says:

since __virtual__ is called before the module is loaded, __salt__ will be unavailable as it will not have been packed into the module at this point in time.

But the second example directly following it actually uses __salt__ inside __virtual__():

def __virtual__():
    if 'cheese.slice' in __salt__:
        return 'cheese'
    else:
        return False, 'The cheese state module cannot be loaded: enzymes unavailable.'

A simple test (with 2016.11.3) reveals that yes, __salt__ is indeed available when __virtual__ is called.

Finally, the document also fails to mention the __init__() function. The only way I found out about that one is because my colleagues assured me it is there, but I haven't found it in the documentation yet.

[gtmanfred]

So, __salt__ should not be relied on in __virtual__, because it might not all be loaded by the time you get there, if it is somewhere, we should fix that.

As for __init__, we should definitely document that.
Thanks,
Daniel

[arnoschuring]

Thanks for the response. If __salt__ might be available but is not guaranteed in all cases, I think it's best to state that explicitly in the documentation (as opposed to "will be unavailable"), to prevent people from relying on it anyway.

[cachedout]

I agree with this. That's a good suggested change.

[arnoschuring]

if I can be permitted to ask a somewhat-related follow-up question: the reason I wanted to do this is because I want to initialize my execution module based on pillar data. Can it be guaranteed that __pillar__ (or even __salt__['pillar.get']) is available during an execution module's __virtual__(), or should I better use some lazy-loading mechanism here?

[gtmanfred]

__pillar__ should be in there already.

This is where they are loaded.

https://github.com/saltstack/salt/blob/2016.11/salt/loader.py#L210

And during the init of the lazy loader, it does the __prep_mod_opts function

https://github.com/saltstack/salt/blob/2016.11/salt/loader.py#L1042

Which packs all the pillars.

https://github.com/saltstack/salt/blob/2016.11/salt/loader.py#L1249

Then the __virtual__ isn't actually run until the _load_module function called later when the module is actually loaded.

https://github.com/saltstack/salt/blob/2016.11/salt/loader.py#L1398

So, I believe that __pillar__ should be available.

And from testing this.

[root@salt ~]# tail -c +0 /srv/pillar/*.sls /srv/salt/_modules/testpillar.py
==> /srv/pillar/test.sls <==
pillars: True

==> /srv/pillar/top.sls <==
base:
  '*':
    - test

==> /srv/salt/_modules/testpillar.py <==
def __virtual__():
    return __pillar__.get('pillars', False)

def ping():
    return True
[root@salt ~]# salt-call --local testpillar.ping
local:
    True

It is available

Thanks,
Daniel

[arnoschuring]

That's perfect. Thanks for the extensive answer

[arnoschuring]

I've taken a stab at updating the documentation myself: https://github.com/arnoschuring/salt/tree/execution-module-docs

Not sure if my changes are correct though, I haven't even tried to build the documentation.

[cachedout]

@arnoschuring Could you go ahead and wrap those changes up into a PR and submit them to us? We'll review them there and then we can close this issue. Thanks!