vzstorage: fix create/delete snapshots

vzstorage doesn't support simultaneous open from different
clients. It means you can't open qcow2 file from cinder-volume
side, if it's used by an instance on nova-compute side.
The solution is to cache QemuImgInfo instance
under <path>.qemu_img_cache for image file <path>

Closes-Bug: #1615667
Change-Id: I8949ddc244e0eef699b2af224303f86507f6f30e
Co-Authored-By: Evgeny Antyshev <eantyshev@virtuozzo.com>

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

@@ -208,8 +208,11 @@ class RemoteFsSnapDriverTestCase(test.TestCase):
                                          self._fake_volume.name,
                                          self._fake_snapshot_path)
         command1 = ['qemu-img', 'create', '-f', 'qcow2', '-o',
-                    'backing_file=%s' % fake_backing_path,
+                    'backing_file=%s,backing_fmt=%s' %
-                    self._fake_snapshot_path]
+                    (fake_backing_path,
+                     mock.sentinel.backing_fmt),
+                    self._fake_snapshot_path,
+                    "%dG" % self._fake_volume.size]
         command2 = ['qemu-img', 'rebase', '-u',
                     '-b', self._fake_volume.name,
                     '-F', mock.sentinel.backing_fmt,

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

@@ -216,8 +216,9 @@ class VZStorageTestCase(test.TestCase):
         drv._check_extend_volume_support = mock.Mock(return_value=True)
         drv._is_file_size_equal = mock.Mock(return_value=True)
 
-        snap_info = """{"volume_format": "raw",
+        snap_info = '{"active": "%s"}' % self.vol.id
-                        "active": "%s"}""" % self.vol.id
+        with mock.patch.object(drv, 'get_volume_format',
+                               return_value="raw"):
             with mock.patch.object(drv, 'local_path',
                                    return_value=self._FAKE_VOLUME_PATH):
                 with mock.patch.object(drv, '_read_file',
@@ -263,7 +264,10 @@ class VZStorageTestCase(test.TestCase):
     def test_copy_volume_from_snapshot(self, mock_convert_image):
         drv = self._vz_driver
 
-        fake_volume_info = {self._FAKE_SNAPSHOT_ID: 'fake_snapshot_file_name'}
+        fake_volume_info = {self._FAKE_SNAPSHOT_ID: 'fake_snapshot_file_name',
+                            'backing-files':
+                            {self._FAKE_SNAPSHOT_ID:
+                             self._FAKE_VOLUME_NAME}}
         fake_img_info = mock.MagicMock()
         fake_img_info.backing_file = self._FAKE_VOLUME_NAME
 

cinder/volume/drivers/remotefs.py

@@ -747,6 +747,24 @@ class RemoteFSSnapDriverBase(RemoteFSDriver, driver.SnapshotVD):
 
         return json.loads(self._read_file(info_path))
 
+    def _get_higher_image_path(self, snapshot):
+        volume = snapshot.volume
+        info_path = self._local_path_volume_info(volume)
+        snap_info = self._read_info_file(info_path)
+
+        snapshot_file = snap_info[snapshot.id]
+        active_file = self.get_active_image_from_info(volume)
+        active_file_path = os.path.join(self._local_volume_dir(volume),
+                                        active_file)
+        backing_chain = self._get_backing_chain_for_path(
+            volume, active_file_path)
+        higher_file = next((os.path.basename(f['filename'])
+                            for f in backing_chain
+                            if f.get('backing-filename', '') ==
+                            snapshot_file),
+                           None)
+        return higher_file
+
     def _get_backing_chain_for_path(self, volume, path):
         """Returns list of dicts containing backing-chain information.
 
@@ -1018,7 +1036,6 @@ class RemoteFSSnapDriverBase(RemoteFSDriver, driver.SnapshotVD):
 
         # Find what file has this as its backing file
         active_file = self.get_active_image_from_info(snapshot.volume)
-        active_file_path = os.path.join(vol_path, active_file)
 
         if volume_status == 'in-use':
             # Online delete
@@ -1065,15 +1082,9 @@ class RemoteFSSnapDriverBase(RemoteFSDriver, driver.SnapshotVD):
             #   exist, not   | committed down) |  exist, needs  |
             #   used here)   |                 |   ptr update)  |
 
-            backing_chain = self._get_backing_chain_for_path(
-                snapshot.volume, active_file_path)
             # This file is guaranteed to exist since we aren't operating on
             # the active file.
-            higher_file = next((os.path.basename(f['filename'])
+            higher_file = self._get_higher_image_path(snapshot)
-                                for f in backing_chain
-                                if f.get('backing-filename', '') ==
-                                snapshot_file),
-                               None)
             if higher_file is None:
                 msg = _('No file found with %s as backing file.') %\
                     snapshot_file
@@ -1132,19 +1143,20 @@ class RemoteFSSnapDriverBase(RemoteFSDriver, driver.SnapshotVD):
             new qcow2 file
         :param new_snap_path: filename of new qcow2 file
         """
-
         backing_path_full_path = os.path.join(
             self._local_volume_dir(snapshot.volume),
             backing_filename)
-
-        command = ['qemu-img', 'create', '-f', 'qcow2', '-o',
-                   'backing_file=%s' % backing_path_full_path, new_snap_path]
-        self._execute(*command, run_as_root=self._execute_as_root)
-
         info = self._qemu_img_info(backing_path_full_path,
                                    snapshot.volume.name)
         backing_fmt = info.file_format
 
+        command = ['qemu-img', 'create', '-f', 'qcow2', '-o',
+                   'backing_file=%s,backing_fmt=%s' %
+                   (backing_path_full_path, backing_fmt),
+                   new_snap_path,
+                   "%dG" % snapshot.volume.size]
+        self._execute(*command, run_as_root=self._execute_as_root)
+
         command = ['qemu-img', 'rebase', '-u',
                    '-b', backing_filename,
                    '-F', backing_fmt,

cinder/volume/drivers/vzstorage.py

@@ -23,6 +23,7 @@ from os_brick.remotefs import remotefs
 from oslo_concurrency import processutils as putils
 from oslo_config import cfg
 from oslo_log import log as logging
+from oslo_utils import imageutils
 from oslo_utils import units
 
 from cinder import exception
@@ -158,8 +159,31 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver):
             vzstorage_mount_options=opts)
 
     def _qemu_img_info(self, path, volume_name):
-        return super(VZStorageDriver, self)._qemu_img_info_base(
+        qemu_img_cache = path + ".qemu_img_info"
-            path, volume_name, self.configuration.vzstorage_mount_point_base)
+        if os.path.isdir(path):
+            # 'parallels' disks stored along with metadata xml as directories
+            # qemu-img should explore base data file inside
+            path = os.path.join(path, PLOOP_BASE_DELTA_NAME)
+        if os.path.isfile(qemu_img_cache):
+            info_tm = os.stat(qemu_img_cache).st_mtime
+            snap_tm = os.stat(path).st_mtime
+        ret = None
+        if not os.path.isfile(qemu_img_cache) or snap_tm > info_tm:
+            LOG.debug("Cached qemu-img info %s not present or outdated,"
+                      " refresh", qemu_img_cache)
+            ret = super(VZStorageDriver, self)._qemu_img_info_base(
+                path, volume_name,
+                self.configuration.vzstorage_mount_point_base)
+            # We need only backing_file and file_format
+            d = {'file_format': ret.file_format,
+                 'backing_file': ret.backing_file}
+            with open(qemu_img_cache, "w") as f:
+                json.dump(d, f)
+        else:
+            ret = imageutils.QemuImgInfo()
+            with open(qemu_img_cache, "r") as f:
+                ret.__dict__ = json.load(f)
+        return ret
 
     @remotefs_drv.locked_volume_id_operation
     def initialize_connection(self, volume, connector):
@@ -297,9 +321,12 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver):
                 self.configuration.vzstorage_default_volume_format)
 
     def get_volume_format(self, volume):
-        info_path = self._local_path_volume_info(volume)
+        active_file = self.get_active_image_from_info(volume)
-        snap_info = self._read_info_file(info_path)
+        active_file_path = os.path.join(self._local_volume_dir(volume),
-        return snap_info['volume_format']
+                                        active_file)
+
+        img_info = self._qemu_img_info(active_file_path, volume.name)
+        return img_info.file_format
 
     def _create_ploop(self, volume_path, volume_size):
         os.mkdir(volume_path)
@@ -312,7 +339,7 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver):
             raise
 
     def _do_create_volume(self, volume):
-        """Create a volume on given smbfs_share.
+        """Create a volume on given vzstorage share.
 
         :param volume: volume reference
         """
@@ -337,10 +364,12 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver):
             self._create_regular_file(volume_path, volume_size)
 
         info_path = self._local_path_volume_info(volume)
-        snap_info = {'volume_format': volume_format,
+        snap_info = {'active': os.path.basename(volume_path)}
-                     'active': 'volume-%s' % volume.id}
         self._write_info_file(info_path, snap_info)
 
+        # Query qemu-img info to cache the output
+        self._qemu_img_info(volume_path, volume.name)
+
     def _delete(self, path):
         self._execute('rm', '-rf', path, run_as_root=True)
 
@@ -418,6 +447,8 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver):
         self._do_extend_volume(self.local_path(volume),
                                volume.size,
                                volume_format)
+        # Query qemu-img info to cache the output
+        self._qemu_img_info(self.local_path(volume), volume.name)
 
     def _copy_volume_from_snapshot(self, snapshot, volume, volume_size):
         """Copy data from snapshot to destination volume.
