openstack/cinder
Code Issues Proposed changes

Fix sshpool.remove code

Browse Source 
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
This commit is contained in:
Surya Ghatty 2016-02-26 19:49:24 +00:00
parent 71388796d3
commit b4f63203ff
2 changed files with 43 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
cinder/ssh_utils.py
View File

@ -171,8 +171,7 @@ class SSHPool(pools.Pool):
def remove(self, ssh):
"""Close an ssh client and remove it from free_items."""
ssh.close()
ssh = None
if ssh in self.free_items:
self.free_items.pop(ssh)
if self.current_size > 0:
self.current_size -= 1
self.free_items.remove(ssh)
if self.current_size > 0:
self.current_size -= 1

41
cinder/tests/unit/test_ssh_utils.py
View File

@ -10,7 +10,6 @@
# License for the specific language governing permissions and limitations
# under the License.
import mock
import paramiko
import uuid
@ -79,6 +78,46 @@ class FakeSSHClient(object):
class SSHPoolTestCase(test.TestCase):
"""Unit test for SSH Connection Pool."""
@mock.patch('cinder.ssh_utils.CONF')
@mock.patch('six.moves.builtins.open')
@mock.patch('paramiko.SSHClient')
@mock.patch('os.path.isfile', return_value=True)
def test_sshpool_remove(self, mock_isfile, mock_sshclient, mock_open,
mock_conf):
ssh_to_remove = mock.MagicMock()
mock_sshclient.side_effect = [mock.MagicMock(),
ssh_to_remove, mock.MagicMock()]
mock_conf.ssh_hosts_key_file.return_value = 'dummy'
sshpool = ssh_utils.SSHPool("127.0.0.1", 22, 10,
"test",
password="test",
min_size=3,
max_size=3)
self.assertIn(ssh_to_remove, list(sshpool.free_items))
sshpool.remove(ssh_to_remove)
self.assertNotIn(ssh_to_remove, list(sshpool.free_items))
@mock.patch('cinder.ssh_utils.CONF')
@mock.patch('six.moves.builtins.open')
@mock.patch('paramiko.SSHClient')
@mock.patch('os.path.isfile', return_value=True)
def test_sshpool_remove_object_not_in_pool(self, mock_isfile,
mock_sshclient, mock_open,
mock_conf):
# create an SSH Client that is not a part of sshpool.
ssh_to_remove = mock.MagicMock()
mock_sshclient.side_effect = [mock.MagicMock(), mock.MagicMock()]
mock_conf.ssh_hosts_key_file.return_value = 'dummy'
sshpool = ssh_utils.SSHPool("127.0.0.1", 22, 10,
"test",
password="test",
min_size=2,
max_size=2)
listBefore = list(sshpool.free_items)
self.assertNotIn(ssh_to_remove, listBefore)
sshpool.remove(ssh_to_remove)
self.assertEqual(listBefore, list(sshpool.free_items))
@mock.patch('cinder.ssh_utils.CONF')
@mock.patch('six.moves.builtins.open')
@mock.patch('paramiko.SSHClient')