Sheepdog:fix the bug of failed to clone image

The current sheepdog driver has two bugs when deciding whether
an image is cloneable:

1.It uses the addr in image_location url to find the image volume,
if the image volume is found, then considers the image is cloneable,
or considers it is not cloneable. Here has a problem, if cinder and
glance use different sheepdog cluster, this may lead the image is
always cloneable. In fact, it should use addr of cinder backend to
find the image volume, not the glance backend.

2.The driver uses 'dog vdi list volumename' command to find a volume,
if the command don't return an err, then considers the volume already
exists. This means that it assumes the command would return an err
code if the volume is not exists. But this is not the fact. Dog
command always returns 0 no matter the volume exists or not:
test for a existent volume:
$ dog vdi list test01 -r
= test01 0 4294967296 0 0 1469012083 40687a 3
$ echo $?
0
test for a nonexistent volume, return 0 and output nothing:
$ dog vdi list testvolume -r
$ echo $?
0

This patch fixed these two bugs.

Change-Id: I17a8a29be5c1d994ad36a70b2f76000f9566fbde
Closes-Bug: #1604740

cinder/tests/unit/volume/drivers/test_sheepdog.py

@@ -107,6 +107,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_vdi_list(self, name):
+        return ('env', 'LC_ALL=C', 'LANG=C', 'dog', 'vdi', 'list', name,
+                '-r', '-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')
@@ -159,6 +163,10 @@ Total 107287605248 3623897354 3% 54760833024
 
     COLLIE_NODE_LIST = """
 0 127.0.0.1:7000 128 1
+"""
+
+    COLLIE_VDI_LIST = """
+= testvolume 0 0 0 0 1467037106 fd32fc 3
 """
 
     COLLIE_CLUSTER_INFO_0_5 = """\
@@ -1063,6 +1071,33 @@ 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_vdi_info_success(self, fake_execute):
+
+        expected_cmd = ('vdi', 'list', self._vdiname, '-r')
+        fake_execute.return_value = (self.test_data.COLLIE_VDI_LIST, '')
+        self.client.get_vdi_info(self._vdiname)
+        fake_execute.assert_called_once_with(*expected_cmd)
+
+    @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
+    @mock.patch.object(sheepdog, 'LOG')
+    def test_get_vdi_info_unknown_error(self, fake_logger, fake_execute):
+        cmd = self.test_data.cmd_dog_vdi_list(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.get_vdi_info, self._vdiname)
+        self.assertTrue(fake_logger.error.called)
+        self.assertEqual(expected_msg, ex.msg)
+
     @mock.patch.object(sheepdog.SheepdogClient, '_run_dog')
     def test_update_node_list_success(self, fake_execute):
         expected_cmd = ('node', 'list', '-r')
@@ -1278,7 +1313,7 @@ class SheepdogDriverTestCase(test.TestCase):
         image_service = ''
 
         patch = mock.patch.object
-        with patch(self.driver, '_try_execute', return_value=True):
+        with patch(self.driver, '_is_cloneable', return_value=True):
             with patch(self.driver, 'create_cloned_volume'):
                 with patch(self.client, 'resize'):
                     model_updated, cloned = self.driver.clone_image(
@@ -1307,32 +1342,27 @@ class SheepdogDriverTestCase(test.TestCase):
 
     def test_is_cloneable(self):
         uuid = '87f1b01c-f46c-4537-bd5d-23962f5f4316'
-        location = 'sheepdog://ip:port:%s' % uuid
+        location = 'sheepdog://127.0.0.1:7000:%s' % uuid
         image_meta = {'id': uuid, 'size': 1, 'disk_format': 'raw'}
         invalid_image_meta = {'id': uuid, 'size': 1, 'disk_format': 'iso'}
 
-        with mock.patch.object(self.driver, '_try_execute') as try_execute:
+        with mock.patch.object(self.client, 'get_vdi_info') as fake_execute:
+            fake_execute.return_value = self.test_data.COLLIE_VDI_LIST
             self.assertTrue(
                 self.driver._is_cloneable(location, image_meta))
-            expected_cmd = ('collie', 'vdi', 'list',
-                            '--address', 'ip',
-                            '--port', 'port',
-                            uuid)
-            try_execute.assert_called_once_with(*expected_cmd)
 
-            # check returning False without executing a command
+            # Test for invalid location
             self.assertFalse(
                 self.driver._is_cloneable('invalid-location', image_meta))
-            self.assertFalse(
-                self.driver._is_cloneable(location, invalid_image_meta))
-            self.assertEqual(1, try_execute.call_count)
 
-        error = processutils.ProcessExecutionError
+            # Test for image not exist in sheepdog cluster
-        with mock.patch.object(self.driver, '_try_execute',
+            fake_execute.return_value = ''
-                               side_effect=error) as fail_try_execute:
             self.assertFalse(
                 self.driver._is_cloneable(location, image_meta))
-            fail_try_execute.assert_called_once_with(*expected_cmd)
+
+            # Test for invalid image meta
+            self.assertFalse(
+                self.driver._is_cloneable(location, invalid_image_meta))
 
     def test_create_volume_from_snapshot(self):
         dst_volume = self.test_data.TEST_CLONED_VOLUME

cinder/volume/drivers/sheepdog.py

@@ -313,6 +313,15 @@ class SheepdogClient(object):
                 LOG.error(_LE('Failed to get volume status. %s'), e)
         return _stdout
 
+    def get_vdi_info(self, vdiname):
+        # Get info of the specified vdi.
+        try:
+            (_stdout, _stderr) = self._run_dog('vdi', 'list', vdiname, '-r')
+        except exception.SheepdogCmdError as e:
+            with excutils.save_and_reraise_exception():
+                LOG.error(_LE('Failed to get vdi info. %s'), e)
+        return _stdout
+
     def update_node_list(self):
         try:
             (_stdout, _stderr) = self._run_dog('node', 'list', '-r')
@@ -463,21 +472,19 @@ class SheepdogDriver(driver.VolumeDriver):
                       image_meta['disk_format'])
             return False
 
-        cloneable = False
         # check whether volume is stored in sheepdog
-        try:
+        # The image location would be like
-            # The image location would be like
+        # "sheepdog://192.168.10.2:7000:Alice"
-            # "sheepdog://192.168.10.2:7000:Alice"
+        (ip, port, name) = image_location[len(prefix):].split(":", 2)
-            (ip, port, name) = image_location[len(prefix):].split(":", 2)
 
-            self._try_execute('collie', 'vdi', 'list', '--address', ip,
+        stdout = self.client.get_vdi_info(name)
-                              '--port', port, name)
+        # Dog command return 0 and has a null output if the volume not exists
-            cloneable = True
+        if stdout:
-        except processutils.ProcessExecutionError as e:
+            return True
-            LOG.debug("Can not find vdi %(image)s: %(err)s",
+        else:
-                      {'image': name, 'err': e})
+            LOG.debug("Can not find vdi %(image)s, is not cloneable",
-
+                      {'image': name})
-        return cloneable
+            return False
 
     def clone_image(self, context, volume,
                     image_location, image_meta,