diff --git a/kiwi/builder/disk.py b/kiwi/builder/disk.py index 1164cc2a419..ee3b350936a 100644 --- a/kiwi/builder/disk.py +++ b/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 diff --git a/kiwi/volume_manager/base.py b/kiwi/volume_manager/base.py index 94fa038f001..5a8ef96f1ee 100644 --- a/kiwi/volume_manager/base.py +++ b/kiwi/volume_manager/base.py @@ -64,10 +64,10 @@ 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. @@ -75,22 +75,22 @@ def __init__( # 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 diff --git a/kiwi/volume_manager/btrfs.py b/kiwi/volume_manager/btrfs.py index c1342d6feb9..54932179b80 100644 --- a/kiwi/volume_manager/btrfs.py +++ b/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() diff --git a/kiwi/volume_manager/lvm.py b/kiwi/volume_manager/lvm.py index aa97b6a0b55..922df563cfd 100644 --- a/kiwi/volume_manager/lvm.py +++ b/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 + ) diff --git a/test/unit/builder/disk_test.py b/test/unit/builder/disk_test.py index 773d0e6ee42..779f630503c 100644 --- a/test/unit/builder/disk_test.py +++ b/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' diff --git a/test/unit/volume_manager/base_test.py b/test/unit/volume_manager/base_test.py index 8eb82c79fd7..d91a7d57caa 100644 --- a/test/unit/volume_manager/base_test.py +++ b/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 diff --git a/test/unit/volume_manager/btrfs_test.py b/test/unit/volume_manager/btrfs_test.py index 699a770905a..5ec0fc3c155 100644 --- a/test/unit/volume_manager/btrfs_test.py +++ b/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() diff --git a/test/unit/volume_manager/lvm_test.py b/test/unit/volume_manager/lvm_test.py index b58e255f447..4bebbd1bb5b 100644 --- a/test/unit/volume_manager/lvm_test.py +++ b/test/unit/volume_manager/lvm_test.py @@ -313,7 +313,7 @@ def test_umount_volumes(self): volume_mount = Mock() volume_mount.mountpoint = 'volume_mount_point' self.volume_manager.mount_list = [volume_mount] - assert self.volume_manager.umount_volumes() is True + self.volume_manager.umount_volumes() volume_mount.umount.assert_called_once_with() def test_get_volumes(self): @@ -357,37 +357,23 @@ def test_get_fstab(self): ] @patch('kiwi.volume_manager.lvm.Command.run') - def test_destructor_busy_volumes(self, mock_command): - self.volume_manager.mountpoint = 'tmpdir' - self.volume_manager.volume_group = 'volume_group' - volume_mount = Mock() - volume_mount.is_mounted.return_value = True - volume_mount.umount.return_value = False - volume_mount.mountpoint = 'volume_mount_point' - volume_mount.device = '/dev/volume_group/LVRoot' - self.volume_manager.mount_list = [volume_mount] - - self.volume_manager.__del__() - - volume_mount.umount.assert_called_once_with() - self.volume_manager.volume_group = None - @patch('kiwi.volume_manager.lvm.VolumeManagerLVM.umount_volumes') - @patch('kiwi.volume_manager.lvm.Command.run') - def test_destructor( - self, mock_command, mock_umount_volumes + @patch('os.path.exists') + def test_context_manager_exit( + self, mock_os_path_exists, mock_VolumeManagerLVM_umount_volumes, + mock_Command_run ): - mock_umount_volumes.return_value = True - mock_command.side_effect = Exception - self.volume_manager.mountpoint = 'tmpdir' - self.volume_manager.volume_group = 'volume_group' + mock_os_path_exists.return_value = True + mock_Command_run.side_effect = Exception + with self._caplog.at_level(logging.ERROR): + with VolumeManagerLVM( + self.device_map, 'root_dir', self.volumes + ) as volume_manager: + volume_manager.volume_group = 'volume_group' - with self._caplog.at_level(logging.WARNING): - self.volume_manager.__del__() - mock_umount_volumes.assert_called_once_with() - mock_command.assert_called_once_with( - ['vgchange'] + self.volume_manager.lvm_tool_options + [ - '-an', 'volume_group' - ] - ) - self.volume_manager.volume_group = None + mock_VolumeManagerLVM_umount_volumes.assert_called_once_with() + mock_Command_run.assert_called_once_with( + ['vgchange'] + self.volume_manager.lvm_tool_options + [ + '-an', 'volume_group' + ] + )