@sipsma Locally I'm repeatedly getting errors like

 => ERROR [stage-2 2/2] COPY --from=build Dockerfile what                                                                                                                                                        0.0s
------
 > [stage-2 2/2] COPY --from=build Dockerfile what:
------
Dockerfile:9
--------------------
   7 |
   8 |     from ubuntu
   9 | >>> copy --from=build Dockerfile foo
  10 |
--------------------
error: failed to solve: failed to commit "qt8n1d9s999afc782sonep325": snapshot "qt8n1d9s999afc782sonep325": already exists

Not exactly sure in what case it appears.

In output one weird case is that commands can appear out of order.

 => [build 3/3] COPY Dockerfile .                                                                                                                                                                                0.0s
 => [build 2/3] RUN apk add curl

Also need some way to remove the status lines for merges from output(ideally combine with copy lines).

Locally I'm repeatedly getting errors like

Updated my PR with what should be a fix for that. That case could happen when a merge ref got removed released but that snapshot stayed (as a prune didn't happen yet). It was trying to remake the snapshot and getting the error when the snapshot already existed. Fixed it by just ignoring the error in that case. Will add test coverage for this too, just wanted to get the fix out first.

EDIT: removed above should have been released

In output one weird case is that commands can appear out of order.
Also need some way to remove the status lines for merges from output(ideally combine with copy lines).

Will take a look at these

Updated my PR with what should be a fix for that. That case could happen when a merge ref got removed but that snapshot stayed (as a prune didn't happen yet). It was trying to remake the snapshot and getting the error when the snapshot already existed. Fixed it by just ignoring the error in that case. Will add test coverage for this too, just wanted to get the fix out first.

For clarity, I never ran prune when I tested this so these releases must have been automatic.

For clarity, I never ran prune when I tested this so these releases must have been automatic.

Yeah exactly, I just didn't have a test case yet where I ran two builds with the exact same merge (there was always a change to merges between builds in my tests), which is what caused this.

@sipsma But is it correct that the merge snapshot should be released as soon as ref is released? Shouldn't it be released by prune(or buildkit gc).

@sipsma But is it correct that the merge snapshot should be released as soon as ref is released? Shouldn't it be released by prune(or buildkit gc).

That's what I'm saying, the behavior is that the snapshots are not released when the ref count goes to 0. However, before my fix, when a new ref got created for the merge again it was incorrectly assuming that its snapshot was always gone. After my fix, it doesn't assume snapshot is gone anymore and instead handles both cases of 1) it still existing and 2) it having been pruned and needing to be recreated

When the build is complete is the merge ref visible in buildctl du? And is there a guarantee that if I run the same build again it will not need to do hardlink-copy for the second time?

the behavior is that the snapshots are not released when the ref count goes to 0.

I get that but should the ref itself be released when ref count goes to 0.

When the build is complete is the merge ref visible in buildctl du?

It should be as I updated the usage code, but I haven't specifically tried w/ buildctl yet. I'll give that a try manually to be 100% sure. Will add some test coverage to client_test.go for this too (unless there's a better place for it).

And is there a guarantee that if I run the same build again it will not need to do hardlink-copy for the second time?

Yes. Obviously if the user called Prune and the snapshot got removed you'd have to remake the merge snapshot but as long as it wasn't manually pruned you will re-use the hardlink-copy snapshot from before.

That's the bug you ran into, it was seeing that the snapshot already existed and can be re-used, but the code was only checking if err != nil instead of ignoring the already exists error type. Now it has the correct behavior.

I get that but should the ref itself be released when ref count goes to 0.

The behavior here is the same as before merge-op. Release gets called on the ref, ref count goes to 0 but the actual cacheRecord and snapshot continue to exist unless a prune occurs on them. In my earlier comment I said "That case could happen when a merge ref got removed" which maybe is causing the confusion, it would have been more clear to say "That case could happen when a merge ref got released". No behavior changed in terms releasing refs/cacheRecords.

@tonistiigi Confirmed that buildctl du shows the merge refs as expected and with what appears to be the correct size. Merging together some inputs that together add up to few hundred megs shows a merge ref that has size of ~73kb, which feels correct (it's >0 because directory inodes can't be hardlinked and thus take up >0 space).

The parents show up as the input ref IDs separated by ;, as we talked about in the PR. The only thing missing is a description field for the ref, which is a 1 line change that I'll update the PR with.

I also double checked and confirmed that re-running the same builds results in the merge refs getting re-used rather than recreated.

I'm still looking into the progress output appearing out of order as you mentioned earlier, will let you know what I find.

I'm still looking into the progress output appearing out of order as you mentioned earlier, will let you know what I find.

The output is expected as COPY doesn't have a dependency on destination directory anymore so it can be done in parallel with the previous RUN. Only merge has the dependency that is on another line. But to the user it is confusing so we should look for some way around it. Maybe we put an order value in the status responses that we can control from llb or frontend. Or try to show items based on graph depth, not execution order.

Just saw you pushed the update and was looking through the remaining test failures. Ignoring the chown-related ones which are expected for now, one significant one seems to be TestSymlinkDestination. I guess this case is another example similar to chown where the result of the copy depends on the contents of the parent input (specifically, whether there is a symlink at the destination of the copy and where it is pointing to).

Trying to think through how we can deal with this... Using the test case as an example, do you think it's possible to set up a conditional dependency between the vertex for COPY foo /symlink and the vertex for ADD t.tar that basically says "invalidate the COPY if the ADD changes at path /symlink"? Then, when the COPY actually runs it will check the value of /symlink in the ADD vertex and copy as expected onto scratch. After that, merge just works as it normally does.

If we can do that, then we'll still get caching benefits when ADD doesn't change. When ADD does change, we'll need to unlazy it, but then if we find that /symlink remains the same, COPY can remain lazy and re-used from cache. If /symlink does change, then COPY gets invalidated and has to run again, which is what we want in this case. So, overall you still get a lot of caching benefits by switching to merge while still retaining the previous behavior.

If that actually is possible, I'm wondering if we can use the same approach for chown, just in that case the conditional dependency will be on whether /etc/passwd changes.

I don't see any solutions for this :( The test uses ADD but same thing can happen in any FROM, RUN or COPY. And unlike #2414 there are no extra flags that would hint that symlink is in use. Even with the "symlink check" you describe, for a common case where the previous merge needs to be mounted to check for symlink(or /etc/passwd) this would be a slower and much more complicated code path, without almost any benefits.

The only options are Dockerfile v2 with updated COPY semantics, extra flag for COPY, or another ADD/COPY-like instruction. With the latter two, the default case would not be optimized.

Yeah I see your point. In terms of the fallback options, I personally wouldn't mind something like COPY --merge that disables following symlinks and other unwanted behavior in exchange for enabling merge-op. Maybe you could also support a directive at the start of a Dockerfile that, if present, results in all COPY statements defaulting to --merge?

That's still not nearly as nice as getting everyone improvements with literally zero effort but personally as a user I wouldn't be bothered by getting a potentially large performance improvement from just adding a single line to my Dockerfile.

replaced by #2596