From b85ef1568917d1bad182ac2d9afc5f7b9be9935a Mon Sep 17 00:00:00 2001 From: Erik Sipsma Date: Thu, 1 Jul 2021 02:50:54 +0000 Subject: [PATCH] Remove unneeded Finalize method from ImmutableRef. 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 --- cache/blobs.go | 4 ++-- cache/manager.go | 8 ++++---- cache/manager_test.go | 6 +++--- cache/refs.go | 18 +++++------------- solver/llbsolver/bridge.go | 11 +---------- 5 files changed, 15 insertions(+), 32 deletions(-) diff --git a/cache/blobs.go b/cache/blobs.go index 767462aeef59..7ae045c988ac 100644 --- a/cache/blobs.go +++ b/cache/blobs.go @@ -31,7 +31,7 @@ func (sr *immutableRef) computeBlobChain(ctx context.Context, createIfNeeded boo return errors.Errorf("missing lease requirement for computeBlobChain") } - if err := sr.Finalize(ctx, true); err != nil { + if err := sr.finalizeLocked(ctx); err != nil { return err } @@ -174,7 +174,7 @@ func (sr *immutableRef) setBlob(ctx context.Context, desc ocispec.Descriptor) er return nil } - if err := sr.finalize(ctx, true); err != nil { + if err := sr.finalize(ctx); err != nil { return err } diff --git a/cache/manager.go b/cache/manager.go index 854a8783abc8..1f3dfa9429ef 100644 --- a/cache/manager.go +++ b/cache/manager.go @@ -128,11 +128,11 @@ func (cm *cacheManager) GetByBlob(ctx context.Context, desc ocispec.Descriptor, if err != nil { return nil, err } - if err := p2.Finalize(ctx, true); err != nil { + p = p2.(*immutableRef) + if err := p.finalizeLocked(ctx); err != nil { return nil, err } - parentID = p2.ID() - p = p2.(*immutableRef) + parentID = p.ID() } releaseParent := false @@ -475,7 +475,7 @@ func (cm *cacheManager) New(ctx context.Context, s ImmutableRef, sess session.Gr } parent = p.(*immutableRef) } - if err := parent.Finalize(ctx, true); err != nil { + if err := parent.finalizeLocked(ctx); err != nil { return nil, err } if err := parent.Extract(ctx, sess); err != nil { diff --git a/cache/manager_test.go b/cache/manager_test.go index ce4d6455fe56..803e889d4758 100644 --- a/cache/manager_test.go +++ b/cache/manager_test.go @@ -209,7 +209,7 @@ func TestManager(t *testing.T) { checkDiskUsage(ctx, t, cm, 1, 0) - err = snap.Finalize(ctx, true) + err = snap.(*immutableRef).finalizeLocked(ctx) require.NoError(t, err) err = snap.Release(ctx) @@ -867,7 +867,7 @@ func TestLazyCommit(t *testing.T) { require.NoError(t, err) // this time finalize commit - err = snap.Finalize(ctx, true) + err = snap.(*immutableRef).finalizeLocked(ctx) require.NoError(t, err) err = snap.Release(ctx) @@ -941,7 +941,7 @@ func TestLazyCommit(t *testing.T) { snap2, err = cm.Get(ctx, snap.ID()) require.NoError(t, err) - err = snap2.Finalize(ctx, true) + err = snap2.(*immutableRef).finalizeLocked(ctx) require.NoError(t, err) err = snap2.Release(ctx) diff --git a/cache/refs.go b/cache/refs.go index d7905d7ed1c3..cd0d0c8513ab 100644 --- a/cache/refs.go +++ b/cache/refs.go @@ -41,7 +41,6 @@ type Ref interface { type ImmutableRef interface { Ref Parent() ImmutableRef - Finalize(ctx context.Context, commit bool) error // Make sure reference is flushed to driver Clone() ImmutableRef Info() RefInfo @@ -255,7 +254,7 @@ func (cr *cacheRecord) mount(ctx context.Context, readonly bool) (snapshot.Mount return setReadonly(m), nil } - if err := cr.finalize(ctx, true); err != nil { + if err := cr.finalize(ctx); err != nil { return nil, err } if cr.viewMount == nil { // TODO: handle this better @@ -717,29 +716,22 @@ func (sr *immutableRef) release(ctx context.Context) error { return nil } -func (sr *immutableRef) Finalize(ctx context.Context, b bool) error { +func (sr *immutableRef) finalizeLocked(ctx context.Context) error { sr.mu.Lock() defer sr.mu.Unlock() - - return sr.finalize(ctx, b) + return sr.finalize(ctx) } func (cr *cacheRecord) Metadata() *metadata.StorageItem { return cr.md } -func (cr *cacheRecord) finalize(ctx context.Context, commit bool) error { +// caller must hold cacheRecord.mu +func (cr *cacheRecord) finalize(ctx context.Context) error { mutable := cr.equalMutable if mutable == nil { return nil } - if !commit { - if HasCachePolicyRetain(mutable) { - CachePolicyRetain(mutable) - return mutable.Metadata().Commit() - } - return nil - } _, err := cr.cm.ManagerOpt.LeaseManager.Create(ctx, func(l *leases.Lease) error { l.ID = cr.ID() diff --git a/solver/llbsolver/bridge.go b/solver/llbsolver/bridge.go index e6a5850fef01..65732710f0f8 100644 --- a/solver/llbsolver/bridge.go +++ b/solver/llbsolver/bridge.go @@ -105,16 +105,7 @@ func (b *llbBridge) loadResult(ctx context.Context, def *pb.Definition, cacheImp if err != nil { return nil, err } - wr, ok := res.Sys().(*worker.WorkerRef) - if !ok { - return nil, errors.Errorf("invalid reference for exporting: %T", res.Sys()) - } - if wr.ImmutableRef != nil { - if err := wr.ImmutableRef.Finalize(ctx, false); err != nil { - return nil, err - } - } - return res, err + return res, nil } func (b *llbBridge) Solve(ctx context.Context, req frontend.SolveRequest, sid string) (res *frontend.Result, err error) {