Better deal with errors when creating machine-id #14135

boucman · 2019-11-24T13:52:32Z

This is a followup to #14131

With this commits applied

booting with a RO /etc and machine-id being a symlink to a (RW) /var will work correctly
- if the symlink is dangling, it will be correctly created and firstboot logic will be applied
- i the symling exist but is empty, it will be filled, but no firstboot logic applied
booting with a RO /etc that becomes RW later works better
instead of not creating a machine-id and booting anyway, systemd will complain but create one in
/run and commit it once /etc has becomed RW. It will also respect firstboot logic
booting with a RO /etc and machine-id being a symlink to /var (which is mounted late by systemd)
same as the previous case. systemd complains, but will create a machine-id for subsequent boots

I do not consider this a complete fix, mainly because I am not sure what happens exactly when
you boot without a machine-id, but I could not find anything wrong.

For the RO => RW, the first boot is without machine-id but the following ones are fine
For the symlink to /var, case, all boots are without machine-id, but the machine-id reported stays the
same (its the one saved in /var at first boot)

boucman · 2019-11-24T13:58:05Z

This might not be enough for the late /var case... the firstboot logic is always applied. and I'm not sure
how to fix that. The information "is this the first boot" has to be stored in a RW area that is available early at boot (at least as RO) so I can't see how to work around that...

ideas welcomed...

poettering · 2019-11-25T14:25:54Z

units/systemd-firstboot.service.in

@@ -14,7 +14,6 @@ DefaultDependencies=no
 Conflicts=shutdown.target
 After=systemd-remount-fs.service
 Before=systemd-sysusers.service sysinit.target shutdown.target
-ConditionPathIsReadWrite=/etc


hmm, not sure i grok this. if /etc is read-only the tool is not going to be able to make any of its changes, no?

I mean, we generally create files atomically, i.e. via O_TMPFILE or random file names which are then moved into place, which means we need write access to /etc as a whole, a specific file underneath it does not suffice

well if /etc/machine-id is a link to /var/machine-id, /etc can be read-only but we can still write to the machine-id file...

If /etc is read-only but /var is RW, this condition prevents firstboot from working correctly, for no real reason that I can see...

but we generally create our files in /etc in an atomic fashion: i.e. create and rename. This means you cannot symlink stuff from /etc to elsewhere reasonably, as that would replace the symlink not the destination.

/etc/machine-id is not initialized by systemd-firstboot.service btw, but by PID 1, much earlier.

poettering · 2019-11-25T14:30:47Z

src/core/machine-id-setup.c

+                                                        "Booting up is supported only when:\n"
+                                                        "1) /etc/machine-id exists and is populated.\n"
+                                                        "2) /etc/machine-id exists and is empty.\n"
+                                                        "3) /etc/machine-id is missing and /etc is writable.\n");


not sure i grok this, why make this non-fatal?

Do you intend here to allow the system to boot up with /etc/machine-id not set up? urks... I'd always say we should insist in /etc/machine-id being readable at any time any process that is not PID 1 runs...

i.e. that means insisting that /etc/machine-id exists or can be created by us. And if it exists as symlink but points elsewhere, then operate on that. i.e. issue chase_symlinks(CHASE_NONEXISTENT) to determine where the symlink (or chain of it) points, and then create/overmount things there.

I'm not making it non-fatal, i'm just making sure that /run/machine-id is created whenever we can't read a correct value from /etc/machine-id.

Currently, the boot continues after this message, so I'm not changing anything really...

The problem with chasing symlink, is that it might points to a /var that is not mounted yet. In that case we can't create anything at the target location (since it's read-only) and we can't bind-mount on top of /etc/machine-id (since we can't mount on top of a dangling symlink) so we are screwed anyway. I'm opened to other ideas on how to deal with that case..

We could "completely break" things here and refuse to boot entirely, or (what I am trying to do) improve things and attempt to commit the machine-id later anyway. So it's setup for the next boot.

