From 734caef0f4560d7aa71502a206514a641b45d94b Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Wed, 13 Jul 2016 16:41:51 -0700 Subject: [PATCH 1/9] Fix storage drivers dropping non EOF errors when listing repositories This fixes errors other than io.EOF from being dropped when a storage driver lists repositories. For example, filesystem driver may point to a missing directory and errors, which then gets subsequently dropped. Signed-off-by: Edgar Lee --- registry/storage/catalog.go | 10 ++++---- registry/storage/catalog_test.go | 40 ++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/registry/storage/catalog.go b/registry/storage/catalog.go index 3b13b7ad..51f31691 100644 --- a/registry/storage/catalog.go +++ b/registry/storage/catalog.go @@ -25,12 +25,12 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri return 0, errors.New("no space in slice") } - root, err := pathFor(repositoriesRootPathSpec{}) - if err != nil { - return 0, err + root, errVal := pathFor(repositoriesRootPathSpec{}) + if errVal != nil { + return 0, errVal } - err = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { + errVal = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { filePath := fileInfo.Path() // lop the base path off @@ -58,7 +58,7 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri n = copy(repos, foundRepos) // Signal that we have no more entries by setting EOF - if len(foundRepos) <= len(repos) && err != ErrFinishedWalk { + if len(foundRepos) <= len(repos) && (errVal == nil || errVal == ErrSkipDir) { errVal = io.EOF } diff --git a/registry/storage/catalog_test.go b/registry/storage/catalog_test.go index eb062c5b..40fa909d 100644 --- a/registry/storage/catalog_test.go +++ b/registry/storage/catalog_test.go @@ -1,6 +1,7 @@ package storage import ( + "fmt" "io" "testing" @@ -123,3 +124,42 @@ func testEq(a, b []string, size int) bool { } return true } + +func setupBadWalkEnv(t *testing.T) *setupEnv { + d := newBadListDriver() + ctx := context.Background() + registry, err := NewRegistry(ctx, d, BlobDescriptorCacheProvider(memory.NewInMemoryBlobDescriptorCacheProvider()), EnableRedirect) + if err != nil { + t.Fatalf("error creating registry: %v", err) + } + + return &setupEnv{ + ctx: ctx, + driver: d, + registry: registry, + } +} + +type badListDriver struct { + driver.StorageDriver +} + +var _ driver.StorageDriver = &badListDriver{} + +func newBadListDriver() *badListDriver { + return &badListDriver{StorageDriver: inmemory.New()} +} + +func (d *badListDriver) List(ctx context.Context, path string) ([]string, error) { + return nil, fmt.Errorf("List error") +} + +func TestCatalogWalkError(t *testing.T) { + env := setupBadWalkEnv(t) + p := make([]string, 1) + + _, err := env.registry.Repositories(env.ctx, p, "") + if err == io.EOF { + t.Errorf("Expected catalog driver list error") + } +} From 22a59f25125cac602cb1bb3ded9743dd9973f1b3 Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Thu, 14 Jul 2016 13:28:08 -0700 Subject: [PATCH 2/9] Refactor errVal named parameter for catalog repositories to err Signed-off-by: Edgar Lee --- registry/storage/catalog.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/registry/storage/catalog.go b/registry/storage/catalog.go index 51f31691..aec5f2e6 100644 --- a/registry/storage/catalog.go +++ b/registry/storage/catalog.go @@ -18,19 +18,19 @@ var ErrFinishedWalk = errors.New("finished walk") // Returns a list, or partial list, of repositories in the registry. // Because it's a quite expensive operation, it should only be used when building up // an initial set of repositories. -func (reg *registry) Repositories(ctx context.Context, repos []string, last string) (n int, errVal error) { +func (reg *registry) Repositories(ctx context.Context, repos []string, last string) (n int, err error) { var foundRepos []string if len(repos) == 0 { return 0, errors.New("no space in slice") } - root, errVal := pathFor(repositoriesRootPathSpec{}) - if errVal != nil { - return 0, errVal + root, err := pathFor(repositoriesRootPathSpec{}) + if err != nil { + return 0, err } - errVal = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { + err = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { filePath := fileInfo.Path() // lop the base path off @@ -58,11 +58,11 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri n = copy(repos, foundRepos) // Signal that we have no more entries by setting EOF - if len(foundRepos) <= len(repos) && (errVal == nil || errVal == ErrSkipDir) { - errVal = io.EOF + if len(foundRepos) <= len(repos) && (err == nil || err == ErrSkipDir) { + err = io.EOF } - return n, errVal + return n, err } // Enumerate applies ingester to each repository From 0567fa3c2ad0414d68e73b88007170b5521e591c Mon Sep 17 00:00:00 2001 From: Sebastien Coavoux Date: Thu, 21 Jul 2016 10:38:42 -0400 Subject: [PATCH 3/9] Fix: Compare path properly when list repository in catalog. #1854 Signed-off-by: Sebastien Coavoux --- registry/storage/catalog.go | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/registry/storage/catalog.go b/registry/storage/catalog.go index aec5f2e6..89616402 100644 --- a/registry/storage/catalog.go +++ b/registry/storage/catalog.go @@ -39,7 +39,7 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri _, file := path.Split(repoPath) if file == "_layers" { repoPath = strings.TrimSuffix(repoPath, "/_layers") - if repoPath > last { + if pathGreaterThan(repoPath, last) { foundRepos = append(foundRepos, repoPath) } return ErrSkipDir @@ -95,3 +95,23 @@ func (reg *registry) Enumerate(ctx context.Context, ingester func(string) error) return nil } + +func pathGreaterThan(pathX, pathY string) (b bool) { + splitPathX := strings.SplitN(pathX, "/", 2) + splitPathY := strings.SplitN(pathY, "/", 2) + + if splitPathX[0] == splitPathY[0] { + if len(splitPathX) == 1 && len(splitPathY) == 1 { + return false + } else if len(splitPathX) == 1 && len(splitPathY) != 1 { + return false + } else if len(splitPathX) != 1 && len(splitPathY) == 1 { + return true + } + + return pathGreaterThan(splitPathX[1], splitPathY[1]) + + } + + return splitPathX[0] > splitPathY[0] +} From fdc51bb1f271696f9a3c52592ebb4c74fe64515c Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Fri, 5 Aug 2016 17:21:48 -0700 Subject: [PATCH 4/9] Stop ErrFinishedWalk from escaping from Repositories walk Signed-off-by: Edgar Lee --- registry/storage/catalog.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/registry/storage/catalog.go b/registry/storage/catalog.go index 89616402..5a00f7cd 100644 --- a/registry/storage/catalog.go +++ b/registry/storage/catalog.go @@ -10,11 +10,6 @@ import ( "github.com/docker/distribution/registry/storage/driver" ) -// ErrFinishedWalk is used when the called walk function no longer wants -// to accept any more values. This is used for pagination when the -// required number of repos have been found. -var ErrFinishedWalk = errors.New("finished walk") - // Returns a list, or partial list, of repositories in the registry. // Because it's a quite expensive operation, it should only be used when building up // an initial set of repositories. @@ -30,6 +25,10 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri return 0, err } + // errFinishedWalk signals an early exit to the walk when the current query + // is satisfied. + errFinishedWalk := errors.New("finished walk") + err = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { filePath := fileInfo.Path() @@ -49,7 +48,7 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri // if we've filled our array, no need to walk any further if len(foundRepos) == len(repos) { - return ErrFinishedWalk + return errFinishedWalk } return nil @@ -57,9 +56,14 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri n = copy(repos, foundRepos) - // Signal that we have no more entries by setting EOF - if len(foundRepos) <= len(repos) && (err == nil || err == ErrSkipDir) { + switch err { + case nil: + // nil means that we completed walk and didn't fill buffer. No more + // records are available. err = io.EOF + case errFinishedWalk: + // more records are available. + err = nil } return n, err From 2aa09ff9a822d523f71db6d37f378366cf55d42f Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Fri, 5 Aug 2016 17:18:43 -0700 Subject: [PATCH 5/9] registry/storage: more efficient path compare in catalog Previous component-wise path comparison is recursive and generates a large amount of garbage. This more efficient version simply replaces the path comparison with the zero-value to sort before everything. We do this by replacing the byte-wise comparison that swaps a single character inline for the separator comparison, such that separators sort first. The resulting implementation provides component-wise path comparison with no cost incurred for allocation or stack frame. Direction of the comparison is also reversed to match Go style. Signed-off-by: Stephen J Day --- registry/storage/catalog.go | 59 +++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 16 deletions(-) diff --git a/registry/storage/catalog.go b/registry/storage/catalog.go index 5a00f7cd..0e119a88 100644 --- a/registry/storage/catalog.go +++ b/registry/storage/catalog.go @@ -38,7 +38,7 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri _, file := path.Split(repoPath) if file == "_layers" { repoPath = strings.TrimSuffix(repoPath, "/_layers") - if pathGreaterThan(repoPath, last) { + if lessPath(last, repoPath) { foundRepos = append(foundRepos, repoPath) } return ErrSkipDir @@ -100,22 +100,49 @@ func (reg *registry) Enumerate(ctx context.Context, ingester func(string) error) } -func pathGreaterThan(pathX, pathY string) (b bool) { - splitPathX := strings.SplitN(pathX, "/", 2) - splitPathY := strings.SplitN(pathY, "/", 2) - - if splitPathX[0] == splitPathY[0] { - if len(splitPathX) == 1 && len(splitPathY) == 1 { - return false - } else if len(splitPathX) == 1 && len(splitPathY) != 1 { - return false - } else if len(splitPathX) != 1 && len(splitPathY) == 1 { - return true - } - - return pathGreaterThan(splitPathX[1], splitPathY[1]) +// lessPath returns true if one path a is less than path b. +// +// A component-wise comparison is done, rather than the lexical comparison of +// strings. +func lessPath(a, b string) bool { + // we provide this behavior by making separator always sort first. + return compareReplaceInline(a, b, '/', '\x00') < 0 +} +// compareReplaceInline modifies runtime.cmpstring to replace old with new +// during a byte-wise comparison. +func compareReplaceInline(s1, s2 string, old, new byte) int { + l := len(s1) + if len(s2) < l { + l = len(s2) } - return splitPathX[0] > splitPathY[0] + for i := 0; i < l; i++ { + c1, c2 := s1[i], s2[i] + if c1 == old { + c1 = new + } + + if c2 == old { + c2 = new + } + + if c1 < c2 { + return -1 + } + + if c1 > c2 { + return +1 + } + } + + if len(s1) < len(s2) { + return -1 + } + + if len(s1) > len(s2) { + return +1 + } + + return 0 } From a405d3e88b8b455ac1e46e38a5cbaec23c64ccd8 Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Tue, 9 Aug 2016 17:42:26 -0700 Subject: [PATCH 6/9] Improve catalog enumerate runtime by an order of magnitude Signed-off-by: Edgar Lee --- registry/storage/catalog.go | 87 ++++++++++++++++---------------- registry/storage/catalog_test.go | 21 ++++++++ 2 files changed, 65 insertions(+), 43 deletions(-) diff --git a/registry/storage/catalog.go b/registry/storage/catalog.go index 0e119a88..7a8a2ac6 100644 --- a/registry/storage/catalog.go +++ b/registry/storage/catalog.go @@ -10,6 +10,10 @@ import ( "github.com/docker/distribution/registry/storage/driver" ) +// errFinishedWalk signals an early exit to the walk when the current query +// is satisfied. +var errFinishedWalk = errors.New("finished walk") + // Returns a list, or partial list, of repositories in the registry. // Because it's a quite expensive operation, it should only be used when building up // an initial set of repositories. @@ -25,25 +29,13 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri return 0, err } - // errFinishedWalk signals an early exit to the walk when the current query - // is satisfied. - errFinishedWalk := errors.New("finished walk") - err = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { - filePath := fileInfo.Path() - - // lop the base path off - repoPath := filePath[len(root)+1:] - - _, file := path.Split(repoPath) - if file == "_layers" { - repoPath = strings.TrimSuffix(repoPath, "/_layers") - if lessPath(last, repoPath) { - foundRepos = append(foundRepos, repoPath) - } - return ErrSkipDir - } else if strings.HasPrefix(file, "_") { - return ErrSkipDir + err := handleRepository(fileInfo, root, last, func(repoPath string) error { + foundRepos = append(foundRepos, repoPath) + return nil + }) + if err != nil { + return err } // if we've filled our array, no need to walk any further @@ -71,33 +63,16 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri // Enumerate applies ingester to each repository func (reg *registry) Enumerate(ctx context.Context, ingester func(string) error) error { - repoNameBuffer := make([]string, 100) - var last string - for { - n, err := reg.Repositories(ctx, repoNameBuffer, last) - if err != nil && err != io.EOF { - return err - } - - if n == 0 { - break - } - - last = repoNameBuffer[n-1] - for i := 0; i < n; i++ { - repoName := repoNameBuffer[i] - err = ingester(repoName) - if err != nil { - return err - } - } - - if err == io.EOF { - break - } + root, err := pathFor(repositoriesRootPathSpec{}) + if err != nil { + return err } - return nil + err = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { + return handleRepository(fileInfo, root, "", ingester) + }) + + return err } // lessPath returns true if one path a is less than path b. @@ -146,3 +121,29 @@ func compareReplaceInline(s1, s2 string, old, new byte) int { return 0 } + +// handleRepository calls function fn with a repository path if fileInfo +// has a path of a repository under root and that it is lexographically +// after last. Otherwise, it will return ErrSkipDir. This should be used +// with Walk to do handling with repositories in a storage. +func handleRepository(fileInfo driver.FileInfo, root, last string, fn func(repoPath string) error) error { + filePath := fileInfo.Path() + + // lop the base path off + repo := filePath[len(root)+1:] + + _, file := path.Split(repo) + if file == "_layers" { + repo = strings.TrimSuffix(repo, "/_layers") + if lessPath(last, repo) { + if err := fn(repo); err != nil { + return err + } + } + return ErrSkipDir + } else if strings.HasPrefix(file, "_") { + return ErrSkipDir + } + + return nil +} diff --git a/registry/storage/catalog_test.go b/registry/storage/catalog_test.go index 40fa909d..7fe133f6 100644 --- a/registry/storage/catalog_test.go +++ b/registry/storage/catalog_test.go @@ -113,7 +113,28 @@ func TestCatalogInParts(t *testing.T) { if !testEq(p, env.expected[chunkLen*2:chunkLen*3-1], numFilled) { t.Errorf("Expected catalog third chunk err") } +} +func TestCatalogEnumerate(t *testing.T) { + env := setupFS(t) + + var repos []string + repositoryEnumerator := env.registry.(distribution.RepositoryEnumerator) + err := repositoryEnumerator.Enumerate(env.ctx, func(repoName string) error { + repos = append(repos, repoName) + return nil + }) + if err != nil { + t.Errorf("Expected catalog enumerate err") + } + + if len(repos) != len(env.expected) { + t.Errorf("Expected catalog enumerate doesn't have correct number of values") + } + + if !testEq(repos, env.expected, len(env.expected)) { + t.Errorf("Expected catalog enumerate not over all values") + } } func testEq(a, b []string, size int) bool { From 8160a430be34e16abe38f8a48754c5e43fd23323 Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Thu, 14 Jul 2016 13:24:16 -0700 Subject: [PATCH 7/9] Handle new errors returned from catalog repository listing Signed-off-by: Edgar Lee --- registry/handlers/catalog.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/registry/handlers/catalog.go b/registry/handlers/catalog.go index 6ec1fe55..083ebfd0 100644 --- a/registry/handlers/catalog.go +++ b/registry/handlers/catalog.go @@ -6,9 +6,12 @@ import ( "io" "net/http" "net/url" + "reflect" "strconv" "github.com/docker/distribution/registry/api/errcode" + "github.com/docker/distribution/registry/storage" + "github.com/docker/distribution/registry/storage/driver" "github.com/gorilla/handlers" ) @@ -45,9 +48,10 @@ func (ch *catalogHandler) GetCatalog(w http.ResponseWriter, r *http.Request) { repos := make([]string, maxEntries) filled, err := ch.App.registry.Repositories(ch.Context, repos, lastEntry) - if err == io.EOF { + + if err == io.EOF || reflect.TypeOf(err) == reflect.TypeOf(driver.PathNotFoundError{}) { moreEntries = false - } else if err != nil { + } else if err != nil && err != storage.ErrFinishedWalk { ch.Errors = append(ch.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) return } From 45b84c95129de5f4bcf7f8c1e013d23ca9b9f136 Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Thu, 14 Jul 2016 15:03:18 -0700 Subject: [PATCH 8/9] Use typecast over reflect for error type checking Signed-off-by: Edgar Lee --- registry/handlers/catalog.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/registry/handlers/catalog.go b/registry/handlers/catalog.go index 083ebfd0..4e95bfb0 100644 --- a/registry/handlers/catalog.go +++ b/registry/handlers/catalog.go @@ -6,7 +6,6 @@ import ( "io" "net/http" "net/url" - "reflect" "strconv" "github.com/docker/distribution/registry/api/errcode" @@ -48,8 +47,9 @@ func (ch *catalogHandler) GetCatalog(w http.ResponseWriter, r *http.Request) { repos := make([]string, maxEntries) filled, err := ch.App.registry.Repositories(ch.Context, repos, lastEntry) + _, pathNotFound := err.(driver.PathNotFoundError) - if err == io.EOF || reflect.TypeOf(err) == reflect.TypeOf(driver.PathNotFoundError{}) { + if err == io.EOF || pathNotFound { moreEntries = false } else if err != nil && err != storage.ErrFinishedWalk { ch.Errors = append(ch.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) From 12acdf0a6c1e56d965ac6eb395d2bce687bf22fc Mon Sep 17 00:00:00 2001 From: Edgar Lee Date: Fri, 5 Aug 2016 17:21:48 -0700 Subject: [PATCH 9/9] Stop ErrFinishedWalk from escaping from Repositories walk Signed-off-by: Edgar Lee --- registry/handlers/catalog.go | 3 +-- registry/storage/catalog.go | 4 ++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/registry/handlers/catalog.go b/registry/handlers/catalog.go index 4e95bfb0..eca98468 100644 --- a/registry/handlers/catalog.go +++ b/registry/handlers/catalog.go @@ -9,7 +9,6 @@ import ( "strconv" "github.com/docker/distribution/registry/api/errcode" - "github.com/docker/distribution/registry/storage" "github.com/docker/distribution/registry/storage/driver" "github.com/gorilla/handlers" ) @@ -51,7 +50,7 @@ func (ch *catalogHandler) GetCatalog(w http.ResponseWriter, r *http.Request) { if err == io.EOF || pathNotFound { moreEntries = false - } else if err != nil && err != storage.ErrFinishedWalk { + } else if err != nil { ch.Errors = append(ch.Errors, errcode.ErrorCodeUnknown.WithDetail(err)) return } diff --git a/registry/storage/catalog.go b/registry/storage/catalog.go index 7a8a2ac6..eb669b77 100644 --- a/registry/storage/catalog.go +++ b/registry/storage/catalog.go @@ -29,6 +29,10 @@ func (reg *registry) Repositories(ctx context.Context, repos []string, last stri return 0, err } + // errFinishedWalk signals an early exit to the walk when the current query + // is satisfied. + errFinishedWalk := errors.New("finished walk") + err = Walk(ctx, reg.blobStore.driver, root, func(fileInfo driver.FileInfo) error { err := handleRepository(fileInfo, root, last, func(repoPath string) error { foundRepos = append(foundRepos, repoPath)