From 12d69183887a068510eb25ec148599fc9d427e43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcus=20Sch=C3=A4fer?= Date: Wed, 12 Jul 2023 17:58:41 +0200 Subject: [PATCH] Evaluate the @root volume name also for btrfs In a volume setup the special volume declaration was only evaluated for the LVM volume manager. In case of btrfs a hardcoded root volume name '@' was used. This commit allows to specify a custom name for the root volume for btrfs as well and also allows to specify that there should be no such root volume. Example: Name the root volume '@'. If not specified this stays as the default to stay compatible Indicate no root volume is wanted. All subvolumes resides below root (/) Name the root volume 'foo' This is related to Issue #2316 and a first patch to address the requested changes --- kiwi/volume_manager/btrfs.py | 100 ++++++++++++++++++------- kiwi/xml_state.py | 5 +- test/unit/volume_manager/btrfs_test.py | 62 +++++++++++++-- 3 files changed, 133 insertions(+), 34 deletions(-) diff --git a/kiwi/volume_manager/btrfs.py b/kiwi/volume_manager/btrfs.py index 34c63a30172..abbfa214f32 100644 --- a/kiwi/volume_manager/btrfs.py +++ b/kiwi/volume_manager/btrfs.py @@ -74,6 +74,18 @@ def post_init(self, custom_args): if 'quota_groups' not in self.custom_args: self.custom_args['quota_groups'] = False + self.root_volume_name = '@' + canonical_volume_list = self.get_canonical_volume_list() + for volume in canonical_volume_list.volumes: + if volume.is_root_volume and volume.name: + self.root_volume_name = volume.name + + if self.custom_args['root_is_snapshot'] and \ + self.root_volume_name == '/': + log.warning('root_is_snapshot requires a toplevel sub-volume') + log.warning('root_is_snapshot has been disabled') + self.custom_args['root_is_snapshot'] = False + self.subvol_mount_list = [] self.toplevel_mount = None self.toplevel_volume = None @@ -82,9 +94,9 @@ def setup(self, name=None): """ Setup btrfs volume management - In case of btrfs a toplevel(@) subvolume is created and marked + In case of btrfs an optional toplevel subvolume is created and marked as default volume. If snapshots are activated via the custom_args - the setup method also created the @/.snapshots/1/snapshot + the setup method also creates the .snapshots/1/snapshot subvolumes. There is no concept of a volume manager name, thus the name argument is not used for btrfs @@ -112,31 +124,37 @@ def setup(self, name=None): Command.run( ['btrfs', 'quota', 'enable', self.mountpoint] ) - root_volume = self.mountpoint + '/@' - Command.run( - ['btrfs', 'subvolume', 'create', root_volume] - ) + if self.root_volume_name != '/': + root_volume = self.mountpoint + f'/{self.root_volume_name}' + Command.run( + ['btrfs', 'subvolume', 'create', root_volume] + ) if self.custom_args['root_is_snapshot']: - snapshot_volume = self.mountpoint + '/@/.snapshots' + snapshot_volume = self.mountpoint + \ + f'/{self.root_volume_name}/.snapshots' Command.run( ['btrfs', 'subvolume', 'create', snapshot_volume] ) os.chmod(snapshot_volume, 0o700) Path.create(snapshot_volume + '/1') - snapshot = self.mountpoint + '/@/.snapshots/1/snapshot' + snapshot = self.mountpoint + \ + f'/{self.root_volume_name}/.snapshots/1/snapshot' Command.run( ['btrfs', 'subvolume', 'create', snapshot] ) - self._set_default_volume('@/.snapshots/1/snapshot') - snapshot = self.mountpoint + '/@/.snapshots/1/snapshot' - # Mount /@/.snapshots as /.snapshots inside the root + self._set_default_volume( + f'{self.root_volume_name}/.snapshots/1/snapshot' + ) + snapshot = self.mountpoint + \ + f'/{self.root_volume_name}/.snapshots/1/snapshot' + # Mount /{some-name}/.snapshots as /.snapshots inside the root snapshots_mount = MountManager( device=self.device, mountpoint=snapshot + '/.snapshots' ) self.subvol_mount_list.append(snapshots_mount) - else: - self._set_default_volume('@') + elif self.root_volume_name != '/': + self._set_default_volume(self.root_volume_name) def create_volumes(self, filesystem_name): """ @@ -163,12 +181,12 @@ def create_volumes(self, filesystem_name): for volume in canonical_volume_list.volumes: if volume.is_root_volume: - # the btrfs root volume named '@' has been created as + # the btrfs root volume has been created as # part of the setup procedure pass else: log.info('--> sub volume %s', volume.realpath) - toplevel = self.mountpoint + '/@/' + toplevel = self.mountpoint + f'/{self.root_volume_name}/' volume_parent_path = os.path.normpath( toplevel + os.path.dirname(volume.realpath) ) @@ -183,15 +201,21 @@ def create_volumes(self, filesystem_name): self.apply_attributes_on_volume( toplevel, volume ) + volume_mountpoint = self.mountpoint + \ + self.root_volume_name + '/' if self.custom_args['root_is_snapshot']: - snapshot = self.mountpoint + '/@/.snapshots/1/snapshot/' - volume_mount = MountManager( - device=self.device, - mountpoint=os.path.normpath(snapshot + volume.realpath) - ) - self.subvol_mount_list.append( - volume_mount + volume_mountpoint = self.mountpoint + \ + f'/{self.root_volume_name}/.snapshots/1/snapshot/' + + volume_mount = MountManager( + device=self.device, + mountpoint=os.path.normpath( + volume_mountpoint + volume.realpath ) + ) + self.subvol_mount_list.append( + volume_mount + ) def get_fstab(self, persistency_type='by-label', filesystem_name=None): """ @@ -219,7 +243,10 @@ def get_fstab(self, persistency_type='by-label', filesystem_name=None): ) fstab_entry = ' '.join( [ - blkid_type + '=' + device_id, subvol_name.replace('@', ''), + blkid_type + '=' + device_id, + subvol_name.replace( + self.root_volume_name, '' + ) if self.root_volume_name != '/' else subvol_name, 'btrfs', ','.join(mount_entry_options), '0 {fs_passno}'.format( fs_passno='2' if fs_check else '0' @@ -245,7 +272,10 @@ def get_volumes(self): 'subvol=' + subvol_name ] + self.custom_filesystem_args['mount_options'] ) - volumes[subvol_name.replace('@', '')] = { + subvol_path = subvol_name.replace( + self.root_volume_name, '' + ) if self.root_volume_name != '/' else subvol_name + volumes[subvol_path] = { 'volume_options': subvol_options, 'volume_device': volume_mount.device } @@ -309,7 +339,9 @@ def get_mountpoint(self) -> str: :rtype: string """ - sync_target: List[str] = [self.mountpoint, '@'] + sync_target: List[str] = [self.mountpoint] + if self.root_volume_name != '/': + sync_target.append(self.root_volume_name) if self.custom_args.get('root_is_snapshot'): sync_target.extend(['.snapshots', '1', 'snapshot']) return os.path.join(*sync_target) @@ -327,7 +359,12 @@ def sync_data(self, exclude=None): sync_target = self.get_mountpoint() if self.custom_args['root_is_snapshot']: self._create_snapshot_info( - ''.join([self.mountpoint, '/@/.snapshots/1/info.xml']) + ''.join( + [ + self.mountpoint, + f'/{self.root_volume_name}/.snapshots/1/info.xml' + ] + ) ) data = DataSync(self.root_dir, sync_target) data.sync_data( @@ -389,7 +426,12 @@ def _xml_pretty(self, toplevel_element): return xml_data_domtree.toprettyxml(indent=" ") def _create_snapper_quota_configuration(self): - root_path = os.sep.join([self.mountpoint, '@/.snapshots/1/snapshot']) + root_path = os.sep.join( + [ + self.mountpoint, + f'{self.root_volume_name}/.snapshots/1/snapshot' + ] + ) snapper_default_conf = Defaults.get_snapper_config_template_file( root_path ) @@ -453,7 +495,9 @@ def _get_subvol_name_from_mountpoint(self, volume_mount): ) if self.toplevel_volume and self.toplevel_volume in subvol_name: subvol_name = subvol_name.replace(self.toplevel_volume, '') - return os.path.normpath(os.sep.join(['@', subvol_name])) + return os.path.normpath( + os.sep.join([self.root_volume_name, subvol_name]).replace('//', '/') + ) def __del__(self): if self.toplevel_mount: diff --git a/kiwi/xml_state.py b/kiwi/xml_state.py index d8bf4cfd939..5cce0004e0b 100644 --- a/kiwi/xml_state.py +++ b/kiwi/xml_state.py @@ -1680,6 +1680,9 @@ def get_volumes(self) -> List[volume_type]: if not have_root_volume_setup: # There must always be a root volume setup. It will be the # full size volume if no other volume has this setup + volume_management = self.get_volume_management() + root_volume_name = \ + defaults.ROOT_VOLUME_NAME if volume_management == 'lvm' else '' if have_full_size_volume: size = 'freespace:' + format( Defaults.get_min_volume_mbytes() @@ -1690,7 +1693,7 @@ def get_volumes(self) -> List[volume_type]: fullsize = True volume_type_list.append( volume_type( - name=defaults.ROOT_VOLUME_NAME, + name=root_volume_name, size=size, fullsize=fullsize, mountpoint=None, diff --git a/test/unit/volume_manager/btrfs_test.py b/test/unit/volume_manager/btrfs_test.py index d9c2fdba80b..8ccbdf40b53 100644 --- a/test/unit/volume_manager/btrfs_test.py +++ b/test/unit/volume_manager/btrfs_test.py @@ -27,12 +27,12 @@ def inject_fixtures(self, caplog): def setup(self, mock_path): self.volumes = [ volume_type( - name='LVRoot', size='freespace:100', realpath='/', + name='@', size='freespace:100', realpath='/', mountpoint=None, fullsize=False, label=None, attributes=[], is_root_volume=True ), volume_type( - name='LVetc', size='freespace:200', realpath='/etc', + name='etc', size='freespace:200', realpath='/etc', mountpoint='/etc', fullsize=False, label=None, attributes=[], is_root_volume=False ), @@ -42,7 +42,7 @@ def setup(self, mock_path): attributes=[], is_root_volume=False ), volume_type( - name='LVhome', size=None, realpath='/home', + name='home', size=None, realpath='/home', mountpoint='/home', fullsize=True, label=None, attributes=[], is_root_volume=False ) @@ -69,6 +69,17 @@ def test_post_init(self): self.volume_manager.post_init({'some-arg': 'some-val'}) assert self.volume_manager.custom_args['some-arg'] == 'some-val' + def test_post_init_root_is_snapshot_without_toplevel_volume(self): + self.volume_manager.volumes = [ + volume_type( + name='/', size='freespace:100', realpath='/', + mountpoint=None, fullsize=False, label=None, + attributes=[], is_root_volume=True + ) + ] + self.volume_manager.post_init({'root_is_snapshot': True}) + assert self.volume_manager.custom_args['root_is_snapshot'] is False + @patch('os.path.exists') @patch('kiwi.volume_manager.btrfs.Command.run') @patch('kiwi.volume_manager.btrfs.FileSystem.new') @@ -159,6 +170,47 @@ def test_setup_volume_id_not_detected( with raises(KiwiVolumeRootIDError): self.volume_manager.setup() + @patch('os.path.exists') + @patch('kiwi.volume_manager.btrfs.Command.run') + @patch('kiwi.volume_manager.btrfs.MountManager') + @patch('kiwi.volume_manager.btrfs.Path.create') + @patch('kiwi.volume_manager.base.VolumeManagerBase.apply_attributes_on_volume') + def test_create_volumes_no_toplevel_volume( + self, mock_attrs, mock_path, mock_mount, mock_command, mock_os_exists + ): + volume_mount = Mock() + mock_mount.return_value = volume_mount + self.volume_manager.mountpoint = 'tmpdir' + self.volume_manager.custom_args['root_is_snapshot'] = False + mock_os_exists.return_value = False + + self.volume_manager.root_volume_name = '/' + self.volume_manager.volumes = [ + volume_type( + name='/', size='freespace:100', realpath='/', + mountpoint=None, fullsize=False, label=None, + attributes=[], is_root_volume=True + ), + volume_type( + name='home', size=None, realpath='/home', + mountpoint='/home', fullsize=True, label=None, + attributes=[], is_root_volume=False + ) + ] + + self.volume_manager.create_volumes('btrfs') + + assert mock_path.call_args_list == [ + call('root_dir/home'), + call('tmpdir') + ] + mock_command.assert_called_once_with( + ['btrfs', 'subvolume', 'create', 'tmpdir/home'] + ) + mock_mount.assert_called_once_with( + device='/dev/storage', mountpoint='tmpdir/home' + ) + @patch('os.path.exists') @patch('kiwi.volume_manager.btrfs.Command.run') @patch('kiwi.volume_manager.btrfs.MountManager') @@ -186,7 +238,7 @@ def test_create_volumes( ), call( 'tmpdir/@/', volume_type( - name='LVetc', size='freespace:200', realpath='/etc', + name='etc', size='freespace:200', realpath='/etc', mountpoint='/etc', fullsize=False, label=None, attributes=[], is_root_volume=False @@ -194,7 +246,7 @@ def test_create_volumes( ), call( 'tmpdir/@/', volume_type( - name='LVhome', size=None, realpath='/home', + name='home', size=None, realpath='/home', mountpoint='/home', fullsize=True, label=None, attributes=[], is_root_volume=False