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

(RHEL-17394) fstab-generator: Chase symlinks where possible #150

Merged
merged 7 commits into from Feb 7, 2024

Conversation

brozs
Copy link
Collaborator

@brozs brozs commented Nov 27, 2023

Backport of three fstab-generator commits:

commit 634735b ("fstab-generator: Chase symlinks where possible (#6293)" -- reverted in PR#149)
commit 98eda38 ("call chase_symlinks without the /sysroot prefix (#6411)" -- fixing the issue that required the revert)
commit f1a2c75 ("fstab-generator: downgrade message when we can't canonicalize fstab entries (#8281)" -- adjust log message priority)

Resolves: RHEL-17394

cgwalters and others added 3 commits November 27, 2023 12:05
This has a long history; see see 5261ba9
which originally introduced the behavior.  Unfortunately that commit
doesn't include any rationale, but IIRC the basic issue is that
systemd wants to model the real mount state as units, and symlinks
make canonicalization much more difficult.

At the same time, on a RHEL6 system (upstart), one can make e.g. `/home` a
symlink, and things work as well as they always did; but one doesn't have
access to the sophistication of mount units (dependencies, introspection, etc.)
Supporting symlinks here will hence make it easier for people to do upgrades to
RHEL7 and beyond.

The `/home` as symlink case also appears prominently for OSTree; see
https://ostree.readthedocs.io/en/latest/manual/adapting-existing/

Further work has landed in the nspawn case for this; see e.g.
d944dc9

A basic limitation with doing this in the fstab generator (and that I hit while
doing some testing) is that we obviously can't chase symlinks into mounts,
since the generator runs early before mounts. Or at least - doing so would
require multiple passes over the fstab data (as well as looking at existing
mount units), and potentially doing multi-phase generation. I'm not sure it's
worth doing that without a real world use case. For now, this will fix at least
the OSTree + `/home` <https://bugzilla.redhat.com/show_bug.cgi?id=1382873> case
mentioned above, and in general anyone who for whatever reason has symlinks in
their `/etc/fstab`.

(cherry picked from commit 634735b)

Resolves: RHEL-17394
In case fstab-generator is called in the initrd, chase_symlinks()
returns with a canonical path "/sysroot/sysroot/<mountpoint>", if the
"/sysroot" prefix is present in the path.

This patch skips the "/sysroot" prefix for the chase_symlinks() call,
because "/sysroot" is already the root directory and chase_symlinks()
prepends the root directory in the canonical path returned.

(cherry picked from commit 98eda38)

Related: RHEL-17394
…ntries (#8281)

Let's make this LOG_DEBUG, as this didn't used to be an issue, and
shouldn't really be still.

Replaces: #8132
(cherry picked from commit f1a2c75)

Related: RHEL-17394
mrc0mmand and others added 3 commits November 27, 2023 20:26
Based on: 0307ea4
Related: RHEL-17394

rhel-only
Otherwise --template= simply doesn't work:

$ ./systemd-escape --template=systemd-mkfs@.service --path /dev/test9
is_valid: 0
is_template: 1
Template name systemd-mkfs@.service is not valid.

Related: RHEL-17394

rhel-only
According to bootup(7) and the behavior when /usr is specified in /etc/fstab, the /sysroot/usr mount should be before initrd-fs.target, not before initrd-root-fs.target.

Related: RHEL-17394

(cherry picked from commit 104bc12)
@mrc0mmand
Copy link
Member

mrc0mmand commented Nov 27, 2023

I added a couple of commits that backport the "fstab-generator" part of TEST-81-GENERATORS from upstream. SInce its behavior differs a lot from the current upstream, the code is littered with comments about what either doesn't work at all or works in a different way in RHEL 7. Also, the test apparently discovered one real bug, so I backported the fix as well.

And for completeness, the test fails as expected if 634735b is cherry-picked without the follow-up.

@mrc0mmand mrc0mmand force-pushed the rhel-7.9 branch 2 times, most recently from c83846d to e16e0dc Compare November 27, 2023 20:40
Some fstab-generator features are not present on RHEL 7 or they behave
differently - in such case there's an inline comment explaining what's
different with a reference to an upstream commit that introduced the
changed behavior.

Also, RHEL 7 systemd doesn't allow overriding (/sysroot)/etc/fstab or
/proc/cmdline, so instead of backporting another bunch of potential
risky commits, let's temporarily bind-mount a modified copy of necessary
files in place of the expected ones. One exception is
$SYSTEMD_IN_INITRD, since systemd checks if the mount for / is a tmpfs,
which is a pain to mock, but the patch for that is, thankfully, pretty
small.

Related: RHEL-17394

rhel-only
Copy link

github-actions bot commented Jan 16, 2024

Commit validation

Tracker - RHEL-17394

The following commits meet all requirements

commit upstream
b47f82a - fstab-generator: Chase symlinks where possible (#6293) systemd/systemd@634735b
8eb5534 - call chase_symlinks without the /sysroot prefix (#6411) systemd/systemd@98eda38
83cb24b - fstab-generator: downgrade message when we can't canonicalize fstab en… systemd/systemd@f1a2c75
2d97c61 - Add $SYSTEMD_IN_INITRD=yes|no override for debugging rhel-only
caa0c0a - escape: call unit_name_is_valid() with correct flags rhel-only
656de45 - fstab-generator: fix ordering of /sysroot/usr mount systemd/systemd@104bc12
ebf05d8 - test: backport TEST-81-GENERATORS (fstab-generator only) rhel-only

Tracker validation

Success

🟢 Tracker RHEL-17394 has set desired product: rhel-7.9.z
🟢 Tracker RHEL-17394 has set desired component: systemd
🟢 Tracker RHEL-17394 has been approved


Pull Request validation

Success

🟢 CI - All checks have passed
🟢 Review - Reviewed by a member
🟢 Approval - Changes were approved


Auto Merge

Failed

🔴 Pull Request has unsupported target branch rhel-7.9, expected branches are: 'main,master'

Success

🟢 Pull Request is not marked as draft and it's not blocked by dont-merge label
🟢 Pull Request meet requirements, title has correct form
🟢 Pull Request meet requirements, mergeable is true
🟢 Pull Request meet requirements, mergeable_state is clean

@brozs
Copy link
Collaborator Author

brozs commented Feb 2, 2024

Would this be a good time to revisit this PR?

@jamacku
Copy link
Member

jamacku commented Feb 2, 2024

@msekletar and @dtardon, could you please have a look? Thanks

@jamacku jamacku requested a review from dtardon February 2, 2024 13:03
@dtardon
Copy link
Member

dtardon commented Feb 7, 2024

@mrc0mmand Are you sure the fstab-generator test actually tests anything? AFAICS it requires systemd/systemd@ed4ad48, systemd/systemd@99e3d47, systemd/systemd@0307ea4, and systemd/systemd@9a135c0.

@mrc0mmand
Copy link
Member

@mrc0mmand Are you sure the fstab-generator test actually tests anything? AFAICS it requires systemd/systemd@ed4ad48, systemd/systemd@99e3d47, systemd/systemd@0307ea4, and systemd/systemd@9a135c0.

Yes, I'm very sure, see the code. The $SYSTEMD_IN_INITRD change is backported, the rest is "synthesized" - https://github.com/redhat-plumbers/systemd-rhel7/pull/150/files#diff-4b8d2baec8d6d69f280e0dffcdcfcc8cca8ad39fda2a5b027b97f788a07799a8R70-R84

@dtardon
Copy link
Member

dtardon commented Feb 7, 2024

Yes, I'm very sure, see the code. The $SYSTEMD_IN_INITRD change is backported, the rest is "synthesized" - https://github.com/redhat-plumbers/systemd-rhel7/pull/150/files#diff-4b8d2baec8d6d69f280e0dffcdcfcc8cca8ad39fda2a5b027b97f788a07799a8R70-R84

All right, I missed that.

Copy link
Member

@dtardon dtardon left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added pr/needs-manual-merge and removed pr/needs-review Formerly needs-review labels Feb 7, 2024
@jamacku jamacku merged commit ef95891 into redhat-plumbers:rhel-7.9 Feb 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants