SSHPool remove uses incorrect parameters

[Eric Harney]

SSHPool's remove() contains:

        if ssh in self.free_items:
            self.free_items.pop(ssh)

But deque pop() does not take an argument. I assume this was meant to be self.free_items.remove(ssh).

This throws a "Too many positional arguments for function call" pylint warning. I'm not sure what the end result is for drivers using SSHPool.

My quick analysis is that eventlet seems to remove items from the left with free_items.popleft(), and pop() will remove them from the right, so it seems problematic. But I feel like there's more going on in this SSHPool code than I fully understand at this point.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/285687

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/285742

[OpenStack Infra (hudson-openstack)]

Change abandoned by Surya Ghatty (<email address hidden>) on branch: master
Review: https://review.openstack.org/285742
Reason: Wrong commit

[Surya Ghatty (ghatty)]

Reply…

Patch Sets (5/5)

Download

"fix sshpool.remove code"

currently, sshpool.remove function under cinder/ssh_utils.py
is broken.

The function tries to locate the passed in sshclient object inside
sshpool.free_items.
However, since the sshclient object is set to “none” at the
beginning, it never finds the object and ends up with
decrementing the current size, without actually removing
the object.

Made the following changes to fix:
1. Removed reset to ‘None’ so that the attempt to locate object
goes through.
2. fixed the code to use free_items.remove(ssh) to remove the ssh
object identified instead of free_items.pop(sh)
3. also updated the code to decrement current size only if a match
is found in free_items.
4. Added test case to test remove() of an ssh client that is in the
free_items
5. Added test case to test that remove code does not inadvertently
remove an object from the pool if no match is found.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/285687
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=b4f63203ffa32e6ce2f678cdf6287621eeff2c59
Submitter: Jenkins
Branch: master

commit b4f63203ffa32e6ce2f678cdf6287621eeff2c59
Author: Surya Ghatty <email address hidden>
Date: Fri Feb 26 19:49:24 2016 +0000

    Fix sshpool.remove code

    Currently, sshpool.remove function under cinder/ssh_utils.py
    is broken. The function tries to locate the passed in
    sshclient object inside sshpool.free_items.

    However, since the sshclient object is set to “None” at the
    beginning, it never finds the object and ends up decrementing
     the current size, without actually removing the object.

    Made the following changes to fix:
    1. Removed reset to ‘None’ so that the attempt to locate object
    goes through.
    2. Fixed the code to use free_items.remove(ssh) to remove the ssh
    object identified instead of free_items.pop(ssh)
    3. Also updated the code to decrement current size only if a match
    is found in free_items.
    4. Added test case to test remove() of an ssh client that is in the
    free_items
    5. Added test case to test that remove code does not inadvertently
    remove an object from the pool if no match is found.

    Change-Id: I4871f4faeb1fc790325f274ab21dc42a8d71fb26
    Closes-Bug: #1463557

[Doug Hellmann (doug-hellmann)]

This issue was fixed in the openstack/cinder 8.0.0.0b3 development milestone.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/637644

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/637644
Committed: https://git.openstack.org/cgit/openstack/manila/commit/?id=1af8026e73f642f4ed702d43c5dc781ae92c03ca
Submitter: Zuul
Branch: master

commit 1af8026e73f642f4ed702d43c5dc781ae92c03ca
Author: Goutham Pacha Ravi <email address hidden>
Date: Mon Feb 18 14:31:18 2019 -0800

    Fix sshpool.remove

    collections.deque does not have a 'pop'
    method, and the sshpool.remove method
    currently leaks SSH connections that
    it creates.

    This bugfix was ported from cinder [1]

    [1] https://review.openstack.org/#/c/285687/

    Change-Id: I2dc9ffc13f11884b3069e6b4a453c933edf16d89
    Co-Authored-By: Surya Ghatty <email address hidden>
    Closes-Bug: #1463557

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/manila 8.0.0.0rc1 release candidate.