Merge pull request #2473 from OSInside/volume_manager_to_context_manager

Move VolumeManager to context manager

kiwi/builder/disk.py

@@ -374,8 +374,24 @@ def create_disk(self) -> Result:
                             device_map, luks_root
                         )
 
+                    # create system layout for root system
+                    device_map = self._create_system_instance(
+                        device_map, stack
+                    )
                     # build bootable disk
-                    self._process_build(device_map, disk)
+                    self._build_main_system(
+                        stack,
+                        device_map,
+                        disk,
+                        self.storage_map['system'],
+                        self.storage_map['system_boot'],
+                        self.storage_map['system_efi'],
+                        self.storage_map['system_spare'],
+                        self.storage_map['system_custom_parts'] or {},
+                        self.storage_map['luks_root'],
+                        self.storage_map['raid_root'],
+                        self.storage_map['integrity_root']
+                    )
 
         # store image bundle_format in result
         if self.bundle_format:
@@ -562,7 +578,9 @@ def _integrity_instance(self, device_map: Dict) -> IntegrityDevice:
             )
         )
 
-    def _create_system_instance(self, device_map: Dict) -> None:
+    def _create_system_instance(
+        self, device_map: Dict, stack: ExitStack
+    ) -> Dict:
         # create spare filesystem on spare partition if present
         self.storage_map[
             'system_spare'
@@ -586,16 +604,20 @@ def _create_system_instance(self, device_map: Dict) -> None:
                 self.volumes,
                 self.volume_manager_custom_parameters
             )
+            stack.push(volume_manager)
             device_map = self._map_root_volume_manager(
                 device_map, volume_manager
             )
         elif self.need_root_filesystem:
