s3: fix dedupe failing to manage blobs correctly (#772)

in order to know which blob is 'real' (has content)
we need to know which was the first blob inserted in cache,
because that is always the real one.

because we can not modify the keys order in boltdb we'll do this
by marking the first blob inserted with a value

when GetBlob() return the blob which is marked
when PutBlob() if is the first one, mark it
when DeleteBlob() in case deleted is marked then mark the next blob

Signed-off-by: Petu Eusebiu <peusebiu@cisco.com>

pkg/storage/s3/s3.go

@@ -1273,10 +1273,6 @@ retry:
 			return err
 		}
 
-		if dstRecord == dst {
-			is.log.Warn().Msg("FOUND equal dsts")
-		}
-
 		// prevent overwrite original blob
 		if fileInfo == nil && dstRecord != dst {
 			// put empty file so that we are compliant with oci layout, this will act as a deduped blob
@@ -1286,8 +1282,12 @@ retry:
 
 				return err
 			}
-		} else {
-			is.log.Warn().Msg("prevent overwrite")
+
+			if err := is.cache.PutBlob(dstDigest.String(), dst); err != nil {
+				is.log.Error().Err(err).Str("blobPath", dst).Msg("dedupe: unable to insert blob record")
+
+				return err
+			}
 		}
 
 		// remove temp blobupload
@@ -1445,33 +1445,37 @@ func (is *ObjectStorage) GetBlob(repo, digest, mediaType string) (io.ReadCloser,
 	}
 
 	// is a 'deduped' blob
-	if binfo.Size() == 0 && is.cache != nil {
+	if binfo.Size() == 0 {
 		// Check blobs in cache
 		dstRecord, err := is.checkCacheBlob(digest)
-		if err == nil {
-			binfo, err := is.store.Stat(context.Background(), dstRecord)
-			if err != nil {
-				is.log.Error().Err(err).Str("blob", dstRecord).Msg("failed to stat blob")
+		if err != nil {
+			is.log.Error().Err(err).Str("digest", digest).Msg("cache: not found")
 
-				// the actual blob on disk may have been removed by GC, so sync the cache
-				if err := is.cache.DeleteBlob(digest, dstRecord); err != nil {
-					is.log.Error().Err(err).Str("dstDigest", digest).Str("dst", dstRecord).Msg("dedupe: unable to delete blob record")
+			return nil, -1, zerr.ErrBlobNotFound
+		}
 
-					return nil, -1, err
-				}
+		binfo, err := is.store.Stat(context.Background(), dstRecord)
+		if err != nil {
+			is.log.Error().Err(err).Str("blob", dstRecord).Msg("failed to stat blob")
 
-				return nil, -1, zerr.ErrBlobNotFound
-			}
-
-			blobReadCloser, err := is.store.Reader(context.Background(), dstRecord, 0)
-			if err != nil {
-				is.log.Error().Err(err).Str("blob", dstRecord).Msg("failed to open blob")
+			// the actual blob on disk may have been removed by GC, so sync the cache
+			if err := is.cache.DeleteBlob(digest, dstRecord); err != nil {
+				is.log.Error().Err(err).Str("dstDigest", digest).Str("dst", dstRecord).Msg("dedupe: unable to delete blob record")
 
 				return nil, -1, err
 			}
 
-			return blobReadCloser, binfo.Size(), nil
+			return nil, -1, zerr.ErrBlobNotFound
 		}
+
+		blobReadCloser, err := is.store.Reader(context.Background(), dstRecord, 0)
+		if err != nil {
+			is.log.Error().Err(err).Str("blob", dstRecord).Msg("failed to open blob")
+
+			return nil, -1, err
+		}
+
+		return blobReadCloser, binfo.Size(), nil
 	}
 
 	// The caller function is responsible for calling Close()

pkg/storage/s3/s3_test.go

@@ -1649,6 +1649,22 @@ func TestS3DedupeErr(t *testing.T) {
 		So(err, ShouldNotBeNil)
 	})
 
+	Convey("Test DedupeBlob - error on cache.PutBlob()", t, func(c C) {
+		tdir := t.TempDir()
+		imgStore = createMockStorage(testDir, tdir, true, &StorageDriverMock{
+			StatFn: func(ctx context.Context, path string) (driver.FileInfo, error) {
+				return nil, nil
+			},
+		})
+
+		digest := godigest.NewDigestFromEncoded(godigest.SHA256, "digest")
+		err := imgStore.DedupeBlob("", digest, "dst")
+		So(err, ShouldBeNil)
+
+		err = imgStore.DedupeBlob("", digest, "")
+		So(err, ShouldNotBeNil)
+	})
+
 	Convey("Test DedupeBlob - error on store.Delete()", t, func(c C) {
 		tdir := t.TempDir()
 		imgStore = createMockStorage(testDir, tdir, true, &StorageDriverMock{
@@ -1812,6 +1828,26 @@ func TestS3DedupeErr(t *testing.T) {
 		So(err, ShouldNotBeNil)
 	})
 
+	Convey("Test GetBlob() - error on checkCacheBlob()", t, func(c C) {
+		tdir := t.TempDir()
+
+		digest := godigest.NewDigestFromEncoded(godigest.SHA256,
+			"7173b809ca12ec5dee4506cd86be934c4596dd234ee82c0662eac04a8c2c71dc")
+
+		imgStore = createMockStorage(testDir, tdir, true, &StorageDriverMock{
+			StatFn: func(ctx context.Context, path string) (driver.FileInfo, error) {
+				return &FileInfoMock{
+					SizeFn: func() int64 {
+						return 0
+					},
+				}, nil
+			},
+		})
+
+		_, _, err = imgStore.GetBlob("repo2", digest.String(), "application/vnd.oci.image.layer.v1.tar+gzip")
+		So(err, ShouldNotBeNil)
+	})
+
 	Convey("Test DeleteBlob() - error on store.Move()", t, func(c C) {
 		tdir := t.TempDir()
 		hash := "7173b809ca12ec5dee4506cd86be934c4596dd234ee82c0662eac04a8c2c71dc"