Avoid logging inside the varstack virtual function #48436

max-arnold · 2018-07-04T09:20:21Z

What does this PR do?

Removes the [ERROR ] Can't find varstack master_top from logs when varstack is not used. This seems to be the recommended practice: https://docs.saltstack.com/en/latest/ref/modules/#logging-restrictions

Tests written?

No

Commits signed with GPG?

No

isbm · 2018-07-04T11:52:02Z

salt/tops/varstack.py

 # Define the module's virtual name
 __virtualname__ = 'varstack'


 def __virtual__():
    if not HAS_VARSTACK:
-        log.error("Can't find varstack master_top")
-        return False
+        return (False, 'The varstack master_tops plugin could not be loaded: varstack module not found.')


Parenthesis are extra here. Please remove them.

isbm · 2018-07-04T11:53:58Z

salt/tops/varstack.py

@@ -45,27 +45,20 @@

 from __future__ import absolute_import, print_function, unicode_literals

-# Import python libs
-import logging
-
 try:
    HAS_VARSTACK = False
    import varstack
    HAS_VARSTACK = True
 except ImportError:
    pass



If you already doing this, then also please remove these HAS_FOO vars as well. As simple as that:

try: import varstack except ImportError: varstack = None def __virtual__(): return bool(varstack), 'The varstack module could not be loaded: varstack dependency is missing.'

Note, I've updated the error message. Feel free to just copypaste all that. 😉

Not quite the same. Original code returned __virtualname__ on success.

isbm · 2018-07-04T11:55:51Z

salt/tops/varstack.py

 try:
    HAS_VARSTACK = False
    import varstack
    HAS_VARSTACK = True
 except ImportError:
    pass

-# Set up logging
-log = logging.getLogger(__name__)
-
 # Define the module's virtual name
 __virtualname__ = 'varstack'


You don't need that.

@isbm Why? The module does not use any logging, so I removed it.

isbm · 2018-07-04T11:59:52Z

salt/pillar/varstack_pillar.py

@@ -31,16 +29,14 @@
 except ImportError:
    pass


try: import varstack except ImportError: varstack = None def __virtual__(): return bool(varstack), 'message here...'

isbm · 2018-07-04T12:00:13Z

salt/pillar/varstack_pillar.py

@@ -31,16 +29,14 @@
 except ImportError:
    pass

-# Set up logging
-log = logging.getLogger(__name__)

 # Define the module's virtual name
 __virtualname__ = 'varstack'


You don't need that.

