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.

Sign up for GitHub

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

Jump to bottom

task_executor - use correct value for ssh connection retries #75155

Merged
merged 6 commits into from Jul 6, 2021
Merged

task_executor - use correct value for ssh connection retries #75155

merged 6 commits into from Jul 6, 2021

Conversation

samdoran
Copy link
Contributor

@samdoran samdoran commented Jul 1, 2021 •
edited

SUMMARY

Since the task and connection both have the same 'retries' keyword, the task default would override the connection value. Making sure not to pass the task value for 'retries' to the connection options resolves the issue.

Set the default value for ssh connection retries to 0.

Rename the config key from retries to reconnection_retries as a belt and suspenders approach.

Fixes #75142.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/executor/task_executor.py

@samdoran
task_exectuor - use correct value for ssh connection retries
0dabcb1 
Since the task and connection both have the same 'retries' keyword, the task default
would override the connection value.
@samdoran
Set ssh_connection retries default value back to 0
6c5b1e8 
It was 0 before the move to config and was changed to 3 by accident.
@ansibot ansibot added affects_2.12 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. 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 Jul 1, 2021
@samdoran
Cleaner fix
35fe592 
Simply do not pass 'retries' from the task to the connection options.
@samdoran samdoran mentioned this pull request Jul 1, 2021
Closed
1 task
bcoca
bcoca approved these changes Jul 1, 2021
View reviewed changes
Copy link
Member

@bcoca bcoca left a comment

Choose a reason for hiding this comment

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

short term this fix is fine, long term I want to resurrect the keyword config option to allow plugins to map to EXISTING specific keywords vs doing auto-matching on option name, to avoid collisions, falling back to the automap.

In the end, completely solving the issue will require adding a deprecation and removal of the current automap function.

@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Jul 1, 2021
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jul 1, 2021
@samdoran
Change option name
b9025fd 
Belt and suspenders
@samdoran samdoran force-pushed the issue/75142-retries-config-collision branch from e1359ee to f105d7d Compare July 1, 2021 19:56
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 1, 2021
@samdoran
Copy link
Contributor Author

samdoran commented Jul 1, 2021

Ideally we could remove the code in task_executor.py that explicitly deletes the retries key and just rely on renaming the key in the connection plugin. However, there are at least two collections using retries in a connection plugin.

https://github.com/ansible-collections/community.aws/blob/a2bbdeaf474b15e8a9f9b3c706085a3fa1e56464/plugins/connection/aws_ssm.py#L61

https://github.com/ansible-collections/ibm_zos_core/blob/74c0f217ebcd01a2df391f3d71ad544a26b3f916/plugins/connection/zos_ssh.py#L144

I will open PRs to rename the key name in those collections.

