curtin-dev team mailing list archive

[Dan Bungert <mp+448442@xxxxxxxxxxxxxxxxxx>]

Do we have the ability to test the Dasd parts of this?  I think it looks safe enough but if it's wrong then a future change is doomed.

Diff comments:

> diff --git a/curtin/block/mkfs.py b/curtin/block/mkfs.py
> index 20ee7b8..4e716e7 100644
> --- a/curtin/block/mkfs.py
> +++ b/curtin/block/mkfs.py
> @@ -230,15 +230,12 @@ def mkfs(path, fstype, strict=False, label=None, uuid=None, force=False,
>      return uuid
>  
>  
> -def mkfs_from_config(path, info, strict=False):
> +def mkfs_from_action(path, format, strict=False):

I think we want type names on these

>      """Make filesystem on block device with given path according to storage
>         config given"""
> -    fstype = info.get('fstype')
> -    if fstype is None:
> -        raise ValueError("fstype must be specified")
>      # NOTE: Since old metadata on partitions that have not been wiped can cause
>      #       some mkfs commands to refuse to work, it's best to use force=True
> -    mkfs(path, fstype, strict=strict, force=True, uuid=info.get('uuid'),
> -         label=info.get('label'), extra_options=info.get('extra_options'))
> +    mkfs(path, format.fstype, strict=strict, force=True, uuid=format.uuid,
> +         label=format.label, extra_options=format.extra_options)
>  
>  # vi: ts=4 expandtab syntax=python
> diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
> index 11972b9..fc61e21 100644
> --- a/curtin/commands/block_meta.py
> +++ b/curtin/commands/block_meta.py
> @@ -733,92 +748,103 @@ def dasd_handler(info, storage_config, context):
>       'disk_layout': 'cdl',
>      }
>      """
> -    device_id = info.get('device_id')
> -    blocksize = info.get('blocksize')
> -    disk_layout = info.get('disk_layout')
> -    label = info.get('label')
> -    mode = info.get('mode')
> -    force_format = config.value_as_boolean(info.get('wipe'))
> -
> -    dasd_device = dasd.DasdDevice(device_id)
> -    if (force_format or dasd_device.needs_formatting(blocksize,
> -                                                     disk_layout, label)):
> -        if config.value_as_boolean(info.get('preserve')):
> +    dasd: Dasd = storage_actions.asobject(info)
> +    force_format = config.value_as_boolean(dasd.wipe)
> +
> +    dasd_device = DasdDevice(dasd.device_id)
> +    if (force_format or dasd_device.needs_formatting(dasd.blocksize,
> +                                                     dasd.disk_layout,
> +                                                     dasd.label)):
> +        if config.value_as_boolean(dasd.preserve):
>              raise ValueError(
>                  "dasd '%s' does not match configured properties and"
>                  "preserve is set to true.  The dasd needs formatting"
> -                "with the specified parameters to continue." % info.get('id'))
> +                "with the specified parameters to continue." % dasd.id)
>  
>          LOG.debug('Formatting dasd id=%s device_id=%s devname=%s',
> -                  info.get('id'), device_id, dasd_device.devname)
> -        dasd_device.format(blksize=blocksize, layout=disk_layout,
> -                           set_label=label, mode=mode)
> +                  dasd.id, dasd.device_id, dasd_device.devname)
> +        dasd_device.format(blksize=dasd.blocksize, layout=dasd.disk_layout,
> +                           set_label=dasd.label, mode=dasd.mode)
>  
>          # check post-format to ensure values match
> -        if dasd_device.needs_formatting(blocksize, disk_layout, label):
> +        if dasd_device.needs_formatting(
> +                dasd.blocksize, dasd.disk_layout, dasd.label):
>              raise RuntimeError(
>                  "Dasd %s failed to format" % dasd_device.devname)
>  
>  
> +@attr.s(auto_attribs=True)
> +class _Partitionable:
> +    ptable: Optional[str] = None
> +    preserve: bool = storage_actions.boolean(default=False)
> +    wipe: Optional[str] = None
> +    name: Optional[str] = None
> +
> +
> +@storage_actions.define("disk")
> +class Disk(_Partitionable):
> +    pass
> +
> +
>  def disk_handler(info, storage_config, context):
> +    disk: _Partitionable = storage_actions.asobject(info)
>      _dos_names = ['dos', 'msdos']
> -    ptable = info.get('ptable')
> -    if ptable and ptable not in PTABLES_VALID:
> +    if disk.ptable and disk.ptable not in PTABLES_VALID:
>          raise ValueError(
> -            'Invalid partition table type: %s in %s' % (ptable, info))
> +            'Invalid partition table type: %s in %s' % (disk.ptable, disk))
>  
> -    disk = get_path_to_storage_volume(info.get('id'), storage_config)
> -    context.id_to_device[info['id']] = disk
> +    disk_path = get_path_to_storage_volume(disk.id, storage_config)
> +    context.id_to_device[disk.id] = disk_path
>      # For disks, 'preserve' is what indicates whether the partition
>      # table should be reused or recreated but for compound devices
>      # such as raids, it indicates if the raid should be created or
>      # assumed to already exist. So to allow a pre-existing raid to get
>      # a new partition table, we use presence of 'wipe' field to
>      # indicate if the disk should be reformatted or not.
> -    if info['type'] == 'disk':
> -        preserve_ptable = config.value_as_boolean(info.get('preserve'))
> +    if disk.type == 'disk':
> +        preserve_ptable = disk.preserve
>      else:
> -        preserve_ptable = config.value_as_boolean(info.get('preserve')) \
> -                          and not config.value_as_boolean(info.get('wipe'))
> +        preserve_ptable = disk.preserve and not disk.wipe
>      if preserve_ptable:
>          # Handle preserve flag, verifying if ptable specified in config
> -        if ptable and ptable != PTABLE_UNSUPPORTED:
> -            current_ptable = block.get_part_table_type(disk)
> +        if disk.ptable and disk.ptable != PTABLE_UNSUPPORTED:
> +            current_ptable = block.get_part_table_type(disk_path)
>              LOG.debug('disk: current ptable type: %s', current_ptable)
>              if current_ptable not in PTABLES_SUPPORTED:
>                  raise ValueError(
>                      "disk '%s' does not have correct partition table or "
>                      "cannot be read, but preserve is set to true (or wipe is "
>                      "not set).  cannot continue installation." %
> -                    info.get('id'))
> +                    disk.id)
>          LOG.info("disk '%s' marked to be preserved, so keeping partition "
> -                 "table" % disk)
> +                 "table" % disk.id)
>      else:
>          # wipe the disk and create the partition table if instructed to do so
> -        if config.value_as_boolean(info.get('wipe')):
> -            block.wipe_volume(disk, mode=info.get('wipe'))
> -        if config.value_as_boolean(ptable):
> -            LOG.info("labeling device: '%s' with '%s' partition table", disk,
> -                     ptable)
> -            if ptable == "gpt":
> +        if config.value_as_boolean(disk.wipe):
> +            block.wipe_volume(disk_path, mode=disk.wipe)
> +        if disk.ptable is not None:
> +            LOG.info(
> +                "labeling device: '%s' with '%s' partition table", disk_path,
> +                disk.ptable)
> +            if disk.ptable == "gpt":
>                  # Wipe both MBR and GPT that may be present on the disk.
>                  # N.B.: wipe_volume wipes 1M at front and end of the disk.
>                  # This could destroy disk data in filesystems that lived
>                  # there.
> -                block.wipe_volume(disk, mode='superblock')
> -            elif ptable in _dos_names:
> -                util.subp(["parted", disk, "--script", "mklabel", "msdos"])
> -            elif ptable == "vtoc":
> -                util.subp(["fdasd", "-c", "/dev/null", disk])
> -        holders = clear_holders.get_holders(disk)
> +                block.wipe_volume(disk_path, mode='superblock')

totally unrelated to this PR but the above comment about front and end of the disk wiping makes me think we need to make that an opt-in behavior for wipe_volume to reduce surprises

> +            elif disk.ptable in _dos_names:
> +                util.subp(["parted", disk_path, "--script", "mklabel", "msdos"])
> +            elif disk.ptable == "vtoc":
> +                util.subp(["fdasd", "-c", "/dev/null", disk_path])
> +        holders = clear_holders.get_holders(disk_path)
>          if len(holders) > 0:
>              LOG.info('Detected block holders on disk %s: %s', disk, holders)
> -            clear_holders.clear_holders(disk)
> -            clear_holders.assert_clear(disk)
> +            clear_holders.clear_holders(disk_path)
> +            clear_holders.assert_clear(disk_path)
>  
>      # Make the name if needed
> -    if info.get('name'):
> -        make_dname(info.get('id'), storage_config)
> +    if disk.name:
> +        make_dname(disk.id, storage_config)
>  
>  
>  def getnumberoflogicaldisks(device, storage_config):
> @@ -1002,23 +1027,28 @@ def check_passed_path(info, actual_path):
>              info["type"], info, info['path'], actual_path))
>  
>  
> +@storage_actions.define("partition")
> +class Partition:

We have people with devices out there that have ptables on a partition and I don't know why that is done or how that would appear without forcing it, but I think we have to at least handle it.  So should Partition be a _Partitionable?  (could save that fun for a future PR if it causes too much trouble)

> +    device: str
> +    size: int = storage_actions.size()
> +    flag: Optional[str] = None
> +    path: Optional[str] = None
> +    preserve: bool = storage_actions.boolean(default=False)
> +    wipe: Optional[str] = None
> +
> +
>  def partition_handler(info, storage_config, context):
> -    device = info.get('device')
> -    size = info.get('size')
> -    flag = info.get('flag')
> -    disk_ptable = storage_config.get(device).get('ptable')
> +    partition: Partition = storage_actions.asobject(info)

I wonder if asobject should take an expected type value.  We already know that manual partitioning through curtin actions is a bit of a challenge, getting the type value incorrect can be as easy as a copy-paste error.

If the type name is outright flawed, _type_to_cls will fail to find it, so that's not my concern.  more along the lines that we're writing curtin storage config and attempting to construct a partition object and we copy-paste a disk and leave it as type: disk, could such an object make it to here?

> +    disk_ptable = storage_config.get(partition.device).get('ptable')
>      partition_type = None
> -    if not device:
> -        raise ValueError("device must be set for partition to be created")
> -    if not size:
> -        raise ValueError("size must be specified for partition to be created")
>  
> -    disk = get_path_to_storage_volume(device, storage_config)
> -    partnumber = determine_partition_number(info.get('id'), storage_config)
> +    disk = get_path_to_storage_volume(partition.device, storage_config)
> +    partnumber = determine_partition_number(partition.id, storage_config)
>      disk_kname = block.path_to_kname(disk)
>      part_path = block.dev_path(block.partition_kname(disk_kname, partnumber))
> -    check_passed_path(info, part_path)
> -    context.id_to_device[info['id']] = part_path
> +    if partition.path is not None:

this is a small logic change that drops a warning, but we actually use the fixed value so I think existing users are OK.

Are you intentionally dropping the warning for the `partition.path is None` case?

> +        check_passed_path({'path': partition.path}, part_path)
> +    context.id_to_device[partition.id] = part_path
>  
>      # consider the disks logical sector size when calculating sectors
>      try:
> @@ -1080,26 +1112,25 @@ def partition_handler(info, storage_config, context):
>                                        previous_size_sectors +
>                                        alignment_offset)
>  
> -    length_bytes = util.human2bytes(size)
>      # start sector is part of the sectors that define the partitions size
>      # so length has to be "size in sectors - 1"
> -    length_sectors = int(length_bytes / logical_block_size_bytes) - 1
> +    length_sectors = int(partition.size / logical_block_size_bytes) - 1
>      # logical partitions can't share their start sector with the extended
>      # partition and logical partitions can't go head-to-head, so we have to
>      # realign and for that increase size as required
> -    if info.get('flag') == "extended":
> -        logdisks = getnumberoflogicaldisks(device, storage_config)
> +    if partition.flag == "extended":
> +        logdisks = getnumberoflogicaldisks(partition.device, storage_config)
>          length_sectors = length_sectors + (logdisks * alignment_offset)
>  
>      # Handle preserve flag
>      create_partition = True
> -    if config.value_as_boolean(info.get('preserve')):
> +    if partition.preserve:

In some cases we're keeping the value_as_boolean and some not (maybe it was wipe), what would we like to standardize on?

>          if disk_ptable == 'vtoc':
> -            partition_verify_fdasd(disk, partnumber, info)
> +            partition_verify_fdasd(disk, partnumber, partition)
>          else:
>              sfdisk_info = block.sfdisk_info(disk)
>              part_info = block.get_partition_sfdisk_info(part_path, sfdisk_info)
> -            partition_verify_sfdisk(info, sfdisk_info['label'], part_info)
> +            partition_verify_sfdisk(partition, sfdisk_info['label'], part_info)
>          LOG.debug(
>              '%s partition %s already present, skipping create',
>              disk, partnumber)


-- 
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/448442
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:action-objects-more into curtin:master.