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

Solves 494: added destructor to ssm connection plugin #542

Merged
merged 4 commits into from
Apr 14, 2021

Conversation

Chilinot
Copy link
Contributor

SUMMARY

Fixes #494

Adds destructor to the SSM connection plugin to ensure connections are properly cleaned up after usage.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

SSM connection plugin

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review connection connection plugin needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Apr 14, 2021
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Hi @Chilinot,

Thanks for taking the time to dig into the issue and open this PR. If possible please could you add a changelog fragment: https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to

The change itself looks sensible.

@tremble
Copy link
Contributor

tremble commented Apr 14, 2021

@hgrgic you reported also experiencing the issue on #494 if possible please could you test the change in this PR and see if it fixes the issue you're seeing.

@hgrgic
Copy link

hgrgic commented Apr 14, 2021

Hi @tremble and @Chilinot. Thanks for this update.

I have tested the proposed fix with my playbooks. Immediately after adding the proposed change I noticed that sessions were terminated properly. At the same time, looking in the AWS Session Manager console, under sessions, at most time I saw only 1 connection being open.

I have repeated the test with the same playbook but before repeating I have removed the proposed change. In this instance I saw more than 20 connection remaining open even after the execution of playbooks has finished. This tells me that the fix is indeed effective.

I recommend merging this PR as soon as possible!

@Chilinot
Copy link
Contributor Author

@tremble I have now added a changelog fragment as required.

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

LGTM - I'll let the test suites do their thing and hopefully we can get this merged.

@tremble
Copy link
Contributor

tremble commented Apr 14, 2021

Tests didn't run in zuul for some reason. Tested locally and they still pass.

@tremble tremble added the gate label Apr 14, 2021
@ansible-zuul ansible-zuul bot merged commit 44a9d82 into ansible-collections:main Apr 14, 2021
@hgrgic
Copy link

hgrgic commented Apr 15, 2021

@tremble What is usually the expected cadence for this merged fix to be included in a release?

@tremble
Copy link
Contributor

tremble commented Apr 15, 2021

Our last release was almost 2 months ago, I'd expect 1.5.0 to land 'soon'.

alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug community_review connection connection plugin needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSM connection plugin doesnt properly close connections
5 participants