From c7ed1cfc290a89a414b0109fafd3e4e39f30195b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcus=20Sch=C3=A4fer?= Date: Fri, 29 May 2020 17:41:15 +0200 Subject: [PATCH] Allow to access disk root after sync_data In preparation to allow a chroot operation into the loop mounted disk this commit refactors the process when filesystems gets umounted and also fixes the canonical order for calling the destructors. Related to Issue #1464 --- kiwi/boot/image/base.py | 28 +++++------------------ kiwi/boot/image/builtin_kiwi.py | 16 +++++++++---- kiwi/builder/disk.py | 23 +++++++++---------- kiwi/builder/install.py | 2 ++ kiwi/filesystem/base.py | 14 +++++++++++- kiwi/volume_manager/base.py | 21 +++++++++-------- kiwi/volume_manager/btrfs.py | 2 +- kiwi/volume_manager/lvm.py | 4 ++-- test/unit/boot/image/base_test.py | 13 ++--------- test/unit/boot/image/builtin_kiwi_test.py | 18 +++++++++++---- test/unit/builder/install_test.py | 2 ++ test/unit/filesystem/base_test.py | 5 +++- test/unit/volume_manager/base_test.py | 14 ++++-------- 13 files changed, 84 insertions(+), 78 deletions(-) diff --git a/kiwi/boot/image/base.py b/kiwi/boot/image/base.py index be25966c..1c9cd20a 100644 --- a/kiwi/boot/image/base.py +++ b/kiwi/boot/image/base.py @@ -56,8 +56,6 @@ class BootImageBase: self.initrd_filename = None self.boot_xml_state = None self.setup = None - self.temp_directories = [] - self.call_destructor = True self.signing_keys = signing_keys self.boot_root_directory = root_dir @@ -151,24 +149,11 @@ class BootImageBase: try: with open(filename, 'wb') as boot_image: pickle.dump(self, boot_image) - self.disable_cleanup() except Exception as e: raise KiwiBootImageDumpError( 'Failed to pickle dump boot image: %s' % format(e) ) - def disable_cleanup(self): - """ - Deactivate cleanup(deletion) of boot root directory - """ - self.call_destructor = False - - def enable_cleanup(self): - """ - Activate cleanup(deletion) of boot root directory - """ - self.call_destructor = True - def get_boot_names(self): """ Provides kernel and initrd names for the boot image @@ -383,6 +368,12 @@ class BootImageBase: boot_description return boot_description + def cleanup(self): + """ + Cleanup temporary boot image data if any + """ + pass + def _get_boot_image_output_file_format(self, kernel_version): """ The initrd output file format varies between @@ -451,10 +442,3 @@ class BootImageBase: return os.path.basename(initrd_file).replace( kernel_version, '{kernel_version}' ) - - def __del__(self): - if self.call_destructor: - log.info('Cleaning up %s instance', type(self).__name__) - for directory in self.temp_directories: - if directory and os.path.exists(directory): - Path.wipe(directory) diff --git a/kiwi/boot/image/builtin_kiwi.py b/kiwi/boot/image/builtin_kiwi.py index f9a0651f..8d6a3ec6 100644 --- a/kiwi/boot/image/builtin_kiwi.py +++ b/kiwi/boot/image/builtin_kiwi.py @@ -49,17 +49,18 @@ class BootImageKiwi(BootImageBase): root filesystem which is a separate image to create the initrd from """ + self.temp_directories = [] + + def prepare(self): + """ + Prepare new root system suitable to create a kiwi initrd from it + """ self.boot_root_directory = mkdtemp( prefix='kiwi_boot_root.', dir=self.target_dir ) self.temp_directories.append( self.boot_root_directory ) - - def prepare(self): - """ - Prepare new root system suitable to create a kiwi initrd from it - """ self.load_boot_xml_description() boot_image_name = self.boot_xml_state.xml_data.get_name() @@ -180,3 +181,8 @@ class BootImageKiwi(BootImageBase): ['--check=crc32', '--lzma2=dict=1MiB', '--threads=0'] ) self.initrd_filename = compress.compressed_filename + + def cleanup(self): + for directory in self.temp_directories: + if directory and os.path.exists(directory): + Path.wipe(directory) diff --git a/kiwi/builder/disk.py b/kiwi/builder/disk.py index d12cff7f..3353ee2d 100644 --- a/kiwi/builder/disk.py +++ b/kiwi/builder/disk.py @@ -234,13 +234,13 @@ class DiskBuilder: # create the disk log.info('Creating raw disk image %s', self.diskname) - loop_provider = LoopDevice( + self.loop_provider = LoopDevice( self.diskname, disksize_mbytes, self.blocksize ) - loop_provider.create() + self.loop_provider.create() self.disk = Disk( - self.firmware.get_partition_table_type(), loop_provider, + self.firmware.get_partition_table_type(), self.loop_provider, self.xml_state.get_disk_start_sector() ) @@ -249,7 +249,7 @@ class DiskBuilder: self.bootloader, self.xml_state, root_dir=self.root_dir, boot_dir=self.root_dir, custom_args={ 'targetbase': - loop_provider.get_device(), + self.loop_provider.get_device(), 'grub_directory_name': Defaults.get_grub_boot_directory_name(self.root_dir), 'boot_is_crypto': @@ -320,22 +320,22 @@ class DiskBuilder: 'image_type': self.xml_state.get_build_type_name() } - volume_manager = VolumeManager( + self.volume_manager = VolumeManager( self.volume_manager_name, device_map, self.root_dir + '/', self.volumes, volume_manager_custom_parameters ) - volume_manager.setup( + self.volume_manager.setup( self.volume_group_name ) - volume_manager.create_volumes( + self.volume_manager.create_volumes( self.requested_filesystem ) - volume_manager.mount_volumes() - self.system = volume_manager - device_map['root'] = volume_manager.get_device().get('root') - device_map['swap'] = volume_manager.get_device().get('swap') + self.volume_manager.mount_volumes() + self.system = self.volume_manager + device_map['root'] = self.volume_manager.get_device().get('root') + device_map['swap'] = self.volume_manager.get_device().get('swap') else: log.info( 'Creating root(%s) filesystem on %s', @@ -603,7 +603,6 @@ class DiskBuilder: try: with open(boot_image_dump_file, 'rb') as boot_image_dump: boot_image = pickle.load(boot_image_dump) - boot_image.enable_cleanup() Path.wipe(boot_image_dump_file) except Exception as e: raise KiwiInstallMediaError( diff --git a/kiwi/builder/install.py b/kiwi/builder/install.py index 90ae6890..311b6ab9 100644 --- a/kiwi/builder/install.py +++ b/kiwi/builder/install.py @@ -238,6 +238,7 @@ class InstallImageBuilder: custom_args=self.custom_iso_args ) iso_image.create_on_file(self.isoname) + self.boot_image_task.cleanup() def create_install_pxe_archive(self): """ @@ -345,6 +346,7 @@ class InstallImageBuilder: archive = ArchiveTar(self.pxetarball) archive.create(self.pxe_dir) + self.boot_image_task.cleanup() def _create_pxe_install_kernel_and_initrd(self): kernelname = 'pxeboot.{0}.kernel'.format(self.pxename) diff --git a/kiwi/filesystem/base.py b/kiwi/filesystem/base.py index 784b897d..91e6b21f 100644 --- a/kiwi/filesystem/base.py +++ b/kiwi/filesystem/base.py @@ -122,6 +122,19 @@ class FileSystemBase: """ raise NotImplementedError + def get_mountpoint(self): + """ + Provides mount point directory + + Effective use of the directory is guaranteed only after sync_data + + :return: directory path name + + :rtype: string + """ + if self.filesystem_mount: + return self.filesystem_mount.mountpoint + def sync_data(self, exclude=None): """ Copy root data tree into filesystem @@ -150,7 +163,6 @@ class FileSystemBase: options=['-a', '-H', '-X', '-A', '--one-file-system'], exclude=exclude ) - self.filesystem_mount.umount() def _apply_attributes(self): """ diff --git a/kiwi/volume_manager/base.py b/kiwi/volume_manager/base.py index 63566473..f4181ab8 100644 --- a/kiwi/volume_manager/base.py +++ b/kiwi/volume_manager/base.py @@ -312,6 +312,18 @@ class VolumeManagerBase(DeviceProvider): ) return mbsize + def get_mountpoint(self): + """ + Provides mount point directory + + Effective use of the directory is guaranteed only after sync_data + + :return: directory path name + + :rtype: string + """ + return self.mountpoint + def sync_data(self, exclude=None): """ Implements sync of root directory to mounted volumes @@ -327,7 +339,6 @@ class VolumeManagerBase(DeviceProvider): options=['-a', '-H', '-X', '-A', '--one-file-system'], exclude=exclude ) - self.umount_volumes() def set_property_readonly_root(self): """ @@ -343,11 +354,3 @@ class VolumeManagerBase(DeviceProvider): the mounts of all volumes """ self.mountpoint = mkdtemp(prefix='kiwi_volumes.') - - def __del__(self): - """ - Implements destructor to cleanup all volume subsystems - and mount processes. overwrite in specialized volume - management class - """ - pass diff --git a/kiwi/volume_manager/btrfs.py b/kiwi/volume_manager/btrfs.py index 56242870..c189a23a 100644 --- a/kiwi/volume_manager/btrfs.py +++ b/kiwi/volume_manager/btrfs.py @@ -92,7 +92,7 @@ class VolumeManagerBtrfs(VolumeManagerBase): filesystem = FileSystem( name='btrfs', device_provider=MappedDevice( - device=self.device, device_provider=self + device=self.device, device_provider=self.device_provider_root ), custom_args=self.custom_filesystem_args ) diff --git a/kiwi/volume_manager/lvm.py b/kiwi/volume_manager/lvm.py index 1133dba4..3b0af216 100644 --- a/kiwi/volume_manager/lvm.py +++ b/kiwi/volume_manager/lvm.py @@ -81,7 +81,7 @@ class VolumeManagerLVM(VolumeManagerBase): # the same key to put them on the same level volume_name = 'swap' device_map[volume_name] = MappedDevice( - device=volume_node, device_provider=self + device=volume_node, device_provider=self.device_provider_root ) return device_map @@ -296,7 +296,7 @@ class VolumeManagerLVM(VolumeManagerBase): filesystem = FileSystem( name=filesystem_name, device_provider=MappedDevice( - device=device_node, device_provider=self + device=device_node, device_provider=self.device_provider_root ), custom_args=self.custom_filesystem_args ) diff --git a/test/unit/boot/image/base_test.py b/test/unit/boot/image/base_test.py index 6f8be52e..abf00bbd 100644 --- a/test/unit/boot/image/base_test.py +++ b/test/unit/boot/image/base_test.py @@ -69,8 +69,7 @@ class TestBootImageBase: self.boot_image.dump('filename') @patch('pickle.dump') - @patch('kiwi.boot.image.base.BootImageBase.disable_cleanup') - def test_dump(self, mock_disable_cleanup, mock_dump): + def test_dump(self, mock_dump): with patch('builtins.open', create=True) as mock_open: mock_open.return_value = MagicMock(spec=io.IOBase) file_handle = mock_open.return_value.__enter__.return_value @@ -79,15 +78,6 @@ class TestBootImageBase: mock_dump.assert_called_once_with( self.boot_image, file_handle ) - mock_disable_cleanup.assert_called_once_with() - - def test_disable_cleanup(self): - self.boot_image.disable_cleanup() - assert self.boot_image.call_destructor is False - - def test_enable_cleanup(self): - self.boot_image.enable_cleanup() - assert self.boot_image.call_destructor is True @patch('os.listdir') def test_is_prepared(self, mock_listdir): @@ -198,3 +188,4 @@ class TestBootImageBase: self.boot_image.include_module('module') self.boot_image.omit_module('module') self.boot_image.write_system_config_file({'config_key': 'value'}) + self.boot_image.cleanup() diff --git a/test/unit/boot/image/builtin_kiwi_test.py b/test/unit/boot/image/builtin_kiwi_test.py index 05e2a25c..eee905ba 100644 --- a/test/unit/boot/image/builtin_kiwi_test.py +++ b/test/unit/boot/image/builtin_kiwi_test.py @@ -53,7 +53,9 @@ class TestBootImageKiwi: self.boot_image.include_file('/root/a') @patch('kiwi.defaults.Defaults.get_boot_image_description_path') - def test_prepare(self, mock_boot_path): + @patch('kiwi.boot.image.builtin_kiwi.mkdtemp') + def test_prepare(self, mock_mkdtemp, mock_boot_path): + mock_mkdtemp.return_value = 'boot-root-directory' mock_boot_path.return_value = '../data' self.boot_image.prepare() self.system_prepare.setup_repositories.assert_called_once_with( @@ -82,7 +84,11 @@ class TestBootImageKiwi: self.setup.call_image_script.assert_called_once_with() @patch('os.path.exists') - def test_prepare_no_boot_description_found(self, mock_os_path): + @patch('kiwi.boot.image.builtin_kiwi.mkdtemp') + def test_prepare_no_boot_description_found( + self, mock_mkdtemp, mock_os_path + ): + mock_mkdtemp.return_value = 'boot-root-directory' mock_os_path.return_value = False with raises(KiwiConfigFileNotFound): self.boot_image.prepare() @@ -102,6 +108,7 @@ class TestBootImageKiwi: mock_sync.return_value = data mock_mkdtemp.return_value = 'temp-boot-directory' mock_prepared.return_value = True + self.boot_image.boot_root_directory = 'boot-root-directory' mbrid = mock.Mock() mbrid.write = mock.Mock() cpio = mock.Mock() @@ -158,11 +165,12 @@ class TestBootImageKiwi: self.boot_image.target_dir + '/foo' ) - @patch('kiwi.boot.image.base.Path.wipe') + @patch('kiwi.boot.image.builtin_kiwi.Path.wipe') @patch('os.path.exists') - def test_destructor(self, mock_path, mock_wipe): + def test_cleanup(self, mock_path, mock_wipe): mock_path.return_value = True - self.boot_image.__del__() + self.boot_image.temp_directories.append('boot-root-directory') + self.boot_image.cleanup() mock_wipe.assert_called_once_with('boot-root-directory') def teardown(self): diff --git a/test/unit/builder/install_test.py b/test/unit/builder/install_test.py index b9834001..2e1d469f 100644 --- a/test/unit/builder/install_test.py +++ b/test/unit/builder/install_test.py @@ -178,6 +178,7 @@ class TestInstallImageBuilder: self.boot_image_task.create_initrd.assert_called_once_with( self.mbrid, 'initrd_kiwi_install', install_initrd=True ) + self.boot_image_task.cleanup.assert_called_once_with() self.kernel.copy_kernel.assert_called_once_with( 'temp_media_dir/boot/x86_64/loader', '/linux' ) @@ -354,6 +355,7 @@ class TestInstallImageBuilder: self.boot_image_task.create_initrd.assert_called_once_with( self.mbrid, 'initrd_kiwi_install', install_initrd=True ) + self.boot_image_task.cleanup.assert_called_once_with() assert mock_command.call_args_list[1] == call( [ 'mv', 'initrd', diff --git a/test/unit/filesystem/base_test.py b/test/unit/filesystem/base_test.py index a091f8e9..765c1de5 100644 --- a/test/unit/filesystem/base_test.py +++ b/test/unit/filesystem/base_test.py @@ -36,6 +36,9 @@ class TestFileSystemBase: with raises(NotImplementedError): self.fsbase.create_on_file('myimage') + def test_get_mountpoint(self): + assert self.fsbase.get_mountpoint() is None + @patch('kiwi.filesystem.base.MountManager') @patch('kiwi.filesystem.base.DataSync') @patch('kiwi.filesystem.base.Command.run') @@ -66,7 +69,7 @@ class TestFileSystemBase: ['chattr', '+C', 'tmpdir'] ) filesystem_mount.mount.assert_called_once_with([]) - filesystem_mount.umount.assert_called_once_with() + assert self.fsbase.get_mountpoint() == 'tmpdir' def test_destructor_valid_mountpoint(self): self.fsbase.filesystem_mount = mock.Mock() diff --git a/test/unit/volume_manager/base_test.py b/test/unit/volume_manager/base_test.py index 2b8df751..891303fe 100644 --- a/test/unit/volume_manager/base_test.py +++ b/test/unit/volume_manager/base_test.py @@ -206,13 +206,13 @@ class TestVolumeManagerBase: with raises(NotImplementedError): self.volume_manager.get_volumes() + def test_get_mountpoint(self): + assert self.volume_manager.get_mountpoint() is None + @patch('kiwi.volume_manager.base.DataSync') @patch('kiwi.volume_manager.base.MountManager.is_mounted') @patch('kiwi.volume_manager.base.VolumeManagerBase.mount_volumes') - @patch('kiwi.volume_manager.base.VolumeManagerBase.umount_volumes') - def test_sync_data( - self, mock_umount_volumes, mock_mount_volumes, mock_mounted, mock_sync - ): + def test_sync_data(self, mock_mount_volumes, mock_mounted, mock_sync): data_sync = Mock() mock_sync.return_value = data_sync mock_mounted.return_value = False @@ -226,7 +226,7 @@ class TestVolumeManagerBase: exclude=['exclude_me'], options=['-a', '-H', '-X', '-A', '--one-file-system'] ) - mock_umount_volumes.assert_called_once_with() + assert self.volume_manager.get_mountpoint() == 'mountpoint' @patch('kiwi.volume_manager.base.mkdtemp') def test_setup_mountpoint(self, mock_mkdtemp): @@ -250,7 +250,3 @@ class TestVolumeManagerBase: mock_command.assert_called_once_with( ['chattr', '+C', 'toplevel/etc'] ) - - def test_destructor(self): - # does nothing by default, just pass - self.volume_manager.__del__()