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

Add support for reset_connection in persistent connection #61317

Closed
wants to merge 3 commits into from

Conversation

@ganeshrn
Copy link
Member

commented Aug 26, 2019

SUMMARY
  • Implement reset() method for persistent connections
    (network_cli/netconf/httpapi) to support
    meta: reset_connection
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ansible/plugins/connection/init.py
ansible/plugins/connection/httpapi.py
ansible/plugins/connection/netconf.py
ansible/plugins/connection/network_cli.py

ADDITIONAL INFORMATION

Add support for reset_connection in persistent connection
*  Implement `reset()` method for persistent connections
   (network_cli/netconf/httpapi) to support
   meta: reset_connection

@ganeshrn ganeshrn force-pushed the ganeshrn:reset_connection branch from 463f56b to 900513c Aug 26, 2019

@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/plugins/connection/__init__.py:355:56: bad-whitespace No space allowed before bracket                     if os.path.exists(self._socket_path ):                                                         ^
lib/ansible/plugins/connection/__init__.py:356:52: bad-whitespace No space allowed before bracket                         os.remove(self._socket_path )                                                     ^

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/plugins/connection/__init__.py:355:56: E202 whitespace before ')'
lib/ansible/plugins/connection/__init__.py:356:52: E202 whitespace before ')'

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Aug 26, 2019

@ansible-zuul

This comment has been minimized.

Copy link

commented Aug 26, 2019

@ansible-zuul

This comment has been minimized.

@ganeshrn

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

recheck

@ansible-zuul

This comment has been minimized.

super(Connection, self).close()

def reset(self):

This comment has been minimized.

Copy link
@Qalthos

Qalthos Aug 29, 2019

Contributor

This method is already in NetworkConnectionBase, why are we adding it to the subclasses too?
And why does this implementation differ from the others?

This comment has been minimized.

Copy link
@ganeshrn

ganeshrn Aug 29, 2019

Author Member

reset needs to be implemented in the individual connection class as invloking close() in the parent class will call close method defined either in the same class or it's parent and not that of the child class (eg: the one defined in network_cli)

This comment has been minimized.

Copy link
@Qalthos

Qalthos Aug 29, 2019

Contributor

invloking close() in the parent class will call close method defined either in the same class or it's parent and not that of the child class

That is not true. If it were, none of this would work. We rely on the fact that overridden methods get called properly despite being called in parent classes all the time.

As it is implemented presently, reset_connection works fine for the transport connection, it just doesn't do anything about the persistent connection socket. And... I'm really not sure it should? At least, I'm open to being convinced that the persistent connection process is worth being able to be reset like this, but I'm not sure what the need is.

lib/ansible/plugins/connection/__init__.py Outdated Show resolved Hide resolved
if os.path.exists(self._socket_path):
os.remove(self._socket_path)
self._socket_path = None
self._connected = False

This comment has been minimized.

Copy link
@Qalthos

Qalthos Aug 29, 2019

Contributor

This line is executed in NetworkConnectionBase.close() (and, is in fact the only thing that method does) which is universally called immediately after shutdown.

I don't think it belongs in a finally block
(I don't think a finally block even makes sense after a no-op Pokemon exception)
I also don't think it belongs inside the conditional

You now call shutdown in every implementing plugin's close method, so why not move this code into NetworkConnectionBase.close (or at least call shutdown in NetworkConnectionBase.close) Is there some reason to keep the two separate?

@ansibot ansibot removed the needs_triage label Aug 29, 2019

@ansible-zuul

This comment has been minimized.

Copy link

commented Aug 29, 2019

@ganeshrn

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

Closing this PR as it will be fixed by #61591

@ganeshrn ganeshrn closed this Aug 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.