Raise exception if command timeout is triggered #43078

ganeshrn · 2018-07-20T10:48:17Z

SUMMARY

If persistent connection timeout is triggered, riase
exception which will be send over socket to module code
instead of silently shutting down the socket.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

bin/ansible-connection

ANSIBLE VERSION

2.7

ADDITIONAL INFORMATION

Before:

ok: [ios] => {
    "changed": false,
    "invocation": {
        "module_args": {
            "auth_pass": null,
            "authorize": null,
            "commands": [
                "show running-config all"
            ],
            "host": null,
            "interval": 1,
            "match": "all",
            "password": null,
            "port": null,
            "provider": null,
            "retries": 10,
            "ssh_keyfile": null,
            "timeout": null,
            "username": null,
            "wait_for": null
        }
    },
    "stdout": [
        "None"
    ],
    "stdout_lines": [
        [
            "None"
        ]
    ]
}

After:

[ios]: FAILED! => {
    "changed": false,
    "module_stderr": "Traceback (most recent call last):\n  File \"/var/folders/8l/27cd4l3n2vnc2cpk3498__680000gn/T/ansible_BoEKEj/ansible_module_ios_command.py\", line 247, in <module>\n    main()\n  File \"/var/folders/8l/27cd4l3n2vnc2cpk3498__680000gn/T/ansible_BoEKEj/ansible_module_ios_command.py\", line 217, in main\n    responses = run_commands(module, commands)\n  File \"/var/folders/8l/27cd4l3n2vnc2cpk3498__680000gn/T/ansible_BoEKEj/ansible_modlib.zip/ansible/module_utils/network/ios/ios.py\", line 125, in run_commands\n  File \"/var/folders/8l/27cd4l3n2vnc2cpk3498__680000gn/T/ansible_BoEKEj/ansible_modlib.zip/ansible/module_utils/connection.py\", line 149, in __rpc__\nansible.module_utils.connection.ConnectionError: command timeout triggered, timeout value is 1 secs\n",
    "module_stdout": "",
    "msg": "MODULE FAILURE",
    "rc": 1
}

ansibot · 2018-07-20T12:31:43Z

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

bin/ansible-connection:150:8: unreachable Unreachable code
bin/ansible-connection:159:8: unreachable Unreachable code
bin/ansible-connection:168:8: unreachable Unreachable code

click here for bot help

Qalthos

I would put these in a common method in case we want to change it later or add another handler. The handlers are still fine, but something like display_and_quit(msg) would cut down on repetition

Qalthos · 2018-07-20T14:15:50Z

bin/ansible-connection

-                        % self.connection.get_option('persistent_connect_timeout'), log_only=True)
+        msg = 'persistent connection idle timeout triggered, timeout value is %s secs' % self.connection.get_option('persistent_connect_timeout')
+        display.display(msg, log_only=True)
+        raise Exception(msg)


Also, ansibot is right, you can't raise an exception here and still call shudown. If you really want the exception raised, you'd have to do it after shutdown

Yes, that's right I missed it completely. Though the issue here is after shutdown there is no active socket over which message can be sen to module side. Looking at alternate ways to implement this functionality.

Fixes ansible#43076 If persistent connection timeout is triggered, riase exception which will be send over socket to module code instead of silently shutting down the socket.

ganeshrn · 2018-07-24T13:45:13Z

ready_for_review

ganeshrn · 2018-07-24T14:49:31Z

@Qalthos Socket shutdown after timeout is triggered is handled here, that's the reason after raising exception, it is not required to explicitly call shutdown

ganeshrn · 2018-07-25T18:34:06Z

@samccann As per our offline discussion added more context to error message. New message looks as below

fatal: [ios01]: FAILED! => {
    "changed": false,
    "invocation": {
        "module_args": {
            "auth_pass": null,
            "authorize": null,
            "commands": [
                "show running-config all",
                "show version"
            ],
            "host": null,
            "interval": 1,
            "match": "all",
            "password": null,
            "port": null,
            "provider": null,
            "retries": 10,
            "ssh_keyfile": null,
            "timeout": null,
            "username": null,
            "wait_for": null
        }
    },
    "msg": "command timeout triggered, timeout value is 1 secs.\nFor more details refer https://docs.ansible.com/ansible/latest/network/user_guide/network_debug_troubleshooting.html#timeout-issues"
}

ansibot · 2018-07-26T04:44:30Z

cc @gdpak @gundalow @privateip @rcarrillocruz @trishnaguha
click here for bot help

samccann · 2018-07-26T13:38:47Z

@ganeshrn - I'm a little worried about embedding a website URL inside an error message. If the website changes, the error message becomes outdated. @acozine your thoughts here? Perhaps the error message should just mention the command(s) that could fix this timeout issue?

Qalthos · 2018-07-26T22:04:51Z

bin/ansible-connection


    def handler(self, signum, frame):
-        display.display('signal handler called with signal %s' % signum, log_only=True)
-        self.shutdown()
+        msg = 'signal handler called with signal %s.\nFor more details refer %s' % (signum, TIMEOUT_TROUBLESHOOT_URL)


The url discussion aside, I really don't think we need it here- this is not a timeout, it's SIGTERM.

ganeshrn · 2018-07-27T05:39:02Z

@samccann Any thoughts on what should be the error message. As mentioned in the troubleshooting doc there are 3 different ways to increase the timeout value and I am not sure how to fit it in a single error message.

samccann · 2018-07-27T14:22:22Z

@ganeshrn How about. "See command_timeout, timeout, or ansible_command_timeout options to resolve this issue." At least that way, the customer has a string to look up on docs.ansible.com to find the information. And leaves us the flexibility to restructure content in the future w/o ending up with broken URL links in error messages.

ganeshrn · 2018-07-27T16:37:27Z

@samccann
The timeout value to use also depends on connection type used (for legacy reasons)

command_timeout, timeout (provider timeout), and ANSIBLE_PERSISTENT_COMMAND_TIMEOUT works with local connection type
command_timeout , ansible_command_timeout and ANSIBLE_PERSISTENT_COMMAND_TIMEOUT work with netconf and network_cli connection type

Mentioning all the 3 options in error message might be confusing to users

samccann · 2018-07-27T17:52:25Z

Okay then perhaps "See the timeout setting options in the Network Debug and Troubleshooting Guide."

ganeshrn · 2018-07-27T20:54:49Z

ready_for_review

ganeshrn requested review from Qalthos, rcarrillocruz and privateip July 20, 2018 10:48

ganeshrn added networking Network category and removed needs_triage Needs a first human triage before being processed. labels Jul 20, 2018

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

Qalthos reviewed Jul 20, 2018

View reviewed changes

Qalthos suggested changes Jul 20, 2018

View reviewed changes

ganeshrn added 3 commits July 24, 2018 18:45

Raise exception if command timeout is triggered

a27d83d

Fixes ansible#43076 If persistent connection timeout is triggered, riase exception which will be send over socket to module code instead of silently shutting down the socket.

Fix CI failure

e9fad3f

Fix review comment

6e405e7

ganeshrn force-pushed the command_timeout_raise branch from 9fae997 to 6e405e7 Compare July 24, 2018 13:16

Fix CI failure

62fb3a4

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 24, 2018

Qalthos mentioned this pull request Jul 25, 2018

Network Working Group Meeting Agenda 2 ansible/community#247

Closed

Fix review comment

d7d2bc5

ganeshrn requested a review from samccann July 25, 2018 18:36

ganeshrn added this to Docsite work in Ansible-maintained Collections Documentation via automation Jul 25, 2018

Qalthos suggested changes Jul 26, 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 Jul 26, 2018

Fix review comment

de12207

samccann approved these changes Jul 30, 2018

View reviewed changes

Qalthos approved these changes Aug 1, 2018

View reviewed changes

ganeshrn merged commit 3f3101d into ansible:devel Aug 2, 2018

Ansible-maintained Collections Documentation automation moved this from Docsite work to Done Aug 2, 2018

ganeshrn deleted the command_timeout_raise branch August 2, 2018 06:38

gundalow added this to Needs Triage in Networking Dec 7, 2018

ikhan2010 moved this from Needs Triage to 2.8 MUST in Networking Dec 10, 2018

ikhan2010 moved this from 2.8 MUST to Done in Networking Dec 10, 2018

ansible locked and limited conversation to collaborators Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise exception if command timeout is triggered #43078

Raise exception if command timeout is triggered #43078

ganeshrn commented Jul 20, 2018

ansibot commented Jul 20, 2018

Qalthos left a comment

Qalthos Jul 20, 2018 •

edited

ganeshrn Jul 20, 2018

ganeshrn commented Jul 24, 2018

ganeshrn commented Jul 24, 2018 •

edited

ganeshrn commented Jul 25, 2018

ansibot commented Jul 26, 2018

samccann commented Jul 26, 2018

Qalthos Jul 26, 2018

ganeshrn Jul 27, 2018

ganeshrn commented Jul 27, 2018 •

edited

samccann commented Jul 27, 2018

ganeshrn commented Jul 27, 2018

samccann commented Jul 27, 2018

ganeshrn commented Jul 27, 2018

Raise exception if command timeout is triggered #43078

Raise exception if command timeout is triggered #43078

Conversation

ganeshrn commented Jul 20, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Jul 20, 2018

Qalthos left a comment

Choose a reason for hiding this comment

Qalthos Jul 20, 2018 • edited

Choose a reason for hiding this comment

ganeshrn Jul 20, 2018

Choose a reason for hiding this comment

ganeshrn commented Jul 24, 2018

ganeshrn commented Jul 24, 2018 • edited

ganeshrn commented Jul 25, 2018

ansibot commented Jul 26, 2018

samccann commented Jul 26, 2018

Qalthos Jul 26, 2018

Choose a reason for hiding this comment

ganeshrn Jul 27, 2018

Choose a reason for hiding this comment

ganeshrn commented Jul 27, 2018 • edited

samccann commented Jul 27, 2018

ganeshrn commented Jul 27, 2018

samccann commented Jul 27, 2018

ganeshrn commented Jul 27, 2018

Qalthos Jul 20, 2018 •

edited

ganeshrn commented Jul 24, 2018 •

edited

ganeshrn commented Jul 27, 2018 •

edited