-
Notifications
You must be signed in to change notification settings - Fork 34
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
specs/extension_image: Add optional mutability for extensions #78
Conversation
(as mentioned elsewhere: let's wait until an implementation of the spec exists. specs made out of thin air that haven't seen an implementation yet, are typically not the highest quality) |
Makes sense. We'll take a stab at adding it to systemd-sysext then. We'd like to keep this PR open during implementation so we have a place for people to read up on what we want to do. |
@keszybz Thanks for the review! Please pardon the long turn-around, haven't looked at the PR for some time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I agree with Lennart that we need an implementation for this. This text contains a lot of very specific details.
specs/extension_image.md
Outdated
In a future version of this specification options for enforcing uniqueness may be provided. | ||
|
||
## Base Directory Immutability / Mutability | ||
The contents of system and configuration extension images are immutable and cannot be changed after |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should say "by default" or otherwise give a hint that it's not always true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the extension contents are always immutable. The whole sentence is misleading though since that section discusses base directory mutability. I'll remove it.
specs/extension_image.md
Outdated
2. Alternatively, _upperdir_ may be an entirely separate directory: modifications will be captured | ||
but the base directory will remain unchanged, retaining its state from before the extension was merged. | ||
3. *Ephemeral Mode* - Similar to mutable mode (2.) above but writes are explicitly stored | ||
in a temporary directory (possibly in RAM) and discarded as soon as extensions are un-merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tmpfs is swappable, so it's not actually "RAM". I think it's better to say "in a temporary file system" and nothing more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change it to "will only be stored temporarily while sysexts are merged". I'd like to avoid implementation details and I feel my phrasing above is too specific already.
97782c5
to
a90e041
Compare
Addressed all feedback, thanks @keszybz and @krnowak for the review! FYI there's a WIP PR for the above (mutability mode 1 only) now: systemd/systemd#31000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Should the specification say anything about implementation's behavior when there are zero extension images, but there is a mutable layer configured?
With @poettering we discussed that |
systemd-sysext will check if /var/lib/extension-data/${hierarchy}.local 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.
systemd-sysext will check if /var/lib/extension-data/${hierarchy}.local 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.
systemd-sysext will check if /var/lib/extension-data/${hierarchy}.local 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.
systemd-sysext will check if /var/lib/extension-data/${hierarchy}.local 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.
specs/extension_image.md
Outdated
for compatibility across implementations. | ||
|
||
Mutability modes may be configured in the following ways: | ||
1. By creating qualified paths or soft-links below `/var/lib/extension-data/`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the rationale for this dir? it sounds weirdly redundant with its "-data" suffix. That doesn't tell you anything what this is.
I originally assumed the mutable layer would be just /usr.local/. Here you are suggesting an entirely different dir however. what's your rationale for this?
So I think there might be value in placing this in /var/lib/. My thinking has evolved a bit on this. Specifically: I think we should keep the option open to one day have "systemd-sysext commit" and "systemd-confext commit", which would take the current mutable layer and automatically turn it into an proper extension (i.e. an immutable layer), and then opening a new mutable layer. the operation for that would be very simple, just move/rename the upper layer dir, so that it becomes a lower layer dir, mkdir a new upper layer dir, and replace the overlayfs mount with the new arrangement.
Hence, from that angle there's value in having both the immutable sysexts/confexts and the mutable layers on the same fs, so that the move/rename can safely work.
Right now systemd suggests putting confexts in /var/lib/confexts/ and sysexts into /bar/lib/extensions/. I think we should sooner or later probably move the latter to /var/lib/sysexts/ actually. But maybe taking inspiration from that the mutable layer should be:
/var/lib/confexts.mutable/
/var/lib/sysexts.mutable/
maybe?
opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other idea would be to put the mutable layer into /var/lib/confexts/mutable/
But I am not sure how much I like that, because it means there would suddenly be a magic layer whose name doesn't really communicate that it's special.
Maybe /var/lib/confexts/@mutable/?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not nest and make the whole thing more complicated. A sibling directory in /var/lib/, explicitly called "mutable" or writable or whatever, sounds best to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how descriptive the /var/lib/confexts.mutable/etc/
and /var/lib/sysexts.mutable/usr/
paths are.
As for the rename, maybe keep it consistent and stick to /var/lib/extensions.mutable/usr/
for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed directories are quite self-explanatory, I like it. I like Kai's proposal best as it's quite generic. Will amend the PR.
specs/extension_image.md
Outdated
* `/var/lib/extension-data/usr.local` - directory or soft-link to a directory to store writes to | ||
`/usr`. This is for system extensions. | ||
* `/var/lib/extension-data/opt.local` - directory or soft-link to a directory to store writes to | ||
`/opt`. This is for system extensions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, with the discussion above I am pretty sure that mutable extensions should look exactly like regular extensions, so that we can turn them into them. Or in other words, the stuff that shows up under /etc/ should be in a dir called "etc", not under one called "etc.local".
Or in other words:
/var/lib/sysexts.mutable/opt/
/var/lib/sysexts.mutable/usr/
/var/lib/confexts.mutable/etc/
are the actually populated dirs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never liked the .local suffix either, so that looks better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, will update using Kai's proposal.
Updated the PR and addressed all feedback. Mutability is not assumed anymore by the spec. I've left the discussion around directories backing mutable storage open, but this should be addressed now, too: I went with @poettering @bluca @pothos @keszybz would you be available for another review round? |
systemd-sysext will check if /var/lib/extension-data/${hierarchy}.local 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.
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.
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.
lgtm |
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.
From a fast skim of this topic, i understand that you guys want to make the merged content become R/W right? What about #31392 🤔 |
@TriMoon That's correct - though only optionally (i.e. when explicitly requested). See systemd/systemd#31000 😉 |
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.
Should there be a sentence or a paragraph about implementation being free to gate the mutability mode in a way it wants? Like through additional command-line flags or configuration files? Because right now I'm not sure if the spec says "if |
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.
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.
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.
btw, where are we with this? is this good to merge? does the stuff merged in systemd and this spec match up 100% right now? |
Systemd currently implements the symlink configuration option (config option #1, the only mandatory option in the spec). While changes have been merged in systemd, I was planning to conclude this PR only after a new systemd release includes the new feature (this would be systemd-256 if everything works out). |
There was a question that came up during one review whether confext mutable directory should be |
Where are we with this now? I think everything in systemd got merged that was suppsoed to be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
specs/extension_image.md
Outdated
* Path is a _directory, subvolume, or mount point_ - the path at | ||
`/var/lib/extensions.mutable/<basedir>/` is used to store writes to `/<basedir>/`. | ||
* A tmpfs mount at the qualified path may be used for a custom ephemeral mode. | ||
In this case, clean-up of the tmpfs is left to the user and/or is implementation specific. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"implementation-specific" (with a hyphen), here and below.
I think we can merge it. systemd-256 hasn't been released yet, but we're in late rcs, and it's not going to change substantially. |
Something went wrong with that push. I see a merge commit. |
Yes, that's github's "sync fork to upstream" button breaking stuff. I hate it's not doing a rebase... Will fix. |
Signed-off-by: Thilo Fromm <[email protected]> Co-authored-by: Krzesimir Nowak <[email protected]>
Aaah, that's better. Also changed |
This PR extends the extension specification (covering confext and sysext) by optional mutability modes for extensions.
Specifically, it discusses 3 modes:
A total of 3 configuration options are discussed for setting up mutable modes for base directories.
Please refer to the diff included in this PR for the changes, and /or to the full document at https://github.com/flatcar/uapi-group-specifications/blob/main/specs/extension_image.md .
Prior discussions here: flatcar/Flatcar#986 (comment) , systemd/systemd#24864 (comment) .