Skip to content
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

[PoC] Allow connection plugins to decline pipelining dynamically #78065

Draft
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

sivel
Copy link
Member

@sivel sivel commented Jun 15, 2022

SUMMARY

Allow connection plugins to decline pipelining dynamically

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

Allows a plugin, regardless of the pipelining configuration, to decline the ability to use pipelining dynamically. Such as the event where -tt or -o RequestTTY=force is providing in ansible_ssh_extra_args

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.14 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 15, 2022
@bcoca
Copy link
Member

bcoca commented Jun 15, 2022

We already have a class attribute has_pipelining that is checked (but mostly for module execution, that is bypassed/ignored for discovery). Which discovery seems to already look at https://github.com/ansible/ansible/blob/devel/lib/ansible/executor/interpreter_discovery.py#L99

We could disable pipelining dynamically by updating this attribute instead of creating a new check function, then just use this in the discovery code.

@@ -398,6 +399,10 @@
SSHPASS_AVAILABLE = None
SSH_DEBUG = re.compile(r'^debug\d+: .*')

tty_parser = argparse.ArgumentParser()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

he, i do love the use of argparse to deal with 'figuring out' the ssh args passed
+1

@sivel
Copy link
Member Author

sivel commented Jun 16, 2022

We already have a class attribute has_pipelining that is checked

I was conflicted on whether or not to use that. I was operating off of the assumption that has_pipelining indicated that the plugin supported it, as opposed to some dynamic decision about whether it could be used in a specific instance, so I didn't want to overload.

We can discuss this more.

But I did overlook that line in interpreter discovery, so I could at minimum move the additional check there, and out of ActionBase for discovery.

@bcoca
Copy link
Member

bcoca commented Jun 16, 2022

plugins that support toggle for 'pipelinging' now do it in config, should we integrate this into that?

@nitzmahone nitzmahone removed the needs_triage Needs a first human triage before being processed. label Jun 16, 2022
@bcoca bcoca mentioned this pull request Jun 21, 2022
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 29, 2022
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.14 bug This issue/PR relates to a bug. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants