Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

machine-id-setup: acquire machine ID from /run/machine-id if possible #32915

Merged
merged 2 commits into from
May 19, 2024

Conversation

yuwata
Copy link
Member

@yuwata yuwata commented May 19, 2024

If machine ID is previously stored at /run/machine-id, then let's reuse
it. This is important on switching root and /etc/machine-id was previously
a mount point.

Fixes #32908.

@yuwata yuwata added the pid1 label May 19, 2024
@github-actions github-actions bot added util-lib please-review PR is ready for (re-)review by a maintainer labels May 19, 2024
Copy link

github-actions bot commented May 19, 2024

Note

We had successfully released a new major release. We are no longer in a development freeze phase.
We will try our best to get back to your PR as soon as possible. Thank you for your patience.

Copy link
Member

@bluca bluca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah noticed the same thing on soft-reboot some time ago

@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels May 19, 2024
@bluca bluca added this to the v256 milestone May 19, 2024
@YHNdnzj YHNdnzj added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels May 19, 2024
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels May 19, 2024
@bluca bluca added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels May 19, 2024
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels May 19, 2024
@YHNdnzj YHNdnzj added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels May 19, 2024
If machine ID is previously stored at /run/machine-id, then let's reuse
it. This is important on switching root and /etc/machine-id was previously
a mount point.

Fixes systemd#32908.
@bluca bluca merged commit eb0c2da into systemd:main May 19, 2024
44 of 49 checks passed
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label May 19, 2024
@yuwata yuwata deleted the machine-id-setup branch May 19, 2024 22:43
@@ -61,7 +61,7 @@ static int generate_machine_id(const char *root, sd_id128_t *ret) {
return 0;
}

if (isempty(root) && running_in_chroot() <= 0) {
if (empty_or_root(root) && running_in_chroot() <= 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this really desirable? in many cases we allow --root=/ as a mechanism for forcing an "offline" mode, while still operating on the root dir. if we do the getenv_for_pid() thing below I'd claim this is very much an "online" operation, and hence --root=/ should really disable that.

/* First, try reading the D-Bus machine id, unless it is a symlink */
/* First, try reading the machine ID from /run/machine-id, which may not be mounted on
* /etc/machine-id yet. This is important on switching root, Otherwise, machine ID may be changed
* after the transition. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, when we run in the initrd, then we write the machine ID to /etc/machine-id, and don't bother with /run/machine-id? hence what does this actually change here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about the initrd case, however I hit this on softreboot when /etc/machined is an empty read-only file, so we mount over it, but we lose this mount on softreboot and you get a different machine-id every time

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so i guess doing what you suggest here is OK, but the comment is misleading, since it references switch root, where this shouldn't really matter, as mentioned. I do see how it would matter for soft-reboot with immutable rootfs however.

/* First, try reading the machine ID from /run/machine-id, which may not be mounted on
* /etc/machine-id yet. This is important on switching root, Otherwise, machine ID may be changed
* after the transition. */
if (empty_or_root(root) && running_in_chroot() <= 0 &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too, I think we should allow forcing an "offline" mode via --root=/ where we do not do things based on other system/execution properties.

@poettering
Copy link
Member

This one confuses me. the commit msgs suggest this had an effect on regular initrd→host transitions, but i don't see how that could be, because regular initrds should not be initializing /run/machine-id, hence this should not have any effect.

What am I missing?

I mean, I see benefit in allowing the initrd's machine ID to propagate into the host if the host's is not set yet. But I doubt /run/machine-id is the way to go with that, because that seems "too strong" to me, as it it should be left to the target system to decide whether it wants to import the initrds machine id or use something else.

There's a TODO list item since a long time somwhere to propagate the initrd's into the host via an env var or via a system cred. I think in particular the latter would make a ton of sense (i.e. at a reasonable place stupidly copy /etc/machine-id to /run/credentials/@initrd/system.machine_id if that doesn't exist yet).

@yuwata
Copy link
Member Author

yuwata commented May 21, 2024

This is mostly for soft-reboot, rather than initrd -> host transition. Though, the same can be applied if /run/machine-id is used in initrd. But I do not think it happens in general.
Sorry, I confused you because of the title of the issue #32908. But the logs are from the TEST-82-SOFTREBOOT, and the machine-id change seems to happen in soft-reboot.
I will update the comment in the code.

About 'offline' vs 'online' or empty_or_root() vs isempty() thing. I could not find anything about that in the man page. Maybe, we should mention that?

yuwata added a commit to yuwata/systemd that referenced this pull request May 24, 2024
This effectively reverts ba540e9.

systemd#32915 (comment)
> In many cases we allow --root=/ as a mechanism for forcing an "offline" mode,
> while still operating on the root dir. if we do the getenv_for_pid() thing
> below I'd claim this is very much an "online" operation, and hence --root=/
> should really disable that.
bluca added a commit that referenced this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

mkosi: machine ID in initrd and host are different??
4 participants