The module does not use any logging at all (or I'm missing something).

You do not need __virtualname__ if the filename is the same. You use it only in case the module is one of those like zypper/apt/yum/whatever but we refer to it as pkg. Please remove this.

The module filename is varstack_pillar.py and __virtualname__ = 'varstack', so they are different.

Srsly, I never understand why it is called that way. It is salt.pillar.varstack and salt.module.varstack. Why on Earth it needs to be called "Pizza pepperoni with meat pepperoni". Rename the file? Yes, I know there are other files like that. They are subject to rename too. 😉

I;ve just looked up. There are no imports, so it is safe.

Also see https://github.com/saltstack/salt/blob/develop/salt/loader.py#L1789 (virtualname should match the returned value)

This is when you have it:
https://github.com/saltstack/salt/blob/develop/salt/loader.py#L1770

But if you return just True and your filename is properly named, it is fine.

If you have $SALT/modules/example.py:

def __virtual__(): return True, 'Some error message' def hello(): return 'hello'

Then:

salt-call --local example.hello -l error local: hello

But:

def __virtual__(): return 'foo', 'Some error message'

...will render to this:

salt-call --local example.hello -l error [ERROR ] Failed to read the virtual function for module: example

And it won't even load the module.

isbm · 2018-07-04T12:00:32Z

salt/pillar/varstack_pillar.py


 # Define the module's virtual name
 __virtualname__ = 'varstack'


 def __virtual__():
    if not HAS_VARSTACK:
-        return False
+        return (False, 'The varstack_pillar module could not be loaded: varstack module not found.')


Extra parenthesis. Please lookup the example in my other comments.

max-arnold · 2018-07-04T12:31:04Z

Thanks for the feedback, I updated the PR (but not exactly as requested, see my questions above).

isbm · 2018-07-04T12:36:51Z

salt/pillar/varstack_pillar.py

-    return __virtualname__
+    if varstack:
+        return __virtualname__
+    return False, 'The varstack_pillar module could not be loaded: varstack module not found.'


You do not need this. Please return just this:

return bool(varstack), '......

Your module is not any virtual, just True is totally sufficient. In case varstack is None, the error message next to it will be reused.

isbm · 2018-07-04T12:38:08Z

salt/tops/varstack.py

-
-# Set up logging
-log = logging.getLogger(__name__)
+    varstack = None

 # Define the module's virtual name
 __virtualname__ = 'varstack'


You need this only in case you want to rename the module and join it to some other group of those, like pkg for example. Otherwise keep it simple.

We should probably document a standard. There is a mix of both styles at present.

isbm · 2018-07-04T12:38:19Z

salt/tops/varstack.py

-    return __virtualname__
+    if varstack:
+        return __virtualname__
+    return False, 'The varstack module could not be loaded: varstack dependency is missing.'


Same as above. e=mc2 means "errors = more code ** 2" 😉

max-arnold · 2018-07-04T14:33:09Z

@isbm Done

gtmanfred · 2018-07-05T13:51:46Z

doc/man/salt.7

@@ -288913,7 +288913,7 @@ data to return as pillar information. From there you can take a look at the
 varstack on how this file is evaluated.
 .INDENT 0.0
 .TP
-.B salt.pillar.varstack_pillar.ext_pillar(minion_id, pillar, conf)
+.B salt.pillar.varstack.ext_pillar(minion_id, pillar, conf)
 Parse varstack data and return the result
 .UNINDENT


You do not need to make changes to this file.

We regenerate it with each release from all of the other doc strings.

gtmanfred · 2018-07-05T13:53:55Z

salt/tops/varstack.py

-        log.error("Can't find varstack master_top")
-        return False
-    return __virtualname__
+    return bool(varstack), 'The varstack module could not be loaded: varstack dependency is missing.'


This will need to return the virtual name when renamed.

Technically that is not true in this case. When the __virtual__ returns True, the loader will fall back to the module name (i.e. without the .py). In this case, since it is called varstack.py, it would be loaded as varstack. So, it would work, though via a happy accident.

So it should still be changed to something like this:

if varstack is None: return (False, 'The varstack module could not be loaded: varstack dependency is missing.') return __virtualname__

Note that the same change should be made to the external pillar module.

apparently my other comment didn't get posted.

I was also requesting that varstack.py gets renamed back to varstack_pillar.py, so that we do not run into the same problems we have seen with boto/boto3 and docker -> dockermod

But this is really odd. Sounds to me like PHP problems: everything in one pile. Loader problems? Python has namespace, why is that now foo.bar.spam needs to clash with foo.fred.spam?

Guys, what I am missing here?

Also, apparently I jumped over to the varstack tops file when reviewing and didn't realize it.

it isn't a problem with namespaces for modules.

It is a problem that we are doing import varstack inside this module.

https://github.com/saltstack/salt/pull/48436/files#diff-caa464e4e9ee6e852e118724583cc313L29

@terminalmage yeah, in that way it will crash for sure, as it doesn't use the namespace. How we can fix this (without randomising names of the components)? Enforce full import? Reject modules that are so? Other ideas? Because it is nicer to have salt.modules.docker and salt.pillar.docker instead of salt.modules.dockermodule and salt.pillar.docker_this_time_a_pillar.

@gtmanfred well, then just fix the import?

So it should still be changed to something like this:

if varstack is None: return (False, 'The varstack module could not be loaded: varstack dependency is missing.') return __virtualname__

@terminalmage @gtmanfred This is basically what I did in my initial PR, but then changed it based on the feedback.

If necessary, I can resubmit my initial PR which was more idiomatic, as far as I can tell by looking at existing modules.

@max-arnold OK, the loader seem has a problems that we need to tackle them later. The way it is now "fixed" is just that you don't have the same name in the module/pillars. And so when we have varstack in pillars, varstack in modules and varstack in Python, then things gets messy.

In that regard, lets do the following:

Rename (or leave?) salt.pillars.varstack_pillar

Rename salt.modules.varstackmod

varstack — comes from the Python

__virtualname__ = `varstack` def __virtual__(): return (varstack and __virtualname__ or False, 'The varstack module could not be loaded: varstack dependency is missing.')

Then this should work. In a long run there should be either rules of the naming of the modules/pillars/states or a way to stop these sort of clashes.

… varstack_top

max-arnold · 2018-07-09T11:49:34Z

Rebased to the latest develop branch
Renamed salt/tops/varstack.py to salt/tops/varstack_top.py (this way it seems to be more consistent with the corresponding pillar/varstack_pillar.py)
Dropped all documentation changes

gtmanfred · 2018-07-09T13:41:14Z

salt/pillar/varstack_pillar.py

+    return (
+        varstack and __virtualname__ or False,
+        'The varstack module could not be loaded: varstack dependency is missing.'
+    )


This looks weird, what is it going to return? Why are we not just using.

if varstack is None: return False, 'The varstack module could not be loaded: varstack dependency is missing.' return __virtualname__

It is going to return __virtualname__ if varstack is a module, False otherwise. The error message is ignored if everything is OK.

Multiple returns considered bad style in Python, especially if you can avoid that.

I would still contend that the other way is more readable, but whatever

Do you want me to change this part? I see several +1s, and actually, this is how it looked in my initial PR.

you are fine. just waiting for @isbm to 👍 the review.

cachedout · 2018-07-11T17:28:51Z

salt/tops/varstack.py

-
-# Set up logging
-log = logging.getLogger(__name__)
+    varstack = None

 # Define the module's virtual name
 __virtualname__ = 'varstack'


We should probably document a standard. There is a mix of both styles at present.

cachedout · 2018-07-11T17:29:30Z

@isbm Are you OK with this as it sits right now?

isbm

@max-arnold Thanks!
@cachedout While we have to describe the __virtual* standard, we should make sure we are not enforcing workarounds for our monkeypatch problem in Loader, but we should first look there and fix it. We have import clashes and this is bad, and "just rename" is a workaround we would prefer to get rid of. Other then that yes, I am fine as it is now. But we should return to __virtual* topic soon.

max-arnold · 2018-10-05T04:11:37Z

@rallytime I'm getting a warning on the latest develop branch during make -C doc clean html:

WARNING: [autosummary] failed to import u'salt.tops.varstack': no module named salt.tops.varstack

Not sure if this could be fixed by running apidoc-saltmods.sh (it fails for me with ./apidoc-saltmods.sh: 59: ./apidoc-saltmods.sh: 1: Dirname to exclude is required.), but it may be a good idea to add SPHINXOPTS = -W to doc/Makefile to turn any sphinx wanings into errors.

max-arnold requested a review from a team as a code owner July 4, 2018 09:20

ghost self-requested a review July 4, 2018 09:20

isbm suggested changes Jul 4, 2018

View reviewed changes

max-arnold force-pushed the develop branch from 6666ce6 to 3e3eb7e Compare July 4, 2018 12:29

isbm suggested changes Jul 4, 2018

View reviewed changes

max-arnold force-pushed the develop branch 3 times, most recently from a6350ba to 6daa168 Compare July 4, 2018 14:10

gtmanfred suggested changes Jul 5, 2018

View reviewed changes

Drop logging in the varstack __virtual__ function, rename varstack to…

19fbeae

… varstack_top

max-arnold force-pushed the develop branch from 6daa168 to 19fbeae Compare July 9, 2018 11:43

gtmanfred reviewed Jul 9, 2018

View reviewed changes

gtmanfred approved these changes Jul 9, 2018

View reviewed changes

cachedout approved these changes Jul 11, 2018

View reviewed changes

isbm approved these changes Jul 12, 2018

View reviewed changes

Merge branch 'develop' into develop

5f7c6a7

rallytime merged commit 9fbe6d5 into saltstack:develop Jul 12, 2018

Avoid logging inside the varstack __virtual__ function #48436

Avoid logging inside the varstack __virtual__ function #48436

Conversation

max-arnold commented Jul 4, 2018 • edited Loading

What does this PR do?

Tests written?

Commits signed with GPG?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isbm Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-arnold commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isbm Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

max-arnold commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isbm Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isbm Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

max-arnold commented Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

isbm Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cachedout commented Jul 11, 2018

isbm left a comment • edited Loading

Choose a reason for hiding this comment

max-arnold commented Oct 5, 2018

Avoid logging inside the varstack virtual function #48436

Avoid logging inside the varstack virtual function #48436

max-arnold commented Jul 4, 2018 •

edited

Loading

isbm Jul 4, 2018 •

edited

Loading

isbm Jul 4, 2018 •

edited

Loading

isbm Jul 5, 2018 •

edited

Loading

isbm Jul 6, 2018 •

edited

Loading

max-arnold commented Jul 9, 2018 •

edited

Loading

isbm Jul 9, 2018 •

edited

Loading

isbm left a comment •

edited

Loading