Naive diff should mount all layers read-only #1121
Conversation
|
The idea makes sense to me, but I have not tested it |
|
@leahneukirchen Could you please sign the commit. |
|
Of course. Can anyone look at the questions I stated above? |
leahneukirchen
force-pushed
the
naive-diff-on-ro
branch
from
8a536ba
to
4648931
Compare
We don't need to write for a diff, and if the layers are on read-only additionalimagestores, trying to mount them fails when they are used as upperdir. Similar to e96cd86. Signed-off-by: Leah Neukirchen <leah@vuxu.org>
leahneukirchen
force-pushed
the
naive-diff-on-ro
branch
from
4648931
to
6888068
Compare
|
LGTM |
|
@nalind PTAL |
|
Yes, it's blocking me shipping a feature in our software. :) |
|
Mounting layers read-only could cause problems in cases where we suddenly needed them to be writable, for example if we needed to start a container that used such a layer as its rootfs while that rootfs was mounted read-only. The naive differ currently mounts the parent layer read-only, and since we intend to never modify layers that are parents of other layers, it's fine if we always do that. If we extended that to layers that don't have parents, though, committing containers would cause their root filesystems to be mounted read-only (even though they can't be in additional stores, so they're always on writable storage), and that's a potential problem that I would much prefer to avoid. I'm guessing that having the lower-level driver (which in this case means overlay, since it and vfs are the only two that have the feature, and vfs doesn't actually mount anything) forcibly enable the read-only flag when it knows that a layer that it's being asked to mount is in an additional store would also solve the problem at hand. |
|
That sounds even better, but I wasn't sure how to get that information at that point. |
|
I've taken a stab at it using the two commits on the tip of https://github.com/nalind/storage/tree/ro-for-diff. Does it also solve the problem you're seeing? |
|
That also solves the problem for me! I still think that |
|
@1123 is merged, do we still need this one? |
|
Nope, thanks! |
We don't need to write for a diff, and if the layers are on read-only
additionalimagestores, trying to mount them fails when they are used
as upperdir.
Similar to e96cd86.
This is pretty much shot-in-the-dark, but it seems to fix the issue for me.
We should also investigate whether Diff and DiffSize need the same treatment.
The code also loses the default mount options here due to passing its own,
not sure if this is problematic.
Removing the /merged dir fails, but not sure why that is done in first place,
as overlay directories usually have empty /merged.
Podman issue: #12926