Repository: buchgr/bazel-remote
Pull request #193 (2020-02-14 13:11:04)

make Contains also return the size (or -1 if unknown)
In order to implement the remote asset API, we will need to check for blobs and return a Digest. At the moment, our Contains function only returns a boolean, but not the size of the item which is also required.

In most cases, we have the size information when Contains returns true, but the value wasn't being plumbed through.

In case a proxy backend does not provide size information, return -1.


Patch:
diff --git a/cache/cache.go b/cache/cache.go
index 061f654..d729fc7 100644
--- a/cache/cache.go
+++ b/cache/cache.go
@@ -59,6 +59,7 @@ type CacheProxy interface {
 	Get(kind EntryKind, hash string) (io.ReadCloser, int64, error)
 
 	// Contains returns whether or not the cache item exists on the
-	// remote end.
-	Contains(kind EntryKind, hash string) bool
+	// remote end, and the size if it exists (and -1 if the size is
+	// unknown).
+	Contains(kind EntryKind, hash string) (bool, int64)
 }
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index 7d435bc..40b49d3 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -521,32 +521,41 @@ func (c *DiskCache) Get(kind cache.EntryKind, hash string) (io.ReadCloser, int64
 	return nil, -1, err
 }
 
-// Contains returns true if the `hash` key exists in the cache.
+// Contains returns true if the `hash` key exists in the cache, and
+// the size if known (or -1 if unknown).
+//
 // If there is a local cache miss, the proxy backend (if there is
 // one) will be checked.
