curtin-dev team mailing list archive

[Ryan Harper <rharper@xxxxxxxxxxx>]

Thanks for the review, I'll address your 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

Sure, defaulting to 0 makes sense.  Thanks.

>          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]

Yes; I was looking for that but didn't find the right flag name.

> +
> +    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']:

This is currently Ubuntu/Debian only; primarily because we don't have a parse_rpm_version() implemented in curtin/distro.py ;  it requires more research into which kernel versions are required for swapfile support.

> +        # 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']:

W.r.t to XFS, it's not clear to me which kernel version would support fallocate for creating swapfiles; this issue was filed/fixed earlier this year specifically about fallocate on xfs not working.

https://github.com/canonical/cloud-init/pull/70
https://bugzilla.redhat.com/show_bug.cgi?id=1772505

If/when xfs can support fallocate, we can update this.

For zfs, yes I can add that.

> +        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.