curtin-dev team mailing list archive

[Ryan Harper <rharper@xxxxxxxxxxx>]

Review: Needs Fixing

Thanks for submitting the MR.

<rant>
Personally I don't think this is the right choice to make by default.  I don't think we install anything but journaled filesystems in which fsck will merely replay the journal just like the kernel already does.  Look at fsck.xfs(8)  manpage for a laugh.  
</rant>

We should at least:

1) Add a curtin feature flag indicating curtin will now write fstabs which will enable fsck by default (curtin/__init__.py)
2) Add a config option to control the default fstab fsck value (block-meta: fstab_passno_default)
3) sync with MAAS/subiquity so they opt-in to fsck by default feature.
4) Consider not enabling the feature by default such that all MAAS/Subiquity users which move up to newer curtin are generating different installs than they did before the update.

I'll note that both MAAS and subiquity *can* fsck by default today if they construct storage config mount entries with the 'spec' field filled out they way they want.

Diff comments:

> diff --git a/curtin/block/schemas.py b/curtin/block/schemas.py
> index 4dc2f0a..5196012 100644
> --- a/curtin/block/schemas.py
> +++ b/curtin/block/schemas.py
> @@ -262,6 +262,11 @@ MOUNT = {
>              ],
>          },
>          'spec': {'type': 'string'},  # XXX: Tighten this to fstab fs_spec
> +        'freq': {'type': 'integer',
> +                 'pattern': r'[0-9]'},

glibc says the fs_freq[1] is an integer holding the number of days between dumps; I think this should be r'[0-9]+' to allow for more than 9 days (not that anyone really sets this AFAIK, but just in case).

1. https://www.gnu.org/software/libc/manual/html_node/fstab.html

> +        'passno': {'type': ['integer', 'string'],
> +                   'pattern': r'(0|[1-9][0-9]*)',

I think we need to accept -1 here as well.  If someone just enumerates the default values it won't pass this schema.

> +                   'minimum': 0},
>      },
>  }
>  PARTITION = {
> diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
> index cf6bc02..9199afb 100644
> --- a/curtin/commands/block_meta.py
> +++ b/curtin/commands/block_meta.py
> @@ -1151,8 +1151,18 @@ def fstab_line_for_data(fdata):
>      else:
>          comment = None
>  
> +    passno = fdata.passno
> +    if passno == "-1":
> +        # / and /boot things get 1
> +        if path == "/" or path[1:].split('/')[0] in ('', 'boot'):

If path in ["/", "/boot", "/boot/efi"]: ?

If someone has /boot/whatever, we don't care about that w.r.t fsck.

> +            passno = "1"
> +        elif path != "none":
> +            passno = "2"
> +        else:
> +            passno = "0"
> +
>      entry = ' '.join((spec, path, fdata.fstype, options,
> -                      fdata.freq, fdata.passno)) + "\n"
> +                      fdata.freq, passno)) + "\n"
>      line = '\n'.join([comment, entry] if comment else [entry])
>      return line
>  
> diff --git a/doc/topics/storage.rst b/doc/topics/storage.rst
> index 75c5537..58e93f7 100644
> --- a/doc/topics/storage.rst
> +++ b/doc/topics/storage.rst
> @@ -542,6 +542,11 @@ One of ``device`` or ``spec`` must be present.
>    fstab entry will contain ``_netdev`` to indicate networking is
>    required to mount this filesystem.
>  
> +**freq**: *<dump(8) integer from 0-9 inclusive>*

Integer value indicating the dump frequency in days.

> +
> +The ``freq`` key refers to the freq as defined in dump(8).
> +Defaults to ``0`` if unspecified.
> +
>  **fstype**: *<fileystem type>*
>  
>  ``fstype`` is only required if ``device`` is not present.  It indicates


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