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

systemd-sysext: Implement optional mutability for extensions #31000

Merged
merged 9 commits into from
Feb 26, 2024

Conversation

krnowak
Copy link
Contributor

@krnowak krnowak commented Jan 18, 2024

The PR implements some parts of the proposed update to the Extension Images specifications - uapi-group/specifications#78 - implemented are minimal requirements for enabling mutability and the --mutable command line flag. There is no configurability through config files. There is no ephemeral mode.

Fixes: #31392

@github-actions github-actions bot added tests please-review PR is ready for (re-)review by a maintainer labels Jan 18, 2024
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
@poettering poettering 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 Jan 18, 2024
@pothos
Copy link
Contributor

pothos commented Jan 19, 2024

In a test with mkosi qemu and usr.local pointing to /usr I ran into this:

fedora kernel: overlayfs: upper fs missing required features.
fedora kernel: overlayfs: upper fs does not support tmpfile.
fedora kernel: overlayfs: failed to set xattr on upper
fedora kernel: overlayfs: ...falling back to redirect_dir=nofollow.
fedora kernel: overlayfs: ...falling back to uuid=null.
fedora kernel: overlayfs: try mounting with 'userxattr' option

Edit: Here the docs: User xattr: The "-o userxattr" mount option forces overlayfs to use the "user.overlay." xattr namespace instead of "trusted.overlay.". This is useful for unprivileged mounting of overlayfs.
No idea why this is needed, and when.
Edit2: Might be because of the rootfs being virtiofs

@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 Jan 19, 2024
@krnowak
Copy link
Contributor Author

krnowak commented Jan 19, 2024

Hm, I'm wondering if the path stored in /usr/.systemd-sysext/work_dir should have arg_root prefix removed before written to the file when merging. When unmerging, arg_root could be then prepended.

src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
src/sysext/sysext.c Outdated Show resolved Hide resolved
umount_verbose is already doing it for us.
We will use it later when adding workdir and upperdir options for overlayfs
mount operation.
The follow-up commit will refactor some code in systemd-sysext, so add some
tests to make sure that things didn't break. The tests will be later extended
with cases for new features added.
Divide the merge_hierarchy function into code that:

- determines the lower directories for overlayfs

  - determination of lower directories was further split into top, middle and
    bottom directories:

    - bottom - possibly the hierarchy itself

    - middle - hierarchies from extensions

    - top - metadata directory

- mounts the overlayfs using determined directories

- writes information to the metadata directory

- makes the merged hierarchy read-only
systemd-sysext will check if /var/lib/extensions.mutable/${hierarchy} exists
and use it as an overlayfs upperdir for storing writes. This allows having
mutable hierarchy after merging the extension images.

The implementation is following a proposed update to the Extension Images
specification at uapi-group/specifications#78.
The flag takes "auto" or "import" or a boolean value.

"auto" causes systemd-sysext to make a decision about mutability of the merged
hierarchy based on existence of the upper directory in
`/var/lib/extensions.mutable/${hierarchy}`.

"import" causes the existing upper dir to be actually used as another lower
dir, which results in read-only merged hierarchy.

True value makes systemd-sysext to create the upper dir if it's missing and to
make the merged hierarchy mutable.

False value makes systemd-sysext to ignore upper dir completely, and create a
read-only merged hierarchy.

The default is false value.
@krnowak
Copy link
Contributor Author

krnowak commented Feb 22, 2024

Made --mutable flag to default to no. Moved the tests to TEST-50. Redid the tests commit (now there's a commit with initial tests before adding mutability functionality, and another commit that adds the rest of the tests after implementing the feature).

@t-lo: Feel free to update the PR.

@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 Feb 22, 2024
@krnowak
Copy link
Contributor Author

krnowak commented Feb 22, 2024

About CI failures: previously fedora failed with some pgp check failures when installing some fedora package, so it's unrelated to the PR. In the autopkg test - I have no idea where to look, the log file is enormous. But it seems to be that some logind tests failed, but I can't see how relevant these are to the changes in this PR.


<para>The following modes are supported:
<orderedlist>
<listitem><para><option>auto</option>: Automatic mode; the default. Mutability is disabled by default
Copy link
Member

Choose a reason for hiding this comment

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

this bit is out of date

Copy link
Contributor

Choose a reason for hiding this comment

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

See #31000 (comment), will update today.

@@ -205,6 +212,51 @@
to tie the most frequently configured options to runtime updateable flags that can be changed without a
system reboot. This will help reduce servicing times when there is a need for changing the OS configuration.</para></refsect1>

<refsect1>
<title>Mutability</title>
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph should include the version snippet, not sure if the xi:include works in this context, if it doesn't please write by hand that it was introduced in v256

Copy link
Contributor

Choose a reason for hiding this comment

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

xi:include seems to work - tests succeed locally, no error reported.

<filename>/opt/</filename>, and <filename>/etc/</filename> if write routing sub-directories
or symlinks are present in <filename>/var/lib/extensions.mutable/</filename>; disable otherwise.
See "Mutability" above for more information on write routing.
This is the default.</para>
Copy link
Member

Choose a reason for hiding this comment

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

needs to be updated too

@t-lo
Copy link
Contributor

t-lo commented Feb 23, 2024

Addressed @bluca's feedback and updated the man page. Thank you for the review!

Rendered man page attached for convenience (again as .txt because GH thinks .8 files are evil):

systemd-sysext.8.txt

Signed-off-by: Thilo Fromm <thilofromm@microsoft.com>
@t-lo
Copy link
Contributor

t-lo commented Feb 26, 2024

@poettering @bluca Is there anything else amiss or are we good to merge? There's a single failing test of Ubuntu Jammy on s390x, which fails with

fwupd-refresh.service: Main process exited, code=exited, status=1/FAILURE

but I believe its unrelated to our changes. We didn't touch fwupdated and the test succeeded in the runs before I updated the man page.

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.

mutable /etc/ leayers in confext
5 participants