commit: Support merging docker-style layers #578

alexlarsson · 2016-11-16T22:15:27Z

This adds support to OstreeMutableTree for merging one mtree into another, using the mechanism for whiteouts that docker/OCI uses (i.e. .wh. whiteout files).

Since the spec doesn't guarantee the order of the whiteouts (i.e. a file from this layer can appear before a whiteout that affects it) we can't really apply layer in a streaming fashion. Thus we read the layer into an mtree end then we apply it.

This adds two operations to commit:
--tree=layer-tar=TARFILE
and
--tree=layer-ref=COMMIT
which are different ways to apply layers when commiting.

This PR contains two extra commits (from #401 and #577) that make the testsuite work. They should probably be merged first and then dropped from this PR before merging it.

alexlarsson · 2016-11-16T22:32:05Z

Unclear why the CI failed here:

prohibit_empty_lines_at_EOF
tests/base.tar.gz
tests/layer.tar.gz
maint.mk: empty line(s) or no newline at EOF

Why does it look at the .tar.gz file for newlines at EOF?

giuseppe · 2016-11-16T22:53:23Z

@alexlarsson I think it should be enough to add *.gz to VC_LIST_ALWAYS_EXCLUDE_REGEX in the cfg.mk file. You can quickly test it locally with make syntax-check

alexlarsson · 2016-11-16T22:55:35Z

@giuseppe Thanks!

giuseppe · 2016-11-16T22:59:45Z

Once this gets merged, I'll probably have to rework #491 to use the new APIs

alexlarsson · 2016-11-16T23:23:07Z

Seems to fail the test-commit.sh test in the installed variant. I think this really is an issue with the commit that i stole from #401

giuseppe

useful API. It can be used by atomic for system containers as well.

giuseppe · 2016-11-17T10:50:36Z

src/libostree/ostree-mutable-tree.c

+          if (!ostree_mutable_tree_remove_child (self, whiteout_file_name, &local_error) &&
+              !g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND))
+            {
+              g_propagate_error (error, g_steal_pointer (&local_error));


What do you think if we don't error out here? I have tried with a manually crafted Docker image that has a whiteout for a file that doesn't exist in a lower layer. The image could be imported and run without errors. It'll probably turn to be useful for testing layers that are not built as part of the same image.

What do you mean? It especially ignores the "not found" error and only reports if you have some other error. So, removing something from the base layers thats not there should work. Did it not?

ok sorry, I misread the condition, it is doing exactly what I meant :)

alexlarsson · 2016-11-17T11:12:01Z

Pushed a new version, based on updated patch for the --tree=ref issue from #581
Hopefully this one passed the tests.

This adds support to OstreeMutableTree for merging one mtree into another, using the mechanism for whiteouts that docker/OCI uses (i.e. .wh. whiteout files). Since the spec doesn't guarantee the order of the whiteouts (i.e. a file from this layer can appear before a whiteout that affects it) we can't really apply layer in a streaming fashion. Thus we read the layer into an mtree end then we apply it. This adds two operations to commit: --tree=layer-tar=TARFILE and --tree=layer-ref=COMMIT which are different ways to apply layers when commiting.

alexlarsson · 2016-11-17T15:31:13Z

New version based on git master without any extra commits. Please review.

jlebon · 2016-11-17T15:52:53Z

src/libostree/ostree-mutable-tree.c

+                                         GError           **error)
+{
+  g_hash_table_foreach_remove (self->subdirs, remove_all, NULL);
+  g_hash_table_foreach_remove (self->files, remove_all, NULL);


Can't we just use g_hash_table_remove_all()?

Yeah, forgot that.

jlebon · 2016-11-17T16:07:39Z

Looks good to me. A bit bikeshed, though I wonder if the new APIs and CLI switches should use oci_layer/oci-layer since "layer" is a bit generic.

cgwalters · 2016-11-17T18:02:17Z

I'd vote for oci-layer too.

Hum...so can you briefly describe the big picture here? Would tools store both "layer refs" like the atomic/ostree integration is doing today, as well as generate "squashed commits" using this?

And the benefit here is...making it faster to check out?

alexlarsson · 2016-11-17T18:20:11Z

This is more of a tool that can be used multiple ways. Either you can store layers as refs or you can combine refs and/or tarballs into a resolved branch.

In the case of flatpak, we never really use layers. Things are always fully resolved. However we run into layers as we start importing apps from oci images which may have multiple layers that need to be combined.

cgwalters · 2016-11-17T20:42:39Z

https://mail.gnome.org/archives/ostree-list/2016-November/msg00012.html

openshift-ci-robot · 2021-04-12T21:19:18Z

@alexlarsson: PR needs rebase.

Details

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.

openshift-ci · 2023-06-29T14:46:21Z

@alexlarsson: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/sanity	`ca02e0e`	link	true	`/test sanity`
ci/prow/images	`ca02e0e`	link	true	`/test images`
ci/prow/fcos-e2e	`ca02e0e`	link	true	`/test fcos-e2e`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

giuseppe reviewed Nov 17, 2016

View reviewed changes

alexlarsson mentioned this pull request Nov 17, 2016

[merged] ostree commit: Fix combining trees with multiple --tree=ref arguments (alternative to https://github.com/ostreedev/ostree/pull/401) #581

Closed

alexlarsson force-pushed the commit-layers branch from 9833f70 to c2c5a30 Compare November 17, 2016 11:10

alexlarsson force-pushed the commit-layers branch from c2c5a30 to 6f6b55d Compare November 17, 2016 15:30

jlebon reviewed Nov 17, 2016

View reviewed changes

fixup! commit: Support merging docker-style layers

ca02e0e

cgwalters mentioned this pull request Dec 16, 2016

ostree-union-fuse: new fuse filesystem to access multiple commits #491

Closed

openshift-ci-robot added the needs-rebase label Apr 12, 2021

commit: Support merging docker-style layers #578

Are you sure you want to change the base?

commit: Support merging docker-style layers #578

Uh oh!

Conversation

alexlarsson commented Nov 16, 2016

Uh oh!

alexlarsson commented Nov 16, 2016

Uh oh!

giuseppe commented Nov 16, 2016

Uh oh!

alexlarsson commented Nov 16, 2016

Uh oh!

giuseppe commented Nov 16, 2016

Uh oh!

alexlarsson commented Nov 16, 2016

Uh oh!

giuseppe left a comment

Choose a reason for hiding this comment

Uh oh!

giuseppe Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

alexlarsson Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

giuseppe Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

alexlarsson commented Nov 17, 2016

Uh oh!

alexlarsson commented Nov 17, 2016

Uh oh!

jlebon Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

alexlarsson Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

alexlarsson Nov 17, 2016

Choose a reason for hiding this comment

Uh oh!

jlebon commented Nov 17, 2016

Uh oh!

cgwalters commented Nov 17, 2016

Uh oh!

alexlarsson commented Nov 17, 2016

Uh oh!

cgwalters commented Nov 17, 2016

Uh oh!

openshift-ci-robot commented Apr 12, 2021

Uh oh!

openshift-ci bot commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants