Move VolumeManager to context manager

Change the VolumeManager Factory to be a context manager.
All code using VolumeManager was updated to the following
with statement:

    with VolumeManager(...).new as volume_manager:
        volume_manager.some_member()

This is related to Issue #2412

kiwi/builder/disk.py

@@ -562,7 +562,7 @@ 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) -> Dict:
         # create spare filesystem on spare partition if present
         self.storage_map[
             'system_spare'
@@ -580,15 +580,15 @@ def _create_system_instance(self, device_map: Dict) -> None:
             self._build_boot_filesystems(device_map)
 
         if self.volume_manager_name:
-            volume_manager = VolumeManager.new(
+            with VolumeManager.new(
                 self.volume_manager_name, device_map,
                 self.root_dir + '/',
                 self.volumes,
                 self.volume_manager_custom_parameters
-            )
-            device_map = self._map_root_volume_manager(
-                device_map, volume_manager
-            )
+            ) as volume_manager:
+                device_map = self._map_root_volume_manager(
+                    device_map, volume_manager
+                )
         elif self.need_root_filesystem:
             with FileSystem.new(
                 self.requested_filesystem, device_map['root'],
@@ -597,6 +597,8 @@ def _create_system_instance(self, device_map: Dict) -> None:
             ) as filesystem:
                 self._map_root_filesystem(device_map, filesystem)
 
+        return device_map
+
     def _map_raid(
         self, device_map: Dict, disk: Disk, raid_root: RaidDevice
     ) -> Dict:
@@ -711,7 +713,7 @@ def _map_root_filesystem(
 
     def _process_build(self, device_map: Dict, disk: Disk) -> None:
         # create system layout for root system
-        self._create_system_instance(device_map)
+        device_map = 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

kiwi/volume_manager/base.py

@@ -64,33 +64,33 @@ def __init__(
         # 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
+        # root mountpoint for volumes
         self.mountpoint: Optional[str] = None
 
-        #: dictionary of mapped DeviceProviders
+        # 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.
         self.device_provider_root = device_map['root']
 
-        #: An indicator for the mount of the filesystem and its volumes
-        #: when mounted for the first time
+        # An indicator for the mount of the filesystem and its volumes
+        # when mounted for the first time
         self.volumes_mounted_initially = False
 
-        #: root directory path name
+        # root directory path name
         self.root_dir = root_dir
-        #: list of volumes from :class:`XMLState::get_volumes()`
+        # list of volumes from :class:`XMLState::get_volumes()`
         self.volumes = volumes
-        #: volume group name
+        # volume group name
         self.volume_group = None
-        #: map volume name to device node
+        # map volume name to device node
         self.volume_map: Dict[str, str] = {}
-        #: list of volume MountManager's
+        # list of volume MountManager's
         self.mount_list: List[MountManager] = []
 
-        #: main storage device node name
+        # main storage device node name
         self.device = self.device_provider_root.get_device()
 
         if not os.path.exists(root_dir):
@@ -100,8 +100,9 @@ def __init__(
 
         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 filesystem creation and mount arguments, subset of the
+        # custom_args information suitable to be passed to a
+        # FileSystem instance
         self.custom_filesystem_args: Dict[str, Any] = {
             'create_options': [],
             'mount_options': []
@@ -121,6 +122,9 @@ def __init__(
 
         self.post_init(custom_args)
 
+    def __enter__(self):
+        return self
+
     def post_init(self, custom_args):
         """
         Post initialization method
@@ -164,7 +168,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 +376,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 +429,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

@@ -306,17 +306,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 +390,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 Exception as issue:
+                log.error(
+                    'volume group {0} detach failed with {1}'.format(
+                        self.volume_group, issue
                     )
-                    return
+                )

test/unit/builder/disk_test.py

@@ -468,7 +468,7 @@ def test_create_lvm_disk_standard_root_with_clone(
         volume_manager.get_fstab = Mock(
             return_value=['fstab_volume_entries']
         )
-        mock_volume_manager.return_value = volume_manager
+        mock_volume_manager.return_value.__enter__.return_value = volume_manager
         self.disk_builder.root_clone_count = 1
         self.disk_builder.boot_clone_count = 1
         self.disk_builder.volume_manager_name = 'lvm'
@@ -1371,7 +1371,7 @@ def test_create_disk_btrfs_managed_root(
         volume_manager.get_root_volume_name = Mock(
             return_value='@'
         )
-        mock_volume_manager.return_value = volume_manager
+        mock_volume_manager.return_value.__enter__.return_value = volume_manager
         filesystem = Mock()
         mock_fs.return_value.__enter__.return_value = filesystem
         self.disk_builder.volume_manager_name = 'btrfs'
@@ -1416,7 +1416,7 @@ def test_create_disk_volume_managed_root(
         volume_manager.get_fstab = Mock(
             return_value=['fstab_volume_entries']
         )
-        mock_volume_manager.return_value = volume_manager
+        mock_volume_manager.return_value.__enter__.return_value = volume_manager
         filesystem = Mock()
         mock_fs.return_value.__enter__.return_value = filesystem
         self.disk_builder.volume_manager_name = 'lvm'

test/unit/volume_manager/base_test.py

@@ -291,3 +291,10 @@ def test_apply_attributes_on_volume(self, mock_command):
         mock_command.assert_called_once_with(
             ['chattr', '+C', 'toplevel/etc']
         )
+
+    @patch('os.path.exists')
+    def test_context_manager_exit(self, mock_os_path_exists):
+        mock_os_path_exists.return_value = True
+        with VolumeManagerBase(self.device_map, 'root_dir', Mock()):
+            pass
+            # just pass

test/unit/volume_manager/btrfs_test.py

@@ -1,5 +1,4 @@
 import datetime
-import logging
 from unittest.mock import (
     patch, call, Mock, mock_open
 )
@@ -455,32 +454,6 @@ def test_umount_volumes(self):
         self.volume_manager.toplevel_mount.is_mounted.assert_called_once_with()
         self.volume_manager.toplevel_mount.umount.assert_called_once_with()
 
-    def test_umount_sub_volumes_busy(self):
-        self.volume_manager.toplevel_mount = Mock()
-        volume_mount = Mock()
-        volume_mount.is_mounted = Mock(
-            return_value=True
-        )
-        volume_mount.umount = Mock(
-            return_value=False
-        )
-        self.volume_manager.subvol_mount_list = [volume_mount]
-        assert self.volume_manager.umount_volumes() is False
-
-    def test_umount_toplevel_busy(self):
-        self.volume_manager.toplevel_mount = Mock()
-        volume_mount = Mock()
-        volume_mount.is_mounted = Mock(
-            return_value=True
-        )
-        self.volume_manager.toplevel_mount.is_mounted = Mock(
-            return_value=True
-        )
-        self.volume_manager.toplevel_mount.umount = Mock(
-            return_value=False
-        )
-        assert self.volume_manager.umount_volumes() is False
-
     @patch('kiwi.volume_manager.btrfs.SysConfig')
     @patch('kiwi.volume_manager.btrfs.DataSync')
     @patch('kiwi.volume_manager.btrfs.Command.run')
@@ -656,14 +629,14 @@ def test_set_property_readonly_root(self, mock_command):
         )
 
     @patch('kiwi.volume_manager.btrfs.VolumeManagerBtrfs.umount_volumes')
-    def test_destructor(self, mock_umount_volumes):
-        mock_umount_volumes.return_value = True
-        self.volume_manager.toplevel_mount = Mock()
-        with self._caplog.at_level(logging.INFO):
-            self.volume_manager.__del__()
-            mock_umount_volumes.assert_called_once_with()
-        mock_umount_volumes.reset_mock()
-        mock_umount_volumes.return_value = False
-        with self._caplog.at_level(logging.WARNING):
-            self.volume_manager.__del__()
-            mock_umount_volumes.assert_called_once_with()
+    @patch('os.path.exists')
+    def test_context_manager_exit(
+        self, mock_os_path_exists, mock_VolumeManagerBtrfs_umount_volumes
+    ):
+        mock_os_path_exists.return_value = True
+        with VolumeManagerBtrfs(
+            self.device_map, 'root_dir', self.volumes
+        ) as volume_manager:
+            volume_manager.toplevel_mount = Mock()
+
+        mock_VolumeManagerBtrfs_umount_volumes.assert_called_once_with()