Again, we currently boot after that scary warning, so the only change I do is making the commit of machine-id work correctly if it's possible when the commit service is invoked.

(arguably, i'm a bit in the dark here about what the "correct thing" to do is. I want to support symlink to /var, but having /var mounted be mandatory before systemd is started is good enough for my use-case. having it mounted later is just a bonus)

Hmm, then change the message maybe? Saying "System cannot boot" and then booting is not very consitent.

yes, i'll rework it even more. Adding a new correct case (symlink)

boucman · 2019-12-04T22:43:40Z

Is there anything I can do to move this forward ?

the first patch seems trivial enough to me and solves a real lifer problem
the second patch doesn't solves the problem of the no-machine-id boot, but makes things strictly better and allow a first-boot-in-factory type of setup to work well enough for the second boot to be correct...

I'm not really sure how to improve stuff, but I will have some time in the no-too-distant future to work a bit more on that, so guidance would be welcomed...

boucman · 2019-12-16T13:58:18Z

ping ?

boucman · 2020-02-05T09:08:44Z

Can I have a go/no-go on this patch ?
We will probably have to keep that patch downstream since being able to have a locally-generated machine-id on a machine that is booted read-only is a pretty important use-case for us.

keszybz

I think the approach looks good, but I think it should be combined with a change to sd_id128_get_machine() to query /run/machine-id too, so the we don't fail to query it during the time when it's not present in /etc.

Needs a rebase also.

keszybz · 2020-02-06T10:15:07Z

src/core/machine-id-setup.c

+                                                        "Booting up is supported only when:\n"
+                                                        "1) /etc/machine-id exists and is populated.\n"
+                                                        "2) /etc/machine-id exists and is empty.\n"
+                                                        "3) /etc/machine-id is missing and /etc is writable.\n");


Hmm, then change the message maybe? Saying "System cannot boot" and then booting is not very consitent.

keszybz · 2020-02-06T10:16:29Z

src/core/machine-id-setup.c

-                (void) unlink_noerrno(run_machine_id);
+                /* We failed to mount over /etc/machine-id, but let's leave /run/machine-id
+                 * so we can commit it later if /etc/machine-id becomes writable
+                 * (this will be done by systemd-machine-id-commit.service) */
                return log_error_errno(errno, "Failed to mount %s: %m", etc_machine_id);
        }


got tricked by the multi-line comment, will do.

keszybz · 2020-02-06T10:18:22Z

src/core/machine-id-setup.c

                return log_error_errno(r, "Failed to determine whether %s is a mount point: %m", etc_machine_id);
