checkout: honor opaque checkouts #1486

giuseppe · 2018-03-05T23:06:16Z

Signed-off-by: Giuseppe Scrivano [email protected]

giuseppe · 2018-03-27T17:47:46Z

@nalind does this implementation look good to you?

giuseppe · 2018-07-30T12:06:12Z

Dropping the WIP label as I think it is ready for review

giuseppe · 2018-10-15T12:54:57Z

tests are passing

giuseppe · 2018-10-25T08:01:18Z

/cc @cgwalters

cgwalters · 2018-10-26T14:11:11Z

A little bit more information - a link to some other code that handles this format e.g. - would be useful in the commit message or as a code comment.

This is a serialization of the

A directory is made opaque by setting the xattr "trusted.overlay.opaque"

from the kernel Documentation/filesystems/overlayfs.txt?

cgwalters · 2018-10-26T14:18:23Z

src/libostree/ostree-repo-checkout.c

+        {
+          is_opaque_whiteout = (g_str_equal (fname, OPAQUE_WHITEOUT_NAME));
+          if (is_opaque_whiteout)
+            break;


One thing I don't quite understand is why we're looping over all of the children here. Doesn't there need to be name matching against the target somehow? Or hmm...is the serialization that there's exactly one child with that name?

I am looking for a child ".wh..wh..opq" and it needs to have exactly that name. I didn't find a better way for having this search in O(1). Is there a better way to look for it?

I am looking for a child ".wh..wh..opq" and it needs to have exactly that name.

My question is - should that be the only child?

no, there can be other children and they will appear normally in the directory. The ".wh..wh..opq" file is only to hide anything that is in the lower layers, but the same layer can add more content

giuseppe · 2018-10-26T15:31:05Z

A little bit more information - a link to some other code that handles this format e.g. - would be useful in the commit message or as a code comment.

This PR came after the comment here: containers/storage#137 (comment)

What I've done is:

skopeo copy docker://docker.io/redis@sha256:26c93c5b06eaa323bb1089500f42b0dd158138772348b865e364127f1d554982  ostree:foo@/ostree/repo
ostree ls ociimage/67b22db27e5144c14efdf55d9cef9c3dadffb43593790ec99663fb1b783dbb6b /var/lib/apt/lists/partial
d00755 0 0      0 /var/lib/apt/lists/partial
-00755 0 0      0 /var/lib/apt/lists/partial/.wh..wh..opq

what this file does is to rm -rf every entry in the directory where it is present so that /var/lib/apt/lists/partial exists but it is empty

I'll amend more information in the commit message

if a file ".wh..wh..opq" is present in a directory, delete anything from lower layers that is already in that directory. Signed-off-by: Giuseppe Scrivano <[email protected]>

cgwalters · 2018-10-26T18:26:48Z

@rh-atomic-bot r+ 035e693

rh-atomic-bot · 2018-10-26T18:26:54Z

⚡ Test exempted: merge already tested.

cgwalters · 2018-10-26T18:30:25Z

OK, thanks! I think I understand now. Just in general though I really prefer if even nearly trivial changes have some sort of rationale. For example in c9a9e6c

I could have just said "Add bindings section" since it's really obvious what I'm doing but even there I added a why I'm doing it. Another example is:
5183c8f
Where the why is "Making this change since I saw an unprefixed error in an issue.".

This is a far from trivial change and I think deserved a commit message; thanks for fleshing it out a bit! We should probably still add comments to the code but not blocking on that since the previous code didn't have a lot either (and in some places in libostree we're relatively decent with comments, others not so much).

cgwalters · 2018-10-26T18:32:23Z

And sorry again about the delay here...I think I must have missed notifications, and I sort of use the default github PR viewer as a "review stack" but this one got too buried. Wish there was a way to change that view to default to "recently changed" and not "initial creation time".

giuseppe · 2018-10-26T19:42:59Z

And sorry again about the delay here...I think I must have missed notifications, and I sort of use the default github PR viewer as a "review stack" but this one got too buried. Wish there was a way to change that view to default to "recently changed" and not "initial creation time".

no problem :-) I've left it marked as WIP for a long time.

Also, for reference, I've used this Dockerfile to get an opaque checkout:

FROM alpine
RUN rm -rf /srv && mkdir -p /srv && touch /srv/a

Once it is imported into ostree I see:

# ostree ls -R ociimage/aa1c5b4533c0d2c7d18d06c7618a7e4318a71764b9a394202840cc85e39d7630 /
d00755 0 0      0 /
d00755 0 0      0 /etc
-00755 0 0      0 /etc/resolv.conf
d00755 0 0      0 /run
d00755 0 0      0 /run/secrets
d00755 0 0      0 /srv
-00755 0 0      0 /srv/.wh..wh..opq
-00644 0 0      0 /srv/a

giuseppe mentioned this pull request Mar 5, 2018

ostree: new option for the overlay driver containers/storage#137

Merged

jlebon added the WIP label Mar 23, 2018

giuseppe force-pushed the opaque-whiteout-support branch from e39b942 to d1fc1d9 Compare May 31, 2018 13:26

giuseppe force-pushed the opaque-whiteout-support branch from d1fc1d9 to 7984b3a Compare July 30, 2018 12:05

giuseppe changed the title ~~[WIP] checkout: honor opaque checkouts~~ checkout: honor opaque checkouts Jul 30, 2018

giuseppe removed the WIP label Jul 30, 2018

giuseppe force-pushed the opaque-whiteout-support branch from 7984b3a to 5bedcad Compare August 7, 2018 08:23

giuseppe force-pushed the opaque-whiteout-support branch from 5bedcad to 1b9ce05 Compare September 3, 2018 07:02

giuseppe force-pushed the opaque-whiteout-support branch 2 times, most recently from b413b02 to a6fc88d Compare October 3, 2018 09:20

giuseppe force-pushed the opaque-whiteout-support branch 4 times, most recently from 05fc070 to 72b6720 Compare October 15, 2018 11:39

giuseppe force-pushed the opaque-whiteout-support branch from 72b6720 to 7334ffd Compare October 17, 2018 20:01

cgwalters reviewed Oct 26, 2018

View reviewed changes

checkout: honor opaque checkouts

035e693

if a file ".wh..wh..opq" is present in a directory, delete anything from lower layers that is already in that directory. Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the opaque-whiteout-support branch from 7334ffd to 035e693 Compare October 26, 2018 16:17

rh-atomic-bot closed this in 51752ba Oct 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checkout: honor opaque checkouts #1486

checkout: honor opaque checkouts #1486

giuseppe commented Mar 5, 2018

giuseppe commented Mar 27, 2018

giuseppe commented Jul 30, 2018 •

edited

Loading

giuseppe commented Oct 15, 2018

giuseppe commented Oct 25, 2018

cgwalters commented Oct 26, 2018

cgwalters Oct 26, 2018

giuseppe Oct 26, 2018

cgwalters Oct 26, 2018

giuseppe Oct 26, 2018

giuseppe commented Oct 26, 2018

cgwalters commented Oct 26, 2018

rh-atomic-bot commented Oct 26, 2018

cgwalters commented Oct 26, 2018

cgwalters commented Oct 26, 2018

giuseppe commented Oct 26, 2018

checkout: honor opaque checkouts #1486

checkout: honor opaque checkouts #1486

Conversation

giuseppe commented Mar 5, 2018

giuseppe commented Mar 27, 2018

giuseppe commented Jul 30, 2018 • edited Loading

giuseppe commented Oct 15, 2018

giuseppe commented Oct 25, 2018

cgwalters commented Oct 26, 2018

cgwalters Oct 26, 2018

Choose a reason for hiding this comment

giuseppe Oct 26, 2018

Choose a reason for hiding this comment

cgwalters Oct 26, 2018

Choose a reason for hiding this comment

giuseppe Oct 26, 2018

Choose a reason for hiding this comment

giuseppe commented Oct 26, 2018

cgwalters commented Oct 26, 2018

rh-atomic-bot commented Oct 26, 2018

cgwalters commented Oct 26, 2018

cgwalters commented Oct 26, 2018

giuseppe commented Oct 26, 2018

giuseppe commented Jul 30, 2018 •

edited

Loading