RBD: Improve close and flush in IOWrapper

On Change-Id Ib3b066a7da071b1c2de78a1a4e569676539bd335 we improved the
RBDVolumeIOWrapper's flush and close methods, and this patch improves
them even further.

If the IOWrapper's close is not explicitly called and it's just
dereferenced (happens in unit tests) then during Garbage Collection the
wrapped image may be destroyed before the wrapper, which would trigger
the image being closed without the wrapper knowing, so when the wrapper
gets destroyed it will fail because it calls its close method, which
calls its flush, which calls the underlying image's flush, which will
fail because the underlying image was already closed.

We need to check if the underlying image has already being flushed
before calling the flush.

Calling the underlying close method for the Image or IOWrapper classes
is not a problem because they are idempotent.

Change-Id: Ib5a517d58427df0d1d1b22ad3dc66f673da147fe

os_brick/initiator/linuxrbd.py

@@ -235,8 +235,17 @@ class RBDVolumeIOWrapper(io.RawIOBase):
     def flush(self) -> None:
         # Raise ValueError if already closed
         super().flush()
+        # Don't fail on flush by calling it when underlying image is closed.
         try:
-            self._rbd_volume.image.flush()
+            self.rbd_image.require_not_closed()
+        except rbd.InvalidArgument:  # Image is closed
+            LOG.warning("RBDVolumeIOWrapper's underlying image %s was closed "
+                        "directly (probably by the GC) instead of through the "
+                        "wrapper", self.rbd_image.name)
+            return
+
+        try:
+            self.rbd_image.flush()
         except AttributeError:
             LOG.warning("flush() not supported in this version of librbd")
 

os_brick/tests/initiator/test_linuxrbd.py

@@ -98,6 +98,10 @@ class RBDClientTestCase(base.TestCase):
         self.assertRaises(exception.BrickException, test)
 
 
+class InvalidArgument(Exception):
+    pass
+
+
 class RBDVolumeIOWrapperTestCase(base.TestCase):
 
     def setUp(self):
@@ -107,6 +111,8 @@ class RBDVolumeIOWrapperTestCase(base.TestCase):
             linuxrbd.RBDVolumeIOWrapper(self.mock_volume)
         self.data_length = 1024
         self.full_data = 'abcd' * 256
+        self._rbd_lib = self.patch('os_brick.initiator.linuxrbd.rbd')
+        self._rbd_lib.InvalidArgument = InvalidArgument
 
     def test_init(self):
         self.assertEqual(self.mock_volume,
@@ -187,12 +193,29 @@ class RBDVolumeIOWrapperTestCase(base.TestCase):
             self.mock_volume.image.flush = mock.Mock()
             self.mock_volume_wrapper.flush()
             self.assertEqual(1, self.mock_volume.image.flush.call_count)
+            self.mock_volume.image.require_not_closed.assert_called_once_with()
             self.mock_volume.image.flush.reset_mock()
+            self.mock_volume.image.require_not_closed.reset_mock()
             # this should be caught and logged silently.
             self.mock_volume.image.flush.side_effect = AttributeError
             self.mock_volume_wrapper.flush()
             self.assertEqual(1, self.mock_volume.image.flush.call_count)
             self.assertEqual(1, mock_logger.warning.call_count)
+            self.mock_volume.image.require_not_closed.assert_called_once_with()
+
+    def test_flush_closed_image(self):
+        """Test when image is closed but wrapper isn't"""
+        with mock.patch.object(linuxrbd, 'LOG') as mock_logger:
+            self.mock_volume.image.require_not_closed.side_effect = \
+                InvalidArgument
+            self.mock_volume.image.flush = mock.Mock()
+            self.mock_volume_wrapper.flush()
+            self.mock_volume.image.flush.assert_not_called()
+            self.assertEqual(1, mock_logger.warning.call_count)
+            log_msg = mock_logger.warning.call_args[0][0]
+            self.assertTrue(
+                log_msg.startswith("RBDVolumeIOWrapper's underlying image"))
+            self.mock_volume.image.require_not_closed.assert_called_once_with()
 
     def test_flush_on_closed(self):
         self.mock_volume_wrapper.close()
@@ -201,6 +224,15 @@ class RBDVolumeIOWrapperTestCase(base.TestCase):
         self.mock_volume.image.flush.reset_mock()
         self.assertRaises(ValueError, self.mock_volume_wrapper.flush)
         self.mock_volume.image.flush.assert_not_called()
+        self.mock_volume.image.require_not_closed.assert_called_once_with()
+
+    def test_flush_on_image_closed(self):
+        self.mock_volume.image.require_not_closed.side_effect = InvalidArgument
+        self.mock_volume_wrapper.close()
+        self.mock_volume.image.flush.assert_not_called()
+        self.assertTrue(self.mock_volume_wrapper.closed)
+        self.mock_volume.image.close.assert_called_once_with()
+        self.mock_volume.image.require_not_closed.assert_called_once_with()
 
     def test_fileno(self):
         self.assertRaises(IOError, self.mock_volume_wrapper.fileno)
@@ -213,7 +245,8 @@ class RBDVolumeIOWrapperTestCase(base.TestCase):
         rbd_volume = linuxrbd.RBDVolume(rbd_client, 'volume')
         rbd_handle = linuxrbd.RBDVolumeIOWrapper(
             linuxrbd.RBDImageMetadata(rbd_volume, 'pool', 'user', None))
-        rbd_handle.close()
+        with mock.patch.object(rbd_volume, 'closed', False):
+            rbd_handle.close()
         self.assertEqual(1, rbd_disconnect.call_count)
         # Confirm the handle now reports that is closed (this attribute cannot
         # be modified directly)