fix(LVMSR): vdi_revert fix coalesce issue, leaking vdi and remove tmp vdi

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

Author: klmp200

drivers/LVMSR.py

@@ -96,21 +96,18 @@ def __init__(
         uuid: str,
         lvname: str,
         backup_lvname: str,
-        tmp_lvname: str,
         is_cbt_enabled: bool,
     ):
         self.uuid = uuid
         self.lvname = lvname
         self.backup_lvname = backup_lvname
-        self.tmp_lvname = tmp_lvname
         self.is_cbt_enabled = is_cbt_enabled
 
     def to_dict(self) -> Dict[str, Union[str, bool]]:
         return {
             "uuid": self.uuid,
             "lvname": self.lvname,
             "backup_lvname": self.backup_lvname,
-            "tmp_lvname": self.tmp_lvname,
             "is_cbt_enabled": self.is_cbt_enabled,
         }
 
@@ -120,7 +117,6 @@ def from_dict(cls, data: Dict[str, Union[str, bool]]) -> "RevertLogDestinationVD
             uuid=str(data["uuid"]),
             lvname=str(data["lvname"]),
             backup_lvname=str(data["backup_lvname"]),
-            tmp_lvname=str(data["tmp_lvname"]),
             is_cbt_enabled=bool(data["is_cbt_enabled"]),
         )
 
@@ -334,14 +330,13 @@ def load(self, sr_uuid) -> None:
         if lvutil._checkVG(self.vgname):
             # Disable SR on slaves when mounting VDIs if
             # a critical journal is pending
-            if (
-                not self.isMaster
-                and self.cmd not in ["vdi_detach", "vdi_activate", "vdi_deactivate", "nop"]
-                and self._has_critical_journals()
-            ):
-                raise xs_errors.XenError(
-                    "SRUnavailable", opterr="Critical journals are pending"
-                )
+            # and force journal undo if on master
+            if self._has_critical_journals():
+                if not self.isMaster:
+                    raise xs_errors.XenError(
+                        "SRUnavailable", opterr="Critical journals are pending. A scan is required."
+                    )
+                self._undoAllJournals()
 
             if self.isMaster and not self.cmd in [
                 "vdi_attach",
@@ -350,7 +345,6 @@ def load(self, sr_uuid) -> None:
                 "vdi_deactivate",
             ]:
                 self._undoAllJournals()
-
             if not self.cmd in ["sr_attach", "sr_probe"]:
                 self._checkMetadataVolume()
 
@@ -1334,10 +1328,6 @@ def _undo_revert_journals(self):
             self.journaler.remove(RevertLogEntry.JRN_KEY, journal_id)
 
     def _rollback_revert_vdi(self, entry: RevertLogEntry):
-        util.SMlog(f"Removing temporary file {entry.dest.tmp_lvname}")
-        if self.lvmCache.checkLV(entry.dest.tmp_lvname):
-            self.lvmCache.remove(entry.dest.tmp_lvname)
-
         util.SMlog(
             f"Reverting vdi {entry.dest.uuid} from backup {entry.dest.backup_lvname}"
         )
@@ -1362,6 +1352,7 @@ def _rollback_revert_vdi(self, entry: RevertLogEntry):
             src.cowutil.setHidden(str(src.path), False)
             src.update_from_disk()
             src._db_update()
+        src.disable_leaf_on_secondary(src.uuid)
 
         # CBT cannot be added by accident on src, we can skip checking if it exists
         util.SMlog(f"CBT rollback: restoring cbtlog for src {entry.src.uuid}")
@@ -1375,6 +1366,17 @@ def _rollback_revert_vdi(self, entry: RevertLogEntry):
         if self.lvmCache.checkLV(entry.dest.lvname):
             self.lvmCache.remove(entry.dest.lvname)
 