@@ -453,19 +484,15 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver):
             with PloopDevice(self.local_path(snapshot.volume),
                              snapshot.id,
                              execute=self._execute) as dev:
-                image_utils.convert_image(dev, volume_path, out_format)
+                base_file = os.path.join(volume_path, 'root.hds')
+                image_utils.convert_image(dev, base_file, out_format)
         else:
             msg = _("Unsupported volume format %s") % volume_format
             raise exception.InvalidVolume(msg)
 
-        if out_format == DISK_FORMAT_PLOOP:
-            img_path = os.path.join(volume_path, 'root.hds')
-            os.rename(volume_path, volume_path + '.tmp')
-            os.mkdir(volume_path)
-            os.rename(volume_path + '.tmp', img_path)
-            self._execute('ploop', 'restore-descriptor', volume_path, img_path)
-
         self._extend_volume(volume, volume_size, out_format)
+        # Query qemu-img info to cache the output
+        img_info = self._qemu_img_info(volume_path, volume.name)
 
     @remotefs_drv.locked_volume_id_operation
     def delete_volume(self, volume):
@@ -475,7 +502,6 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver):
                      'specified, skipping.') % volume.name)
             LOG.error(msg)
             return
-#            raise exception.VzStorageException(msg)
 
         self._ensure_share_mounted(volume.provider_location)
         volume_dir = self._local_volume_dir(volume)
@@ -483,6 +509,7 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver):
                                     self.get_active_image_from_info(volume))
         if os.path.exists(mounted_path):
             self._delete(mounted_path)
+            self._delete(mounted_path + ".qemu_img_info")
         else:
             LOG.info(_LI("Skipping deletion of volume %s "
                          "as it does not exist."), mounted_path)
@@ -533,13 +560,69 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver):
         else:
             super(VZStorageDriver, self)._create_snapshot(snapshot)
 
