WIP/DNM / Support overlayfs whiteout character files #2713

mangelajo · 2022-09-13T16:33:30Z

This is yet incomplete and untested, it doesn't work yet.

Related-Issue: #2712

openshift-ci · 2022-09-13T16:33:42Z

Hi @mangelajo. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lucab · 2022-09-14T13:48:26Z

/hold

I'd like to push back on this, I don't think it is a good idea. The thread at #2712 is still ongoing, let's find better alternatives there.

mangelajo · 2022-09-14T15:53:46Z

/hold

I'd like to push back on this, I don't think it is a good idea. The thread at #2712 is still ongoing, let's find better alternatives there.

please participate on the thread and provide alternatives, and please elaborate why you think it's not a good idea.

lucab · 2022-09-14T16:07:20Z

I'll let @cgwalters drive the conversation there to avoid too many voices, but I pretty much share the hints in #2712 (comment).

Related-Issue: ostreedev#2712

cgwalters · 2022-09-16T15:10:26Z

src/libostree/ostree-core.c

@@ -2014,7 +2014,7 @@ file_header_parse (GVariant         *metadata,
  mode = GUINT32_FROM_BE (mode);
  g_autoptr(GFileInfo) ret_file_info = _ostree_mode_uidgid_to_gfileinfo (mode, uid, gid);

-  if (S_ISREG (mode))
+  if (S_ISREG (mode) || S_ISCHR(mode))


I think we should have a common helper that checks specifically for whiteouts, which needs to take either a GFileInfo or a struct stat.

We still do want to error out for any character devices that are not whiteouts.

Yes in this case we are reconstructing the info from the ostree data and we have a check in L2009 that verifies rdev==0, so that´s assumed down here.

We could put the rdev info back into the GFileInfo and then have a validator to make it more explicit.

cgwalters · 2022-09-16T15:16:58Z

src/libostree/ostree-repo.c

@@ -4474,7 +4475,7 @@ ostree_repo_load_file (OstreeRepo         *self,
          if (S_ISLNK (stbuf.st_mode))
            g_file_info_set_symlink_target (*out_file_info, symlink_target);
          else
-            g_assert (S_ISREG (stbuf.st_mode));
+            g_assert (S_ISREG (stbuf.st_mode) || (S_ISCHR(stbuf.st_mode) && stbuf.st_rdev == 0));


This is the check we want to generalize everywhere I think, as e.g. _ostree_is_whiteout (&stbuf).

cgwalters · 2022-09-16T15:51:02Z

/ok-to-test

cgwalters · 2022-09-16T15:53:15Z

I do want to note here the last bit of #2712 (comment)

There's a bunch of code that will need touching for this, not just in ostree core but also in e.g. https://github.com/ostreedev/ostree-rs-ext/tree/main/lib/src/tar

which is that we could land this in ostree and that would probably work for your use case, but all that code that lives in ostree-rs-ext, such as e.g. ostree container encapsulate would fail.

And that gets into the whole topic of being able to "nest" container storage layers in container layers, which I think we do want longer term, which leads to #2712 (comment) or so.

mangelajo · 2022-09-19T08:25:57Z

src/libostree/ostree-core.c

@@ -2014,7 +2014,7 @@ file_header_parse (GVariant         *metadata,
  mode = GUINT32_FROM_BE (mode);
  g_autoptr(GFileInfo) ret_file_info = _ostree_mode_uidgid_to_gfileinfo (mode, uid, gid);

-  if (S_ISREG (mode))
+  if (S_ISREG (mode) || S_ISCHR(mode))


Yes in this case we are reconstructing the info from the ostree data and we have a check in L2009 that verifies rdev==0, so that´s assumed down here.

We could put the rdev info back into the GFileInfo and then have a validator to make it more explicit.

mangelajo · 2022-09-19T12:33:43Z

src/libostree/ostree-core.c

@@ -2368,7 +2385,7 @@ _ostree_validate_bareuseronly_mode (guint32     content_mode,
        return glnx_throw (error, "Content object %s: invalid mode 0%04o with bits 0%04o",
                           checksum, content_mode, invalid_modebits);
    }
-  else if (S_ISLNK (content_mode))
+  else if (S_ISLNK (content_mode) || S_ISCHR(content_mode))


I couldn't use the helper functions here since at this point it's only mode

mangelajo · 2022-09-19T12:33:50Z

src/libostree/ostree-core.c

@@ -2400,7 +2417,7 @@ gboolean
 ostree_validate_structureof_file_mode (guint32            mode,
                                       GError           **error)
 {
-  if (!(S_ISREG (mode) || S_ISLNK (mode)))
+  if (!(S_ISREG (mode) || S_ISLNK (mode) || S_ISCHR(mode)))


mangelajo · 2022-09-19T12:34:47Z

src/libostree/ostree-repo-commit.c

@@ -301,7 +332,7 @@ commit_loose_regfile_object (OstreeRepo        *self,
            return FALSE;
        }
      else
-        g_assert (S_ISLNK (mode));
+        g_assert (S_ISLNK (mode) || S_ISCHR(mode));


mangelajo · 2022-09-19T12:39:42Z

I'm working on another PR that follows the other suggested approach.

mangelajo · 2022-09-19T13:27:16Z

src/libostree/ostree-repo-checkout.c

-                /* Otherwise, fall through and do the link, we should
-                 * get EEXIST.
-                 */
+                  /* Otherwise, fall through and do the link, we should


This change shouldn't be here

mangelajo · 2022-09-23T11:21:56Z

Dropping this approach in favor of #2717

openshift-ci bot added do-not-merge/work-in-progress needs-ok-to-test labels Sep 13, 2022

openshift-ci bot added the do-not-merge/hold label Sep 14, 2022

mangelajo force-pushed the support-char-whiteouts branch from 729ecf6 to 49077e9 Compare September 14, 2022 15:50

WIP/DNM / Support overlayfs whiteout character files

ecd0a0f

Related-Issue: ostreedev#2712

mangelajo force-pushed the support-char-whiteouts branch from 49077e9 to ecd0a0f Compare September 16, 2022 10:43

cgwalters reviewed Sep 16, 2022

View reviewed changes

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Sep 16, 2022

mangelajo commented Sep 19, 2022

View reviewed changes

mangelajo force-pushed the support-char-whiteouts branch from 25f2623 to 11a4e2b Compare September 19, 2022 12:37

Apply reviewer comments

c2ff1cc

mangelajo force-pushed the support-char-whiteouts branch from 11a4e2b to c2ff1cc Compare September 19, 2022 12:38

mangelajo commented Sep 19, 2022

View reviewed changes

mangelajo closed this Sep 23, 2022

mangelajo mentioned this pull request Sep 27, 2022

Support overlayfs whiteouts on checkout #2717

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP/DNM / Support overlayfs whiteout character files #2713

WIP/DNM / Support overlayfs whiteout character files #2713

mangelajo commented Sep 13, 2022 •

edited

Loading

openshift-ci bot commented Sep 13, 2022

lucab commented Sep 14, 2022

mangelajo commented Sep 14, 2022 •

edited

Loading

lucab commented Sep 14, 2022

cgwalters Sep 16, 2022

mangelajo Sep 19, 2022

cgwalters Sep 16, 2022

cgwalters commented Sep 16, 2022

cgwalters commented Sep 16, 2022

mangelajo Sep 19, 2022

mangelajo Sep 19, 2022

mangelajo Sep 19, 2022

mangelajo Sep 19, 2022

mangelajo commented Sep 19, 2022

mangelajo Sep 19, 2022 •

edited

Loading

mangelajo commented Sep 23, 2022

WIP/DNM / Support overlayfs whiteout character files #2713

WIP/DNM / Support overlayfs whiteout character files #2713

Conversation

mangelajo commented Sep 13, 2022 • edited Loading

openshift-ci bot commented Sep 13, 2022

lucab commented Sep 14, 2022

mangelajo commented Sep 14, 2022 • edited Loading

lucab commented Sep 14, 2022

cgwalters Sep 16, 2022

Choose a reason for hiding this comment

mangelajo Sep 19, 2022

Choose a reason for hiding this comment

cgwalters Sep 16, 2022

Choose a reason for hiding this comment

cgwalters commented Sep 16, 2022

cgwalters commented Sep 16, 2022

mangelajo Sep 19, 2022

Choose a reason for hiding this comment

mangelajo Sep 19, 2022

Choose a reason for hiding this comment

mangelajo Sep 19, 2022

Choose a reason for hiding this comment

mangelajo Sep 19, 2022

Choose a reason for hiding this comment

mangelajo commented Sep 19, 2022

mangelajo Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

mangelajo commented Sep 23, 2022

mangelajo commented Sep 13, 2022 •

edited

Loading

mangelajo commented Sep 14, 2022 •

edited

Loading

mangelajo Sep 19, 2022 •

edited

Loading