Sheepdog: improve create and delete operation

This patch changes create_volume and delete_volume to use
SheepdogClient Class method for detailed Error logs.

SheepdogClient Class methods are implemented to enable
fine grained Error handling.

So, the administrators are enabled to do error recovery
more easily using those logs.

Change-Id: I54d320475c143c5f96d70abd7314320ef79f55bf
Depends-On: I738c23b9213ebd781ab399a3198551c8b8dfe382

diff --git a/cinder/tests/unit/test_sheepdog.py b/cinder/tests/unit/test_sheepdog.py
index 56de0021836969626267cc7a930f7b77a3bcd653..74f0c474c58de71de3d45bf281e1a279f811eed1 100644 (file)
--- a/cinder/tests/unit/test_sheepdog.py
+++ b/cinder/tests/unit/test_sheepdog.py
@@ -59,6 +59,14 @@ class SheepdogDriverTestDataGenerator(object):
         return fake_volume.fake_volume_obj(context.get_admin_context(),
                                            **volume_data)
 
+    def cmd_dog_vdi_create(self, name, size):
+        return ('env', 'LC_ALL=C', 'LANG=C', 'dog', 'vdi', 'create', name,
+                '%sG' % size, '-a', SHEEP_ADDR, '-p', str(SHEEP_PORT))
+
+    def cmd_dog_vdi_delete(self, name):
+        return ('env', 'LC_ALL=C', 'LANG=C', 'dog', 'vdi', 'delete', name,
+                '-a', SHEEP_ADDR, '-p', str(SHEEP_PORT))
+
     CMD_DOG_CLUSTER_INFO = ('env', 'LC_ALL=C', 'LANG=C', 'dog', 'cluster',
                             'info', '-a', SHEEP_ADDR, '-p', str(SHEEP_PORT))
 