-        if (r == 0) {
-                log_debug("%s is not a mount point. Nothing to do.", etc_machine_id);
+        if (r == 0 || ( r < 0 && errno == ENOENT) ) {


Instead of this ugly repeated condition, maybe reverse the order of the branches

if (r > 0) { } else { }

?

keszybz · 2020-02-06T10:18:57Z

src/core/machine-id-setup.c

-        }
+        } else {
+                /* Replaces a tmpfs bind mount of /etc/machine-id by a proper file, atomically. For this, the umount is removed
+                 * in a mount namespace, a new file is created at the right place. Afterwards the mount is also removed in the


", a new" → " and a new"

boucman · 2020-02-09T08:12:31Z

Ok, pushed a new version with all requsted change.

Build failure seems unrelated to me (a test related to lid-switch)

PTAL

boucman · 2020-03-31T11:42:18Z

Ping ? anything let for me to do ?

poettering · 2020-05-13T17:03:07Z

units/systemd-firstboot.service

@@ -14,7 +14,6 @@ DefaultDependencies=no
 Conflicts=shutdown.target
 After=systemd-remount-fs.service
 Before=systemd-sysusers.service sysinit.target shutdown.target
-ConditionPathIsReadWrite=/etc


still not grokking this. at the moment when systemd-firstboot runs /etc must be writable because we create files in /etc via atomic create/rename. the condition check runs immediately before the service binary is invoked, hence it should be OK to have

poettering · 2020-05-13T17:03:37Z

units/systemd-firstboot.service.in

@@ -14,7 +14,6 @@ DefaultDependencies=no
 Conflicts=shutdown.target
 After=systemd-remount-fs.service
 Before=systemd-sysusers.service sysinit.target shutdown.target
-ConditionPathIsReadWrite=/etc


/etc/machine-id is not initialized by systemd-firstboot.service btw, but by PID 1, much earlier.

boucman · 2020-05-23T13:10:28Z

Ok, it took me some time to understand the philosophical misunderstanding about allowing systemd-firstboot on read-only /etc

my point of view was : we can have first-boots with read-only /etc (but rw /etc/machine-id) and we should run systemd-firstboot when we are in that case

@poettering's logic was : why run systemd-firstboot, since /etc is read-only, we can't do the job we are supposed to do anyway...

Now that I get that, I agree that it's a bad idea, and dropped the patch.

AFAIU, there is no more remarks on the remaining patch, but if I missed something please point it out and i'll respin

Thx

keszybz · 2020-07-24T09:33:43Z

Oops, this got lost again. @boucman could you do a rebase please?

Previously, systemd would error out when anything went wrong when positionning /etc/machine-id. An error would be displayed and boot would go on without a machine-id Now, unless /etc/machine-id is correct, systemd will create a /run/machine-id, even if it can't be bind mount it over /etc. systemd-machine-id-commit will detect if /run/machine-id exists instead of relying on the fact that /etc/machine-id is a mountpoint.

boucman · 2020-07-24T16:57:31Z

And rebased...

keszybz · 2020-08-03T13:20:30Z

src/core/machine-id-setup.c


        r = path_is_mount_point(etc_machine_id, NULL, 0);
-        if (r < 0)
+        if (r < 0 && errno != ENOENT)


Where is this errno coming from? if (r < 0 && r != -ENOENT) ?

keszybz · 2020-08-03T13:21:36Z

src/core/machine-id-setup.c

-        return 0;
+                return 0;
+        } else {
+                /* File does not exist or is not a mountpoint


Periods at ends of sentences please.

keszybz · 2020-08-03T13:30:45Z

src/core/machine-id-setup.c

+                                /* /etc/machine-id already contains a valid machine-id, don't do anything */
+                                return 0;
+                }
+                if (fd < 0 && errno != ENOENT)


Same issue here: errno can only be used directly after a glibc operation that is documented to return errno. Here, the call to id128_read_fd() "destroys" the errno value, in the sense that it might be changed.

poettering

hm, i am not sure i like this patch. i.e. we so far gave the guarantee that /etc/machine-id is readable and stable for all userspace invoked by host PID 1, regardless when it is run. This patch does way with that, and I am not sure i grok why this was desirable?

I mean, i think the following scenarios make sense to support:

/etc/machine-id readable, exists and is populated
/etc/machine-id does not exist and /etc/ writable
/etc/machine-id read-only, exists, and is empty

But as I understand this patch you want to support on more scenario:

/etc/machine-id does not exist and /etc/ is read-only.

But I don't grok why that is a scenario worth supporting though. If you have a read-only /etc, you might as well create /etc/machine-id empty in it, no?

i.e. what is the precise setup you are looking to support?

poettering · 2020-08-18T15:46:03Z

src/core/machine-id-setup.c