This was referenced Jul 1, 2021
Merged
Merged
@Akasurde Akasurde changed the title task_exectuor - use correct value for ssh connection retries task_executor - use correct value for ssh connection retries Jul 5, 2021
@Akasurde Akasurde force-pushed the issue/75142-retries-config-collision branch from f105d7d to e85ef3f Compare July 5, 2021 08:51
@samdoran samdoran merged commit a8de35e into ansible:devel Jul 6, 2021
@samdoran samdoran deleted the issue/75142-retries-config-collision branch July 6, 2021 14:43
samdoran added a commit to samdoran/ansible that referenced this pull request Jul 6, 2021
@samdoran
[stable-2.11] task_executor - use correct value for ssh connection re…
8cca68e 
…tries (ansible#75155)

Since the task and connection both have the same 'retries' keyword, the task default
would override the connection value.

Do not pass 'retries' from the task to the connection options.

* Set ssh_connection retries default value back to 0
  It was 0 before the move to config and was changed to 3 by accident.
(cherry picked from commit a8de35e)

Co-authored-by: Sam Doran <sdoran@redhat.com>
@samdoran samdoran mentioned this pull request Jul 6, 2021
Merged
ansible-zuul bot added a commit to ansible-collections/community.aws that referenced this pull request Jul 7, 2021
@ansible-zuul
Merge pull request #619 from samdoran/connect-retries-rename
9ed46f8 
cdaws_ssm - rename retries setting

SUMMARY

There was a bug in Ansible that would result in task retries overriding the connection retries. Using a different name for the connection retries setting avoids this issue on versions of Ansible that are missing the bug fix.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

plugins/connection/aws_ssm.py
ADDITIONAL INFORMATION
Related to ansible/ansible#75155.

Reviewed-by: Jill R <None>
relrod pushed a commit that referenced this pull request Jul 12, 2021
@samdoran
[stable-2.11] task_executor - use correct value for ssh connection re…
a2239d8 
…tries (#75155) (#75191)

Since the task and connection both have the same 'retries' keyword, the task default
would override the connection value.

Do not pass 'retries' from the task to the connection options.

* Set ssh_connection retries default value back to 0
  It was 0 before the move to config and was changed to 3 by accident.
(cherry picked from commit a8de35e)

Co-authored-by: Sam Doran <sdoran@redhat.com>
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 15, 2021
@ansible-zuul
[promoted]Merge pull request ansible-collections#619 from samdoran/co…
bcbba5c 
…nnect-retries-rename

cdaws_ssm - rename retries setting

SUMMARY

There was a bug in Ansible that would result in task retries overriding the connection retries. Using a different name for the connection retries setting avoids this issue on versions of Ansible that are missing the bug fix.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

plugins/connection/aws_ssm.py
ADDITIONAL INFORMATION
Related to ansible/ansible#75155.

Reviewed-by: Jill R <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@9ed46f8
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 16, 2021
@ansible-zuul
[promoted]Merge pull request ansible-collections#619 from samdoran/co…
0201bf0 
…nnect-retries-rename

cdaws_ssm - rename retries setting

SUMMARY

There was a bug in Ansible that would result in task retries overriding the connection retries. Using a different name for the connection retries setting avoids this issue on versions of Ansible that are missing the bug fix.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

plugins/connection/aws_ssm.py
ADDITIONAL INFORMATION
Related to ansible/ansible#75155.

Reviewed-by: Jill R <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@9ed46f8
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 17, 2021
@ansible-zuul
[promoted]Merge pull request ansible-collections#619 from samdoran/co…
0204072 
…nnect-retries-rename

cdaws_ssm - rename retries setting

SUMMARY

There was a bug in Ansible that would result in task retries overriding the connection retries. Using a different name for the connection retries setting avoids this issue on versions of Ansible that are missing the bug fix.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

plugins/connection/aws_ssm.py
ADDITIONAL INFORMATION
Related to ansible/ansible#75155.

Reviewed-by: Jill R <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@f78cafe
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@ansible-zuul
Merge pull request ansible-collections#619 from samdoran/connect-retr…
4a5d6eb 
…ies-rename

cdaws_ssm - rename retries setting

SUMMARY

There was a bug in Ansible that would result in task retries overriding the connection retries. Using a different name for the connection retries setting avoids this issue on versions of Ansible that are missing the bug fix.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

plugins/connection/aws_ssm.py
ADDITIONAL INFORMATION
Related to ansible/ansible#75155.

Reviewed-by: Jill R <None>
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@ansible-zuul
Merge pull request ansible-collections#619 from samdoran/connect-retr…
4c1b883 
…ies-rename

cdaws_ssm - rename retries setting

SUMMARY

There was a bug in Ansible that would result in task retries overriding the connection retries. Using a different name for the connection retries setting avoids this issue on versions of Ansible that are missing the bug fix.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

plugins/connection/aws_ssm.py
ADDITIONAL INFORMATION
Related to ansible/ansible#75155.

Reviewed-by: Jill R <None>
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@ansible-zuul
Merge pull request ansible-collections#619 from samdoran/connect-retr…
f78cafe 
…ies-rename

cdaws_ssm - rename retries setting

SUMMARY

There was a bug in Ansible that would result in task retries overriding the connection retries. Using a different name for the connection retries setting avoids this issue on versions of Ansible that are missing the bug fix.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

plugins/connection/aws_ssm.py
ADDITIONAL INFORMATION
Related to ansible/ansible#75155.

Reviewed-by: Jill R <None>
@ansible ansible locked and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vladimir-mencl-eresearch vladimir-mencl-eresearch vladimir-mencl-eresearch left review comments

@bcoca bcoca bcoca approved these changes

@s-hertel s-hertel s-hertel approved these changes

@nitzmahone nitzmahone Awaiting requested review from nitzmahone

@sivel sivel Awaiting requested review from sivel

Assignees
No one assigned
Labels
affects_2.12 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. has_issue support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SSH module takes retry option from task keywords, defaulting to 3
5 participants
@samdoran @vladimir-mencl-eresearch @bcoca @s-hertel @ansibot