@@ -123,6 +131,14 @@ Epoch Time           Version [Host:Port:V-Nodes,,,]
 
     DOG_CLUSTER_INFO_SHUTTING_DOWN = """\
 Cluster status: System is shutting down
+"""
+
+    DOG_VDI_CREATE_VDI_ALREADY_EXISTS = """\
+Failed to create VDI %(vdiname)s: VDI exists already
+"""
+
+    DOG_VDI_DELETE_VDI_NOT_EXISTS = """\
+Failed to open VDI %(vdiname)s (snapshot id: 0 snapshot tag: ): No VDI found
 """
 
     DOG_COMMAND_ERROR_FAIL_TO_CONNECT = """\
@@ -249,6 +265,8 @@ class SheepdogClientTestCase(test.TestCase):
         self.driver.do_setup(None)
         self.test_data = SheepdogDriverTestDataGenerator()
         self.client = self.driver.client
+        self._vdiname = self.test_data.TEST_VOLUME.name
+        self._vdisize = self.test_data.TEST_VOLUME.size
 
     @mock.patch.object(utils, 'execute')
     def test_run_dog_success(self, fake_execute):
@@ -396,6 +414,134 @@ class SheepdogClientTestCase(test.TestCase):
                                self.client.check_cluster_status)
         self.assertEqual(expected_msg, ex.msg)
 
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    def test_create_success(self, fake_execute):
+        expected_cmd = ('vdi', 'create', self._vdiname, '%sG' % self._vdisize)
+        fake_execute.return_value = ('', '')
+        self.client.create(self._vdiname, self._vdisize)
+        fake_execute.assert_called_once_with(*expected_cmd)
+
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    @mock.patch.object(sheepdog, 'LOG')
+    def test_create_fail_to_connect(self, fake_logger, fake_execute):
+        cmd = self.test_data.cmd_dog_vdi_create(self._vdiname, self._vdisize)
+        exit_code = 2
+        stdout = ''
+        stderr = self.test_data.DOG_COMMAND_ERROR_FAIL_TO_CONNECT
+        expected_msg = self.test_data.sheepdog_cmd_error(
+            cmd=cmd, exit_code=exit_code, stdout=stdout, stderr=stderr)
+        fake_execute.side_effect = exception.SheepdogCmdError(
+            cmd=cmd, exit_code=exit_code, stdout=stdout.replace('\n', '\\n'),
+            stderr=stderr.replace('\n', '\\n'))
+        ex = self.assertRaises(exception.SheepdogCmdError, self.client.create,
+                               self._vdiname, self._vdisize)
+        self.assertTrue(fake_logger.error.called)
+        self.assertEqual(expected_msg, ex.msg)
+
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    @mock.patch.object(sheepdog, 'LOG')
+    def test_create_vdi_already_exists(self, fake_logger, fake_execute):
+        cmd = self.test_data.cmd_dog_vdi_create(self._vdiname, self._vdisize)
+        exit_code = 1
+        stdout = ''
+        stderr = (self.test_data.DOG_VDI_CREATE_VDI_ALREADY_EXISTS %
+                  {'vdiname': self._vdiname})
+        expected_msg = self.test_data.sheepdog_cmd_error(
+            cmd=cmd, exit_code=exit_code, stdout=stdout, stderr=stderr)
+        fake_execute.side_effect = exception.SheepdogCmdError(
+            cmd=cmd, exit_code=exit_code, stdout=stdout.replace('\n', '\\n'),
+            stderr=stderr.replace('\n', '\\n'))
+        ex = self.assertRaises(exception.SheepdogCmdError, self.client.create,
+                               self._vdiname, self._vdisize)
+        self.assertTrue(fake_logger.error.called)
+        self.assertEqual(expected_msg, ex.msg)
+
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    @mock.patch.object(sheepdog, 'LOG')
+    def test_create_unknown_error(self, fake_logger, fake_execute):
+        cmd = self.test_data.cmd_dog_vdi_create(self._vdiname, self._vdisize)
+        exit_code = 1
+        stdout = 'stdout_dummy'
+        stderr = 'stderr_dummy'
+        expected_msg = self.test_data.sheepdog_cmd_error(
+            cmd=cmd, exit_code=exit_code, stdout=stdout, stderr=stderr)
+        fake_execute.side_effect = exception.SheepdogCmdError(
+            cmd=cmd, exit_code=exit_code, stdout=stdout.replace('\n', '\\n'),
+            stderr=stderr.replace('\n', '\\n'))
+        ex = self.assertRaises(exception.SheepdogCmdError, self.client.create,
+                               self._vdiname, self._vdisize)
+        self.assertTrue(fake_logger.error.called)
+        self.assertEqual(expected_msg, ex.msg)
+
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    def test_delete_success(self, fake_execute):
+        expected_cmd = ('vdi', 'delete', self._vdiname)
+        fake_execute.return_value = ('', '')
+        self.client.delete(self._vdiname)
+        fake_execute.assert_called_once_with(*expected_cmd)
+
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    @mock.patch.object(sheepdog, 'LOG')
+    def test_delete_vdi_not_found(self, fake_logger, fake_execute):
+        stdout = ''
+        stderr = (self.test_data.DOG_VDI_DELETE_VDI_NOT_EXISTS %
+                  {'vdiname': self._vdiname})
+        fake_execute.return_value = (stdout, stderr)
+        self.client.delete(self._vdiname)
+        self.assertTrue(fake_logger.warning.called)
+
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    def test_delete_fail_to_connect_bugcase(self, fake_execute):
+        # NOTE(tishizaki): Sheepdog's bug case.
+        # details are written to Sheepdog driver code.
+        stdout = ''
+        stderr = self.test_data.DOG_COMMAND_ERROR_FAIL_TO_CONNECT
+        expected_reason = (_('Failed to connect to sheep daemon. '
+                           'addr: %(addr)s, port: %(port)s'),
+                           {'addr': SHEEP_ADDR, 'port': SHEEP_PORT})
+        fake_execute.return_value = (stdout, stderr)
+        ex = self.assertRaises(exception.SheepdogError,
+                               self.client.delete, self._vdiname)
+        self.assertEqual(expected_reason, ex.kwargs['reason'])
+
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    @mock.patch.object(sheepdog, 'LOG')
+    def test_delete_fail_to_connect(self, fake_logger, fake_execute):
+        cmd = self.test_data.cmd_dog_vdi_delete(self._vdiname)
+        exit_code = 2
+        stdout = 'stdout_dummy'
+        stderr = self.test_data.DOG_COMMAND_ERROR_FAIL_TO_CONNECT
+        expected_msg = self.test_data.sheepdog_cmd_error(cmd=cmd,
+                                                         exit_code=exit_code,
+                                                         stdout=stdout,
+                                                         stderr=stderr)
+        fake_execute.side_effect = exception.SheepdogCmdError(
+            cmd=cmd, exit_code=exit_code, stdout=stdout.replace('\n', '\\n'),
+            stderr=stderr.replace('\n', '\\n'))
+        ex = self.assertRaises(exception.SheepdogCmdError,
+                               self.client.delete, self._vdiname)
+        self.assertTrue(fake_logger.error.called)
+        self.assertEqual(expected_msg, ex.msg)
+
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    @mock.patch.object(sheepdog, 'LOG')
+    def test_delete_unknown_error(self, fake_logger, fake_execute):
+        cmd = self.test_data.cmd_dog_vdi_delete(self._vdiname)
+        exit_code = 2
+        stdout = 'stdout_dummy'
+        stderr = 'stderr_dummy'
+        expected_msg = self.test_data.sheepdog_cmd_error(cmd=cmd,
+                                                         exit_code=exit_code,
+                                                         stdout=stdout,
+                                                         stderr=stderr)
+        fake_execute.side_effect = exception.SheepdogCmdError(
+            cmd=cmd, exit_code=exit_code, stdout=stdout.replace('\n', '\\n'),
+            stderr=stderr.replace('\n', '\\n'))
+        ex = self.assertRaises(exception.SheepdogCmdError,
+                               self.client.delete, self._vdiname)
+        self.assertTrue(fake_logger.error.called)
+        self.assertEqual(expected_msg, ex.msg)
+
 
 class SheepdogDriverTestCase(test.TestCase):
     def setUp(self):
@@ -410,12 +556,24 @@ class SheepdogDriverTestCase(test.TestCase):
         self.driver.do_setup(None)
         self.test_data = SheepdogDriverTestDataGenerator()
         self.client = self.driver.client
+        self._vdiname = self.test_data.TEST_VOLUME.name
+        self._vdisize = self.test_data.TEST_VOLUME.size
 
     @mock.patch.object(sheepdog.SheepdogClient, 'check_cluster_status')
     def test_check_for_setup_error(self, fake_execute):
         self.driver.check_for_setup_error()
         fake_execute.assert_called_once_with()
 
+    @mock.patch.object(sheepdog.SheepdogClient, 'create')
+    def test_create_volume(self, fake_execute):
+        self.driver.create_volume(self.test_data.TEST_VOLUME)
+        fake_execute.assert_called_once_with(self._vdiname, self._vdisize)
+
+    @mock.patch.object(sheepdog.SheepdogClient, 'delete')
+    def test_delete_volume(self, fake_execute):
+        self.driver.delete_volume(self.test_data.TEST_VOLUME)
+        fake_execute.assert_called_once_with(self._vdiname)
+
     def test_update_volume_stats(self):
         def fake_stats(*args):
             return self.test_data.COLLIE_NODE_INFO, ''
@@ -448,7 +606,8 @@ class SheepdogDriverTestCase(test.TestCase):
         actual = self.driver.get_volume_stats(True)
         self.assertDictMatch(expected, actual)
 
-    def test_copy_image_to_volume(self):
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    def test_copy_image_to_volume(self, fake_run_dog):
         @contextlib.contextmanager
         def fake_temp_file():
             class FakeTmp(object):
@@ -467,8 +626,8 @@ class SheepdogDriverTestCase(test.TestCase):
         self.stubs.Set(sheepdog.SheepdogDriver,
                        '_try_execute',
                        fake_try_execute)
-        self.driver.copy_image_to_volume(None, {'name': 'test',
-                                                'size': 1},
+        fake_run_dog.return_value = ('fake_stdout', 'fake_stderr')
+        self.driver.copy_image_to_volume(None, self.test_data.TEST_VOLUME,
                                          FakeImageService(), None)
 
     def test_copy_volume_to_image(self):

diff --git a/cinder/volume/drivers/sheepdog.py b/cinder/volume/drivers/sheepdog.py
index ff54f6e373ed557e804c303990778d6a76a96788..66443a3061a224d32d0ad1aa53db3e843c5aa032 100644 (file)
--- a/cinder/volume/drivers/sheepdog.py
+++ b/cinder/volume/drivers/sheepdog.py
@@ -31,7 +31,7 @@ from oslo_utils import excutils
 from oslo_utils import units
 
 from cinder import exception
-from cinder.i18n import _, _LE
+from cinder.i18n import _, _LE, _LW
 from cinder.image import image_utils
 from cinder import utils
 from cinder.volume import driver
@@ -62,6 +62,8 @@ class SheepdogClient(object):
                                       'Waiting for cluster to be formatted')
     DOG_RESP_CLUSTER_WAITING = ('Cluster status: '
                                 'Waiting for other nodes to join cluster')
+    DOG_RESP_VDI_ALREADY_EXISTS = ': VDI exists already'
+    DOG_RESP_VDI_NOT_FOUND = ': No VDI found'
 
     def __init__(self, addr, port):
         self.addr = addr
@@ -95,7 +97,7 @@ class SheepdogClient(object):
             _stderr = e.kwargs['stderr']
             with excutils.save_and_reraise_exception():
                 if _stderr.startswith(self.DOG_RESP_CONNECTION_ERROR):
-                    msg = _LE('Failed to connect sheep daemon. '
+                    msg = _LE('Failed to connect to sheep daemon. '
                               'addr: %(addr)s, port: %(port)s')
                     LOG.error(msg, {'addr': self.addr, 'port': self.port})
                 else:
@@ -115,6 +117,49 @@ class SheepdogClient(object):
                        'Ensure all sheep daemons are running.')
         raise exception.SheepdogError(reason=reason)
 
+    def create(self, vdiname, size):
+        try:
+            self._run_dog('vdi', 'create', vdiname, '%sG' % size)
+        except exception.SheepdogCmdError as e:
+            _stderr = e.kwargs['stderr']
+            with excutils.save_and_reraise_exception():
+                if _stderr.startswith(self.DOG_RESP_CONNECTION_ERROR):
+                    LOG.error(_LE("Failed to connect to sheep daemon. "
+                              "addr: %(addr)s, port: %(port)s"),
+                              {'addr': self.addr, 'port': self.port})
+                elif _stderr.rstrip('\\n').endswith(
+                        self.DOG_RESP_VDI_ALREADY_EXISTS):
+                    LOG.error(_LE('Volume already exists. %s'), vdiname)
+                else:
+                    LOG.error(_LE('Failed to create volume. %s'), vdiname)
+
+    def delete(self, vdiname):
+        try:
+            (_stdout, _stderr) = self._run_dog('vdi', 'delete', vdiname)
+            if _stderr.rstrip().endswith(self.DOG_RESP_VDI_NOT_FOUND):
+                LOG.warning(_LW('Volume not found. %s'), vdiname)
+            elif _stderr.startswith(self.DOG_RESP_CONNECTION_ERROR):
+                # NOTE(tishizaki)
+                # Dog command does not return error_code although
+                # dog command cannot connect to sheep process.
+                # That is a Sheepdog's bug.
+                # To avoid a Sheepdog's bug, now we need to check stderr.
+                # If Sheepdog has been fixed, this check logic is needed
+                # by old Sheepdog users.
+                reason = (_('Failed to connect to sheep daemon. '
+                          'addr: %(addr)s, port: %(port)s'),
+                          {'addr': self.addr, 'port': self.port})
+                raise exception.SheepdogError(reason=reason)
+        except exception.SheepdogCmdError as e:
+            _stderr = e.kwargs['stderr']
+            with excutils.save_and_reraise_exception():
+                if _stderr.startswith(self.DOG_RESP_CONNECTION_ERROR):
+                    LOG.error(_LE('Failed to connect to sheep daemon. '
+                              'addr: %(addr)s, port: %(port)s'),
+                              {'addr': self.addr, 'port': self.port})
+                else:
+                    LOG.error(_LE('Failed to delete volume. %s'), vdiname)
+
 
 class SheepdogIOWrapper(io.RawIOBase):
     """File-like object with Sheepdog backend."""
@@ -303,9 +348,7 @@ class SheepdogDriver(driver.VolumeDriver):
 
     def create_volume(self, volume):
         """Create a sheepdog volume."""
-        self._try_execute('qemu-img', 'create',
-                          "sheepdog:%s" % volume['name'],
-                          '%sG' % volume['size'])
+        self.client.create(volume.name, volume.size)
 
     def create_volume_from_snapshot(self, volume, snapshot):
         """Create a sheepdog volume from a snapshot."""
@@ -317,7 +360,7 @@ class SheepdogDriver(driver.VolumeDriver):
 
     def delete_volume(self, volume):
         """Delete a logical volume."""
-        self._delete(volume)
+        self.client.delete(volume.name)
 
     def _resize(self, volume, size=None):
         if not size:
@@ -326,10 +369,6 @@ class SheepdogDriver(driver.VolumeDriver):
         self._try_execute('collie', 'vdi', 'resize',
                           volume['name'], size)
 
-    def _delete(self, volume):
-        self._try_execute('collie', 'vdi', 'delete',
-                          volume['name'])
-
     def copy_image_to_volume(self, context, volume, image_service, image_id):
         with image_utils.temporary_file() as tmp:
             # (wenhao): we don't need to convert to raw for sheepdog.
@@ -338,7 +377,7 @@ class SheepdogDriver(driver.VolumeDriver):
 
             # remove the image created by import before this function.
             # see volume/drivers/manager.py:_create_volume
-            self._delete(volume)
+            self.client.delete(volume.name)
             # convert and store into sheepdog
             image_utils.convert_image(tmp, 'sheepdog:%s' % volume['name'],
                                       'raw')