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')