From ebf12dbda08633375ab12387255d3f617ee9be38 Mon Sep 17 00:00:00 2001
From: Vikram bir Singh <vikrambir.singh@docker.com>
Date: Tue, 3 Sep 2019 19:11:21 +0000
Subject: [PATCH] Reimplement iteration over fileInfos in getOrphan.

1. Reduce complexity due to nested if blocks by using early
return/continue
2. Improve logging

Changes suggested as a part of code review comments in 39748

Signed-off-by: Vikram bir Singh <vikrambir.singh@docker.com>
---
 layer/filestore.go | 56 ++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/layer/filestore.go b/layer/filestore.go
index 37a927022a..a1726a5a40 100644
--- a/layer/filestore.go
+++ b/layer/filestore.go
@@ -14,7 +14,7 @@ import (
 
 	"github.com/docker/distribution"
 	"github.com/docker/docker/pkg/ioutils"
-	"github.com/opencontainers/go-digest"
+	digest "github.com/opencontainers/go-digest"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
@@ -317,32 +317,40 @@ func (fms *fileMetadataStore) getOrphan() ([]roLayer, error) {
 		}
 
 		for _, fi := range fileInfos {
-			if fi.IsDir() && strings.Contains(fi.Name(), "-removing") {
-				nameSplit := strings.Split(fi.Name(), "-")
-				dgst := digest.NewDigestFromEncoded(algorithm, nameSplit[0])
-				if err := dgst.Validate(); err != nil {
-					logrus.Debugf("Ignoring invalid digest %s:%s", algorithm, nameSplit[0])
-				} else {
-					chainID := ChainID(dgst)
-					chainFile := filepath.Join(fms.root, string(algorithm), fi.Name(), "cache-id")
-					contentBytes, err := ioutil.ReadFile(chainFile)
-					if err != nil {
-						logrus.WithError(err).WithField("digest", dgst).Error("cannot get cache ID")
-					}
-					cacheID := strings.TrimSpace(string(contentBytes))
-					if cacheID == "" {
-						logrus.Errorf("invalid cache id value")
-					}
-
-					l := &roLayer{
-						chainID: chainID,
-						cacheID: cacheID,
-					}
-					orphanLayers = append(orphanLayers, *l)
-				}
+			if !fi.IsDir() || !strings.HasSuffix(fi.Name(), "-removing") {
+				continue
 			}
+			// At this stage, fi.Name value looks like <digest>-<random>-removing
+			// Split on '-' to get the digest value.
+			nameSplit := strings.Split(fi.Name(), "-")
+			dgst := digest.NewDigestFromEncoded(algorithm, nameSplit[0])
+			if err := dgst.Validate(); err != nil {
+				logrus.WithError(err).WithField("digest", string(algorithm)+":"+nameSplit[0]).Debug("ignoring invalid digest")
+				continue
+			}
+
+			chainFile := filepath.Join(fms.root, string(algorithm), fi.Name(), "cache-id")
+			contentBytes, err := ioutil.ReadFile(chainFile)
+			if err != nil {
+				if !os.IsNotExist(err) {
+					logrus.WithError(err).WithField("digest", dgst).Error("failed to read cache ID")
+				}
+				continue
+			}
+			cacheID := strings.TrimSpace(string(contentBytes))
+			if cacheID == "" {
+				logrus.Error("invalid cache ID")
+				continue
+			}
+
+			l := &roLayer{
+				chainID: ChainID(dgst),
+				cacheID: cacheID,
+			}
+			orphanLayers = append(orphanLayers, *l)
 		}
 	}
+
 	return orphanLayers, nil
 }