-func (c *DiskCache) Contains(kind cache.EntryKind, hash string) bool {
+func (c *DiskCache) Contains(kind cache.EntryKind, hash string) (bool, int64) {
 
 	// The hash format is checked properly in the http/grpc code.
 	// Just perform a simple/fast check here, to catch bad tests.
 	if len(hash) != sha256HashStrSize {
-		return false
+		return false, int64(-1)
 	}
 
+	var foundLocally bool
+	size := int64(-1)
+
 	c.mu.Lock()
 	val, found := c.lru.Get(cacheKey(kind, hash))
 	// Uncommitted (i.e. uploading items) should be reported as not ok
-	foundLocally := found && val.(*lruItem).committed
+	if found {
+		item := val.(*lruItem)
+		foundLocally = item.committed
+		size = item.size
+	}
 	c.mu.Unlock()
 
 	if foundLocally {
-		return true
+		return true, size
 	}
 
 	if c.proxy != nil {
 		return c.proxy.Contains(kind, hash)
 	}
 
-	return false
+	return false, int64(-1)
 }
 
 // MaxSize returns the maximum cache size in bytes.
@@ -608,7 +617,8 @@ func (c *DiskCache) GetValidatedActionResult(hash string) (*pb.ActionResult, []b
 
 	for _, f := range result.OutputFiles {
 		if len(f.Contents) == 0 && f.Digest.SizeBytes > 0 {
-			if !c.Contains(cache.CAS, f.Digest.Hash) {
+			found, _ := c.Contains(cache.CAS, f.Digest.Hash)
+			if !found {
 				return nil, nil, nil // aka "not found"
 			}
 		}
@@ -645,7 +655,8 @@ func (c *DiskCache) GetValidatedActionResult(hash string) (*pb.ActionResult, []b
 			if f.Digest == nil {
 				continue
 			}
-			if !c.Contains(cache.CAS, f.Digest.Hash) {
+			found, _ := c.Contains(cache.CAS, f.Digest.Hash)
+			if !found {
 				return nil, nil, nil // aka "not found"
 			}
 		}
@@ -655,7 +666,8 @@ func (c *DiskCache) GetValidatedActionResult(hash string) (*pb.ActionResult, []b
 				if f.Digest == nil {
 					continue
 				}
-				if !c.Contains(cache.CAS, f.Digest.Hash) {
+				found, _ := c.Contains(cache.CAS, f.Digest.Hash)
+				if !found {
 					return nil, nil, nil // aka "not found"
 				}
 			}
@@ -663,13 +675,15 @@ func (c *DiskCache) GetValidatedActionResult(hash string) (*pb.ActionResult, []b
 	}
 
 	if result.StdoutDigest != nil && result.StdoutDigest.SizeBytes > 0 {
-		if !c.Contains(cache.CAS, result.StdoutDigest.Hash) {
+		found, _ := c.Contains(cache.CAS, result.StdoutDigest.Hash)
+		if !found {
 			return nil, nil, nil // aka "not found"
 		}
 	}
 
 	if result.StderrDigest != nil && result.StderrDigest.SizeBytes > 0 {
-		if !c.Contains(cache.CAS, result.StderrDigest.Hash) {
+		found, _ := c.Contains(cache.CAS, result.StderrDigest.Hash)
+		if !found {
 			return nil, nil, nil // aka "not found"
 		}
 	}
diff --git a/cache/http/http.go b/cache/http/http.go
index 51bf052..2780b81 100644
--- a/cache/http/http.go
+++ b/cache/http/http.go
@@ -172,16 +172,16 @@ func (r *remoteHTTPProxyCache) Get(kind cache.EntryKind, hash string) (io.ReadCl
 	return rsp.Body, sizeBytes, err
 }
 
-func (r *remoteHTTPProxyCache) Contains(kind cache.EntryKind, hash string) bool {
+func (r *remoteHTTPProxyCache) Contains(kind cache.EntryKind, hash string) (bool, int64) {
 
 	url := requestURL(r.baseURL, hash, kind)
 
 	rsp, err := r.remote.Head(url)
 	if err == nil && rsp.StatusCode == http.StatusOK {
-		return true
+		return true, rsp.ContentLength
 	}
 
-	return false
+	return false, int64(-1)
 }
 
 func requestURL(baseURL *url.URL, hash string, kind cache.EntryKind) string {
diff --git a/cache/s3/s3.go b/cache/s3/s3.go
index 068e4ef..b633aed 100644
--- a/cache/s3/s3.go
+++ b/cache/s3/s3.go
@@ -157,9 +157,10 @@ func (c *s3Cache) Get(kind cache.EntryKind, hash string) (io.ReadCloser, int64,
 	return object, info.Size, nil
 }
 
-func (c *s3Cache) Contains(kind cache.EntryKind, hash string) bool {
+func (c *s3Cache) Contains(kind cache.EntryKind, hash string) (bool, int64) {
+	size := int64(-1)
 
-	_, err := c.mcore.StatObject(
+	s, err := c.mcore.StatObject(
 		c.bucket,                  // bucketName
 		c.objectKey(hash, kind),   // objectName
 		minio.StatObjectOptions{}, // opts
@@ -168,8 +169,11 @@ func (c *s3Cache) Contains(kind cache.EntryKind, hash string) bool {
 	exists := (err == nil)
 	if err != nil {
 		err = errNotFound
+	} else {
+		size = s.Size
 	}
+
 	logResponse(c.accessLogger, "CONTAINS", c.bucket, c.objectKey(hash, kind), err)
 
-	return exists
+	return exists, size
 }
diff --git a/server/grpc_ac.go b/server/grpc_ac.go
index 0056285..bebf3e6 100644
--- a/server/grpc_ac.go
+++ b/server/grpc_ac.go
@@ -113,7 +113,8 @@ func (s *grpcServer) maybeInline(inline bool, slice *[]byte, digest **pb.Digest,
 			}
 		}
 
-		if !s.cache.Contains(cache.CAS, (*digest).Hash) {
+		found, _ := s.cache.Contains(cache.CAS, (*digest).Hash)
+		if !found {
 			err := s.cache.Put(cache.CAS, (*digest).Hash, (*digest).SizeBytes,
 				bytes.NewReader(*slice))
 			if err != nil {
diff --git a/server/grpc_cas.go b/server/grpc_cas.go
index 62853d9..8f34234 100644
--- a/server/grpc_cas.go
+++ b/server/grpc_cas.go
@@ -43,7 +43,8 @@ func (s *grpcServer) FindMissingBlobs(ctx context.Context,
 			continue
 		}
 
-		if !s.cache.Contains(cache.CAS, hash) {
+		found, _ := s.cache.Contains(cache.CAS, hash)
+		if !found {
 			s.accessLogger.Printf("GRPC CAS HEAD %s NOT FOUND", hash)
 			resp.MissingBlobDigests = append(resp.MissingBlobDigests, digest)
 		} else {
diff --git a/server/http.go b/server/http.go
index e423d4f..367c87d 100644
--- a/server/http.go
+++ b/server/http.go
@@ -281,7 +281,7 @@ func (h *httpCache) CacheHandler(w http.ResponseWriter, r *http.Request) {
 
 		// Unvalidated path:
 
-		ok := h.cache.Contains(kind, hash)
+		ok, _ := h.cache.Contains(kind, hash)
 		if !ok {
 			http.Error(w, "Not found", http.StatusNotFound)
 			h.logResponse(http.StatusNotFound, r)


Test patch:
diff --git a/cache/disk/disk_test.go b/cache/disk/disk_test.go
index 86934aa..1cb14ab 100644
--- a/cache/disk/disk_test.go
+++ b/cache/disk/disk_test.go
@@ -290,7 +290,7 @@ func TestCacheExistingFiles(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	found := testCache.Contains(cache.CAS, "f53b46209596d170f7659a414c9ff9f6b545cf77ffd6e1cbe9bcc57e1afacfbd")
+	found, _ := testCache.Contains(cache.CAS, "f53b46209596d170f7659a414c9ff9f6b545cf77ffd6e1cbe9bcc57e1afacfbd")
 	if found {
 		t.Fatalf("%s should have been evicted", items[0])
 	}
@@ -361,13 +361,20 @@ func TestMigrateFromOldDirectoryStructure(t *testing.T) {
 	if numItems != 3 {
 		t.Fatalf("Expected test cache size 3 but was %d", numItems)
 	}
-	if !testCache.Contains(cache.AC, acHash) {
+
+	var found bool
+	found, _ = testCache.Contains(cache.AC, acHash)
+	if !found {
 		t.Fatalf("Expected cache to contain AC entry '%s'", acHash)
 	}
-	if !testCache.Contains(cache.CAS, casHash1) {
+
+	found, _ = testCache.Contains(cache.CAS, casHash1)
+	if !found {
 		t.Fatalf("Expected cache to contain CAS entry '%s'", casHash1)
 	}
-	if !testCache.Contains(cache.CAS, casHash2) {
+
+	found, _ = testCache.Contains(cache.CAS, casHash2)
+	if !found {
 		t.Fatalf("Expected cache to contain CAS entry '%s'", casHash2)
 	}
 }
@@ -399,13 +406,21 @@ func TestLoadExistingEntries(t *testing.T) {
 		t.Fatalf("Expected test cache size %d but was %d",
 			numBlobs, numItems)
 	}
-	if !testCache.Contains(cache.AC, acHash) {
+
+	var found bool
+
+	found, _ = testCache.Contains(cache.AC, acHash)
+	if !found {
 		t.Fatalf("Expected cache to contain AC entry '%s'", acHash)
 	}
-	if !testCache.Contains(cache.CAS, casHash) {
+
+	found, _ = testCache.Contains(cache.CAS, casHash)
+	if !found {
 		t.Fatalf("Expected cache to contain CAS entry '%s'", casHash)
 	}
-	if !testCache.Contains(cache.RAW, rawHash) {
+
+	found, _ = testCache.Contains(cache.RAW, rawHash)
+	if !found {
 		t.Fatalf("Expected cache to contain RAW entry '%s'", rawHash)
 	}
 }
@@ -572,7 +587,7 @@ func TestHttpProxyBackend(t *testing.T) {
 	// Confirm that it does not contain the item we added to the
 	// first testCache and the proxy backend.
 
-	found := testCache.Contains(cache.CAS, casHash)
+	found, _ := testCache.Contains(cache.CAS, casHash)
 	if found {
 		t.Fatalf("Expected the cache not to contain %s", casHash)
 	}
@@ -588,7 +603,7 @@ func TestHttpProxyBackend(t *testing.T) {
 	// Add the proxy backend and check that we can Get the item.
 	testCache.proxy = proxy
 
-	found = testCache.Contains(cache.CAS, casHash)
+	found, _ = testCache.Contains(cache.CAS, casHash)
 	if !found {
 		t.Fatalf("Expected the cache to contain %s (via the proxy)",
 			casHash)