SheepdogDriver: Improve get_volume_stats operation

This patch improves get_volume_stats method to invoke
dog command in SheepdogClient Class method instead of
in SheepdogDriver class method directly. Here are two
benefits we can realize as a result:

1.The current implementation can only get volume status
by local sheepdog node, but the SheepdogClient Class also
supports the method to run dog command with remote sheepdog
node.

2.SheepdogClient Class methods are implemented to run dog
command with fine grained Error handling. So it can improve
the robustness and is more readable to run dog command in
SheepdogClient Class method.

Change-Id: I64ca193cd50e6914d0d5fb6cf711c760dfb65b8c
Closes-Bug: #1512287

diff --git a/cinder/tests/unit/test_sheepdog.py b/cinder/tests/unit/test_sheepdog.py
index dbbd53bb4186f2b5a9dfe53bc3735fda2dc2047a..3b909e61bae4fd641c1b652e6961080bd8a6df35 100644 (file)
--- a/cinder/tests/unit/test_sheepdog.py
+++ b/cinder/tests/unit/test_sheepdog.py
@@ -106,6 +106,10 @@ class SheepdogDriverTestDataGenerator(object):
         return ('env', 'LC_ALL=C', 'LANG=C', 'dog', 'vdi', 'resize', name,
                 size, '-a', SHEEP_ADDR, '-p', SHEEP_PORT)
 
+    def cmd_dog_node_info(self):
+        return ('env', 'LC_ALL=C', 'LANG=C', 'dog', 'node', 'info',
+                '-a', SHEEP_ADDR, '-p', SHEEP_PORT, '-r')
+
     CMD_DOG_CLUSTER_INFO = ('env', 'LC_ALL=C', 'LANG=C', 'dog', 'cluster',
                             'info', '-a', SHEEP_ADDR, '-p', SHEEP_PORT)
 
@@ -1137,6 +1141,51 @@ class SheepdogClientTestCase(test.TestCase):
         self.assertTrue(fake_logger.error.called)
         self.assertEqual(expected_msg, ex.msg)
 
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    def test_get_volume_stats_success(self, fake_execute):
+        expected_cmd = ('node', 'info', '-r')
+        fake_execute.return_value = (self.test_data.COLLIE_NODE_INFO, '')
+        self.client.get_volume_stats()
+        fake_execute.assert_called_once_with(*expected_cmd)
+
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    @mock.patch.object(sheepdog, 'LOG')
+    def test_get_volume_stats_fail_to_connect(self, fake_logger, fake_execute):
+        cmd = self.test_data.cmd_dog_node_info()
+        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.get_volume_stats)
+        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_get_volume_stats_unknown_error(self, fake_logger, fake_execute):
+        cmd = self.test_data.cmd_dog_node_info()
+        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.get_volume_stats)
+        self.assertTrue(fake_logger.error.called)
+        self.assertEqual(expected_msg, ex.msg)
+
 
 class SheepdogDriverTestCase(test.TestCase):
     def setUp(self):
@@ -1173,10 +1222,9 @@ class SheepdogDriverTestCase(test.TestCase):
         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, ''
-        self.stubs.Set(self.driver, '_execute', fake_stats)
+    @mock.patch.object(sheepdog.SheepdogClient, 'get_volume_stats')
+    def test_update_volume_stats(self, fake_execute):
+        fake_execute.return_value = self.test_data.COLLIE_NODE_INFO
         expected = dict(
             volume_backend_name='sheepdog',
             vendor_name='Open Source',
@@ -1189,22 +1237,6 @@ class SheepdogDriverTestCase(test.TestCase):
         actual = self.driver.get_volume_stats(True)
         self.assertDictMatch(expected, actual)
 
-    def test_update_volume_stats_error(self):
-        def fake_stats(*args):
-            raise processutils.ProcessExecutionError()
-        self.stubs.Set(self.driver, '_execute', fake_stats)
-        expected = dict(
-            volume_backend_name='sheepdog',
-            vendor_name='Open Source',
-            driver_version=self.driver.VERSION,
-            storage_protocol='sheepdog',
-            total_capacity_gb='unknown',
-            free_capacity_gb='unknown',
-            reserved_percentage=0,
-            QoS_support=False)
-        actual = self.driver.get_volume_stats(True)
-        self.assertDictMatch(expected, actual)
-
     @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
     def test_copy_image_to_volume(self, fake_run_dog):
         @contextlib.contextmanager

diff --git a/cinder/volume/drivers/sheepdog.py b/cinder/volume/drivers/sheepdog.py
index 21956cdd5768c8dbc62440ec24b69dfeb6de9823..b41c1b26e776caf34dd7a0a424621316ed85d546 100644 (file)
--- a/cinder/volume/drivers/sheepdog.py
+++ b/cinder/volume/drivers/sheepdog.py
@@ -320,6 +320,20 @@ class SheepdogClient(object):
                                   'vdi: %(vdiname)s new size: %(size)s'),
                               {'vdiname': vdiname, 'size': size})
 
+    def get_volume_stats(self):
+        try:
+            (_stdout, _stderr) = self._run_dog('node', 'info', '-r')
+        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 get volume status. '))
+        return _stdout
+
 
 class SheepdogIOWrapper(io.RawIOBase):
     """File-like object with Sheepdog backend."""
@@ -606,15 +620,12 @@ class SheepdogDriver(driver.VolumeDriver):
         stats['reserved_percentage'] = 0
         stats['QoS_support'] = False
 
-        try:
-            stdout, _err = self._execute('collie', 'node', 'info', '-r')
-            m = self.stats_pattern.match(stdout)
-            total = float(m.group(1))
-            used = float(m.group(2))
-            stats['total_capacity_gb'] = total / units.Gi
-            stats['free_capacity_gb'] = (total - used) / units.Gi
-        except processutils.ProcessExecutionError:
-            LOG.exception(_LE('error refreshing volume stats'))
+        stdout = self.client.get_volume_stats()
+        m = self.stats_pattern.match(stdout)
+        total = float(m.group(1))
+        used = float(m.group(2))
+        stats['total_capacity_gb'] = total / units.Gi
+        stats['free_capacity_gb'] = (total - used) / units.Gi
 
         self._stats = stats