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

module.wait does not fail when called state fails #30559

Closed
kaidokert opened this issue Jan 22, 2016 · 6 comments
Closed

module.wait does not fail when called state fails #30559

kaidokert opened this issue Jan 22, 2016 · 6 comments
Labels
fixed-pls-verify fix is linked, bug author to confirm fix Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Milestone

Comments

@kaidokert
Copy link
Contributor

Not sure if i'm doing something wrong here, but this does not seem to behave as one would expect.

nginx_config_file_override:
  file.managed:
    - name: /etc/nginx/nginx.conf
    - source: salt://server_tune/files/nginx.conf

test_nginx_config:
  module.wait:
    - name: nginx.configtest
    - watch:
      - file: nginx_config_file_override

Module.wait does get executed when the config file gets updated, but it will always return True regardless if nginx.configtest passes or not.
Sample run:

----------
          ID: nginx_config_file_override
    Function: file.managed
        Name: /etc/nginx/nginx.conf
      Result: True
     Comment: File /etc/nginx/nginx.conf updated
     Started: 19:32:14.123340
    Duration: 45.778 ms
     Changes:
              ----------
              diff:
                  ---
                  +++
                  @@ -8,7 +8,7 @@
                  -foo: defbroke;
                  +eating: furrypandas;
----------
          ID: test_nginx_config
    Function: module.wait
        Name: nginx.configtest
      Result: True
     Comment: Module function nginx.configtest executed
     Started: 19:32:14.170061
    Duration: 10.971 ms
     Changes:
              ----------
              ret:
                  ----------
                  comment:
                      Syntax Error
                  result:
                      False
                  stderr:
                      nginx: [emerg] unknown directive "eating:" in /etc/nginx/nginx.conf:11
                      nginx: configuration file /etc/nginx/nginx.conf test failed

Notice how nginx.configtest returns False, but test_nginx_config sets Result:True. I would have expected the watching state to fail ? Or i'm missing some bits here ?

 salt --versions
                  Salt: 2015.5.3
                Python: 2.7.6 (default, Jun 22 2015, 17:58:13)
                Jinja2: 2.7.2
              M2Crypto: 0.21.1
        msgpack-python: 0.3.0
          msgpack-pure: Not Installed
              pycrypto: 2.6.1
               libnacl: Not Installed
                PyYAML: 3.10
                 ioflo: Not Installed
                 PyZMQ: 14.0.1
                  RAET: Not Installed
                   ZMQ: 4.0.4
                  Mako: Not Installed
               Tornado: Not Installed
 Debian source package: 2015.5.3+ds-1trusty1
@kaidokert
Copy link
Contributor Author

Ok, i traced this down and it seems like this was fixed in ca0be8b

The result dictionary 'result' value was previously not checked at all, only 'retcode'. Not sure if this commit was in response to a previously opened bug or not ?

EDIT: to clarify, this affected module.run too. Easy way to check without any state files:
salt '*' state.single module.run nginx.configtest

The fix is in 2015.8, not in 2015.5 tho

@jfindlay jfindlay added the fixed-pls-verify fix is linked, bug author to confirm fix label Jan 22, 2016
@jfindlay jfindlay added this to the Blocked milestone Jan 22, 2016
@jfindlay jfindlay added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jan 22, 2016
@jfindlay
Copy link
Contributor

@kaidokert, thanks for the report. When Boron is released in two months, 2015.5 will drop out of general support and I'm not sure if we're going to make another release beyond 2015.5.9, we still might be able to backport this to 2015.5 though.

@kaidokert
Copy link
Contributor Author

Understood.
Is there any sort of 'LTS' strategy for versions that will get bugfixes ? Currently upgrading from 2015.5 is not a small step as ubuntu PPA is gone etc.

@kaidokert
Copy link
Contributor Author

Uh oh. Just ran into another issue 07db5a7 with the 2015.5.3 PPA packaged version, that is already fixed in 2015.5.6 but PPA builds arent getting fixes anymore.
I guess its high time to move off launchpad PPA and put a big red warning sign there.

@jfindlay
Copy link
Contributor

@kaidokert, we prioritize bug fixes on the latest stable release, when more than one are supported. 2015.5 and 2015.8 are a special case since they were released 3 months apart. since 2015.8, we've decided to go to 6 month release cycles. Other than this, each release will have LTS features, some of which must purchased with a support contract.

Since @joehealy has moved on, we haven't had an ubuntu community maintainer, although @BABILEN has offered to update debian packages.

@rallytime
Copy link
Contributor

I have back-ported the commit mentioned above that fixes the issue with module.py. I am not sure if we will be releasing from 2015.5 again or not, but if we do, at least now this fix will be there.

Since the discussion about moving from the launchpad PPA has already been addressed in terms of upgrading to new salt versions, I'm going to close this as resolved. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed-pls-verify fix is linked, bug author to confirm fix Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

No branches or pull requests

3 participants