FR-5404 - The 'fstab' of 'customization' destroy original fstab content

[upils]

Fixes https://bugs.launchpad.net/ubuntu-image/+bug/2031889

[codecov]

Codecov Report

Merging #143 (381615b) into main (d5576cb) will decrease coverage by 0.17%.
Report is 11 commits behind head on main.
The diff coverage is 87.34%.

❗ Current head 381615b differs from pull request most recent head 9a382e9. Consider uploading reports for the commit 9a382e9 to get more accurate results

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   90.68%   90.52%   -0.17%     
==========================================
  Files          13       13              
  Lines        2545     2607      +62     
==========================================
+ Hits         2308     2360      +52     
- Misses        210      215       +5     
- Partials       27       32       +5     

Files	Coverage Δ
cmd/ubuntu-image/main.go	92.17% <100.00%> (+0.20%)	⬆️
internal/helper/helper.go	36.33% <100.00%> (+0.22%)	⬆️
internal/statemachine/helper.go	98.65% <ø> (ø)
internal/statemachine/classic_states.go	96.42% <86.30%> (-0.95%)	⬇️

[upils]

I just found https://bugs.launchpad.net/ubuntu-image/+bug/1955019 so I will rework this to also solve this bug.

[sil2100]

Didn't take a look at the code in detail just yet, but I'm already thinking about the concept of fstab customization itself.

I think we should only support the case of overwriting the image fstab via customizations. I'm trying to think of a case where we'd need to 'modify' an fstab, and it feels very hm, hacky. It's not 100% possible of course, but we try for the image definition file to be as deterministic as possible with the options it provides (minus the manual customizations). By allowing appending fstab entries, we willingly remove some of the determinism here. This can lead to duplicate entries in fstab or other weird behavior you might not see instantly.

Did we have a request from someone to actually support appending entries to fstab instead of just writing it from scratch? The fstab customization was always supposed to be a bit of a 'convenience' function. In my mind things like this should be done by customization packages, but this way it's just faster to iterate on it.

Were there some cases in livecd-rootfs that require this? I'd like to understand a bit better if we really need this, since otherwise I'd just stick with overwriting in all cases.

[upils]

Based on the bug wording I thought that at least the OEM enablement team considered the fstab customization was supposed to append to the existing fstab, but this is not obvious now that I read it again. The idea was to make this feature flexible and make sure the behavior was clear.

I have looked at the livecd-rootfs code (especially hooks) and also found 2 bugs related to fstab management:

• https://bugs.launchpad.net/ubuntu/+source/livecd-rootfs/+bug/1902103
• https://bugs.launchpad.net/cloud-images/+bug/1881006

I understand that the fstab is:

• sometimes tweaked for some specific images (raspi 2, cpc)
• sometimes completely rewritten
• sometimes even ignored (omitted from squashfs)

I also see cloud-init enables users setting custom fstab entries.

I fear that if we only allow overriding completely the existing fstab (current behavior prior to this PR) we won't be able to satisfy the need of some team when they try to use ubuntu-image. Based on the spec mentioning the fstab customization in image definition, we never properly defined what we wanted to do with this.

Based on our various image definition in ubuntu-images it looks like the following config became more or less a default in almost every conf:

  fstab:
    -
      label: "writable"
      mountpoint: "/"
      filesystem-type: "ext4"
      dump: false
      fsck-order: 1
    -
      label: "system-boot"
      mountpoint: "/boot/firmware"
      filesystem-type: "vfat"
      mount-options: "defaults"
      dump: false
      fsck-order: 1

I am not 100% sure we should add the "FstabTruncate" feature but I think we should decide what is the responsibility of ubuntu-image regarding the resulting fstab. I see several possibilities:

• just make sure we have a "valid" fstab that will enable mounting the root. This part is done (and fixed in this PR) but could be improved (to remove the use of the writable label).
• enable adding fstab entries while keeping the existing fstab (current behavior). But it should be super clear in the doc that the current fstab will be destroyed.
• enable giving full control of the fstab content (override or append). This is what FstabTruncate option try to do.

Unfortunately even the last solution won't cover every customization needs that is currently covered by hooks, customization packages or cloud-init configuration because:

• we cannot define "complex" logic to deal with the fstab content in the image definition
• at build time we do not know some unique properties of the machine the image will be installed on (disk UUIDs, etc.).

So since we will never be able to fully manage the fstab in one place in oneshot, I suggest to:

• limit ubuntu-image responsibility to generate either the default simple fstab (fixed by this PR) and enable user defining a complete fstab (overriding completely the existing fstab)
• redirect users wanting to do more exotic things with the fstab to a customization package or cloud-init conf.

[sil2100]

I think your suggestion is sane. I think ubuntu-image modifying the fstab to allow boot (aka. the writable bits) + allowing setting (and overwriting) the fstab should be the immediate way forward. If we get additional requests from users about more fine grained requirements, we can revisit this when working on imagecraft.
Please proceed!

[sil2100]

LGTM! Some small inline questions, but nothing blocking.

[sil2100]

+1 on having this be a separate state, thanks!

[sil2100]

I'm not 100% into go, but is this file name convention correct? I always thought _test.go files were for the actual tests, not test helpers.

[upils]

Files with _test suffix are excluded from regular package builds but will be included when the "go test" command is run. Otherwise you can put whatever functions you want in these files. Here I wanted to signal that what is in this file is used in the various tests across the package, and the tests only. I wanted to avoid the confusion with helper_test.go which is (by convention) a file that should contains functions to test what is in helper.go (and not contains test helpers).

If ubuntu-image had been destined to evolve more, I would have moved this to a dedicated "test" pkg at some point I think.