mounts: fix mount opts string for ephemeral disk

[cjp256]

Fixes the spaces introduced in #1213

[blackboxsw]

Wow, ok this exposes our integration test coverage failures there. I'd like to get integration test coverage to at least assert that the expected defaults mount opts are seen without error. Willl provide a patch suggestion within the hour on this.

[blackboxsw]

Man, mount -a behavior leaves a bit to be desired. Given our def_mnt_opts behavior mount -a still exits 0 despite invalid fstab directives:

root@cloudinit-0207-183840ro7sno9j:~# mount -a
mount: /etc/fstab: parse error at line 3 -- ignored
mount: /etc/fstab: parse error at line 4 -- ignored
root@cloudinit-0207-183840ro7sno9j:~# echo $?
0

I think we probably want to grow more awareness of this case to error earlier for users instead of silent success. It doesn't have to be in this PR though.

For this PR I'd like the following integration test to make sure our defaults don't again break on a deployment:

diff --git a/tests/integration_tests/modules/test_disk_setup.py b/tests/integration_tests/modules/test_disk_setup.py
index 8f9d5f40..c640d9cd 100644
--- a/tests/integration_tests/modules/test_disk_setup.py
+++ b/tests/integration_tests/modules/test_disk_setup.py
@@ -74,6 +74,9 @@ class TestDeviceAliases:
         assert sdb["children"][1]["name"] == "sdb2"
         assert sdb["children"][1]["mountpoint"] == "/mnt2"
 
+        mount_output = client.execute("mount -a")
+        assert "parse error" not in mount_output
+
 
 PARTPROBE_USERDATA = """\
 #cloud-config

For a separate PR, we probably should generally think about a bit better error-handling/warning for potentialy invalid /etc/fstab config.
A 'fragile' approach to handling this could be parsing the output of mount -a to check for parse errors. But, that message handling would fall over due to LOCALE translations.

[cjp256]

Before doing this fixup, I had not realized nofail was already in the mounted options. While _netdev seems fair for ordering without nofail, I'm not sure this change should have been required.

According to the systemd mount docs:

nofail

With nofail, this mount will be only wanted, not required, by local-fs.target or remote-fs.target. Moreover the mount unit is not ordered before these target units. This means that the boot will continue without waiting for the mount unit and regardless whether the mount point can be mounted successfully.

IIUC as this mount has nofail, perhaps there is still an issue with systemd?

[cjp256]

+        mount_output = client.execute("mount -a")
+        assert "parse error" not in mount_output

I'm wondering if we should broaden this check for any stderr or stdout? Are there cases of mount printing output to stderr/stdout for normal configuration?

[blackboxsw]

Much better and covers all cases. Thanks for the findmnt -J that will give us a concrete non-zero exit code for this test if we end up breaking defaults again in the future.

[cjp256]

No problem, and you can thank yourself for findmnt, I just saw your mention in irc :D 👍

[blackboxsw]

Before doing this fixup, I had not realized nofail was already in the mounted options. While _netdev seems fair for ordering without nofail, I'm not sure this change should have been required.

According to the systemd mount docs:

nofail

With nofail, this mount will be only wanted, not required, by local-fs.target or remote-fs.target. Moreover the mount unit is not ordered before these target units. This means that the boot will continue without waiting for the mount unit and regardless whether the mount point can be mounted successfully.

IIUC as this mount has nofail, perhaps there is still an issue with systemd?

I'll run a quick test without nofail to see if we hit any errors w/ systemd in this case and then I think we can merge this PR

[TheRealFalcon]

If it doesn't hurt anything, I'd be inclined to leave it, just because the comments in #1213 indicate the problem isn't consistent.

[blackboxsw]

If it doesn't hurt anything, I'd be inclined to leave it, just because the comments in #1213 indicate the problem isn't consistent.

Sorry must've misinterpreted my comment. I was going to check without nofail, but including _netdev to make sure the new _netdev doesn't introduce a failure that our nofail setting effectively ignores.

[blackboxsw]

Thanks again for this @cjp256 I just went through the exercise of testing this live and found a discrepancy unrelated to your PR where this integration test fails because lsblk --json has backwards incompatible key mountpoints instead of mountpoint.

If you are willing to also accept this minor patch then we can land this without me providing a followup PR

diff --git a/tests/integration_tests/modules/test_disk_setup.py b/tests/integration_tests/modules/test_disk_setup.py
index 76c132ad7..7aaba7db6 100644
--- a/tests/integration_tests/modules/test_disk_setup.py
+++ b/tests/integration_tests/modules/test_disk_setup.py
@@ -70,9 +70,13 @@ class TestDeviceAliases:
         sdb = [x for x in lsblk["blockdevices"] if x["name"] == "sdb"][0]
         assert len(sdb["children"]) == 2
         assert sdb["children"][0]["name"] == "sdb1"
-        assert sdb["children"][0]["mountpoint"] == "/mnt1"
         assert sdb["children"][1]["name"] == "sdb2"
-        assert sdb["children"][1]["mountpoint"] == "/mnt2"
+        if "mountpoint" in sdb["children"][0]:
+            assert sdb["children"][0]["mountpoint"] == "/mnt1"
+            assert sdb["children"][1]["mountpoint"] == "/mnt2"
+        else:
+            assert sdb["children"][0]["mountpoints"] == ["/mnt1"]
+            assert sdb["children"][1]["mountpoints"] == ["/mnt2"]
         result = client.execute("mount -a")
         assert result.return_code == 0
         assert result.stdout.strip() == ""
@@ -137,9 +141,13 @@ class TestPartProbeAvailability:
         sdb = [x for x in lsblk["blockdevices"] if x["name"] == "sdb"][0]
         assert len(sdb["children"]) == 2
         assert sdb["children"][0]["name"] == "sdb1"
-        assert sdb["children"][0]["mountpoint"] == "/mnt1"
         assert sdb["children"][1]["name"] == "sdb2"
-        assert sdb["children"][1]["mountpoint"] == "/mnt2"
+        if "mountpoint" in sdb["children"][0]:
+            assert sdb["children"][0]["mountpoint"] == "/mnt1"
+            assert sdb["children"][1]["mountpoint"] == "/mnt2"
+        else:
+            assert sdb["children"][0]["mountpoints"] == ["/mnt1"]
+            assert sdb["children"][1]["mountpoints"] == ["/mnt2"]
 
     # Not bionic because the LXD agent gets in the way of us
     # changing the userdata
@@ -183,7 +191,10 @@ class TestPartProbeAvailability:
         sdb = [x for x in lsblk["blockdevices"] if x["name"] == "sdb"][0]
         assert len(sdb["children"]) == 1
         assert sdb["children"][0]["name"] == "sdb1"
-        assert sdb["children"][0]["mountpoint"] == "/mnt3"
+        if "mountpoint" in sdb["children"][0]:
+            assert sdb["children"][0]["mountpoint"] == "/mnt3"
+        else:
+            assert sdb["children"][0]["mountpoints"] == ["/mnt3"]
 
     def test_disk_setup_no_partprobe(
         self, create_disk, client: IntegrationInstance

If desired, we can make this integraiton test chage a separate PR

[blackboxsw]

I'll pull this patch suggestion to a separate PR thx @cjp256

[blackboxsw]

related PR up with integration test fix for jammy #1261