-                                                  "1) /etc/machine-id exists and is populated.\n"
-                                                  "2) /etc/machine-id exists and is empty.\n"
-                                                  "3) /etc/machine-id is missing and /etc is writable.\n");
+                                        log_error_errno(errno,


if we proceed here we should downgrade this to log_warning

poettering · 2020-08-18T15:46:11Z

src/core/machine-id-setup.c

                                else
-                                        return log_error_errno(errno, "Cannot open %s: %m", etc_machine_id);
+                                        log_error_errno(errno, "Cannot find %s: %m", etc_machine_id);


poettering · 2020-08-18T15:47:34Z

src/core/machine-id-setup.c

+                 *
+                 * We read the existing machine-id  in /etc */
+                fd = open(etc_machine_id, O_RDONLY|O_CLOEXEC|O_NOCTTY);
+                if (fd > 0) {


fds can be zero (unlikely, but still

boucman · 2020-08-18T19:47:03Z

I'm away from a real computer right now, so I'll rework the patch later, but i'll answer the use-case question.

In embedded system we usually have a "gold" image that is flashed on every device. Each device needs to have a different, persistant machine-id setup.

Now it is also very common in embedded systems to boot with a read-only rootfs that becomes read-write later.

So the scenario I want to have is that /root is early-read-only, but becomes read-write later.

I want to have a stable machine-id created and used early, and written back to /etc/machine-id once the root-fs has been remounted RW.

poettering · 2020-08-19T08:11:25Z

@boucman but why isn't your case covered by my case 3? i.e. have a read-only /etc initially but an /etc/machine-id file in there that is empty. systemd then overmounts that with a random machine id and then write it to disk replacing the mount as soon as the fs becomes writable.

boucman · 2020-08-19T09:13:43Z

The current behaviour is better documented in #14131
In particular, if /etc/machine-id exists but is empty, first boot logic is not run (and i'm not sure what ConditionFirstBoot will be)

I'm away from any real developement env so I can't re-test any specific behaviour. So I can't be more detailed than that right now... sorry

poettering · 2020-08-19T10:47:56Z

The current behaviour is better documented in #14131
In particular, if /etc/machine-id exists but is empty, first boot logic is not run (and i'm not sure what ConditionFirstBoot will be)

But we should probably change semantics so that empty /etc/machine-id qualifies for first boot too.

That said, one of the key bits of first boot is to apply presets, which we can't do if /etc is readonly...

boucman · 2020-08-19T13:07:10Z

I think one of the problem is that we test "/etc is read-only" early (before remounting the rootfs) but run first-boot late. and the readability of /etc can change in the mean time

that's what the changes on systemd-machine-id-commit.service are for.

poettering · 2020-08-19T14:24:07Z

I think one of the problem is that we test "/etc is read-only" early (before remounting the rootfs) but run first-boot late. and the readability of /etc can change in the mean time

What do you mean "run first-boot late"?

boucman · 2020-08-19T14:35:15Z

I mean first-boot is ordered after rootfs is remounted RW, but ConditionPathIsReadWrite is checked before that.

poettering · 2020-08-19T15:52:54Z

still not following. what do you mean by first-boot?

poettering · 2020-09-03T22:23:43Z

So #16939 would fix the issue this is about too, right? I like the approach of #16939 a lot I must say, hence maybe a replacement for this PR?

boucman · 2020-09-08T06:00:07Z

see my answer in #14131 (comment) and the problem with read/only /etc for the embedded use-case

poettering · 2020-09-10T18:51:28Z

Let's drop this from the milestone. it's not clear to me that this is the way forward, I find the usecase a bit too unclear and I don't think we should allow booting without /etc/machine-id or a changing /etc/machine-id.

I am pretty sure #16939 addresses some of the issues here, but even if not this can be solved outside of systemd too if you really really want to mount the machine-id from /var: pre-mounted /var in the initrd, and bind mount things there, so that systemd sees /etc/machine-id pre-initialized and doesn't do anything. Alternatively, we could generalize the $container_uuid env var and allow such an env var to be set in the initrd for the host PID 1: $container_uuid when set is currently used to initialized /etc/machine-id instead of generating it randomly. This would allow your system to acquire the uuid from some source in the initrd and then pass it to PID 1, so that PID 1 would not randomize things but just use what you supply, even if it overmounts /etc/machine-id transiently.

I think this would be a much nicer approach since it doesn't break with the rule that /etc/machine-id is always valid, accessible and stable once host PID 1 is invoked.

poettering · 2022-03-18T12:11:03Z

So, what's the state of this one? Is there any usecase we still don't cover nicely that is worth covering? Or should we just close this?

boucman · 2022-03-20T18:20:58Z

Honestly, I havn't touched this patch for quite some time and I think this area has moved quite a bit.
There might be a case which is not covered but I would need to retest everything.

the complex case was read-only rootfs, RW /var (wmhich might not be mounted at systemd startup time) and /etc/machine-id being a (possibly dangling) symlink to /var

I don't really have time to work on this before a couple of weeks at best, and I still don't have a clear idea if systemd considers this a proper use-case or if we should just drop the thing.

Cheers
Jérémy

poettering · 2022-03-21T15:56:48Z

the complex case was read-only rootfs, RW /var (wmhich might not be mounted at systemd startup time) and /etc/machine-id being a (possibly dangling) symlink to /var

Reading the machine ID off a file system not mounted when we transition from initrd into host (or if you operate initrd-less: at the moment of first PID 1 invocation) is simply not supported and I doubt we should bother.

i.e. the guarantee that the machine ID remains valid and stable from the time the host PID 1 initializes until it shuts down is very important to us. People should just be able to rely on the machine ID to be readable, and I am not convinced we should weaken that model.

If people want /etc/machine-id to be backed by storage that is not the root fs, then they should pre-mount that other storage in the initrd really.

boucman · 2022-03-22T20:20:37Z

Ok, so this is unsuported, let's just close this issue.

that was the kind of answer I was waiting for...

poettering reviewed Nov 25, 2019

View reviewed changes

anitazha added firstboot pid1 labels Dec 6, 2019

boucman mentioned this pull request Jan 29, 2020

Allow to configure the firstboot detection logic #14059

Closed

keszybz requested changes Feb 6, 2020

View reviewed changes

keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Feb 6, 2020

boucman force-pushed the machineid_firstboot_fixes branch from d86960d to f999986 Compare February 8, 2020 13:31

yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Feb 9, 2020

poettering reviewed May 13, 2020

View reviewed changes

boucman force-pushed the machineid_firstboot_fixes branch from f999986 to 9ada865 Compare May 23, 2020 13:07

keszybz added the needs-rebase label Jul 24, 2020

boucman force-pushed the machineid_firstboot_fixes branch from 9ada865 to 3c6ce1b Compare July 24, 2020 16:56

keszybz added this to the v247 milestone Jul 25, 2020

keszybz removed the needs-rebase label Aug 3, 2020

keszybz requested changes Aug 3, 2020

View reviewed changes

keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Aug 3, 2020

poettering requested changes Aug 18, 2020

View reviewed changes

poettering removed this from the v247 milestone Sep 10, 2020

bluca added the needs-rebase label Dec 14, 2020

Base automatically changed from master to main January 21, 2021 11:54

boucman closed this Mar 22, 2022

Better deal with errors when creating machine-id #14135

Better deal with errors when creating machine-id #14135

Conversation

boucman commented Nov 24, 2019

boucman commented Nov 24, 2019

Choose a reason for hiding this comment

boucman Nov 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boucman commented Dec 4, 2019

boucman commented Dec 16, 2019

boucman commented Feb 5, 2020

keszybz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boucman commented Feb 9, 2020

boucman commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boucman commented May 23, 2020

keszybz commented Jul 24, 2020

boucman commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boucman commented Aug 18, 2020

poettering commented Aug 19, 2020

boucman commented Aug 19, 2020

poettering commented Aug 19, 2020 • edited

boucman commented Aug 19, 2020

poettering commented Aug 19, 2020

boucman commented Aug 19, 2020

poettering commented Aug 19, 2020

poettering commented Sep 3, 2020

boucman commented Sep 8, 2020

poettering commented Sep 10, 2020

poettering commented Mar 18, 2022

boucman commented Mar 20, 2022

poettering commented Mar 21, 2022

boucman commented Mar 22, 2022

boucman Nov 25, 2019 •

edited

poettering commented Aug 19, 2020 •

edited