feat(lvmsr): support vdi_revert on lvm

* Support `vdi_revert`
* Add journals for rollback in case of failure
* Rewrite lvm journal creation with security feature against writing too much inside allocated space
* Read more in lvm journal
* Support CBT on vdi revert

Signed-off-by: Antoine Bartuccio <antoine.bartuccio@vates.tech>

Author: klmp200

drivers/LVMSR.py

@@ -34,7 +34,7 @@
                 "VDI_GENERATE_CONFIG", "VDI_SNAPSHOT", "VDI_CLONE",
                 "VDI_RESIZE", "ATOMIC_PAUSE", "VDI_RESET_ON_BOOT/2",
                 "VDI_UPDATE", "VDI_MIRROR", "VDI_CONFIG_CBT", "VDI_ACTIVATE",
-                "VDI_DEACTIVATE"]
+                "VDI_DEACTIVATE", "VDI_REVERT"]
 
 CONFIGURATION = [['SCSIid', 'The scsi_id of the destination LUN'],
                 ['allocation', 'Valid values are thick or thin(optional,\

drivers/LVMoFCoESR.py

@@ -41,7 +41,7 @@
                 "VDI_CREATE", "VDI_DELETE", "VDI_ATTACH", "VDI_DETACH",
                 "VDI_GENERATE_CONFIG", "VDI_SNAPSHOT", "VDI_CLONE", "VDI_MIRROR",
                 "VDI_RESIZE", "ATOMIC_PAUSE", "VDI_RESET_ON_BOOT/2",
-                "VDI_UPDATE", "VDI_CONFIG_CBT", "VDI_ACTIVATE", "VDI_DEACTIVATE"]
+                "VDI_UPDATE", "VDI_CONFIG_CBT", "VDI_ACTIVATE", "VDI_DEACTIVATE", "VDI_REVERT"]
 
 CONFIGURATION = [['SCSIid', 'The scsi_id of the destination LUN'], \
                   ['allocation', 'Valid values are thick or thin (optional, defaults to thick)']]

drivers/LVMoHBASR.py

@@ -46,7 +46,7 @@
                 "VDI_GENERATE_CONFIG", "VDI_CLONE", "VDI_SNAPSHOT",
                 "VDI_RESIZE", "ATOMIC_PAUSE", "VDI_RESET_ON_BOOT/2",
                 "VDI_UPDATE", "VDI_MIRROR", "VDI_CONFIG_CBT",
