Merge ~dbungert/curtin:nonzero-fs_pass into curtin:master

[Dan Bungert]

Output a 1 or 2 passno for fstab lines if not overridden

When outputing the fstab line, examine the path and use it to determine to
output a passno of 1 or 2. 'none' paths get a 0.

Also document that passno/freq can be set in the yaml.

LP: #1717584, LP: #1785354

Assign a default fstab passno of -1, and if that has not been overridden with a
real value, output 0/1/2 depending on the target path.
The decision was made to not pay attention to bind mounts and fsck them anyhow,
with the belief of use cases where this would not be redundant.
tmpfs and similar end up with a passno of 2, but this seems harmless.

[Dan Bungert (dbungert)]

One of the last changes broke unit tests, fixing.

fa4b7bb... by Dan Bungert on 2021-03-17

Update schema for mount for passno/freq

[Ryan Harper (raharper)]

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.

review: Needs Fixing

[Michael Hudson-Doyle (mwhudson)]

In parallel with Ryan writing his comment we found that absolutely nothing in modern Ubuntu cares about passno==1 vs passno==2 so we should definitely drop that logic.

[Michael Hudson-Doyle (mwhudson)]

We discussed this internally a bit and still think we should change the default. (I haven't heard back from the MAAS team yet). The kernel is not the only thing that reads filesystems (UEFI firmware and grub both do), and for the filesystems that do not implement any kind of non-trivial fsck, we're not at the boot optimization point where adding an invocation of essentially /bin/true will have a noticeable impact. So the (small) upsides seem to outweigh the (even smaller) downsides and I don't think this makes enough difference to anyone to warrant a feature flag.

[Michael Hudson-Doyle (mwhudson)]

Also we have changed things so fsck is run in clouds recently after not doing so was causing problems (don't know the details) and we should be consistent about this.

[Ryan Harper (raharper)]

> We discussed this internally a bit and still think we should change the

I do wish this sort of discussion would happen either on discourse or
ubuntu-devel/ubuntu-server.

> default. (I haven't heard back from the MAAS team yet). The kernel is not the
> only thing that reads filesystems (UEFI firmware and grub both do), and for

VFAT is not a journaled filesystem, I'm 100% fine with enabling fsck on this.

As for grub the design of journaled filesystems are such that it's always
readable even on crash. Worst case IIUC is a newer files could not show up
until the journal replays.

Additionally, this journal replay (and fsck) happens after grub has run; any
new writes to the filesystem won't have been checked with fsck unless fsck
happens on shutdown (I don't think this is enabled anywhere in Ubuntu) and
still won't help in the case of a crash.

> the filesystems that do not implement any kind of non-trivial fsck, we're not
> at the boot optimization point where adding an invocation of essentially

This isn't a boot optimization; rather it can only slow boot down and for zero
benefit for journaled filesystems.

> /bin/true will have a noticeable impact. So the (small) upsides seem to
> outweigh the (even smaller) downsides and I don't think this makes enough
> difference to anyone to warrant a feature flag.

Changing behavior always warrants feature flags; they're free just like
version bumps. Let's not hide this "feature".

>
> Also we have changed things so fsck is run in clouds recently after not doing
> so was causing problems (don't know the details) and we should be consistent
> about this.

I've been opposed to this as well. For all of the same reasons here and a few
more (cloud instances not being pets, not having interactive consoles to do
repairs, fsck having a non-zero effect on time it takes to mount the rootfs)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:4430c43a185818723b19e2de2f96a735cd5b08aa
https://jenkins.ubuntu.com/server/job/curtin-ci/100/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/100/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/100/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/100/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/100/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/100//rebuild

review: Needs Fixing (continuous-integration)

Unmerged commits

fa4b7bb... by Dan Bungert on 2021-03-17

Update schema for mount for passno/freq

4430c43... by Dan Bungert on 2021-03-17

Update fs_battery test for passno/freq config overrides

Given that I've added docs for passno/freq mount config parameters,
let's also test that doing so actually behaves.

9304e20... by Dan Bungert on 2021-03-17

Doc update

d51b18a... by Dan Bungert on 2021-03-17

Update passno in vmtests

35f95b1... by Dan Bungert on 2021-03-17

Add and adjust tests for passno generation

7ed5782... by Dan Bungert on 2021-03-16

Allow fstab_line_for_data to choose passno

* fstab data structure FstabData is initialized with passno = "-1"
* other initializers are expected to default to that value
* fstab_line_for_data, when encountering a -1 passno, shall apply simple
  logic to decide on a passno value to generate
* mount to / or /boot things given passno 1
* other mounts that aren't none given passno 2
* 'none' mounts given passno 0
* callers can override passno - supply a passno value that isn't "-1"

Unit tests pass, further testing pending.

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]'},
+        'passno': {'type': ['integer', 'string'],
+                   'pattern': r'(0|[1-9][0-9]*)',
+                   '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
@@ -27,7 +27,7 @@ import time
 FstabData = namedtuple(
     "FstabData", ('spec', 'path', 'fstype', 'options', 'freq', 'passno',
                   'device'))
-FstabData.__new__.__defaults__ = (None, None, None, "", "0", "0", None)
+FstabData.__new__.__defaults__ = (None, None, None, "", "0", "-1", None)
 
 
 SIMPLE = 'simple'
@@ -1020,7 +1020,7 @@ def mount_data(info, storage_config):
     fstype = info.get('fstype')
     path = info.get('path')
     freq = str(info.get('freq', 0))
-    passno = str(info.get('passno', 0))
+    passno = str(info.get('passno', -1))
 
     # turn empty options into "defaults", which works in fstab and mount -o.
     if not info.get('options'):
@@ -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'):
+            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>*
+
+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
@@ -552,7 +557,7 @@ to ``/etc/fstab``
 
 The ``options`` key will replace the default options value of ``defaults``.
 
-.. warning:: 
+.. warning::
   The kernel and user-space utilities may differ between the install
   environment and the runtime environment.  Not all kernels and user-space
   combinations will support all options.  Providing options for a mount point
@@ -565,6 +570,11 @@ The ``options`` key will replace the default options value of ``defaults``.
   If either of the environments (install or target) do not have support for
   the provided options, the behavior is undefined.
 
+**passno**: *<fsck(8) non-negative integer, typically 0-2>*
+
+The ``passno`` key refers to the fs_passno as defined in fsck(8).
+If unspecified, ``curtin`` will choose a value based on the other properties.
+
 **spec**: *<fs_spec>*
 
 The ``spec`` attribute defines the fsspec as defined in fstab(5).
@@ -596,7 +606,7 @@ Below is an example of configuring a bind mount.
 
 That would result in a fstab entry like::
 
-  /my/bind-over-var-lib /var/lib none bind 0 0
+  /my/bind-over-var-lib /var/lib none bind 0 2
 
 **Tmpfs Mount**
 
@@ -613,7 +623,7 @@ Below is an example of configuring a tmpfsbind mount.
 
 That would result in a fstab entry like::
 
-  none /my/tmpfs tmpfs size=4194304 0 0
+  none /my/tmpfs tmpfs size=4194304 0 2
 
 
 Lvm Volgroup Command
diff --git a/examples/tests/filesystem_battery.yaml b/examples/tests/filesystem_battery.yaml
index 8166360..406a087 100644
--- a/examples/tests/filesystem_battery.yaml
+++ b/examples/tests/filesystem_battery.yaml
@@ -111,15 +111,18 @@ storage:
       spec: "none"
       path: "/my/ramfs"
       fstype: "ramfs"
+      passno: 0
     - id: bind1
       fstype: "none"
       options: "bind"
       path: "/var/cache"
       spec: "/my/bind-over-var-cache"
       type: mount
+      freq: 3
     - id: bind2
       fstype: "none"
       options: "bind,ro"
       path: "/my/bind-ro-etc"
       spec: "/etc"
       type: mount
+      freq: 1
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 8cfd6af..da8b2e2 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -361,7 +361,7 @@ class TestBlockMeta(CiTestCase):
         block_meta.mount_handler(mount_info, self.storage_config)
         options = 'defaults'
         comment = "# / was on /wark/xxx during curtin installation"
-        expected = "%s\n%s %s %s %s 0 0\n" % (comment,
+        expected = "%s\n%s %s %s %s 0 1\n" % (comment,
                                               disk_info['path'],
                                               mount_info['path'],
                                               fs_info['fstype'], options)
@@ -389,7 +389,7 @@ class TestBlockMeta(CiTestCase):
         block_meta.mount_handler(mount_info, self.storage_config)
         options = 'ro'
         comment = "# /readonly was on /wark/xxx during curtin installation"
-        expected = "%s\n%s %s %s %s 0 0\n" % (comment,
+        expected = "%s\n%s %s %s %s 0 2\n" % (comment,
                                               disk_info['path'],
                                               mount_info['path'],
                                               fs_info['fstype'], options)
@@ -418,7 +418,7 @@ class TestBlockMeta(CiTestCase):
         block_meta.mount_handler(mount_info, self.storage_config)
         options = 'defaults'
         comment = "# /readonly was on /wark/xxx during curtin installation"
-        expected = "%s\n%s %s %s %s 0 0\n" % (comment,
+        expected = "%s\n%s %s %s %s 0 2\n" % (comment,
                                               disk_info['path'],
                                               mount_info['path'],
                                               fs_info['fstype'], options)
@@ -449,7 +449,7 @@ class TestBlockMeta(CiTestCase):
         block_meta.mount_handler(mount_info, self.storage_config)
         options = 'defaults'
         comment = "# /readonly was on /wark/xxx during curtin installation"
-        expected = "#curtin-test\n%s\n%s %s %s %s 0 0\n" % (comment,
+        expected = "#curtin-test\n%s\n%s %s %s %s 0 2\n" % (comment,
                                                             disk_info['path'],
                                                             mount_info['path'],
                                                             fs_info['fstype'],
@@ -610,7 +610,7 @@ class TestFstabData(CiTestCase):
         self.assertEqual(
             block_meta.FstabData(
                 spec="none", fstype="tmpfs", path="/tmpfs",
-                options="defaults", freq="0", passno="0", device=None),
+                options="defaults", freq="0", device=None),
             block_meta.mount_data(info, {'xm1': info}))
 
     @patch('curtin.block.iscsi.volpath_is_iscsi')
@@ -625,7 +625,7 @@ class TestFstabData(CiTestCase):
         self.assertEqual(
             block_meta.FstabData(
                 spec=None, fstype="ext4", path="/",
-                options="noatime", freq="0", passno="0", device="/dev/xda1"),
+                options="noatime", freq="0", device="/dev/xda1"),
             block_meta.mount_data(scfg['m1'], scfg))
 
     @patch('curtin.block.iscsi.volpath_is_iscsi', return_value=False)
@@ -643,7 +643,7 @@ class TestFstabData(CiTestCase):
         self.assertEqual(
             block_meta.FstabData(
                 spec=None, fstype="vfat", path="/boot/efi",
-                options="defaults", freq="0", passno="0", device="/dev/xda1"),
+                options="defaults", freq="0", device="/dev/xda1"),
             block_meta.mount_data(scfg['m1'], scfg))
 
     @patch('curtin.block.iscsi.volpath_is_iscsi')
@@ -657,7 +657,7 @@ class TestFstabData(CiTestCase):
         self.assertEqual(
             block_meta.FstabData(
                 spec=None, fstype="ext4", path="/",
-                options="noatime,_netdev", freq="0", passno="0",
+                options="noatime,_netdev", freq="0",
                 device="/dev/xda1"),
             block_meta.mount_data(scfg['m1'], scfg))
 
@@ -683,7 +683,7 @@ class TestFstabData(CiTestCase):
         self.assertEqual(
             block_meta.FstabData(
                 spec=myspec, fstype="ext3", path="/",
-                options="noatime", freq="0", passno="0",
+                options="noatime", freq="0",
                 device=None),
             block_meta.mount_data(mnt, scfg))
 
@@ -761,15 +761,53 @@ class TestFstabData(CiTestCase):
             spec="/dev/disk2", path="/mnt", fstype='btrfs', options='noatime')
         lines = block_meta.fstab_line_for_data(fdata).splitlines()
         self.assertEqual(
-            ["/dev/disk2", "/mnt", "btrfs", "noatime", "0", "0"],
+            ["/dev/disk2", "/mnt", "btrfs", "noatime", "0", "2"],
+            lines[1].split())
+
+    def test_fstab_line_root_and_no_passno(self):
+        """fstab_line_for_data passno autoselect for /."""
+        fdata = block_meta.FstabData(
+            spec="/dev/disk2", path="/", fstype='btrfs', options='noatime')
+        lines = block_meta.fstab_line_for_data(fdata).splitlines()
+        self.assertEqual(
+            ["/dev/disk2", "/", "btrfs", "noatime", "0", "1"],
+            lines[1].split())
+
+    def test_fstab_line_boot_and_no_passno(self):
+        """fstab_line_for_data passno autoselect for /boot."""
+        fdata = block_meta.FstabData(
+            spec="/dev/disk2", path="/boot", fstype='btrfs', options='noatime')
+        lines = block_meta.fstab_line_for_data(fdata).splitlines()
+        self.assertEqual(
+            ["/dev/disk2", "/boot", "btrfs", "noatime", "0", "1"],
+            lines[1].split())
+
+    def test_fstab_line_boot_efi_and_no_passno(self):
+        """fstab_line_for_data passno autoselect for /boot/efi."""
+        fdata = block_meta.FstabData(
+            spec="/dev/disk2", path="/boot/efi", fstype='btrfs',
+            options='noatime')
+        lines = block_meta.fstab_line_for_data(fdata).splitlines()
+        self.assertEqual(
+            ["/dev/disk2", "/boot/efi", "btrfs", "noatime", "0", "1"],
+            lines[1].split())
+
+    def test_fstab_line_almost_boot(self):
+        """fstab_line_for_data passno that pretends to be /boot."""
+        fdata = block_meta.FstabData(
+            spec="/dev/disk2", path="/boots", fstype='btrfs',
+            options='noatime')
+        lines = block_meta.fstab_line_for_data(fdata).splitlines()
+        self.assertEqual(
+            ["/dev/disk2", "/boots", "btrfs", "noatime", "0", "2"],
             lines[1].split())
 
     def test_fstab_line_for_data_with_passno_and_freq(self):
         """fstab_line_for_data should respect passno and freq."""
         fdata = block_meta.FstabData(
-            spec="/dev/d1", path="/mnt", fstype='ext4', freq="1", passno="2")
+            spec="/dev/d1", path="/mnt", fstype='ext4', freq="1", passno="1")
         lines = block_meta.fstab_line_for_data(fdata).splitlines()
-        self.assertEqual(["1", "2"], lines[1].split()[4:6])
+        self.assertEqual(["1", "1"], lines[1].split()[4:6])
 
     def test_fstab_line_for_data_raises_error_without_spec_or_device(self):
         """fstab_line_for_data should raise ValueError if no spec or device."""
@@ -797,7 +835,7 @@ class TestFstabData(CiTestCase):
             "# /mnt was on /dev/disk2 during curtin installation",
             lines[0])
         self.assertEqual(
-            [by_uuid, "/mnt", "ext4", "defaults", "0", "0"],
+            [by_uuid, "/mnt", "ext4", "defaults", "0", "2"],
             lines[1].split())
         self.assertEqual(1, m_uinfo.call_count)
         self.assertEqual(1, m_vol_type.call_count)
@@ -819,7 +857,7 @@ class TestFstabData(CiTestCase):
             "# /mnt was on /dev/disk2 during curtin installation",
             lines[0])
         self.assertEqual(
-            ["/dev/disk2", "/mnt", "ext4", "defaults", "0", "0"],
+            ["/dev/disk2", "/mnt", "ext4", "defaults", "0", "2"],
             lines[1].split())
         self.assertEqual(1, m_uinfo.call_count)
         self.assertEqual(1, m_vol_type.call_count)
@@ -837,7 +875,7 @@ class TestFstabData(CiTestCase):
             '# /mnt was on /dev/xvda1 during curtin installation',
             lines[0])
         self.assertEqual(
-            ["/dev/xvda1", "/mnt", "ext4", "defaults", "0", "0"],
+            ["/dev/xvda1", "/mnt", "ext4", "defaults", "0", "2"],
             lines[1].split())
         self.assertEqual(0, m_get_uuid.call_count)
 
diff --git a/tests/vmtests/test_fs_battery.py b/tests/vmtests/test_fs_battery.py
index 7d7b494..e5903f1 100644
--- a/tests/vmtests/test_fs_battery.py
+++ b/tests/vmtests/test_fs_battery.py
@@ -159,10 +159,10 @@ class TestFsBattery(VMBaseClass):
     def test_fstab_has_mounts(self):
         """Verify each of the expected "my" mounts got into fstab."""
         expected = [
-            "none /my/tmpfs tmpfs size=4194304 0 0".split(),
+            "none /my/tmpfs tmpfs size=4194304 0 2".split(),
             "none /my/ramfs ramfs defaults 0 0".split(),
-            "/my/bind-over-var-cache /var/cache none bind 0 0".split(),
-            "/etc /my/bind-ro-etc none bind,ro 0 0".split(),
+            "/my/bind-over-var-cache /var/cache none bind 3 2".split(),
+            "/etc /my/bind-ro-etc none bind,ro 1 2".split(),
         ]
         fstab_found = [
             line.split() for line in self.load_collect_file(