wait_for: treat broken connections as "unready"

[sethp-nr]

SUMMARY

We have observed the following condition while waiting for hosts:

Traceback (most recent call last):
  File "/var/folders/f8/23xp00654plcv2b2tcc028680000gn/T/ansible_8hxm4_/ansible_module_wait_for.py", line 585, in <module>
    main()
  File "/var/folders/f8/23xp00654plcv2b2tcc028680000gn/T/ansible_8hxm4_/ansible_module_wait_for.py", line 535, in main
    s.shutdown(socket.SHUT_RDWR)
  File "/usr/local/opt/python/Frameworks/Python.framework/Versions/2.7/lib/python2.7/socket.py", line 228, in meth
    return getattr(self._sock,name)(*args)
socket.error: [Errno 57] Socket is not connected

This appears to happen while the host is still starting; we believe something is
accepting our connection but immediately resetting it. In these cases, we'd
prefer to continue waiting instead of immediately failing the play.

This patch has been applied locally for some time, and we have seen no adverse
effects.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

wait_for module

ANSIBLE VERSION

ansible 2.3.1.0
  config file =
  configured module search path = Default w/o overrides
  python version = 2.7.13 (default, Apr  4 2017, 08:47:57) [GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.38)]

[ansibot]

cc @AnderEnder @gregswift @jarv @jhoekx
click here for bot help

[dagwieers]

Is the wait_for_connection action plugin not what you need ?

[sethp-nr]

Ah! That's a neat new addition in 2.3. I'll take a look at that action, but I think still need this change for at least the case where we're restarting into a system that doesn't have python installed yet, so ping will never run successfully.

[dagwieers]

@sethp-nr From your description it isn't clear what you are testing or what connection you are talking about.

[sethp-nr]

Sorry for the confusion, we've not managed to isolate the ultimate cause but the general flow is as follows:

1. Our ansible play targets a host and makes a change that requires reboot
2. We trigger a shutdown, and use wait_for to await the system's restart
3. The play continues on the rebooted host

The issue this patch addresses is that often step 2 fails (spuriously) because the shutdown call in the module races with packets from the remote host that put the connection in the state that Darwin throws errno 59. Since that happens to say "we can't close this connection because it's not connected", it seems appropriate to say "ok, thanks Darwin, no problem" and continue on rather than raising that transient error and failing the play.

For the common case, we can migrate to using the (relatively new) wait_for_connection, but we'll need to continue to use wait_for in the case that step 1 is "install a new Core OS image" and step 2 is "reboot into that install'd OS". Core OS doesn't ship with python installed, so we need to run a bootstrap play with raw commands before the ping module that wait_for_connection action relies on has any chance of working.

[dagwieers]

Right, for that case wait_for_connection was designed. Testing a TCP port is not sufficient to continue your playbook, you need to know if the transport works end-to-end.

There is an open project I am doing to implement a platform-independent reboot module that would reuse the exact same logic, but would also verify if the system was effectively rebooted. And would have some additional functionality, you can find some information at #16186

There's some preliminary code. I wanted to finish it for v2.4 but too much on my plate...

[sethp-nr]

Indeed, though I'm not sure the changes here and there are mutually exclusive? Even if I was waiting for a service to become available on a port, I suspect the same race would occur between socket shutdown and a RST (or similar) from the peer. Wouldn't it make sense to consider those errors as spurious?

Further, there is at least one case that we know the transport will not work end-to-end until the play continues (because the next thing the play does is install python, which ping requires) – in that case, we must use the wait_for module and muddle through. For at least that reboot case, this change is still important.

I'm glad to hear you're working on the reboot module! That would indeed make our lives better – I read through that thread, and it wasn't clear to me how you were planning to handle our complex case (booting into a system that has SSH, but not much else). Should I leave a comment over there describing that difficulty?

[sethp-nr]

@dagwieers Just wanted to check in – this change is still important for us in at least the case I mentioned in my last comment, and doesn't seem harmful otherwise. Any thoughts?

[dagwieers]

@sethp-nr I am not the person who can decide if this gets merged. If you want to bring this up to the core team, feel free to add your PR to the core team's agenda and be present on the next core meeting. ansible/community#263

[sethp-nr]

Ah, thank you for the pointer!

[abadger]

cherry-picked to the temp-staging-post-2.4.1 branch for the 2.4.2 release.