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

PSRP: Add read_timeout connection parameters #46850

Merged
merged 3 commits into from
Jan 29, 2019

Conversation

dagwieers
Copy link
Contributor

@dagwieers dagwieers commented Oct 11, 2018

SUMMARY

This PR adds read_timeout parameters for PSRP.
The WinRM variant is in #49701.

This PR requires jborean93/pypsrp#13

This relates to #46108

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

WinRM/PSRP

@dagwieers dagwieers requested review from nitzmahone and jborean93 and removed request for nitzmahone October 11, 2018 22:08
@dagwieers dagwieers added the windows Windows community label Oct 11, 2018
@ansibot

This comment has been minimized.

@ansibot
Copy link
Contributor

ansibot commented Oct 11, 2018

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 feature This issue/PR relates to a feature request. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Oct 11, 2018
@dagwieers
Copy link
Contributor Author

dagwieers commented Oct 13, 2018

@jborean93 I added a check for read_timeout support. Please review !

@nitzmahone What else do we want to do here for WinRM ? Moved to #49701

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Oct 25, 2018
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. core_review In order to be merged, this PR must follow the core review workflow. labels Nov 7, 2018
@mattclay

This comment has been minimized.

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Nov 7, 2018
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 8, 2018
Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

I think you should split out the psrp and winrm changes into separate PRs for 2 reasons;

  • Not sure when the pywinrm PR will be reviewed and merged so I don't want to hold up these changes making it into psrp
  • The current setup with read/connection/operational timeouts and how they relate to Ansible extra vars is a bit convoluted. It will require some more complex kwarg inspection handling and other changes to properly implement.

lib/ansible/plugins/connection/psrp.py Show resolved Hide resolved
lib/ansible/plugins/connection/psrp.py Outdated Show resolved Hide resolved
lib/ansible/plugins/connection/psrp.py Show resolved Hide resolved
@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 Nov 11, 2018
@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 Nov 19, 2018
nitzmahone
nitzmahone previously approved these changes Nov 30, 2018
@dagwieers dagwieers dismissed jborean93’s stale review December 18, 2018 09:01

Fixed version_added

@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 Dec 18, 2018
Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Just need to change the display.debug to a warning and I'm good with the changes here.

lib/ansible/plugins/connection/psrp.py Outdated Show resolved Hide resolved
@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 Dec 18, 2018
@dagwieers
Copy link
Contributor Author

Ready for re-review.

@dagwieers dagwieers dismissed jborean93’s stale review December 19, 2018 12:18

Implemented warning()

@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 Dec 19, 2018
@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 Dec 27, 2018
@jseiser
Copy link

jseiser commented Jan 8, 2019

Im not able to get this to function.

From my Docker File

pip install git+https://github.com/dagwieers/ansible.git@winrm-psrp-read-timeout pywinrm[credssp] git+https://github.com/pymssql/pymssql.git git+https://github.com/jborean93/pypsrp.git python-ldap pymysql

From Ansible Log

TASK [roles/ansible-role-dc : Check if Public Key Installed] *******************
[WARNING]: ansible_psrp_read_timeout is unsupported by the current psrp version installed
ok: [10.254.74.5]

From My Group Var

ansible_port: 5986 
ansible_connection: psrp 
ansible_psrp_auth: credssp 
ansible_psrp_cert_validation: ignore 
ansible_psrp_connection_timeout: 1800 
ansible_psrp_operation_timeout: 1200 
ansible_psrp_read_timeout: 1100 
ansible_winrm_read_timeout: 1100 
ansible_psrp_reconnection_retries: 10
/ $ ansible --version
ansible 2.8.0.dev0
  config file = None
  configured module search path = [u'/home/jenkins/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.15 (default, Aug 16 2018, 14:17:09) [GCC 6.4.0]
/ $ pip show pypsrp
Name: pypsrp
Version: 0.3.1
Summary: PowerShell Remoting Protocol and WinRM for Python
Home-page: https://github.com/jborean93/pypsrp
Author: Jordan Borean
Author-email: jborean93@gmail.com
License: MIT
Location: /usr/lib/python2.7/site-packages
Requires: cryptography, ntlm-auth, requests, six
Required-by: 
/ $ which ansible
/usr/bin/ansible
/ $ python --version
Python 2.7.15
/ $ 

@dagwieers
Copy link
Contributor Author

@jseiser I don't know why it fails for you. I just verified on a new system and it works as expected. Can you perform the following on your system:

$ python -c 'import pypsrp; print pypsrp.FEATURES'

@jseiser
Copy link

jseiser commented Jan 24, 2019

@dagwieers

pip install git+https://github.com/jborean93/pypsrp.git

[cicd-docker-devops] docker exec -it 4c078b1ed1c5 /bin/sh                                                                                                                                                                                                                                               8:16:41  ☁  master ☀
/ $ which python
/usr/bin/python
/ $ python --version
Python 2.7.15
/ $ python -c 'import pypsrp; print pypsrp.FEATURES'
['wsman_locale', 'wsman_read_timeout', 'wsman_reconnections']

@dagwieers
Copy link
Contributor Author

@jseiser Then it is up to you to figure out why this is not working ;-)

# Check if PSRP version supports newer read_timeout argument (needs pypsrp 0.3.0+)
if hasattr(pypsrp, 'FEATURES') and 'wsman_read_timeout' in pypsrp.FEATURES:
self._psrp_conn_kwargs['read_timeout'] = self._psrp_read_timeout
elif self._psrp_read_timeout is not None:
display.warning("ansible_psrp_read_timeout is unsupported by the current psrp version installed, "
"using ansible_psrp_connection_timeout value for read_timeout instead.")

Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Because we set a default value for read_timeout of 30, the current change will always display a warning for a user if they are running on an older version of pypsrp. I think this is an ok tradeoff to make the code simpler and considering this is mostly a new and undocumented connection plugin I doubt many people would be using it right now. Testing it locally I can confirm the values are being passed down to the right location and am happy with the changes here.

@jborean93
Copy link
Contributor

@jseiser I'm not sure why the Ansible environment is not picking up your changes, I've tested this code on both Python 2 and 3 and can confirm it works as expected. The 0.3.0 release of pypsrp contains the changes for the read_timeout configuration so you don't need to install it directly from the master branch, a simple pip install pypsrp will work fine.

@ansibot ansibot removed 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 Jan 29, 2019
@jborean93 jborean93 merged commit 870525d into ansible:devel Jan 29, 2019
@jborean93
Copy link
Contributor

@jseiser so I did come across this issue today but it was because the version of Ansible I was using did not have this PR applied. You would need to make sure you are running from the devel checkout for it to work.

@jseiser
Copy link

jseiser commented Jan 30, 2019 via email

@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. support:core This issue/PR relates to code supported by the Ansible Engineering Team. windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants