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

Use connection locking to protect against competing host key prompts #13344

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@amenonsen
Contributor

amenonsen commented Nov 28, 2015

We acquire the connection lock before executing ssh, and release it as
soon as the unknown host key prompt is negotiated, or we can be sure it
won't be issued at all. This fixes the problem where many prompts pile
up and compete for input.

Unfortunately, we can only lock against other connections: there's no
output_lockfile that can prevent output from other subsystems. See the
FIXME note.

This problem was the original motivation for PR #12276, but although
the bulk of that PR was merged, the locking changes were not.

Submitted for v2 as requested in #13318

Use connection locking to protect against competing host key prompts
We acquire the connection lock before executing ssh, and release it as
soon as the unknown host key prompt is negotiated, or we can be sure it
won't be issued at all. This fixes the problem where many prompts pile
up and compete for input.

Unfortunately, we can only lock against other connections: there's no
output_lockfile that can prevent output from other subsystems. See the
FIXME note.

This problem was the original motivation for PR #12276, but although
the bulk of that PR was merged, the locking changes were not.

Fixes #13318
@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Nov 29, 2015

Contributor

I really think Ansible should have a "display" lock too, even if it's used only for this feature. When running against 16- or 32- or 64-node clusters, you still have prompts being overrun by other output (i.e., not other prompts, but just the task output from hosts that you approved before). @jimi-c said something about it being possible to set up a shared lock in display before forking, but @bcoca had some objection/concern with doing it that way, which I unfortunately can't remember now.

Contributor

amenonsen commented Nov 29, 2015

I really think Ansible should have a "display" lock too, even if it's used only for this feature. When running against 16- or 32- or 64-node clusters, you still have prompts being overrun by other output (i.e., not other prompts, but just the task output from hosts that you approved before). @jimi-c said something about it being possible to set up a shared lock in display before forking, but @bcoca had some objection/concern with doing it that way, which I unfortunately can't remember now.

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Nov 30, 2015

Member

A couple of issues, iirc. The first had to do with serialization of the locks, they did not work well inside the object so I moved it into the class.

The second has to do with certain things failing when using the class level lock, I noticed this when i initially set this up for the debug printing, I have not had time to trace this (since it only affectes debug) but it seems to be related to multiprocess, stdout cloning and the lock 'global'.

Member

bcoca commented Nov 30, 2015

A couple of issues, iirc. The first had to do with serialization of the locks, they did not work well inside the object so I moved it into the class.

The second has to do with certain things failing when using the class level lock, I noticed this when i initially set this up for the debug printing, I have not had time to trace this (since it only affectes debug) but it seems to be related to multiprocess, stdout cloning and the lock 'global'.

@bcoca bcoca added this to the v2 milestone Nov 30, 2015

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Dec 2, 2015

Contributor

The conflict noted by Github is because stable-2.0 doesn't include the pipelining change from #13200, while devel does. Since this was under consideration for inclusion in stable-2.0, I'm leaving this alone. Anyway, the conflict is trivial to resolve by hand.

Contributor

amenonsen commented Dec 2, 2015

The conflict noted by Github is because stable-2.0 doesn't include the pipelining change from #13200, while devel does. Since this was under consideration for inclusion in stable-2.0, I'm leaving this alone. Anyway, the conflict is trivial to resolve by hand.

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Dec 16, 2015

Contributor

I'm closing this PR because @jimi-c has an alternative implementation at the strategy level that avoids all the locking problems.

Contributor

amenonsen commented Dec 16, 2015

I'm closing this PR because @jimi-c has an alternative implementation at the strategy level that avoids all the locking problems.

@amenonsen amenonsen closed this Dec 16, 2015

@amenonsen amenonsen deleted the amenonsen:ssh-locking branch Dec 17, 2015

@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