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

Bug/docker service stderr3 #20510

Closed
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
6 participants
@shabble

shabble commented Jan 20, 2017

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

docker_service

ANSIBLE VERSION
ansible 2.3.0 (bug/docker_service_stderr3 8e15fe9f76) last updated 2017/01/19 21:05:29 (GMT +100)
  config file = /Users/shabble/work/mhn-automation/ansible.cfg
  configured module search path = Default w/o overrides
SUMMARY

Extends the output-capturing behaviour to include stderr when calling docker-compose as a library, since it typically raises empty exceptions and logs useful diagnostic info to stderr by default. Without access to this output, it is extremely difficult to determine why a docker-compose task might be failing, especially with inline definitions or templated compose-files.

This PR mostly just replicates ansible/ansible-modules-core#5165 and extends the handling to consider stderr as well.

There's a little bit of logic in trying to guess the appropriate message to include in the .fail_json msg parameter, but module stderr and stdout are included unmodified in case it guesses wrongly.

Fixes #20480

Output before (-vvv)

[...]
fatal: [ansibledocker.mhn]: FAILED! => {
    "changed": false,
    "failed": true,
    "invocation": {
        "module_args": {
            "api_version": null,
            "build": false,
            "cacert_path": null,
            "cert_path": null,
            "debug": true,
            "definition": {
                "services": {
                    "testcontainer": {
                        "command": "sleep 100",
                        "image": "busybox",
                        "ports": [
                            "8001:8000"
                        ]
                    },
                    "testcontainer2": {
                        "command": "sleep 100",
                        "image": "busybox",
                        "ports": [
                            "8001:8000"
                        ]
                    }
                },
                "version": "2"
            },
            "dependencies": true,
            "docker_host": null,
            "files": null,
            "filter_logger": false,
            "hostname_check": false,
            "key_path": null,
            "nocache": false,
            "project_name": "foo",
            "project_src": null,
            "pull": false,
            "recreate": "smart",
            "remove_images": null,
            "remove_orphans": false,
            "remove_volumes": false,
            "restarted": false,
            "scale": null,
            "services": null,
            "ssl_version": null,
            "state": "present",
            "stopped": false,
            "timeout": 10,
            "tls": null,
            "tls_hostname": null,
            "tls_verify": null
        },
        "module_name": "docker_service"
    },
    "module_stderr": "",
    "module_stdout": "",
    "msg": "Error starting project "
}

Using testcase from bug in #20480

Output after:

fatal: [ansibledocker.mhn]: FAILED! => {
    "changed": false,
    "errors": [
        "ERROR: for testcontainer2  Cannot start service testcontainer2: driver failed programming external connectivity on endpoint foo_testcontainer2_1 (d1b78ff057e5e00b8c2711781292ba76eff9928024e944dc275ecc76b98f9b20): Bind for 0.0.0.0:8001 failed: port is already allocated"
    ],
    "failed": true,
    "invocation": {
        "module_args": {
            "api_version": null,
            "build": false,
            "cacert_path": null,
            "cert_path": null,
            "debug": true,
            "definition": {
                "services": {
                    "testcontainer": {
                        "command": "sleep 100",
                        "image": "busybox",
                        "ports": [
                            "8001:8000"
                        ]
                    },
                    "testcontainer2": {
                        "command": "sleep 100",
                        "image": "busybox",
                        "ports": [
                            "8001:8000"
                        ]
                    }
                },
                "version": "2"
            },
            "dependencies": true,
            "docker_host": null,
            "files": null,
            "filter_logger": false,
            "hostname_check": false,
            "key_path": null,
            "nocache": false,
            "project_name": "foo",
            "project_src": null,
            "pull": false,
            "recreate": "smart",
            "remove_images": null,
            "remove_orphans": false,
            "remove_volumes": false,
            "restarted": false,
            "scale": null,
            "services": null,
            "ssl_version": null,
            "state": "present",
            "stopped": false,
            "timeout": 10,
            "tls": null,
            "tls_hostname": null,
            "tls_verify": null
        },
        "module_name": "docker_service"
    },
    "module_stderr": "\nERROR: for testcontainer2  Cannot start service testcontainer2: driver failed programming external connectivity on endpoint foo_testcontainer2_1 (d1b78ff057e5e00b8c2711781292ba76eff9928024e944dc275ecc76b98f9b20): Bind for 0.0.0.0:8001 failed: port is already allocated\n",
    "module_stdout": "",
    "msg": "Error starting project ERROR: for testcontainer2  Cannot start service testcontainer2: driver failed programming external connectivity on endpoint foo_testcontainer2_1 (d1b78ff057e5e00b8c2711781292ba76eff9928024e944dc275ecc76b98f9b20): Bind for 0.0.0.0:8001 failed: port is already allocated",
    "warnings": []
}
Review considerations
  • Correct use of .encode() when processing output? I'm not totally confident this is currently ideal.
  • Inclusion of empty dicts in returned json? In the example, warnings is empty but still included. I'm not sure if it would make more sense to omit it entirely if so.
@ryansb

This comment has been minimized.

Show comment
Hide comment
@ryansb

ryansb Jan 30, 2017

Member

I think this should have a needs_rebase label since there are conflicts.

Member

ryansb commented Jan 30, 2017

I think this should have a needs_rebase label since there are conflicts.

@ansibot ansibot added the bot_broken label Jan 30, 2017

@mattclay mattclay removed the bot_broken label Feb 15, 2017

@mattclay

This comment has been minimized.

Show comment
Hide comment
@mattclay

mattclay Feb 15, 2017

Member

I've removed the comment and label telling the bot it is broken so we can see if it properly applies the needs_rebase label.

Member

mattclay commented Feb 15, 2017

I've removed the comment and label telling the bot it is broken so we can see if it properly applies the needs_rebase label.

@mattclay

This comment has been minimized.

Show comment
Hide comment
@mattclay

mattclay Feb 15, 2017

Member

Looks like the bot is working properly, as needs_rebase was added.

Member

mattclay commented Feb 15, 2017

Looks like the bot is working properly, as needs_rebase was added.

docker_service: Forward stderr-based output from docker-compose
PR #5165 at ansible/ansible-modules-core#5165
adds redirection and capture of stdout during execution of
docker-compose.

This doesn't necessarily catch all errors, since some are printed to
stderr and lost.

This extends the redirection to include stderr, and does minor string
processing to attempt to find a 'useful' message to present as the
final Ansible error.

@ansibot ansibot removed the needs_rebase label Feb 16, 2017

sys.stderr = old_fh
def make_redirection_tempfiles():
_, out_redir_name = tempfile.mkstemp(prefix="ansible")

This comment has been minimized.

@mattclay

mattclay Feb 16, 2017

Member

CI failure due to PEP 8 issue:

2017-02-16 00:21:02 ERROR: PEP 8: lib/ansible/modules/cloud/docker/docker_service.py:525:59: W291 trailing whitespace (current)
@mattclay

mattclay Feb 16, 2017

Member

CI failure due to PEP 8 issue:

2017-02-16 00:21:02 ERROR: PEP 8: lib/ansible/modules/cloud/docker/docker_service.py:525:59: W291 trailing whitespace (current)
@chouseknecht

This comment has been minimized.

Show comment
Hide comment
@chouseknecht

chouseknecht Jun 8, 2017

Member

Merged via #25456

Member

chouseknecht commented Jun 8, 2017

Merged via #25456

@ansibot ansibot added bug and removed bugfix_pull_request labels Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment