Remove use of contextlib.nested

The contextlib.nested call has been deprecated
in Python 2.7. This causes DeprecationWarning
messages in the unit tests.

There are also known issues with contextlib.nested
that were addressed by the native support for
multiple "with" variables. For instance, if the
first object is created but the second one throws
an exception, the first object's __exit__ is never
called.

Since Cinder no longer supports 2.6 we can remove
the use of these contextlib.nested calls.

Added hacking check to catch if any new instances
are added to the codebase.

Note: line continuation markers (e.g. '\') had to
be used or syntax errors were thrown. While using
parentheses is the preferred way for multiple line
statements it is not a requirement.

Partial-Bug: 1428424
Change-Id: I7bb7d201d31ff239be3402fb64e5f202ede019b0

diff --git a/HACKING.rst b/HACKING.rst
index bca0ccff7f7ce287e83414f8895e5aa6e99cc384..750d93047984055e321c04c1be2e4b5e92e95fbe 100644 (file)
--- a/HACKING.rst
+++ b/HACKING.rst
@@ -14,6 +14,7 @@ Cinder Specific Commandments
 - [N324] Enforce no use of LOG.audit messages.  LOG.info should be used instead.
 - [N327] assert_called_once is not a valid Mock method.
 - [N333] Ensure that oslo namespaces are used for namespaced libraries.
+- [N339] Prevent use of deprecated contextlib.nested.
 
 
 General

diff --git a/cinder/hacking/checks.py b/cinder/hacking/checks.py
index cb27afe0b44b095979d8680982fedd82622e767f..972748658e6f53d2ace9a8e02a980bed52a6259d 100644 (file)
--- a/cinder/hacking/checks.py
+++ b/cinder/hacking/checks.py
@@ -138,6 +138,15 @@ def check_oslo_namespace_imports(logical_line):
         yield(0, msg)
 
 
+def check_no_contextlib_nested(logical_line):
+    msg = ("N339: contextlib.nested is deprecated. With Python 2.7 and later "
+           "the with-statement supports multiple nested objects. See https://"
+           "docs.python.org/2/library/contextlib.html#contextlib.nested "
+           "for more information.")
+    if "with contextlib.nested" in logical_line:
+        yield(0, msg)
+
+
 def factory(register):
     register(no_vi_headers)
     register(no_translate_debug_logs)
@@ -146,3 +155,4 @@ def factory(register):
     register(check_no_log_audit)
     register(check_assert_called_once)
     register(check_oslo_namespace_imports)
+    register(check_no_contextlib_nested)

diff --git a/cinder/tests/compute/test_nova.py b/cinder/tests/compute/test_nova.py
index f655ee5a21c9d29f5b5bd60684de59449437e646..8bc50e9d4e5e394105e46865232be7ba92f1f035 100644 (file)
--- a/cinder/tests/compute/test_nova.py
+++ b/cinder/tests/compute/test_nova.py
@@ -12,8 +12,6 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import contextlib
-
 import mock
 
 from cinder.compute import nova
@@ -102,11 +100,10 @@ class NovaApiTestCase(test.TestCase):
         self.ctx = context.get_admin_context()
 
     def test_update_server_volume(self):
-        with contextlib.nested(
-                mock.patch.object(nova, 'novaclient'),
+        with mock.patch.object(nova, 'novaclient') as mock_novaclient, \
                 mock.patch.object(self.novaclient.volumes,
-                                  'update_server_volume')
-        ) as (mock_novaclient, mock_update_server_volume):
+                                  'update_server_volume') as \
+                mock_update_server_volume:
             mock_novaclient.return_value = self.novaclient
 
             self.api.update_server_volume(self.ctx, 'server_id',

diff --git a/cinder/tests/test_backup_ceph.py b/cinder/tests/test_backup_ceph.py
index d2d0785b79d7d19f48cb9808d8f7f7599e38df79..f65f3a48780a02977dcaba94de5fd56432d24a8f 100644 (file)
--- a/cinder/tests/test_backup_ceph.py
+++ b/cinder/tests/test_backup_ceph.py
@@ -14,7 +14,6 @@
 #    under the License.
 """ Tests for Ceph backup service."""
 
-import contextlib
 import hashlib
 import os
 import tempfile
@@ -481,10 +480,9 @@ class BackupCephTestCase(test.TestCase):
         self.mock_rbd.RBD.list = mock.Mock()
         self.mock_rbd.RBD.list.return_value = [backup_name]
 
-        with contextlib.nested(
-                mock.patch.object(self.service, 'get_backup_snaps'),
-                mock.patch.object(self.service, '_rbd_diff_transfer')
-        ) as (_unused, mock_rbd_diff_transfer):
+        with mock.patch.object(self.service, 'get_backup_snaps'), \
+                mock.patch.object(self.service, '_rbd_diff_transfer') as \
+                mock_rbd_diff_transfer:
             def mock_rbd_diff_transfer_side_effect(src_name, src_pool,
                                                    dest_name, dest_pool,
                                                    src_user, src_conf,
@@ -495,10 +493,11 @@ class BackupCephTestCase(test.TestCase):
             # Raise a pseudo exception.BackupRBDOperationFailed.
             mock_rbd_diff_transfer.side_effect \
                 = mock_rbd_diff_transfer_side_effect
-            with contextlib.nested(
-                    mock.patch.object(self.service, '_full_backup'),
-                    mock.patch.object(self.service, '_try_delete_base_image')
-            ) as (_unused, mock_try_delete_base_image):
+
+            with mock.patch.object(self.service, '_full_backup'), \
+                    mock.patch.object(self.service,
+                                      '_try_delete_base_image') as \
+                    mock_try_delete_base_image:
                 def mock_try_delete_base_image_side_effect(backup_id,
                                                            volume_id,
                                                            base_name):
@@ -556,12 +555,11 @@ class BackupCephTestCase(test.TestCase):
         self.mock_rbd.RBD.list = mock.Mock()
         self.mock_rbd.RBD.list.return_value = [backup_name]
 
-        with contextlib.nested(
-                mock.patch.object(self.service, 'get_backup_snaps'),
-                mock.patch.object(self.service, '_rbd_diff_transfer'),
-                mock.patch.object(self.service, '_full_backup'),
-                mock.patch.object(self.service, '_backup_metadata')
-        ) as (_unused1, _u2, _u3, mock_backup_metadata):
+        with mock.patch.object(self.service, 'get_backup_snaps'), \
+                mock.patch.object(self.service, '_rbd_diff_transfer'), \
+                mock.patch.object(self.service, '_full_backup'), \
+                mock.patch.object(self.service, '_backup_metadata') as \
+                mock_backup_metadata:
 
             def mock_backup_metadata_side_effect(backup):
                 raise exception.BackupOperationError(_('mock'))

diff --git a/cinder/tests/test_cmd.py b/cinder/tests/test_cmd.py
index afe44ab7a7318f2d54f9e75cc258bf25833300fb..5c18be9732865e554937f101f748cc2d724779e1 100644 (file)
--- a/cinder/tests/test_cmd.py
+++ b/cinder/tests/test_cmd.py
@@ -10,7 +10,6 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import contextlib
 import datetime
 import StringIO
 import sys
@@ -706,16 +705,14 @@ class TestCinderRtstoolCmd(test.TestCase):
                           mock.sentinel.iser_enabled)
 
     def _test_create_rtslib_error_network_portal(self, ip):
-        with contextlib.nested(
-            mock.patch('rtslib.NetworkPortal'),
-            mock.patch('rtslib.LUN'),
-            mock.patch('rtslib.TPG'),
-            mock.patch('rtslib.FabricModule'),
-            mock.patch('rtslib.Target'),
-            mock.patch('rtslib.BlockStorageObject'),
-            mock.patch('rtslib.root.RTSRoot')
-        ) as (network_portal, lun, tpg, fabric_module, target,
-              block_storage_object, rts_root):
+        with mock.patch('rtslib.NetworkPortal') as network_portal, \
+                mock.patch('rtslib.LUN') as lun, \
+                mock.patch('rtslib.TPG') as tpg, \
+                mock.patch('rtslib.FabricModule') as fabric_module, \
+                mock.patch('rtslib.Target') as target, \
+                mock.patch('rtslib.BlockStorageObject') as \
+                block_storage_object, \
+                mock.patch('rtslib.root.RTSRoot') as rts_root:
             root_new = mock.MagicMock(storage_objects=mock.MagicMock())
             rts_root.return_value = root_new
             block_storage_object.return_value = mock.sentinel.so_new
@@ -766,16 +763,14 @@ class TestCinderRtstoolCmd(test.TestCase):
         self._test_create_rtslib_error_network_portal('::0')
 
     def _test_create(self, ip):
-        with contextlib.nested(
-            mock.patch('rtslib.NetworkPortal'),
-            mock.patch('rtslib.LUN'),
-            mock.patch('rtslib.TPG'),
-            mock.patch('rtslib.FabricModule'),
-            mock.patch('rtslib.Target'),
-            mock.patch('rtslib.BlockStorageObject'),
-            mock.patch('rtslib.root.RTSRoot')
-        ) as (network_portal, lun, tpg, fabric_module, target,
-              block_storage_object, rts_root):
+        with mock.patch('rtslib.NetworkPortal') as network_portal, \
+                mock.patch('rtslib.LUN') as lun, \
+                mock.patch('rtslib.TPG') as tpg, \
+                mock.patch('rtslib.FabricModule') as fabric_module, \
+                mock.patch('rtslib.Target') as target, \
+                mock.patch('rtslib.BlockStorageObject') as \
+                block_storage_object, \
+                mock.patch('rtslib.root.RTSRoot') as rts_root:
             root_new = mock.MagicMock(storage_objects=mock.MagicMock())
             rts_root.return_value = root_new
             block_storage_object.return_value = mock.sentinel.so_new

diff --git a/cinder/tests/test_hacking.py b/cinder/tests/test_hacking.py
index 8457b07206f301e6981c7c565f149aa598a47025..365152b319d2e51dfb832d25ed3b0e427b30f7cd 100644 (file)
--- a/cinder/tests/test_hacking.py
+++ b/cinder/tests/test_hacking.py
@@ -163,3 +163,9 @@ class HackingTestCase(test.TestCase):
             "from oslo.log import foo"))))
         self.assertEqual(0, len(list(checks.check_oslo_namespace_imports(
             "from oslo_log import bar"))))
+
+    def test_no_contextlib_nested(self):
+        self.assertEqual(1, len(list(checks.check_no_contextlib_nested(
+            "with contextlib.nested("))))
+        self.assertEqual(0, len(list(checks.check_no_contextlib_nested(
+            "with foo as bar"))))

diff --git a/cinder/tests/test_quobyte.py b/cinder/tests/test_quobyte.py
index 90b73f7e81450d3264ed9b68fc52904a147edd0c..d1864669776534e37956fbf1759c7b1957c4e8e8 100644 (file)
--- a/cinder/tests/test_quobyte.py
+++ b/cinder/tests/test_quobyte.py
@@ -15,7 +15,6 @@
 #    under the License.
 """Unit tests for the Quobyte driver module."""
 
-import contextlib
 import errno
 import os
 import StringIO
@@ -126,11 +125,9 @@ class QuobyteDriverTestCase(test.TestCase):
             drv.local_path(volume))
 
     def test_mount_quobyte_should_mount_correctly(self):
-        with contextlib.nested(
-                mock.patch.object(self._driver, '_execute'),
+        with mock.patch.object(self._driver, '_execute') as mock_execute, \
                 mock.patch('cinder.volume.drivers.quobyte.QuobyteDriver'
-                           '.read_proc_mount'),
-        ) as (mock_execute, mock_open):
+                           '.read_proc_mount') as mock_open:
             # Content of /proc/mount (not mounted yet).
             mock_open.return_value = StringIO.StringIO(
                 "/dev/sda5 / ext4 rw,relatime,data=ordered 0 0")
@@ -152,11 +149,9 @@ class QuobyteDriverTestCase(test.TestCase):
                 [mkdir_call, mount_call, getfattr_call], any_order=False)
 
     def test_mount_quobyte_already_mounted_detected_seen_in_proc_mount(self):
-        with contextlib.nested(
-                mock.patch.object(self._driver, '_execute'),
+        with mock.patch.object(self._driver, '_execute') as mock_execute, \
                 mock.patch('cinder.volume.drivers.quobyte.QuobyteDriver'
-                           '.read_proc_mount'),
-        ) as (mock_execute, mock_open):
+                           '.read_proc_mount') as mock_open:
             # Content of /proc/mount (already mounted).
             mock_open.return_value = StringIO.StringIO(
                 "quobyte@%s %s fuse rw,nosuid,nodev,noatime,user_id=1000"
@@ -179,12 +174,10 @@ class QuobyteDriverTestCase(test.TestCase):
            Because _mount_quobyte gets called with ensure=True, the error will
            be suppressed and logged instead.
         """
-        with contextlib.nested(
-                mock.patch.object(self._driver, '_execute'),
+        with mock.patch.object(self._driver, '_execute') as mock_execute, \
                 mock.patch('cinder.volume.drivers.quobyte.QuobyteDriver'
-                           '.read_proc_mount'),
-                mock.patch('cinder.volume.drivers.quobyte.LOG')
-        ) as (mock_execute, mock_open, mock_LOG):
+                           '.read_proc_mount') as mock_open, \
+                mock.patch('cinder.volume.drivers.quobyte.LOG') as mock_LOG:
             # Content of /proc/mount (empty).
             mock_open.return_value = StringIO.StringIO()
             mock_execute.side_effect = [None, putils.ProcessExecutionError(
@@ -209,11 +202,9 @@ class QuobyteDriverTestCase(test.TestCase):
            test_mount_quobyte_should_suppress_and_log_already_mounted_error
            but with ensure=False.
         """
-        with contextlib.nested(
-                mock.patch.object(self._driver, '_execute'),
+        with mock.patch.object(self._driver, '_execute') as mock_execute, \
                 mock.patch('cinder.volume.drivers.quobyte.QuobyteDriver'
-                           '.read_proc_mount')
-        ) as (mock_execute, mock_open):
+                           '.read_proc_mount') as mock_open:
             mock_open.return_value = StringIO.StringIO()
             mock_execute.side_effect = [
                 None,  # mkdir
@@ -312,10 +303,10 @@ class QuobyteDriverTestCase(test.TestCase):
 
     def test_ensure_share_mounted(self):
         """_ensure_share_mounted simple use case."""
-        with contextlib.nested(
-                mock.patch.object(self._driver, '_get_mount_point_for_share'),
-                mock.patch.object(self._driver, '_mount_quobyte')
-        ) as (mock_get_mount_point, mock_mount):
+        with mock.patch.object(self._driver, '_get_mount_point_for_share') as \
+                mock_get_mount_point, \
+                mock.patch.object(self._driver, '_mount_quobyte') as \
+                mock_mount:
             drv = self._driver
             drv._ensure_share_mounted(self.TEST_QUOBYTE_VOLUME)
 
@@ -578,16 +569,19 @@ class QuobyteDriverTestCase(test.TestCase):
         volume_path = '%s/%s' % (self.TEST_MNT_POINT, volume_filename)
         info_file = volume_path + '.info'
 
-        with contextlib.nested(
-                mock.patch.object(self._driver, '_ensure_share_mounted'),
-                mock.patch.object(self._driver, '_local_volume_dir'),
-                mock.patch.object(self._driver, 'get_active_image_from_info'),
-                mock.patch.object(self._driver, '_execute'),
-                mock.patch.object(self._driver, '_local_path_volume'),
-                mock.patch.object(self._driver, '_local_path_volume_info')
-        ) as (mock_ensure_share_mounted, mock_local_volume_dir,
-              mock_active_image_from_info, mock_execute,
-              mock_local_path_volume, mock_local_path_volume_info):
+        with mock.patch.object(self._driver, '_ensure_share_mounted') as \
+                mock_ensure_share_mounted, \
+                mock.patch.object(self._driver, '_local_volume_dir') as \
+                mock_local_volume_dir, \
+                mock.patch.object(self._driver,
+                                  'get_active_image_from_info') as \
+                mock_active_image_from_info, \
+                mock.patch.object(self._driver, '_execute') as \
+                mock_execute, \
+                mock.patch.object(self._driver, '_local_path_volume') as \
+                mock_local_path_volume, \
+                mock.patch.object(self._driver, '_local_path_volume_info') as \
+                mock_local_path_volume_info:
             mock_local_volume_dir.return_value = self.TEST_MNT_POINT
             mock_active_image_from_info.return_value = volume_filename
             mock_local_path_volume.return_value = volume_path
@@ -812,15 +806,16 @@ class QuobyteDriverTestCase(test.TestCase):
         volume_path = '%s/%s' % (self.TEST_MNT_POINT, volume['name'])
         image_meta = {'id': '10958016-e196-42e3-9e7f-5d8927ae3099'}
 
-        with contextlib.nested(
-            mock.patch.object(drv, 'get_active_image_from_info'),
-            mock.patch.object(drv, '_local_volume_dir'),
-            mock.patch.object(image_utils, 'qemu_img_info'),
-            mock.patch.object(image_utils, 'upload_volume'),
-            mock.patch.object(image_utils, 'create_temporary_file')
-        ) as (mock_get_active_image_from_info, mock_local_volume_dir,
-              mock_qemu_img_info, mock_upload_volume,
-              mock_create_temporary_file):
+        with mock.patch.object(drv, 'get_active_image_from_info') as \
+                mock_get_active_image_from_info, \
+                mock.patch.object(drv, '_local_volume_dir') as \
+                mock_local_volume_dir, \
+                mock.patch.object(image_utils, 'qemu_img_info') as \
+                mock_qemu_img_info, \
+                mock.patch.object(image_utils, 'upload_volume') as \
+                mock_upload_volume, \
+                mock.patch.object(image_utils, 'create_temporary_file') as \
+                mock_create_temporary_file:
             mock_get_active_image_from_info.return_value = volume['name']
 
             mock_local_volume_dir.return_value = self.TEST_MNT_POINT
@@ -854,16 +849,18 @@ class QuobyteDriverTestCase(test.TestCase):
         volume_path = '%s/%s' % (self.TEST_MNT_POINT, volume['name'])
         image_meta = {'id': '10958016-e196-42e3-9e7f-5d8927ae3099'}
 
-        with contextlib.nested(
-            mock.patch.object(drv, 'get_active_image_from_info'),
-            mock.patch.object(drv, '_local_volume_dir'),
-            mock.patch.object(image_utils, 'qemu_img_info'),
-            mock.patch.object(image_utils, 'convert_image'),
-            mock.patch.object(image_utils, 'upload_volume'),
-            mock.patch.object(image_utils, 'create_temporary_file')
-        ) as (mock_get_active_image_from_info, mock_local_volume_dir,
-              mock_qemu_img_info, mock_convert_image, mock_upload_volume,
-              mock_create_temporary_file):
+        with mock.patch.object(drv, 'get_active_image_from_info') as \
+                mock_get_active_image_from_info, \
+                mock.patch.object(drv, '_local_volume_dir') as \
+                mock_local_volume_dir, \
+                mock.patch.object(image_utils, 'qemu_img_info') as \
+                mock_qemu_img_info, \
+                mock.patch.object(image_utils, 'convert_image') as \
+                mock_convert_image, \
+                mock.patch.object(image_utils, 'upload_volume') as \
+                mock_upload_volume, \
+                mock.patch.object(image_utils, 'create_temporary_file') as \
+                mock_create_temporary_file:
             mock_get_active_image_from_info.return_value = volume['name']
 
             mock_local_volume_dir.return_value = self.TEST_MNT_POINT
@@ -900,16 +897,18 @@ class QuobyteDriverTestCase(test.TestCase):
         volume_filename = 'volume-%s' % self.VOLUME_UUID
         image_meta = {'id': '10958016-e196-42e3-9e7f-5d8927ae3099'}
 
-        with contextlib.nested(
-            mock.patch.object(drv, 'get_active_image_from_info'),
-            mock.patch.object(drv, '_local_volume_dir'),
-            mock.patch.object(image_utils, 'qemu_img_info'),
-            mock.patch.object(image_utils, 'convert_image'),
-            mock.patch.object(image_utils, 'upload_volume'),
-            mock.patch.object(image_utils, 'create_temporary_file')
-        ) as (mock_get_active_image_from_info, mock_local_volume_dir,
-              mock_qemu_img_info, mock_convert_image, mock_upload_volume,
-              mock_create_temporary_file):
+        with mock.patch.object(drv, 'get_active_image_from_info') as \
+                mock_get_active_image_from_info, \
+                mock.patch.object(drv, '_local_volume_dir') as \
+                mock_local_volume_dir, \
+                mock.patch.object(image_utils, 'qemu_img_info') as \
+                mock_qemu_img_info, \
+                mock.patch.object(image_utils, 'convert_image') as \
+                mock_convert_image, \
+                mock.patch.object(image_utils, 'upload_volume') as \
+                mock_upload_volume, \
+                mock.patch.object(image_utils, 'create_temporary_file') as \
+                mock_create_temporary_file:
             mock_get_active_image_from_info.return_value = volume['name']
 
             mock_local_volume_dir.return_value = self.TEST_MNT_POINT

diff --git a/cinder/tests/test_smbfs.py b/cinder/tests/test_smbfs.py
index aa1dd69fe95cc02fddf2624bd66b0adbebf9845a..b7faf812ac99540b6ce719bcc4138c6d97a9296c 100644 (file)
--- a/cinder/tests/test_smbfs.py
+++ b/cinder/tests/test_smbfs.py
@@ -12,7 +12,6 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import contextlib
 import copy
 import os
 
@@ -321,10 +320,9 @@ class SmbFsTestCase(test.TestCase):
             return_value=mock.Mock(file_format=image_format))
         drv._delete = mock.Mock()
 
-        with contextlib.nested(
-                mock.patch.object(image_utils, 'resize_image'),
-                mock.patch.object(image_utils, 'convert_image')) as (
-                    fake_resize, fake_convert):
+        with mock.patch.object(image_utils, 'resize_image') as fake_resize, \
+                mock.patch.object(image_utils, 'convert_image') as \
+                fake_convert:
             if extend_failed:
                 self.assertRaises(exception.ExtendVolumeError,
                                   drv._extend_volume,
@@ -473,13 +471,9 @@ class SmbFsTestCase(test.TestCase):
             mock.sentinel.block_size)
 
         exc = None
-        with contextlib.nested(
-            mock.patch.object(image_utils,
-                              'fetch_to_volume_format'),
-            mock.patch.object(image_utils,
-                              'qemu_img_info')) as (
-                fake_fetch,
-                fake_qemu_img_info):
+        with mock.patch.object(image_utils, 'fetch_to_volume_format') as \
+                fake_fetch, mock.patch.object(image_utils, 'qemu_img_info') as \
+                fake_qemu_img_info:
 
             if wrong_size_after_fetch:
                 exc = exception.ImageUnacceptable

diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py
index cf66d7fbf6fe405242d46933ec7ef4a5ea0502ce..9e304f86cbf6efd38f0046fc3d232e1776ceb9bd 100644 (file)
--- a/cinder/tests/test_volume.py
+++ b/cinder/tests/test_volume.py
@@ -18,7 +18,6 @@ Tests for Volume Code.
 
 """
 
-import contextlib
 import datetime
 import os
 import shutil
@@ -1618,12 +1617,10 @@ class VolumeTestCase(BaseVolumeTestCase):
                           'key2': 'value2'}
                       }
 
-        with contextlib.nested(
-            mock.patch.object(cinder.volume.volume_types,
-                              'get_volume_type_qos_specs'),
+        with mock.patch.object(cinder.volume.volume_types,
+                               'get_volume_type_qos_specs') as type_qos, \
             mock.patch.object(cinder.tests.fake_driver.FakeISCSIDriver,
-                              'initialize_connection')
-        ) as (type_qos, driver_init):
+                              'initialize_connection') as driver_init:
             type_qos.return_value = dict(qos_specs=qos_values)
             driver_init.return_value = {'data': {}}
             qos_specs_expected = {'key1': 'value1',