Fix ssh connection plugin hiding module errors with 255 exit code #41716

abadger · 2018-06-20T00:09:41Z

Since we no longer have to support Python-2.4 on the client, we can now
use -m to invoke the ansible module. This is possible because of the
changes to the -m switch in PEP-0338:

https://www.python.org/dev/peps/pep-0338/

The change to the ssh connection plugin is because it is overzealous in
thinking that error code 255 can only come from ssh. Python-2.6 can
return 255 in some circumstances and apparently any php error does as
well.

IMPORTANT NOTE: With this change, __file__ referenced in the module changes. It becomes an "offset" into the zipfile (/path/to/ansible_modlib.zip/ansible_module_ping.py for instance). This is an incompatible change but it's not expected that most third party modules will be using that. We'll have to decide whether this is an acceptable backwards incompatible break.

We found a way to only invoke python once ( #41749 ) so the -m portion of this PR is no longer necessary.

The removal of file is still a nice cleanup and the ssh connection plugin fix for modules using exit code 255 is still good.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

executor/module_common.py
plugins/connection/ssh.py

ANSIBLE VERSION

devel

bcoca · 2018-06-20T00:26:53Z

lib/ansible/executor/module_common.py

@@ -159,7 +159,7 @@ def invoke_module(module, modlib_path, json_params):
    else:
        os.environ['PYTHONPATH'] = modlib_path

-    p = subprocess.Popen([%(interpreter)s, module], env=os.environ, shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE)
+    p = subprocess.Popen([%(interpreter)s, '-m', module], env=os.environ, shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE)


wouldnt you be able to capture the 255 error here?

Talked about this on slack and decided not too.

(1) Then it only works for ansiballz, nothing else (for instance, can't fix php).
(2) it introduces parsing and scraping into the wrapper which is a bad environment for debugging anything.
(3) the problem lies with the ssh connection plugin deciding that error code 255 is special so we have to teach the ssh connection plugin when error 255 is not special.

its not the ssh connection plugin thinking that, it is ssh itself, the plugin just follows what the cli tool does.

I don't see why you need parsing since we already capture the exit code.

nitzmahone · 2018-06-20T01:00:44Z

This also potentially has implications for one of the things I was recommending in the plugins redux doc about teaching Ansiballz to use relative imports for internal module_utils inside packaged modules (to allow other plugins to use module_utils as well without hardcoding the namespace). Relative imports and -m don't play nice together under many circumstances... Though since we're hacking up the path at execution time, we might be able to make it work. Would need to do a little experiementation.

webknjaz · 2018-06-20T12:12:54Z

changelogs/fragments/ssh-255-and-python-m.yaml

+bugfixes:
+- If a module exited with error 255 When using the ssh connection plugin, the
+  connection plugin would swallow the error and say we had an ssh error instead.
+  The new code fixes this for select PHP errors and Python tracebacks.


PHP? PHP as a PHP language? 0_O
Where does that come from?

Modules can be any language

webknjaz · 2018-06-20T12:24:43Z

lib/ansible/plugins/connection/ssh.py

@@ -347,7 +352,17 @@ def wrapped(self, *args, **kwargs):
                if return_tuple[0] != 255:
                    break
                else:
+                    SSH_ERROR = True
+                    for signature in NOT_SSH_ERRORS:


I think this would be more elegant with any() semantics + as efficient because of generator:

if not any((s in return_tuple[1]) for s in NOT_SSH_ERRORS): raise AnsibleConnectionFailure("Failed to connect to the host via ssh: %s" % to_native(return_tuple[2]))

or

if all((s not in return_tuple[1]) for s in NOT_SSH_ERRORS): raise AnsibleConnectionFailure("Failed to connect to the host via ssh: %s" % to_native(return_tuple[2]))

One liners are less readable than conditionals. In general we should prefer to use loops and if (sivel and I commented as much on one of your PRs a few weeks ago IIRC). This is also less efficient in general as it has to process every element in the data instead of breaking early. (Might be a wash here as there's only two elements and we're trading a loop in Python for a loop in C).

list comprehensions and generator expressions are good for list processing tasks. It's when they are used to combine loops and conditions that the code starts to smell (or if the amount of logic shoved into the list comprehension becomes too large... although I haven't figured out what the value of "too alre" should be yet).

FTR since all() accepts a generator expression it breaks early :)

webknjaz · 2018-06-20T12:28:35Z

lib/ansible/plugins/connection/ssh.py

@@ -347,7 +352,17 @@ def wrapped(self, *args, **kwargs):
                if return_tuple[0] != 255:


If you drop negation, it will be possible to drop else part:

if return_tuple[0] == 255 and all((s not in return_tuple[1]) for s in NOT_SSH_ERRORS): raise AnsibleConnectionFailure("Failed to connect to the host via ssh: %s" % to_native(return_tuple[2]))

Cool, fixed that. I have to be careful... Every time I touch this, I want to use a for-else. But the double break on top of the for-else makes it confusing for people reading the code ;-)

Honestly I was about to suggest using for-else and then I looked closer :)

webknjaz · 2018-06-20T12:29:54Z

lib/ansible/plugins/connection/ssh.py

@@ -297,6 +297,11 @@
    display = Display()


