From: Jim Branen <james.branen@hp.com>
Date: Thu, 12 Sep 2013 16:24:56 +0000 (-0700)
Subject: Fix client connection leaks in HP3PAR drivers
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=29c759f86530f10023cc3185435b5ceb75721c2d;p=openstack-build%2Fcinder-build.git

Fix client connection leaks in HP3PAR drivers

In the presence of exceptions, hp3parclient connections
did not get closed. This fix adds try/finally blocks
around all client login/logout calls.

Change-Id: I105b06cea5e61f3e9cf0d6e19c7ce430fedad715
Closes-Bug: 1220948
---

diff --git a/cinder/tests/test_hp3par.py b/cinder/tests/test_hp3par.py
index da9ff7f22..c418f9cc8 100644
--- a/cinder/tests/test_hp3par.py
+++ b/cinder/tests/test_hp3par.py
@@ -66,6 +66,8 @@ class FakeHP3ParClient(object):
     api_url = None
     debug = False
 
+    connection_count = 0
+
     volumes = []
     hosts = []
     vluns = []
@@ -108,9 +110,13 @@ class FakeHP3ParClient(object):
         self.debug = flag
 
     def login(self, username, password, optional=None):
+        self.connection_count += 1
         return None
 
     def logout(self):
+        if self.connection_count < 1:
+            raise hpexceptions.CommandError('No connection to log out.')
+        self.connection_count -= 1
         return None
 
     def getVolumes(self):
@@ -672,6 +678,8 @@ class TestHP3PARFCDriver(HP3PARBaseDriver, test.TestCase):
 
     def tearDown(self):
         shutil.rmtree(self.tempdir)
+        self.assertEqual(0, self.driver.common.client.connection_count,
+                         'Leaked hp3parclient connection.')
         super(TestHP3PARFCDriver, self).tearDown()
 
     def setup_driver(self, configuration):
@@ -880,6 +888,8 @@ class TestHP3PARISCSIDriver(HP3PARBaseDriver, test.TestCase):
 
     def tearDown(self):
         shutil.rmtree(self.tempdir)
+        self.assertEqual(0, self.driver.common.client.connection_count,
+                         'Leaked hp3parclient connection.')
         self._hosts = {}
         super(TestHP3PARISCSIDriver, self).tearDown()
 
diff --git a/cinder/volume/drivers/san/hp/hp_3par_fc.py b/cinder/volume/drivers/san/hp/hp_3par_fc.py
index d718db18f..2a811d3eb 100644
--- a/cinder/volume/drivers/san/hp/hp_3par_fc.py
+++ b/cinder/volume/drivers/san/hp/hp_3par_fc.py
@@ -55,9 +55,10 @@ class HP3PARFCDriver(cinder.volume.driver.FibreChannelDriver):
         1.2.0 - Updated the use of the hp3parclient to 2.0.0 and refactored
                 the drivers to use the new APIs.
         1.2.1 - Synchronized extend_volume method.
+        1.2.2 - Added try/finally around client login/logout.
     """
 
-    VERSION = "1.2.1"
+    VERSION = "1.2.2"
 
     def __init__(self, *args, **kwargs):
         super(HP3PARFCDriver, self).__init__(*args, **kwargs)
@@ -78,13 +79,16 @@ class HP3PARFCDriver(cinder.volume.driver.FibreChannelDriver):
     @utils.synchronized('3par', external=True)
     def get_volume_stats(self, refresh):
         self.common.client_login()
-        stats = self.common.get_volume_stats(refresh)
-        stats['storage_protocol'] = 'FC'
-        stats['driver_version'] = self.VERSION
-        backend_name = self.configuration.safe_get('volume_backend_name')
-        stats['volume_backend_name'] = backend_name or self.__class__.__name__
-        self.common.client_logout()
-        return stats
+        try:
+            stats = self.common.get_volume_stats(refresh)
+            stats['storage_protocol'] = 'FC'
+            stats['driver_version'] = self.VERSION
+            backend_name = self.configuration.safe_get('volume_backend_name')
+            stats['volume_backend_name'] = (backend_name or
+                                            self.__class__.__name__)
+            return stats
+        finally:
+            self.common.client_logout()
 
     def do_setup(self, context):
         self.common = self._init_common()
@@ -98,22 +102,28 @@ class HP3PARFCDriver(cinder.volume.driver.FibreChannelDriver):
     @utils.synchronized('3par', external=True)
     def create_volume(self, volume):
         self.common.client_login()
-        metadata = self.common.create_volume(volume)
-        self.common.client_logout()
-        return {'metadata': metadata}
+        try:
+            metadata = self.common.create_volume(volume)
+            return {'metadata': metadata}
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def create_cloned_volume(self, volume, src_vref):
         self.common.client_login()
-        new_vol = self.common.create_cloned_volume(volume, src_vref)
-        self.common.client_logout()
-        return {'metadata': new_vol}
+        try:
+            new_vol = self.common.create_cloned_volume(volume, src_vref)
+            return {'metadata': new_vol}
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def delete_volume(self, volume):
         self.common.client_login()
-        self.common.delete_volume(volume)
-        self.common.client_logout()
+        try:
+            self.common.delete_volume(volume)
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def create_volume_from_snapshot(self, volume, snapshot):
@@ -123,21 +133,28 @@ class HP3PARFCDriver(cinder.volume.driver.FibreChannelDriver):
         TODO: support using the size from the user.
         """
         self.common.client_login()
-        metadata = self.common.create_volume_from_snapshot(volume, snapshot)
-        self.common.client_logout()
-        return {'metadata': metadata}
+        try:
+            metadata = self.common.create_volume_from_snapshot(volume,
+                                                               snapshot)
+            return {'metadata': metadata}
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def create_snapshot(self, snapshot):
         self.common.client_login()
-        self.common.create_snapshot(snapshot)
-        self.common.client_logout()
+        try:
+            self.common.create_snapshot(snapshot)
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def delete_snapshot(self, snapshot):
         self.common.client_login()
-        self.common.delete_snapshot(snapshot)
-        self.common.client_logout()
+        try:
+            self.common.delete_snapshot(snapshot)
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def initialize_connection(self, volume, connector):
@@ -178,33 +195,37 @@ class HP3PARFCDriver(cinder.volume.driver.FibreChannelDriver):
 
         """
         self.common.client_login()
-        # we have to make sure we have a host
-        host = self._create_host(volume, connector)
+        try:
+            # we have to make sure we have a host
+            host = self._create_host(volume, connector)
 
-        # now that we have a host, create the VLUN
-        vlun = self.common.create_vlun(volume, host)
+            # now that we have a host, create the VLUN
+            vlun = self.common.create_vlun(volume, host)
 
-        fc_ports = self.common.get_active_fc_target_ports()
-        wwns = []
+            fc_ports = self.common.get_active_fc_target_ports()
+            wwns = []
 
-        for port in fc_ports:
-            wwns.append(port['portWWN'])
+            for port in fc_ports:
+                wwns.append(port['portWWN'])
 
-        self.common.client_logout()
-        info = {'driver_volume_type': 'fibre_channel',
-                'data': {'target_lun': vlun['lun'],
-                         'target_discovered': True,
-                         'target_wwn': wwns}}
-        return info
+            info = {'driver_volume_type': 'fibre_channel',
+                    'data': {'target_lun': vlun['lun'],
+                             'target_discovered': True,
+                             'target_wwn': wwns}}
+            return info
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def terminate_connection(self, volume, connector, **kwargs):
         """Driver entry point to unattach a volume from an instance."""
         self.common.client_login()
-        hostname = self.common._safe_hostname(connector['host'])
-        self.common.terminate_connection(volume, hostname,
-                                         wwn=connector['wwpns'])
-        self.common.client_logout()
+        try:
+            hostname = self.common._safe_hostname(connector['host'])
+            self.common.terminate_connection(volume, hostname,
+                                             wwn=connector['wwpns'])
+        finally:
+            self.common.client_logout()
 
     def _create_3par_fibrechan_host(self, hostname, wwns, domain, persona_id):
         """Create a 3PAR host.
diff --git a/cinder/volume/drivers/san/hp/hp_3par_iscsi.py b/cinder/volume/drivers/san/hp/hp_3par_iscsi.py
index fa74d4bf1..9cfceff43 100644
--- a/cinder/volume/drivers/san/hp/hp_3par_iscsi.py
+++ b/cinder/volume/drivers/san/hp/hp_3par_iscsi.py
@@ -56,9 +56,10 @@ class HP3PARISCSIDriver(cinder.volume.driver.ISCSIDriver):
         1.2.0 - Updated the use of the hp3parclient to 2.0.0 and refactored
                 the drivers to use the new APIs.
         1.2.1 - Synchronized extend_volume method.
+        1.2.2 - Added try/finally around client login/logout.
     """
 
-    VERSION = "1.2.1"
+    VERSION = "1.2.2"
 
     def __init__(self, *args, **kwargs):
         super(HP3PARISCSIDriver, self).__init__(*args, **kwargs)
@@ -79,19 +80,29 @@ class HP3PARISCSIDriver(cinder.volume.driver.ISCSIDriver):
     @utils.synchronized('3par', external=True)
     def get_volume_stats(self, refresh):
         self.common.client_login()
-        stats = self.common.get_volume_stats(refresh)
-        stats['storage_protocol'] = 'iSCSI'
-        stats['driver_version'] = self.VERSION
-        backend_name = self.configuration.safe_get('volume_backend_name')
-        stats['volume_backend_name'] = backend_name or self.__class__.__name__
-        self.common.client_logout()
-        return stats
+        try:
+            stats = self.common.get_volume_stats(refresh)
+            stats['storage_protocol'] = 'iSCSI'
+            stats['driver_version'] = self.VERSION
+            backend_name = self.configuration.safe_get('volume_backend_name')
+            stats['volume_backend_name'] = (backend_name or
+                                            self.__class__.__name__)
+            return stats
+        finally:
+            self.common.client_logout()
 
     def do_setup(self, context):
         self.common = self._init_common()
         self._check_flags()
         self.common.do_setup(context)
 
+        self.common.client_login()
+        try:
+            self.initialize_iscsi_ports()
+        finally:
+            self.common.client_logout()
+
+    def initialize_iscsi_ports(self):
         # map iscsi_ip-> ip_port
         #             -> iqn
         #             -> nsp
@@ -142,9 +153,9 @@ class HP3PARISCSIDriver(cinder.volume.driver.ISCSIDriver):
 
         # lets see if there are invalid iSCSI IPs left in the temp dict
         if len(temp_iscsi_ip) > 0:
-            msg = _("Found invalid iSCSI IP address(s) in configuration "
-                    "option(s) hp3par_iscsi_ips or iscsi_ip_address '%s.'") % \
-                   (", ".join(temp_iscsi_ip))
+            msg = (_("Found invalid iSCSI IP address(s) in configuration "
+                     "option(s) hp3par_iscsi_ips or iscsi_ip_address '%s.'") %
+                   (", ".join(temp_iscsi_ip)))
             LOG.warn(msg)
 
         if not len(self.iscsi_ips) > 0:
@@ -158,25 +169,29 @@ class HP3PARISCSIDriver(cinder.volume.driver.ISCSIDriver):
     @utils.synchronized('3par', external=True)
     def create_volume(self, volume):
         self.common.client_login()
-        metadata = self.common.create_volume(volume)
-        self.common.client_logout()
-
-        return {'metadata': metadata}
+        try:
+            metadata = self.common.create_volume(volume)
+            return {'metadata': metadata}
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def create_cloned_volume(self, volume, src_vref):
         """Clone an existing volume."""
         self.common.client_login()
-        new_vol = self.common.create_cloned_volume(volume, src_vref)
-        self.common.client_logout()
-
-        return {'metadata': new_vol}
+        try:
+            new_vol = self.common.create_cloned_volume(volume, src_vref)
+            return {'metadata': new_vol}
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def delete_volume(self, volume):
         self.common.client_login()
-        self.common.delete_volume(volume)
-        self.common.client_logout()
+        try:
+            self.common.delete_volume(volume)
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def create_volume_from_snapshot(self, volume, snapshot):
@@ -186,21 +201,28 @@ class HP3PARISCSIDriver(cinder.volume.driver.ISCSIDriver):
         TODO: support using the size from the user.
         """
         self.common.client_login()
-        metadata = self.common.create_volume_from_snapshot(volume, snapshot)
-        self.common.client_logout()
-        return {'metadata': metadata}
+        try:
+            metadata = self.common.create_volume_from_snapshot(volume,
+                                                               snapshot)
+            return {'metadata': metadata}
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def create_snapshot(self, snapshot):
         self.common.client_login()
-        self.common.create_snapshot(snapshot)
-        self.common.client_logout()
+        try:
+            self.common.create_snapshot(snapshot)
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def delete_snapshot(self, snapshot):
         self.common.client_login()
-        self.common.delete_snapshot(snapshot)
-        self.common.client_logout()
+        try:
+            self.common.delete_snapshot(snapshot)
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def initialize_connection(self, volume, connector):
@@ -229,37 +251,40 @@ class HP3PARISCSIDriver(cinder.volume.driver.ISCSIDriver):
           * create vlun on the 3par
         """
         self.common.client_login()
+        try:
 
-        # we have to make sure we have a host
-        host = self._create_host(volume, connector)
-
-        # now that we have a host, create the VLUN
-        vlun = self.common.create_vlun(volume, host)
+            # we have to make sure we have a host
+            host = self._create_host(volume, connector)
 
-        iscsi_ip = self._get_iscsi_ip(host['name'])
+            # now that we have a host, create the VLUN
+            vlun = self.common.create_vlun(volume, host)
 
-        self.common.client_logout()
+            iscsi_ip = self._get_iscsi_ip(host['name'])
 
-        iscsi_ip_port = self.iscsi_ips[iscsi_ip]['ip_port']
-        iscsi_target_iqn = self.iscsi_ips[iscsi_ip]['iqn']
-        info = {'driver_volume_type': 'iscsi',
-                'data': {'target_portal': "%s:%s" %
-                         (iscsi_ip, iscsi_ip_port),
-                         'target_iqn': iscsi_target_iqn,
-                         'target_lun': vlun['lun'],
-                         'target_discovered': True
-                         }
-                }
-        return info
+            iscsi_ip_port = self.iscsi_ips[iscsi_ip]['ip_port']
+            iscsi_target_iqn = self.iscsi_ips[iscsi_ip]['iqn']
+            info = {'driver_volume_type': 'iscsi',
+                    'data': {'target_portal': "%s:%s" %
+                             (iscsi_ip, iscsi_ip_port),
+                             'target_iqn': iscsi_target_iqn,
+                             'target_lun': vlun['lun'],
+                             'target_discovered': True
+                             }
+                    }
+            return info
+        finally:
+            self.common.client_logout()
 
     @utils.synchronized('3par', external=True)
     def terminate_connection(self, volume, connector, **kwargs):
         """Driver entry point to unattach a volume from an instance."""
         self.common.client_login()
-        hostname = self.common._safe_hostname(connector['host'])
-        self.common.terminate_connection(volume, hostname,
-                                         iqn=connector['initiator'])
-        self.common.client_logout()
+        try:
+            hostname = self.common._safe_hostname(connector['host'])
+            self.common.terminate_connection(volume, hostname,
+                                             iqn=connector['initiator'])
+        finally:
+            self.common.client_logout()
 
     def _create_3par_iscsi_host(self, hostname, iscsi_iqn, domain, persona_id):
         """Create a 3PAR host.