-                "VDI_ACTIVATE", "VDI_DEACTIVATE"]
+                "VDI_ACTIVATE", "VDI_DEACTIVATE", "VDI_REVERT"]
 
 CONFIGURATION = [['SCSIid', 'The scsi_id of the destination LUN'], \
                   ['target', 'IP address or hostname of the iSCSI target'], \

drivers/LVMoISCSISR.py

@@ -19,7 +19,7 @@
 
 import util
 import xs_errors
-from srmetadata import open_file, file_read_wrapper, file_write_wrapper
+from srmetadata import open_file, file_read_wrapper, file_write_wrapper, align_data_to_file
 
 LVM_MAX_NAME_LEN = 127
 
@@ -34,6 +34,16 @@ class Journaler:
     SEPARATOR = "_"
     JRN_CLONE = "clone"
     JRN_LEAF = "leaf"
+    JRN_REVERT = "revert"
+
+    @classmethod
+    def has_additional_data(cls, type: str) -> bool:
+        """Return True if journal type contains additional data inside of it"""
+        return type in [
+            cls.JRN_CLONE,
+            cls.JRN_LEAF,
+            cls.JRN_REVERT,
+        ]
 
     def __init__(self, lvmCache):
         self.vgName = lvmCache.vgName
@@ -42,54 +52,63 @@ def __init__(self, lvmCache):
     def create(self, type, id, val):
         """Create an entry of type "type" for "id" with the value "val".
         Error if such an entry already exists."""
-        valExisting = self.get(type, id)
-        writeData = False
-        if valExisting or util.fistpoint.is_active("LVM_journaler_exists"):
-            raise xs_errors.XenError('LVMCreate', opterr="Journal already exists for '%s:%s': %s" % (type, id, valExisting))
-        lvName = self._getNameLV(type, id, val)
+        to_write = None
+        journal_exists = self.get(type, id)
+        if journal_exists or util.fistpoint.is_active("LVM_journaler_exists"):
+            raise xs_errors.XenError('LVMCreate', opterr=f"Journal already exists for '{type}:{id}': {journal_exists}")
+        lv_name = self._getNameLV(type, id, val)
+
+        mapper_device = self._getLVMapperName(lv_name)
+        if len(mapper_device) > LVM_MAX_NAME_LEN:
+            lv_name = self._getNameLV(type, id)
+            mapper_device = self._getLVMapperName(lv_name)
+            assert len(mapper_device) <= LVM_MAX_NAME_LEN
 
-        mapperDevice = self._getLVMapperName(lvName)
-        if len(mapperDevice) > LVM_MAX_NAME_LEN:
-            lvName = self._getNameLV(type, id)
-            writeData = True
-            mapperDevice = self._getLVMapperName(lvName)
-            assert len(mapperDevice) <= LVM_MAX_NAME_LEN
+            try:
+                to_write = ("%d %s" % (len(val), val)).encode()
+            except UnicodeEncodeError as e:
+                util.logException("journaler.create")
+                raise xs_errors.XenError("LVMWrite", opterr=f"Failed to encode data for journal {lv_name}") from e
+            used_size = len(align_data_to_file(to_write))
+            if used_size > self.LV_SIZE:
+                raise xs_errors.XenError("LVMWrite", opterr=f"Size of encoded journal {used_size} exceeds limit of {self.LV_SIZE}")
+
+
+        self.lvmCache.create(lv_name, self.LV_SIZE, self.LV_TAG)
 
-        self.lvmCache.create(lvName, self.LV_SIZE, self.LV_TAG)
+        if not to_write:
+            return
 
-        if writeData:
-            fullPath = self.lvmCache._getPath(lvName)
-            journal_file = open_file(fullPath, True)
+        full_path = self.lvmCache._getPath(lv_name)
+        journal_file = open_file(full_path, True)
+        try:
+            raised_exception = None
             try:
-                e = None
+                file_write_wrapper(journal_file, 0, to_write)
+                if util.fistpoint.is_active("LVM_journaler_writefail"):
+                    raise ValueError("LVM_journaler_writefail FistPoint active")
+            except Exception as e:
+                raised_exception = e
+                raise
+            finally:
                 try:
-                    data = ("%d %s" % (len(val), val)).encode()
-                    file_write_wrapper(journal_file, 0, data)
-                    if util.fistpoint.is_active("LVM_journaler_writefail"):
-                        raise ValueError("LVM_journaler_writefail FistPoint active")
+                    journal_file.close()
+                    self.lvmCache.deactivateNoRefcount(lv_name)
                 except Exception as e:
-                    raise
-                finally:
-                    try:
-                        journal_file.close()
-                        self.lvmCache.deactivateNoRefcount(lvName)
-                    except Exception as e2:
-                        msg = 'failed to close/deactivate %s: %s' \
-                                % (lvName, e2)
-                        if not e:
-                            util.SMlog(msg)
-                            raise e2
-                        else:
-                            util.SMlog('WARNING: %s (error ignored)' % msg)
-
-            except:
-                util.logException("journaler.create")
-                try:
-                    self.lvmCache.remove(lvName)
-                except Exception as e:
-                    util.SMlog('WARNING: failed to clean up failed journal ' \
-                            ' creation: %s (error ignored)' % e)
-                raise xs_errors.XenError('LVMWrite', opterr="Failed to write to journal %s" % lvName)
+                    msg = f"failed to close/deactivate {lv_name}: {e}"
+                    if not raised_exception:
+                        util.SMlog(msg)
+                        raise e
+                    else:
+                        util.SMlog(f"WARNING: {msg} (error ignored)")
+
+        except:
+            util.logException("journaler.create")
+            try:
+                self.lvmCache.remove(lv_name)
+            except Exception as e:
+                util.SMlog(f"WARNING: failed to clean up failed journal creation: {e} (error ignored)")
+            raise xs_errors.XenError('LVMWrite', opterr=f"Failed to write to journal {lv_name}")
 
     def remove(self, type, id):
         """Remove the entry of type "type" for "id". Error if the entry doesn't
@@ -139,26 +158,23 @@ def _getAllEntries(self, readFile=True):
             if len(parts) != 3 or util.fistpoint.is_active("LVM_journaler_badname"):
                 raise xs_errors.XenError('LVMNoVolume', opterr="Bad LV name: %s" % lvName)
             type, id, val = parts
-            if readFile:
-                # For clone and leaf journals, additional
-                # data is written inside file
-                # TODO: Remove dependency on journal type
-                if type == self.JRN_CLONE or type == self.JRN_LEAF:
-                    fullPath = self.lvmCache._getPath(lvName)
-                    self.lvmCache.activateNoRefcount(lvName, False)
-                    journal_file = open_file(fullPath)
+            # TODO: Remove dependency on journal type
+            if readFile and self.has_additional_data(type):
+                fullPath = self.lvmCache._getPath(lvName)
+                self.lvmCache.activateNoRefcount(lvName, False)
+                journal_file = open_file(fullPath)
+                try:
                     try:
-                        try:
-                            data = file_read_wrapper(journal_file, 0)
-                            length, val = data.decode().split(" ", 1)
-                            val = val[:int(length)]
-                            if util.fistpoint.is_active("LVM_journaler_readfail"):
-                                raise ValueError("LVM_journaler_readfail FistPoint active")
-                        except:
-                            raise xs_errors.XenError('LVMRead', opterr="Failed to read from journal %s" % lvName)
-                    finally:
-                        journal_file.close()
-                        self.lvmCache.deactivateNoRefcount(lvName)
+                        data = file_read_wrapper(journal_file, 0, -1)
+                        length, val = data.decode().split(" ", 1)
+                        val = val[:int(length)]
+                        if util.fistpoint.is_active("LVM_journaler_readfail"):
+                            raise ValueError("LVM_journaler_readfail FistPoint active")
+                    except:
+                        raise xs_errors.XenError('LVMRead', opterr="Failed to read from journal %s" % lvName)
+                finally:
+                    journal_file.close()
+                    self.lvmCache.deactivateNoRefcount(lvName)
             if not entries.get(type):
                 entries[type] = dict()
             entries[type][id] = val

drivers/journaler.py

@@ -102,21 +102,25 @@ def open_file(path, write=False):
                 (path, e.errno))
     return file_p
 
+def align_data_to_file(data: bytes) -> bytes:
+    """Add space padding to data to ensure that a complete blocks can be written."""
+    blocksize = METADATA_BLK_SIZE
+    length = len(data)
+    newlength = length
+    if length % blocksize:
+        newlength = length + (blocksize - length % blocksize)
+    return data + b' ' * (newlength - length)
 
 def file_write_wrapper(fd, offset, data):
     """
     Writes data to a file at a given offset. Padding (consisting of spaces)
     may be written out after the given data to ensure that complete blocks are
     written.
     """
+    blocksize = METADATA_BLK_SIZE
     try:
-        blocksize = METADATA_BLK_SIZE
-        length = len(data)
-        newlength = length
-        if length % blocksize:
-            newlength = length + (blocksize - length % blocksize)
+        to_write = align_data_to_file(data)
         fd.seek(offset, SEEK_SET)
-        to_write = data + b' ' * (newlength - length)
         return fd.write(to_write)
     except OSError as e:
         raise OSError(
@@ -127,10 +131,16 @@ def file_write_wrapper(fd, offset, data):
 def file_read_wrapper(fd, offset, bytesToRead=METADATA_BLK_SIZE):
     """
     Reads data from a file at a given offset. If not specified, the amount of
-    data to read defaults to one block.
+    data to read defaults to one block. Using -1 reads the whole file.
     """
     try:
         fd.seek(offset, SEEK_SET)
+        if bytesToRead == -1:
+            # The most reliable solution I could find to read the whole file
+            # is to read it to the end to find the size
+            # This isn't super efficient but in most case where this is used, the file
+            # is quite short and that shouldn't be an issue
+            bytesToRead = len(fd.peek())
         return fd.read(bytesToRead)
     except OSError as e:
         raise OSError(

drivers/srmetadata.py

@@ -1549,6 +1549,9 @@ def list_find(f, seq):
                         "FileSR_revert_create_insert",
                         "FileSR_revert_create_src",
                         "FileSR_revert_create_dest",
+                        "LVM_revert_create_insert",
+                        "LVM_revert_create_src",
+                        "LVM_revert_create_dest",
                         "LVM_journaler_exists",
                         "LVM_journaler_none",
                         "LVM_journaler_badname",

drivers/util.py

@@ -1,3 +1,4 @@
+import XenAPI
 from sm_typing import override
 
 import copy
@@ -698,6 +699,10 @@ def test_update_slaves_on_cbt_disable(self, mock_xenapi, mock_lock):
         Ensure we tell the supporter host when we disable CBT for one of its VMs
         """
         # Arrange
+        def xenapi_failure(_):
+            raise XenAPI.Failure("No uuid")
+
+        mock_xenapi.xapi_local.return_value.xenapi.VDI.get_by_uuid.side_effect = xenapi_failure
         xapi_session = mock_xenapi.xapi_local.return_value
 
         vdi_uuid = str(uuid.uuid4)