+NOT_SSH_ERRORS = ('Traceback (most recent call last):',  # Python-2.6 when there's an exception
+                                                         # while invoking a script via -m
+                  'PHP Parse error'  # Php always returns error 255


Could you please have a trailing comma here for the sake of better future diffs?

Yep, will do

* __file__ won't work if we want to invoke modules via -m or if we figure out how to keep modules from hitting the disk with pipelining. * module.tmpdir is the new way to place a file where it will be cleaned automatically. Change format string to not depend on __file__: * cloud/amazon/ec2_elb_lb.py * cloud/amazon/elb_classic_lb.py Use module.tempdir: * packaging/os/apt.py * files/unarchive.py

ansibot · 2018-06-20T13:58:09Z

cc @jsdalton @mgwilliams @pileofrogs
click here for bot help

abadger · 2018-06-20T14:20:51Z

IMPORTANT NOTE: With this change, file referenced in the module changes. It becomes an "offset" into the zipfile (/path/to/ansible_modlib.zip/ansible_module_ping.py for instance). This is an incompatible change but it's not expected that most third party modules will be using that. We'll have to decide whether this is an acceptable backwards incompatible break.

webknjaz · 2018-06-20T16:08:08Z

lib/ansible/modules/files/unarchive.py

            try:
                rsp, info = fetch_url(module, src)
                # If download fails, raise a proper exception
                if rsp is None:
                    raise Exception(info['msg'])

                # open in binary mode for python3
-                f = open(package, 'wb')
+                f = open(new_src, 'wb')


Mind also using a context manager?

Should be a separate PR.

webknjaz · 2018-06-20T16:09:26Z

lib/ansible/plugins/connection/ssh.py

+                    SSH_ERROR = True
+                    for signature in NOT_SSH_ERRORS:
+                        if signature in return_tuple[1]:
+                            SSH_ERROR = False


You can raise from here.

I don't see how. Could you explain?

Oh.. I swapped the logic in my mind. Yeah, no way w/o else:.
(I still think that semantically all() reads more obvious)

mattclay · 2018-06-27T18:51:59Z

CI failure in integration tests: https://app.shippable.com/github/ansible/ansible/runs/70845/24/tests

The ssh connection plugin is overzealous in thinking that error code 255 can only come from ssh. Python can return 255 in some circumstances and error from php does as well.

ryansb · 2018-07-09T20:40:47Z

ELB changes look good to me

abadger · 2018-07-09T22:51:43Z

Merged to devel for the 2.7.0 release

ansibot added affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 20, 2018

bcoca reviewed Jun 20, 2018

View reviewed changes

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jun 20, 2018

webknjaz reviewed Jun 20, 2018

View reviewed changes

abadger force-pushed the ansiballz-less-disk branch from ae0c31d to 05781d2 Compare June 20, 2018 13:56

ansibot added aws cloud module This issue/PR relates to a module. support:certified This issue/PR relates to certified code. m:unarchive This issue/PR relates to the unarchive module. and removed needs_triage Needs a first human triage before being processed. labels Jun 20, 2018

webknjaz reviewed Jun 20, 2018

View reviewed changes

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jun 27, 2018

abadger force-pushed the ansiballz-less-disk branch from 05781d2 to 8fbc0db Compare July 3, 2018 17:22

abadger changed the title ~~Use -m to invoke the ansible module~~ Fix ssh connection plugin hiding module errors with 255 exit code Jul 3, 2018

Separate some 255 exit codes that are not ssh errors

2c5a373

The ssh connection plugin is overzealous in thinking that error code 255 can only come from ssh. Python can return 255 in some circumstances and error from php does as well.

abadger force-pushed the ansiballz-less-disk branch from 8fbc0db to 2c5a373 Compare July 3, 2018 17:28

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 3, 2018

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 3, 2018

ryansb approved these changes Jul 9, 2018

View reviewed changes

abadger merged commit 673c55f into ansible:devel Jul 9, 2018

abadger deleted the ansiballz-less-disk branch July 9, 2018 22:51

ansible locked and limited conversation to collaborators Jul 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ssh connection plugin hiding module errors with 255 exit code #41716

Fix ssh connection plugin hiding module errors with 255 exit code #41716

abadger commented Jun 20, 2018 •

edited

bcoca Jun 20, 2018

abadger Jun 20, 2018

bcoca Jun 20, 2018

nitzmahone commented Jun 20, 2018

webknjaz Jun 20, 2018

abadger Jun 20, 2018

webknjaz Jun 20, 2018 •

edited

webknjaz Jun 20, 2018

abadger Jun 20, 2018

webknjaz Jun 20, 2018

webknjaz Jun 20, 2018

abadger Jun 20, 2018

abadger Jun 20, 2018

webknjaz Jun 20, 2018

webknjaz Jun 20, 2018

abadger Jun 20, 2018

abadger Jun 20, 2018

ansibot commented Jun 20, 2018

abadger commented Jun 20, 2018

webknjaz Jun 20, 2018

abadger Jun 20, 2018

webknjaz Jun 20, 2018

abadger Jun 20, 2018

webknjaz Jun 21, 2018

mattclay commented Jun 27, 2018

ryansb commented Jul 9, 2018

abadger commented Jul 9, 2018

		@@ -347,7 +352,17 @@ def wrapped(self, args, *kwargs):
		if return_tuple[0] != 255:

Fix ssh connection plugin hiding module errors with 255 exit code #41716

Fix ssh connection plugin hiding module errors with 255 exit code #41716

Conversation

abadger commented Jun 20, 2018 • edited

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitzmahone commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz Jun 20, 2018 • edited

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

Choose a reason for hiding this comment

ansibot commented Jun 20, 2018

abadger commented Jun 20, 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

Choose a reason for hiding this comment

mattclay commented Jun 27, 2018

ryansb commented Jul 9, 2018

abadger commented Jul 9, 2018

abadger commented Jun 20, 2018 •

edited

webknjaz Jun 20, 2018 •

edited