Remove unneeded Finalize method from ImmutableRef.

[sipsma]

Finalize was only used outside the cache package in one place, which
called it with the commit arg set to false. The code path followed
when commit==false turned out to essentially be a no-op because
it set "retain cache" to true if it was already set to true.

It was thus safe to remove the only external call to it and remove it
from the interface. This should be helpful for future efforts to
simplify the equal{Mutable,Immutable} fields in cacheRecord, which exist
due to the "lazy commit" feature that Finalize is tied into.

Signed-off-by: Erik Sipsma erik@sipsma.dev

[tonistiigi]

I guess the code was supposed to be if !HasCachePolicyRetain(mutable) {.

As I understand that Finalize call there is for the case where you do a build and the top layer of the result is uncommitted snapshot. You want to make sure that this layer is retained(normally mutables that get released will get deleted) so you would have cache for it next time. But you don't want to commit it, eg. for the case case where build doing llb.Local() needs to get access to mutable again to incrementally change them on updates. Do you understand how this happens atm without this retain call.

[sipsma]

Do you understand how this happens atm without this retain call.

Isn't it just that the llb.Local snapshots always get created with Retain set?

buildkit/source/local/local.go

Line 129 in 521727e

m, err := ls.cm.New(ctx, nil, s, cache.CachePolicyRetain, cache.WithRecordType(client.UsageRecordTypeLocalSource), cache.WithDescription(fmt.Sprintf("local source for %s", ls.src.Name)))

[tonistiigi]

But what keeps the top layer of the build result from not getting deleted.

[sipsma]

But what keeps the top layer of the build result from not getting deleted.

My understanding is:

1. Local creates the mutable ref, but also calls commit on it to return an immutable ref to use as the snapshot. This immutable ref will internally have equalMutable set to the mutable created before.
2. At the end of the build, the immutable ref will have release called on it, which should see that there are no more refs to it and then call release on its equalMutable.
3. The equalMutable release method will then see that the cache policy was set to retain, so it skips removing itself and also skips removing its equalImmutable. It'll thus stick around after the build.

Am I missing anything here or misunderstanding the concern?

[tonistiigi]

I understand how it happens for Local #2216 (comment) but what about other types of builds. Or is it that in other ways when build returns ImmutabeRef it is always already committed to snapshot and equalMutable is nil?

[sipsma]

I understand how it happens for Local #2216 (comment) but what about other types of builds. Or is it that in other ways when build returns ImmutabeRef it is always already committed to snapshot and equalMutable is nil?

There is also this relevant code block, which I just noticed and believe will be called by the solver after any vertex successfully executes? It only sets cache retain on immutable refs directly, which doesn't necessarily mean any equal mutable will have it set too.

Additionally though, any top layer which is an immutable ref w/ an equalMutable that gets exported will be finalized during the export during the call to GetRemote.

In terms of ops that create mutable refs (basically all except container image):

• HTTP source and FileOps use CachePolicyRetain when creating mutable refs and don't try to do any clever re-use of mutable layers anyways
• Git source uses CachePolicyRetain when creating the mutable ref for the "remote" snapshot.
  • The "checkout" snapshot is not created with CachePolicyRetain though, but gets turned into an immutable ref w/ an equalMutable set. So in a build where it was the top layer but not exported (a weird case):
    1. the code above will set CachePolicyRetain on the immutable ref if the vertex runs successfully
    2. the immutable ref will get released after the build, which will call release on the mutable ref
    3. the mutable ref won't have retain set, but the immutable ref will, so the snapshot won't get removed
• ExecOp doesn't use CachePolicyRetain either, so it's the same as Git checkout snapshot

Overall, while convoluted, I think things behave as expected.

[tonistiigi]

@sipsma Thanks. Not sure if there would be any difference to use cache.Save as the retain place vs just after op.Exec.

[crazy-max]

Looks like it's still used on moby (saw that while vendoring buildkit):

• https://github.com/moby/moby/blob/4283e93e6431c5ff6d59aed2104f0942ae40c838/builder/builder-next/exporter/export.go#L118
• https://github.com/moby/moby/blob/4283e93e6431c5ff6d59aed2104f0942ae40c838/builder/builder-next/worker/worker.go#L251

[tonistiigi]

@sipsma PTAL

[sipsma]

@crazy-max @tonistiigi I see, I wasn't aware at the time this change was made how moby integrated w/ buildkit so I didn't think to look there, that's my bad. I can just send out a commit to re-add Finalize as I don't understand the moby codebase enough to determine if it's safe to drop those finalize calls. Getting rid of it from the interface was nice mostly in anticipation of simplifying some of the other aspects of the lazy-commit feature, but we ended up not doing that so it's not a big deal for Finalize to stick around.