+    def _do_create_snapshot(self, snapshot, backing_filename,
+                            new_snap_path):
+        super(VZStorageDriver, self)._do_create_snapshot(snapshot,
+                                                         backing_filename,
+                                                         new_snap_path)
+        # Cache qemu-img info for created snapshot
+        self._qemu_img_info(new_snap_path, snapshot.volume.name)
+
     def delete_snapshot(self, snapshot):
+        info_path = self._local_path_volume_info(snapshot.volume)
+        snap_info = self._read_info_file(info_path, empty_if_missing=True)
+        snap_file = os.path.join(self._local_volume_dir(snapshot.volume),
+                                 snap_info[snapshot.id])
+        active_file = os.path.join(self._local_volume_dir(snapshot.volume),
+                                   snap_info['active'])
+        higher_file = self._get_higher_image_path(snapshot)
+        if higher_file:
+            higher_file = os.path.join(self._local_volume_dir(snapshot.volume),
+                                       higher_file)
+        elif active_file != snap_file:
+            msg = (_("Expected higher file exists for snapshot %s") %
+                   snapshot.id)
+            raise exception.VzStorageException(msg)
+
+        img_info = self._qemu_img_info(snap_file, snapshot.volume.name)
+        base_file = os.path.join(self._local_volume_dir(snapshot.volume),
+                                 img_info.backing_file)
         volume_format = self.get_volume_format(snapshot.volume)
+        online = snapshot.volume.status == 'in-use'
+
         if volume_format == 'parallels':
             self._delete_snapshot_ploop(snapshot)
         else:
             super(VZStorageDriver, self)._delete_snapshot(snapshot)
 
+        def _qemu_info_cache(fn):
+            return fn + ".qemu_img_info"
+
+        def _update_backing_file(info_src, info_dst):
+            with open(info_src, 'r') as fs, open(info_dst, 'r') as fd:
+                src = json.load(fs)
+                dst = json.load(fd)
+            dst['backing_file'] = src['backing_file']
+            with open(info_dst, 'w') as fdw:
+                json.dump(dst, fdw)
+
+        if volume_format == "qcow2":
+            if snap_file != active_file:
+                # mv snap_file.info higher_file.info
+                _update_backing_file(
+                    _qemu_info_cache(snap_file),
+                    _qemu_info_cache(higher_file))
+                self._delete(_qemu_info_cache(snap_file))
+            elif online:
+                # mv base_file.info snap_file.info
+                _update_backing_file(
+                    _qemu_info_cache(base_file),
+                    _qemu_info_cache(snap_file))
+                self._delete(_qemu_info_cache(base_file))
+            else:
+                # rm snap_file.info
+                self._delete(_qemu_info_cache(snap_file))
+
     def _copy_volume_to_image(self, context, volume, image_service,
                               image_meta):
         """Copy the volume to the specified image."""
@@ -586,8 +669,7 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver):
         try:
             volume.provider_location = src_vref.provider_location
             info_path = self._local_path_volume_info(volume)
-            snap_info = {'volume_format': DISK_FORMAT_PLOOP,
+            snap_info = {'active': 'volume-%s' % volume.id}
-                         'active': 'volume-%s' % volume.id}
             self._write_info_file(info_path, snap_info)
             self._copy_volume_from_snapshot(temp_snapshot,
                                             volume,
@@ -599,10 +681,11 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver):
         return {'provider_location': src_vref.provider_location}
 
     @remotefs_drv.locked_volume_id_operation
-    def create_cloned_volume(self, vol, src_vref):
+    def create_cloned_volume(self, volume, src_vref):
         """Creates a clone of the specified volume."""
         volume_format = self.get_volume_format(src_vref)
         if volume_format == 'parallels':
-            self._create_cloned_volume(vol, src_vref)
+            self._create_cloned_volume(volume, src_vref)
         else:
-            super(VZStorageDriver, self)._create_cloned_volume(vol, src_vref)
+            super(VZStorageDriver, self)._create_cloned_volume(volume,
+                                                               src_vref)