Skip to content

Commit

Permalink
[Cache Proxy] always proxy FindMissingBlobs RPCs (#8073)
Browse files Browse the repository at this point in the history
This should address a cache-inconsistency bug that's possible when
artifacts in the proxy outlive those in the backing cache and are
reported as found by the executor logic that checks for the presence of
artifacts before uploading them.
  • Loading branch information
iain-macdonald authored Dec 17, 2024
1 parent 92009f7 commit 76a90df
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 43 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -89,45 +89,9 @@ func (s *CASServerProxy) FindMissingBlobs(ctx context.Context, req *repb.FindMis
defer spn.End()
tracing.AddStringAttributeToCurrentSpan(ctx, "requested-blobs", strconv.Itoa(len(req.BlobDigests)))

// TODO(iain): This will over-aggressively update remote atimes. If it's a
// problem, we can change the logic around to only update atimes for blobs
// that were found locally.
s.atimeUpdater.EnqueueByFindMissingRequest(ctx, req)

resp := &repb.FindMissingBlobsResponse{
MissingBlobDigests: req.BlobDigests,
}
remoteOnly := authutil.EncryptionEnabled(ctx, s.authenticator)
if !remoteOnly {
localResp, err := s.local.FindMissingBlobs(ctx, req)
if err != nil {
return nil, err
}
resp = localResp
}
tracing.AddStringAttributeToCurrentSpan(ctx, "locally-missing-blobs", strconv.Itoa(len(resp.MissingBlobDigests)))
if len(resp.MissingBlobDigests) == 0 {
recordMetrics("FindMissingBlobs", "hit", map[string]int{"hit": len(req.BlobDigests)})
return resp, nil
}

if remoteOnly {
recordMetrics("FindMissingBlobs", "remote-only", map[string]int{"remote-only": len(req.BlobDigests)})
} else if len(resp.MissingBlobDigests) == len(req.BlobDigests) {
recordMetrics("FindMissingBlobs", "miss", map[string]int{"miss": len(req.BlobDigests)})
} else {
recordMetrics("FindMissingBlobs", "partial", map[string]int{
"hit": len(req.BlobDigests) - len(resp.MissingBlobDigests),
"miss": len(resp.MissingBlobDigests),
})
}

remoteReq := repb.FindMissingBlobsRequest{
InstanceName: req.InstanceName,
BlobDigests: resp.MissingBlobDigests,
DigestFunction: req.DigestFunction,
}
return s.remote.FindMissingBlobs(ctx, &remoteReq)
// Always serve FindMissingBlobs requests out of the backing cache to
// avoid possible cache-inconsistency bugs.
return s.remote.FindMissingBlobs(ctx, req)
}

func (s *CASServerProxy) BatchUpdateBlobs(ctx context.Context, req *repb.BatchUpdateBlobsRequest) (*repb.BatchUpdateBlobsResponse, error) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,23 +235,23 @@ func TestFindMissingBlobs(t *testing.T) {
findMissing(ctx, proxy, []*repb.Digest{fooDigestProto}, []*repb.Digest{fooDigestProto}, t)
require.Equal(t, int32(i), requestCount.Load())
}
expectAtimeUpdate(t, clock, requestCount)
expectNoAtimeUpdate(t, clock, requestCount)

update(ctx, proxy, map[*repb.Digest]string{barDigestProto: "bar"}, t)

requestCount.Store(0)
for i := 1; i < 10; i++ {
findMissing(ctx, proxy, []*repb.Digest{barDigestProto}, []*repb.Digest{}, t)
require.Equal(t, int32(0), requestCount.Load())
require.Equal(t, int32(i), requestCount.Load())
}
expectAtimeUpdate(t, clock, requestCount)
expectNoAtimeUpdate(t, clock, requestCount)

requestCount.Store(0)
for i := 1; i < 10; i++ {
findMissing(ctx, proxy, []*repb.Digest{fooDigestProto, barDigestProto}, []*repb.Digest{fooDigestProto}, t)
require.Equal(t, int32(i), requestCount.Load())
}
expectAtimeUpdate(t, clock, requestCount)
expectNoAtimeUpdate(t, clock, requestCount)
}

func TestReadUpdateBlobs(t *testing.T) {
Expand Down

0 comments on commit 76a90df

Please sign in to comment.