curtin-dev team mailing list archive

[Paride Legovini <paride.legovini@xxxxxxxxxxxxx>]

Review: Needs Fixing

Nice, thanks Ryan! I left some inline comments.

Diff comments:

> diff --git a/curtin/distro.py b/curtin/distro.py
> index 43b0c19..84060df 100644
> --- a/curtin/distro.py
> +++ b/curtin/distro.py
> @@ -484,12 +485,20 @@ def parse_dpkg_version(raw, name=None, semx=None):
>      if semx is None:
>          semx = (10000, 100, 1)
>  
> -    if "-" in raw:
> -        upstream = raw.rsplit('-', 1)[0]
> +    raw_offset = 0
> +    if ':' in raw:
> +        epoch, _, upstream = raw.partition(':')
> +        raw_offset = len(epoch) + 1
>      else:
> -        # this is a native package, package version treated as upstream.
> +        epoch = None

The package version number format deb-version(7) specifies what when the epoch is omitted zero is assumed. I think we should adhere with this convention when constructing a representation of the full version of a package. Having epoch always as an integer also allows simpler version comparisons.

>          upstream = raw
>  
> +    if "-" in raw[raw_offset:]:
> +        upstream = raw[raw_offset:].rsplit('-', 1)[0]
> +    else:
> +        # this is a native package, package version treated as upstream.
> +        upstream = raw[raw_offset:]
> +
>      match = re.search(r'[^0-9.]', upstream)
>      if match:
>          extra = upstream[match.start():]
> diff --git a/curtin/swap.py b/curtin/swap.py
> index d3f29dc..6e2d266 100644
> --- a/curtin/swap.py
> +++ b/curtin/swap.py
> @@ -51,7 +53,61 @@ def suggested_swapsize(memsize=None, maxsize=None, fsys=None):
>      return maxsize
>  
>  
> -def setup_swapfile(target, fstab=None, swapfile=None, size=None, maxsize=None):
> +def get_fstype(target, source):
> +    target_source = paths.target_path(target, source)
> +    try:
> +        out, _ = util.subp(['findmnt', '--target', target_source,
> +                            '-o', 'FSTYPE'], capture=True)
> +    except util.ProcessExecutionError as exc:
> +        LOG.warning('Failed to query %s fstype, findmnt returned error: %s',
> +                    target_source, exc)
> +        return None
> +
> +    if out:
> +        """
> +        $ findmnt --target /btrfs  -o FSTYPE
> +        FSTYPE
> +        btrfs
> +        """
> +        return out.splitlines()[-1]

findmnt has a --noheadings option that suppresses the header line. The practical difference is little in this case, but in general I favor these "machine readable output" modes as they're less likely to give up surprises.

> +
> +    return None
> +
> +
> +def get_target_kernel_version(target):
> +    pkg_ver = None
> +
> +    distro_info = distro.get_distroinfo(target=target)
> +    if not distro_info:
> +        raise RuntimeError('Failed to determine target distro')
> +    osfamily = distro_info.family
> +    if osfamily == distro.DISTROS.debian:
> +        try:
> +            # check in-target version
> +            pkg_ver = distro.get_package_version('linux-image-generic',
> +                                                 target=target)
> +        except Exception as e:
> +            LOG.warn(
> +                "failed reading linux-image-generic package version, %s", e)
> +    return pkg_ver
> +
> +
> +def can_use_swapfile(target, fstype):
> +    if fstype is None:
> +        raise RuntimeError(
> +            'Unknown target filesystem type, may not support swapfiles')
> +    if fstype in ['btrfs', 'zfs', 'xfs']:

At the moment for ZFS and XFS this if stanza checks that get_target_kernel_version() returns something, which at the moment happens only if the os family is debian.

In other words, when fstype = zfs|xfs we just check if the os family is debian, with no restriction on the kernel version. Is this how this is intended to work? This is not unreasonable, support for more OS families can be added later, I just want to be sure.

> +        # check kernel version
> +        pkg_ver = get_target_kernel_version(target)
> +        if not pkg_ver:
> +            raise RuntimeError('Failed to read target kernel version')
> +        if fstype == 'btrfs' and pkg_ver['major'] < 5:
> +            raise RuntimeError(
> +                'btrfs requiers kernel version 5.0+ to use swapfiles')
> +
> +
> +def setup_swapfile(target, fstab=None, swapfile=None, size=None, maxsize=None,
> +                   force=False):
>      if size is None:
>          size = suggested_swapsize(fsys=target, maxsize=maxsize)
>  
> @@ -65,6 +121,21 @@ def setup_swapfile(target, fstab=None, swapfile=None, size=None, maxsize=None):
>      if not swapfile.startswith("/"):
>          swapfile = "/" + swapfile
>  
> +    # query the directory in which swapfile will reside
> +    fstype = get_fstype(target, os.path.dirname(swapfile))
> +    try:
> +        can_use_swapfile(target, fstype)
> +    except RuntimeError as err:
> +        if force:
> +            LOG.warning('swapfile may not work: %s', err)
> +        else:
> +            LOG.debug('Not creating swap: %s', err)
> +            return
> +
> +    allocate_cmd = 'fallocate -l "${2}M" "$1"'
> +    if fstype in ['xfs', 'btrfs']:

fallocate(1) says:

Supported for XFS (since Linux 2.6.38), ext4 (since Linux 3.0), Btrfs (since  Linux  3.7) and tmpfs (since Linux 3.5).

so here it's not clear to me why we explicitly not use it. Should we add a comment/pointer to the reason?

I think ZFS does not support fallocate [1]. Should we add it to the filesystems for which we use dd?

[1] https://github.com/openzfs/zfs/issues/326

> +        allocate_cmd = 'dd if=/dev/zero "of=$1" bs=1M "count=$2"'
> +
>      mbsize = str(int(size / (2 ** 20)))
>      msg = "creating swap file '%s' of %sMB" % (swapfile, mbsize)
>      fpath = os.path.sep.join([target, swapfile])


-- 
https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/386117
Your team curtin developers is subscribed to branch curtin:master.