cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

I generally agree with your comments.
The code needs overhaul and better tests.

I'm trying to avoid doing too much to it now.

Diff comments:

> diff --git a/cloudinit/config/cc_disk_setup.py b/cloudinit/config/cc_disk_setup.py
> index 29eb5dd..e1505b3 100644
> --- a/cloudinit/config/cc_disk_setup.py
> +++ b/cloudinit/config/cc_disk_setup.py
> @@ -504,12 +488,35 @@ def check_partition_gpt_layout(device, layout):
>                          device, e))
>  
>      out_lines = iter(out.splitlines())
> -    # Skip header
> +    # Skip header.  Output looks like:
> +    # ***************************************************************
> +    # Found invalid GPT and valid MBR; converting MBR to GPT format
> +    # in memory.
> +    # ***************************************************************
> +    #
> +    # Disk /dev/vdb: 83886080 sectors, 40.0 GiB
> +    # Logical sector size: 512 bytes
> +    # Disk identifier (GUID): 8A7F11AD-3953-491B-8051-077E01C8E9A7
> +    # Partition table holds up to 128 entries
> +    # First usable sector is 34, last usable sector is 83886046
> +    # Partitions will be aligned on 2048-sector boundaries
> +    # Total free space is 83476413 sectors (39.8 GiB)
> +    #
> +    # Number Start (sector) End (sector) Size       Code  Name
> +    # 1      2048           206847       100.0 MiB  0700  Microsoft basic data
>      for line in out_lines:
>          if line.strip().startswith('Number'):
>              break
>  
> -    return [line.strip().split()[-1] for line in out_lines]

yeah, its wierd. I was confused too.

> +    codes = [line.strip().split()[5] for line in out_lines]
> +    cleaned = []
> +
> +    # user would expect a code '83' to be Linux, but sgdisk outputs 8300.

yeah, at some point we need large overhaul of this.
the changes here are just fixing gaping wounds.  There is definitely a lot of need for improvement on this code.

> +    for code in codes:
> +        if len(code) == 4 and code.endswith("00"):
> +            code = code[0:2]
> +        cleaned.append(code)
> +    return cleaned
>  
>  
>  def check_partition_layout(table_type, device, layout):
> @@ -530,18 +539,17 @@ def check_partition_layout(table_type, device, layout):
>              return True
>          return False
>  
> -    else:
> -        if len(found_layout) != len(layout):
> -            return False
> -        else:
> -            # This just makes sure that the number of requested
> -            # partitions and the type labels are right
> -            for x in range(1, len(layout) + 1):
> -                if isinstance(layout[x - 1], tuple):
> -                    _, part_type = layout[x]
> -                    if int(found_layout[x]) != int(part_type):
> -                        return False
> -            return True
> +    elif len(found_layout) == len(layout):
> +        # This just makes sure that the number of requested
> +        # partitions and the type labels are right

yeah. For now the logic here specifically is not complex.
but this whole function could be passed *in* the desired format and the current format (and the disk size) and then work all on that.  agreed we need improvements.

> +        layout_types = [str(x[1]) if isinstance(x, (tuple, list)) else None
> +                        for x in layout]
> +        LOG.debug("Layout types=%s. Found types=%s",
> +                  layout_types, found_layout)
> +        for itype, ftype in zip(layout_types, found_layout):
> +            if itype is not None and str(ftype) != str(itype):
> +                return False
> +        return True
>  
>      return False
>  


-- 
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324338
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1692087-disk_setup-gpt-improvements into cloud-init:master.