-            with FileSystem.new(
+            filesystem = FileSystem.new(
                 self.requested_filesystem, device_map['root'],
                 self.root_dir + '/',
                 self.filesystem_custom_parameters
-            ) as filesystem:
-                self._map_root_filesystem(device_map, filesystem)
+            )
+            stack.push(filesystem)
+            self._map_root_filesystem(device_map, filesystem)
+
+        return device_map
 
     def _map_raid(
         self, device_map: Dict, disk: Disk, raid_root: RaidDevice
@@ -709,59 +731,6 @@ def _map_root_filesystem(
             )
         self.storage_map['system'] = filesystem
 
-    def _process_build(self, device_map: Dict, disk: Disk) -> None:
-        # create system layout for root system
-        self._create_system_instance(device_map)
-
-        # representing the entire image system except for the boot/ area
-        # which could live on another part of the disk
-        system: Optional[Union[FileSystemBase, VolumeManagerBase]] = \
-            self.storage_map['system']
-
-        # an instance of a class with the sync_data capability
-        # representing the boot/ area of the disk if not part of
-        # system
-        system_boot: Optional[FileSystemBase] = \
-            self.storage_map['system_boot']
-
-        # an instance of a class with the sync_data capability
-        # representing the boot/efi area of the disk
-        system_efi: Optional[FileSystemBase] = \
-            self.storage_map['system_efi']
-
-        # an instance of a class with the sync_data capability
-        # representing the spare_part_mountpoint area of the disk
-        system_spare: Optional[FileSystemBase] = \
-            self.storage_map['system_spare']
-
-        # a dict of instances with the sync_data capability
-        # representing the custom partitions area of the disk
-        system_custom_parts: Dict[str, FileSystemBase] = \
-            self.storage_map['system_custom_parts'] or {}
-
-        luks_root = self.storage_map['luks_root']
-        raid_root = self.storage_map['raid_root']
-        integrity_root = self.storage_map['integrity_root']
-        try:
-            with ExitStack() as stack:
-                self._build_main_system(
-                    stack,
-                    device_map,
-                    disk,
-                    system,
-                    system_boot,
-                    system_efi,
-                    system_spare,
-                    system_custom_parts,
-                    luks_root,
-                    raid_root,
-                    integrity_root
-                )
-        finally:
-            if system:
-                if self.volume_manager_name:
-                    system.umount_volumes()
-
     def _build_main_system(
         self,
         stack: ExitStack,

kiwi/filesystem/base.py

@@ -60,9 +60,7 @@ def __init__(
         # as a file there
         self.filesystem_mount: Optional[MountManager] = None
 
-        # bind the block device providing class instance to this object.
-        # This is done to guarantee the correct destructor order when
-        # the device should be released. This is only required if the
+        # the underlaying device provider. This is only required if the
         # filesystem required a block device to become created
         self.device_provider = device_provider
 

kiwi/storage/disk.py

@@ -68,15 +68,13 @@ def __init__(
             of that extended partition
         """
         self.partition_mapper = RuntimeConfig().get_mapper_tool()
-        # bind the underlaying block device providing class instance
-        # to this object (e.g loop) if present. This is done to guarantee
-        # the correct destructor order when the device should be released.
+        #: the underlaying device provider
         self.storage_provider = storage_provider
 
-        # list of protected map ids. If used in a custom partitions
-        # setup this will lead to a raise conditition in order to
-        # avoid conflicts with the existing partition layout and its
-        # customizaton capabilities
+        #: list of protected map ids. If used in a custom partitions
+        #: setup this will lead to a raise conditition in order to
+        #: avoid conflicts with the existing partition layout and its
+        #: customizaton capabilities
         self.protected_map_ids = [
             'root',
             'readonly',

kiwi/storage/integrity_device.py

@@ -61,9 +61,7 @@ def __init__(
         self, storage_provider: DeviceProvider, integrity_algorithm: str,
         credentials: integrity_credentials_type = None
     ) -> None:
-        # bind the underlaying block device providing class instance
-        # to this object (e.g loop) if present. This is done to guarantee
-        # the correct destructor order when the device should be released.
+        #: the underlaying device provider
         self.storage_provider = storage_provider
 
         self.integrity_device: Optional[str] = None

kiwi/storage/luks_device.py

@@ -40,9 +40,7 @@ class LuksDevice(DeviceProvider):
     :param object storage_provider: Instance of class based on DeviceProvider
     """
     def __init__(self, storage_provider: DeviceProvider) -> None:
-        # bind the underlaying block device providing class instance
-        # to this object (e.g loop) if present. This is done to guarantee
-        # the correct destructor order when the device should be released.
+        #: the underlaying device provider
         self.storage_provider = storage_provider
 
         self.luks_device: Optional[str] = None

kiwi/storage/raid_device.py

@@ -38,9 +38,7 @@ class RaidDevice(DeviceProvider):
     :param object storage_provider: Instance of class based on DeviceProvider
     """
     def __init__(self, storage_provider: DeviceProvider):
-        # bind the underlaying block device providing class instance
-        # to this object (e.g loop) if present. This is done to guarantee
-        # the correct destructor order when the device should be released.
+        #: the underlaying device provider
         self.storage_provider = storage_provider
 
         self.raid_level_map = {

kiwi/system/prepare.py

@@ -105,10 +105,7 @@ def __init__(
         self.profiles = xml_state.profiles
         self.root_bind = root_bind
 
-        # A list of Uri references is stored inside of the System instance
-        # in order to delay the Uri destructors until the System instance
-        # dies. This is needed to keep bind mounted Uri locations alive
-        # for System operations
+        #: A list of Uri references
         self.uri_list: List[Uri] = []
 
     def setup_repositories(

kiwi/volume_manager/base.py

@@ -61,18 +61,15 @@ def __init__(
     ) -> None:
 
         self.temp_directories: List[Temporary] = []
-        # all volumes are combined into one mountpoint. This is
-        # needed at sync_data time. How to mount the volumes is
-        # special to the volume management class
-        #: root mountpoint for volumes
+        #: all volumes are combined into one mountpoint. This is
+        #: needed at sync_data time. How to mount the volumes is
+        #: special to the volume management class
         self.mountpoint: Optional[str] = None
 
         #: dictionary of mapped DeviceProviders
         self.device_map = device_map
 
-        # bind the device providing class instance to this object.
-        # This is done to guarantee the correct destructor order when
-        # the device should be released.
+        #: the underlaying device provider
         self.device_provider_root = device_map['root']
 
         #: An indicator for the mount of the filesystem and its volumes
@@ -98,10 +95,12 @@ def __init__(
                 'given root directory %s does not exist' % root_dir
             )
 
+        #: custom arguments passed to setup the volumes
         self.custom_args: Dict[str, Any] = {}
 
         #: custom filesystem creation and mount arguments, subset of the
-        #: custom_args information suitable to be passed to a FileSystem instance
+        #: custom_args information suitable to be passed to a
+        #: FileSystem instance
         self.custom_filesystem_args: Dict[str, Any] = {
             'create_options': [],
             'mount_options': []
@@ -121,6 +120,9 @@ def __init__(
 
         self.post_init(custom_args)
 
+    def __enter__(self):
+        return self
+
     def post_init(self, custom_args):
         """
         Post initialization method
@@ -164,7 +166,9 @@ def apply_attributes_on_volume(self, toplevel, volume):
                     ]
                 )
 
-    def get_fstab(self, persistency_type: str, filesystem_name: str) -> List[str]:
+    def get_fstab(
+        self, persistency_type: str, filesystem_name: str
+    ) -> List[str]:
         """
         Implements setup of the fstab entries. The method should
         return a list of fstab compatible entries
@@ -370,7 +374,9 @@ def get_root_volume_name(self) -> str:
         """
         return '/'
 
-    def sync_data(self, exclude: Optional[List[str]] = None) -> Optional[MountManager]:
+    def sync_data(
+        self, exclude: Optional[List[str]] = None
+    ) -> Optional[MountManager]:
         """
         Implements sync of root directory to mounted volumes
 
@@ -421,3 +427,6 @@ def setup_mountpoint(self):
         self.mountpoint_tempdir = Temporary(prefix='kiwi_volumes.').new_dir()
         self.mountpoint = self.mountpoint_tempdir.name
         self.temp_directories.append(self.mountpoint_tempdir)
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        pass

kiwi/volume_manager/btrfs.py

@@ -376,26 +376,16 @@ def mount_volumes(self):
 
         self.volumes_mounted_initially = True
 
-    def umount_volumes(self):
+    def umount_volumes(self) -> None:
         """
         Umount btrfs subvolumes
-
-        :return: True if all subvolumes are successfully unmounted
-
-        :rtype: bool
         """
-        all_volumes_umounted = True
         for volume_mount in reversed(self.subvol_mount_list):
             if volume_mount.is_mounted():
-                if not volume_mount.umount():
-                    all_volumes_umounted = False
-
-        if all_volumes_umounted:
-            if self.toplevel_mount.is_mounted():
-                if not self.toplevel_mount.umount():
-                    all_volumes_umounted = False
+                volume_mount.umount()
 
-        return all_volumes_umounted
+        if self.toplevel_mount.is_mounted():
+            self.toplevel_mount.umount()
 
     def get_mountpoint(self) -> str:
         """
@@ -576,8 +566,6 @@ def _create_snapshot_info(self, filename):
         with open(filename, 'w') as snapshot_info_file:
             snapshot_info_file.write(self._xml_pretty(snapshot))
 
-    def __del__(self):
+    def __exit__(self, exc_type, exc_value, traceback):
         if self.toplevel_mount:
-            log.info('Cleaning up %s instance', type(self).__name__)
-            if not self.umount_volumes():
-                log.warning('Subvolumes still busy')
+            self.umount_volumes()

kiwi/volume_manager/lvm.py

@@ -32,7 +32,8 @@
 from kiwi.path import Path
 
 from kiwi.exceptions import (
-    KiwiVolumeGroupConflict
+    KiwiVolumeGroupConflict,
+    KiwiCommandError
 )
 
 log = logging.getLogger('kiwi')
@@ -306,17 +307,10 @@ def mount_volumes(self):
     def umount_volumes(self):
         """
         Umount lvm volumes
-
-        :return: True if all subvolumes are successfully unmounted
-
-        :rtype: bool
         """
-        all_volumes_umounted = True
         for volume_mount in reversed(self.mount_list):
             if volume_mount.is_mounted():
-                if not volume_mount.umount():
-                    all_volumes_umounted = False
-        return all_volumes_umounted
+                volume_mount.umount()
 
     def _is_volume_enabled_for_fs_check(self, volume_name):
         for volume in self.volumes:
@@ -397,18 +391,18 @@ def _is_swap_volume(self, volume_name):
             if volume_name == volume.name and volume.label == 'SWAP':
                 return True
 
-    def __del__(self):
+    def __exit__(self, exc_type, exc_value, traceback):
         if self.volume_group:
-            log.info('Cleaning up %s instance', type(self).__name__)
-            if self.umount_volumes():
-                try:
-                    Command.run(
-                        ['vgchange'] + self.lvm_tool_options + [
-                            '-an', self.volume_group
-                        ]
-                    )
-                except Exception:
-                    log.warning(
-                        'volume group %s still busy', self.volume_group
+            try:
+                self.umount_volumes()
+                Command.run(
+                    ['vgchange'] + self.lvm_tool_options + [
+                        '-an', self.volume_group
+                    ]
+                )
+            except KiwiCommandError as issue:
+                log.error(
+                    'volume group {0} detach failed with {1}'.format(
+                        self.volume_group, issue
                     )
-                    return
+                )