curtin-dev team mailing list archive

[Michael Hudson-Doyle <mwhudsonlp@xxxxxxxxxxx>]

Updated commit message and tweaked how failures are presented as suggested. Take another look?

Diff comments:

> diff --git a/curtin/block/mdadm.py b/curtin/block/mdadm.py
> index a6ac970..a5dfc9f 100644
> --- a/curtin/block/mdadm.py
> +++ b/curtin/block/mdadm.py
> @@ -770,24 +740,24 @@ def md_check_array_state(md_devname):
>      # check array state
>  
>      writable = md_check_array_state_rw(md_devname)
> -    degraded = md_sysfs_attr(md_devname, 'degraded')
> -    sync_action = md_sysfs_attr(md_devname, 'sync_action')
> +    # Raid 0 arrays do not have degraded or sync_action sysfs
> +    # attributes.
> +    degraded = md_sysfs_attr(md_devname, 'degraded', None)
> +    sync_action = md_sysfs_attr(md_devname, 'sync_action', None)
>  
>      if not writable:
>          raise ValueError('Array not in writable state: ' + md_devname)
> -    if degraded != "0":
> +    if degraded is not None and degraded != "0":

Yes that's true but I still sort of prefer handling the "is None" case separately. Maybe that's a bit purist of me.

>          raise ValueError('Array in degraded state: ' + md_devname)
> -    if sync_action != "idle":
> +    if degraded is not None and sync_action != "idle":
>          raise ValueError('Array syncing, not idle state: ' + md_devname)
>  
> -    return True
> -
>  
>  def md_check_uuid(md_devname):
>      md_uuid = md_get_uuid(md_devname)
>      if not md_uuid:
>          raise ValueError('Failed to get md UUID from device: ' + md_devname)
> -    return md_check_array_uuid(md_devname, md_uuid)
> +    md_check_array_uuid(md_devname, md_uuid)
>  
>  
>  def md_check_devices(md_devname, devices):
> diff --git a/curtin/block/schemas.py b/curtin/block/schemas.py
> index 3923321..d846505 100644
> --- a/curtin/block/schemas.py
> +++ b/curtin/block/schemas.py
> @@ -308,9 +308,13 @@ RAID = {
>      'title': 'curtin storage configuration for a RAID.',
>      'description': ('Declarative syntax for specifying RAID.'),
>      'definitions': definitions,
> -    'required': ['id', 'type', 'name', 'raidlevel', 'devices'],
> +    'required': ['id', 'type', 'name', 'raidlevel'],
>      'type': 'object',
>      'additionalProperties': False,
> +    'oneOf': [
> +        {'required': ['devices']},
> +        {'required': ['container']},
> +    ],

Yes, but I think I'm still at the stage of thinking that when the code and schema differ, it's the schema that is wrong. Maybe that can change soon.

>      'properties': {
>          'id': {'$ref': '#/definitions/id'},
>          'devices': {'$ref': '#/definitions/devices'},
> diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
> index 5a087a3..bb5e236 100644
> --- a/curtin/commands/block_meta.py
> +++ b/curtin/commands/block_meta.py
> @@ -1496,24 +1496,22 @@ def dm_crypt_handler(info, storage_config):
>              so not writing crypttab")
>  
>  
> -def verify_md_components(md_devname, raidlevel, device_paths, spare_paths):
> +def verify_md_components(md_devname, raidlevel, device_paths, spare_paths,
> +                         container):
>      # check if the array is already up, if not try to assemble
> -    check_ok = mdadm.md_check(md_devname, raidlevel, device_paths,
> -                              spare_paths)
> -    if not check_ok:
> +    try:
> +        mdadm.md_check(md_devname, raidlevel, device_paths,
> +                       spare_paths, container)
> +    except ValueError:
>          LOG.info("assembling preserved raid for %s", md_devname)
>          mdadm.mdadm_assemble(md_devname, device_paths, spare_paths)
> -        check_ok = mdadm.md_check(md_devname, raidlevel, device_paths,
> -                                  spare_paths)
> -    msg = ('Verifying %s raid composition, found raid is %s'
> -           % (md_devname, 'OK' if check_ok else 'not OK'))
> -    LOG.debug(msg)
> -    if not check_ok:
> -        raise RuntimeError(msg)
> +        mdadm.md_check(md_devname, raidlevel, device_paths,
> +                       spare_paths, container)

Yes, OK. I've changed it to something similar to your code.

>  
>  
> -def raid_verify(md_devname, raidlevel, device_paths, spare_paths):
> -    verify_md_components(md_devname, raidlevel, device_paths, spare_paths)
> +def raid_verify(md_devname, raidlevel, device_paths, spare_paths, container):
> +    verify_md_components(
> +        md_devname, raidlevel, device_paths, spare_paths, container)
>  
>  
>  def raid_handler(info, storage_config):


-- 
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/402384
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:mdadm-check-container into curtin:master.