+        # Ensure that inserted node is hidden so that it doesn't leak
+        if self.lvmCache.checkLV(entry.inserted.lvname):
+            inserted: "LVMVDI" = self.vdi(entry.inserted.uuid) # type: ignore[assignment]
+            with inserted.activated(False):
+                inserted.cowutil.setHidden(str(inserted.path), True)
+                inserted.update_from_disk()
+                inserted_ref = inserted._db_update_or_introduce()
+
+            inserted.session.xenapi.VDI.set_managed(inserted_ref, False)
+            inserted.disable_leaf_on_secondary(inserted.uuid)
+
         # Once we move the backup to it's old name, the journal can't be run again
         # If something fails from now, we might lose some info, most of them
         # will be back from a simple scan
@@ -1979,7 +1981,8 @@ def _revert(
 
         self.sr._ensureSpaceAvailable(size_req)
 
-        dest_tmp_uuid = util.gen_uuid() # Will be replaced by dest.uuid at the end
+        # We create a valid backup VDI so that it properly protect
+        # it's parent chain in case of a coalesce
         dest_backup_name = LV_PREFIX[self.vdi_type] + util.gen_uuid()
 
         # Create journal
@@ -1989,7 +1992,6 @@ def _revert(
                 dest.uuid,
                 dest.lvname,
                 dest_backup_name,
-                LV_PREFIX[self.vdi_type] + dest_tmp_uuid,
                 bool(dest_cbtlog),
             ),
             src=RevertLogSourceVDI(
@@ -2017,6 +2019,8 @@ def _revert(
         self.sr.lvActivator.deactivate(self.uuid, False)
         self.sr.lvmCache.rename(self.lvname, inserted_lvname)
 
+        util.fistpoint.activate("LVM_revert_create_insert", self.sr.uuid)
+
         inserted = LVMVDI(self.sr, inserted_uuid)
         inserted.label = "base copy"
         inserted.read_only = False
@@ -2030,41 +2034,40 @@ def _revert(
         ## Protect the new parent from being coalesced by the GC
         inserted.sr.lvActivator.activate(inserted.uuid, inserted.lvname, False)
         inserted.cowutil.setHidden(inserted.path, False)
+        inserted_ref = inserted._db_introduce()
+
+        util.fistpoint.activate("LVM_revert_create_src", self.sr.uuid)
+
+        inserted.disable_leaf_on_secondary(inserted.uuid, True)
 
         # Recreate src
         ## Since src is a snapshot, we use the minimal size on it
         _ = inserted._createSnap(
             self.uuid, self.vdi_type, inserted_size, False, False
         )
 
-        inserted._db_introduce()
-
         # Update src
         self.sr.lvActivator.activate(self.uuid, self.lvname, False)
         self.update_from_disk()
         self._db_update()
 
         # Create snapshot
-        dest_tmp = inserted._createSnap(
-            dest_tmp_uuid, inserted.vdi_type, destination_size, False, False
+        _ = inserted._createSnap(
+            dest.uuid, inserted.vdi_type, destination_size, False, False
         )
-        ## Force parent (it can be simplified by vhd tools)
-        self.cowutil.setParent(dest_tmp.path, inserted.path, False)
 
-        inserted.read_only = True
-        inserted._db_update()
+        util.fistpoint.activate("LVM_revert_create_dest", self.sr.uuid)
+
+        dest.update_from_disk()
+        dest._db_update()
 
         ## The new parent can now be set hidden and don't need to be protected by the GC
+        inserted.update_from_disk()
+        inserted.read_only = True
+        inserted._db_update()
         inserted.cowutil.setHidden(inserted.path, True)
         inserted.sr.lvmCache.setReadonly(inserted.lvname, True)
-
-        # Apply snapshot
-        dest.sr.lvActivator.deactivate(dest_tmp.uuid, False)
-        dest.sr.lvmCache.rename(dest_tmp.lvname, dest.lvname)
-        dest.sr.lvActivator.activate(dest.uuid, dest.lvname, False)
-
-        dest.update_from_disk()
-        dest._db_update()
+        self.session.xenapi.VDI.set_managed(inserted_ref, False)
 
         if src_cbtlog:
             self._revert_cbt(dest)

drivers/util.py

@@ -1475,6 +1475,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",