From b4f63203ffa32e6ce2f678cdf6287621eeff2c59 Mon Sep 17 00:00:00 2001 From: Surya Ghatty Date: Fri, 26 Feb 2016 19:49:24 +0000 Subject: [PATCH] Fix sshpool.remove code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cinder/ssh_utils.py | 7 +++-- cinder/tests/unit/test_ssh_utils.py | 41 ++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/cinder/ssh_utils.py b/cinder/ssh_utils.py index 22f29a52d04..c3a7160ece4 100644 --- a/cinder/ssh_utils.py +++ b/cinder/ssh_utils.py @@ -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 diff --git a/cinder/tests/unit/test_ssh_utils.py b/cinder/tests/unit/test_ssh_utils.py index 552b82d38c9..0a3cb38c6b6 100644 --- a/cinder/tests/unit/test_ssh_utils.py +++ b/cinder/tests/unit/test_ssh_utils.py @@ -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')