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

upils · 2023-09-25T14:34:00Z

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

codecov · 2023-09-25T15:40:49Z

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 · 2023-10-18T09:10:29Z

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 · 2023-10-24T13:05:04Z

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:

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 · 2023-10-25T08:38:21Z

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!

Tests were inaccurate. The default value was always set for pointers to bool

sil2100

LGTM! Some small inline questions, but nothing blocking.

internal/statemachine/classic_states.go

sil2100 · 2023-10-25T14:26:47Z

internal/statemachine/classic_states.go

+}
+
+// fixFstab makes sure the fstab contains a valid entry for the root mount point
+func (stateMachine *StateMachine) fixFstab() error {


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

sil2100 · 2023-10-25T14:28:52Z

internal/statemachine/tests_helper_test.go

@@ -0,0 +1,65 @@
+package statemachine


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.

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.

upils requested a review from sil2100 September 26, 2023 14:00

upils marked this pull request as ready for review September 26, 2023 14:00

upils force-pushed the invalid-fstab branch 2 times, most recently from 5e20d28 to b6e4387 Compare October 17, 2023 07:38

upils marked this pull request as draft October 18, 2023 09:10

upils marked this pull request as ready for review October 19, 2023 13:05

upils added 3 commits October 20, 2023 08:19

Add test case to reproduce LP: #2031889

02f4105

Refactor populateClassicRootfsContents to make easily readable

e5c46ba

Add a FstabTruncate options to handle existing fstab

c0efc2c

upils force-pushed the invalid-fstab branch 2 times, most recently from 865319c to 31e7c39 Compare October 20, 2023 12:56

upils mentioned this pull request Oct 20, 2023

FR-5720 - Use eatmydata #157

Merged

sil2100 requested changes Oct 24, 2023

View reviewed changes

upils requested a review from sil2100 October 25, 2023 11:51

upils force-pushed the invalid-fstab branch from 152ce53 to 5add20b Compare October 25, 2023 11:56

upils added 11 commits October 25, 2023 14:41

Update changelog

38d8d96

Fix documentation-check github action

1442496

Exclude new Customization option from states generation

3b0e75b

Cover new cases for customizeFstab and populateClassicRootfsContents

c7898e5

Test empty Customization case in populateClassicRootfsContents

38c033e

Lint

8adbe30

Properly fix fstab

9ac37a5

Convert FstabTruncate to a pointer to properly handle default value

c261545

Set osMock in a single dedicated place

62928ee

Fix SetDefaults for pointer to boolean with true as default value

6d72feb

Tests were inaccurate. The default value was always set for pointers to bool

Prevent from displaying help twice

96f2ac5

Remove FstabTruncate option

381615b

upils force-pushed the invalid-fstab branch from 5add20b to 381615b Compare October 25, 2023 12:42

sil2100 approved these changes Oct 25, 2023

View reviewed changes

Use correct flags to open fstab file

9a382e9

sil2100 approved these changes Oct 26, 2023

View reviewed changes

sil2100 merged commit 76189f5 into main Oct 26, 2023
9 of 10 checks passed

sil2100 deleted the invalid-fstab branch October 26, 2023 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

upils commented Sep 25, 2023

codecov bot commented Sep 25, 2023 •

edited

Loading

upils commented Oct 18, 2023

sil2100 left a comment

upils commented Oct 24, 2023

sil2100 commented Oct 25, 2023

sil2100 left a comment

sil2100 Oct 25, 2023

sil2100 Oct 25, 2023

upils Oct 25, 2023

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

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

Conversation

upils commented Sep 25, 2023

codecov bot commented Sep 25, 2023 • edited Loading

Codecov Report

upils commented Oct 18, 2023

sil2100 left a comment

Choose a reason for hiding this comment

upils commented Oct 24, 2023

sil2100 commented Oct 25, 2023

sil2100 left a comment

Choose a reason for hiding this comment

sil2100 Oct 25, 2023

Choose a reason for hiding this comment

sil2100 Oct 25, 2023

Choose a reason for hiding this comment

upils Oct 25, 2023

Choose a reason for hiding this comment

codecov bot commented Sep 25, 2023 •

edited

Loading