From b1f882e1b8f3ed938ced922eef4cf3b27eada04f Mon Sep 17 00:00:00 2001 From: Ramkumar Chinchani Date: Mon, 6 Apr 2020 18:17:24 -0700 Subject: [PATCH 1/2] conformance: align with upstream conformance tests Upstream conformance tests are being updated, so we need to align along with our internal GC and dedupe features. Add a new example config file which plays nice with conformance tests. DeleteImageManifest() updated to deal with the case where the same manifest can be created with multiple tags and deleted with the same digest - so all entries must be deleted. DeleteBlob() delete the digest key (bucket) when last reference is dropped --- examples/config-conformance.json | 15 ++++++++++ pkg/api/routes.go | 7 ++++- pkg/compliance/v1_0_0/check.go | 6 +++- pkg/storage/cache.go | 11 +++++++ pkg/storage/storage.go | 51 ++++++++++++++++++-------------- pkg/storage/storage_test.go | 10 +++++++ 6 files changed, 75 insertions(+), 25 deletions(-) create mode 100644 examples/config-conformance.json diff --git a/examples/config-conformance.json b/examples/config-conformance.json new file mode 100644 index 00000000..c44c4ff9 --- /dev/null +++ b/examples/config-conformance.json @@ -0,0 +1,15 @@ +{ + "version":"0.1.0-dev", + "storage":{ + "rootDirectory":"/tmp/zot", + "gc": false, + "dedupe": false + }, + "http": { + "address":"127.0.0.1", + "port":"8080" + }, + "log":{ + "level":"debug" + } +} diff --git a/pkg/api/routes.go b/pkg/api/routes.go index 3f79607c..6ec6574c 100644 --- a/pkg/api/routes.go +++ b/pkg/api/routes.go @@ -222,7 +222,12 @@ func (rh *RouteHandler) ListTags(w http.ResponseWriter, r *http.Request) { pTags.Tags = tags[i+1 : i+1+n] } - last = pTags.Tags[len(pTags.Tags)-1] + if len(pTags.Tags) == 0 { + last = "" + } else { + last = pTags.Tags[len(pTags.Tags)-1] + } + w.Header().Set("Link", fmt.Sprintf("/v2/%s/tags/list?n=%d&last=%s; rel=\"next\"", name, n, last)) WriteJSON(w, http.StatusOK, pTags) diff --git a/pkg/compliance/v1_0_0/check.go b/pkg/compliance/v1_0_0/check.go index 3b1df400..47e125a7 100644 --- a/pkg/compliance/v1_0_0/check.go +++ b/pkg/compliance/v1_0_0/check.go @@ -565,7 +565,7 @@ func CheckWorkflows(t *testing.T, config *compliance.Config) { // delete manifest by digest resp, err = resty.R().Delete(baseURL + "/v2/repo7/manifests/" + digest.String()) So(err, ShouldBeNil) - So(resp.StatusCode(), ShouldEqual, 202) + So(resp.StatusCode(), ShouldEqual, 404) // delete again should fail resp, err = resty.R().Delete(baseURL + "/v2/repo7/manifests/" + digest.String()) So(err, ShouldBeNil) @@ -656,6 +656,10 @@ func CheckWorkflows(t *testing.T, config *compliance.Config) { So(err, ShouldBeNil) So(resp.StatusCode(), ShouldEqual, 200) + resp, err = resty.R().Get(baseURL + "/v2/page0/tags/list?n=0") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, 200) + resp, err = resty.R().Get(baseURL + "/v2/page0/tags/list?n=3") So(err, ShouldBeNil) So(resp.StatusCode(), ShouldEqual, 200) diff --git a/pkg/storage/cache.go b/pkg/storage/cache.go index 5c7a0758..1c77f76b 100644 --- a/pkg/storage/cache.go +++ b/pkg/storage/cache.go @@ -154,6 +154,17 @@ func (c *Cache) DeleteBlob(digest string, path string) error { return err } + cur := b.Cursor() + k, _ := cur.First() + + if k == nil { + c.log.Debug().Str("digest", digest).Str("path", path).Msg("deleting empty bucket") + if err := root.DeleteBucket([]byte(digest)); err != nil { + c.log.Error().Err(err).Str("digest", digest).Str("path", path).Msg("unable to delete") + return err + } + } + return nil }); err != nil { return err diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 6f64da95..53733489 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -488,7 +488,7 @@ func (is *ImageStore) DeleteImageManifest(repo string, reference string) error { } // as per spec "reference" can only be a digest and not a tag - _, err := godigest.Parse(reference) + digest, err := godigest.Parse(reference) if err != nil { is.log.Error().Err(err).Msg("invalid reference") return errors.ErrBadManifest @@ -509,33 +509,29 @@ func (is *ImageStore) DeleteImageManifest(repo string, reference string) error { found := false - var digest godigest.Digest - - var i int - var m ispec.Descriptor - for i, m = range index.Manifests { - if reference == m.Digest.String() { - digest = m.Digest - found = true + // we are deleting, so keep only those manifests that don't match + outIndex := index + outIndex.Manifests = []ispec.Descriptor{} - break + for _, m = range index.Manifests { + if reference == m.Digest.String() { + found = true + continue } + + outIndex.Manifests = append(outIndex.Manifests, m) } if !found { return errors.ErrManifestNotFound } - // remove the manifest entry, not preserving order - index.Manifests[i] = index.Manifests[len(index.Manifests)-1] - index.Manifests = index.Manifests[:len(index.Manifests)-1] - // now update "index.json" dir = path.Join(is.rootDir, repo) file := path.Join(dir, "index.json") - buf, err = json.Marshal(index) + buf, err = json.Marshal(outIndex) if err != nil { return err @@ -809,44 +805,53 @@ func (is *ImageStore) FullBlobUpload(repo string, body io.Reader, digest string) // nolint (interfacer) func (is *ImageStore) DedupeBlob(src string, dstDigest godigest.Digest, dst string) error { +retry: + is.log.Debug().Str("src", src).Str("dstDigest", dstDigest.String()).Str("dst", dst).Msg("dedupe: ENTER") dstRecord, err := is.cache.GetBlob(dstDigest.String()) if err != nil && err != errors.ErrCacheMiss { - is.log.Error().Err(err).Str("blobPath", dst).Msg("unable to lookup blob record") + is.log.Error().Err(err).Str("blobPath", dst).Msg("dedupe: unable to lookup blob record") return err } if dstRecord == "" { if err := is.cache.PutBlob(dstDigest.String(), dst); err != nil { - is.log.Error().Err(err).Str("blobPath", dst).Msg("unable to insert blob record") + is.log.Error().Err(err).Str("blobPath", dst).Msg("dedupe: unable to insert blob record") return err } // move the blob from uploads to final dest if err := os.Rename(src, dst); err != nil { - is.log.Error().Err(err).Str("src", src).Str("dst", dst).Msg("unable to rename blob") + is.log.Error().Err(err).Str("src", src).Str("dst", dst).Msg("dedupe: unable to rename blob") return err } + is.log.Debug().Str("src", src).Str("dst", dst).Msg("dedupe: rename") } else { dstRecordFi, err := os.Stat(dstRecord) if err != nil { - is.log.Error().Err(err).Str("blobPath", dstRecord).Msg("unable to stat") - return err + is.log.Error().Err(err).Str("blobPath", dstRecord).Msg("dedupe: unable to stat") + // the actual blob on disk may have been removed by GC, so sync the cache + if err := is.cache.DeleteBlob(dstDigest.String(), dst); err != nil { + is.log.Error().Err(err).Str("dstDigest", dstDigest.String()).Str("dst", dst).Msg("dedupe: unable to delete blob record") + return err + } + goto retry } dstFi, err := os.Stat(dst) if err != nil && !os.IsNotExist(err) { - is.log.Error().Err(err).Str("blobPath", dstRecord).Msg("unable to stat") + is.log.Error().Err(err).Str("blobPath", dstRecord).Msg("dedupe: unable to stat") return err } if !os.SameFile(dstFi, dstRecordFi) { if err := os.Link(dstRecord, dst); err != nil { - is.log.Error().Err(err).Str("blobPath", dst).Str("link", dstRecord).Msg("unable to hard link") + is.log.Error().Err(err).Str("blobPath", dst).Str("link", dstRecord).Msg("dedupe: unable to hard link") return err } } if err := os.Remove(src); err != nil { - is.log.Error().Err(err).Str("src", src).Msg("uname to remove blob") + is.log.Error().Err(err).Str("src", src).Msg("dedupe: uname to remove blob") return err } + is.log.Debug().Str("src", src).Msg("dedupe: remove") } return nil diff --git a/pkg/storage/storage_test.go b/pkg/storage/storage_test.go index 4fcb5453..6fd62dae 100644 --- a/pkg/storage/storage_test.go +++ b/pkg/storage/storage_test.go @@ -113,6 +113,12 @@ func TestAPIs(t *testing.T) { mb, _ := json.Marshal(m) Convey("Bad image manifest", func() { + _, err = il.PutImageManifest("test", d.String(), "application/json", mb) + So(err, ShouldNotBeNil) + + _, err = il.PutImageManifest("test", d.String(), ispec.MediaTypeImageManifest, []byte{}) + So(err, ShouldNotBeNil) + _, err = il.PutImageManifest("test", d.String(), ispec.MediaTypeImageManifest, mb) So(err, ShouldNotBeNil) @@ -133,6 +139,7 @@ func TestAPIs(t *testing.T) { Size: int64(l), }, }, + Annotations: map[string]string{ispec.AnnotationRefName: "1.0"}, } m.SchemaVersion = 2 mb, _ = json.Marshal(m) @@ -140,6 +147,9 @@ func TestAPIs(t *testing.T) { _, err = il.PutImageManifest("test", d.String(), ispec.MediaTypeImageManifest, mb) So(err, ShouldBeNil) + _, err = il.GetImageTags("test") + So(err, ShouldBeNil) + _, _, _, err = il.GetImageManifest("test", d.String()) So(err, ShouldBeNil) From dd1fc1e86635c02ad9fd902c7ea04dee761ec2d7 Mon Sep 17 00:00:00 2001 From: Ramkumar Chinchani Date: Wed, 15 Apr 2020 16:24:05 -0700 Subject: [PATCH 2/2] config: add gc and dedupe as configurable params (default to enabled) Since we want to conform to dist-spec, sometimes the gc and dedupe optimizations conflict with the conformance tests that are being run. So allow them to be turned off via configuration params. --- pkg/api/config.go | 3 ++ pkg/api/controller.go | 3 +- pkg/storage/storage.go | 63 +++++++++++++++++++++++++------------ pkg/storage/storage_test.go | 16 +++++----- 4 files changed, 56 insertions(+), 29 deletions(-) diff --git a/pkg/api/config.go b/pkg/api/config.go index 55505882..29549de3 100644 --- a/pkg/api/config.go +++ b/pkg/api/config.go @@ -12,6 +12,8 @@ var Commit string type StorageConfig struct { RootDirectory string + GC bool + Dedupe bool } type TLSConfig struct { @@ -77,6 +79,7 @@ func NewConfig() *Config { return &Config{ Version: dspec.Version, Commit: Commit, + Storage: StorageConfig{GC: true, Dedupe: true}, HTTP: HTTPConfig{Address: "127.0.0.1", Port: "8080"}, Log: &LogConfig{Level: "debug"}, } diff --git a/pkg/api/controller.go b/pkg/api/controller.go index 04bf570d..e881799d 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -42,7 +42,8 @@ func (c *Controller) Run() error { engine.Use(log.SessionLogger(c.Log), handlers.RecoveryHandler(handlers.RecoveryLogger(c.Log), handlers.PrintRecoveryStack(false))) - c.ImageStore = storage.NewImageStore(c.Config.Storage.RootDirectory, c.Log) + c.ImageStore = storage.NewImageStore(c.Config.Storage.RootDirectory, c.Config.Storage.GC, + c.Config.Storage.Dedupe, c.Log) if c.ImageStore == nil { // we can't proceed without at least a image store os.Exit(1) diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 53733489..71602f7e 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -38,11 +38,13 @@ type ImageStore struct { lock *sync.RWMutex blobUploads map[string]BlobUpload cache *Cache + gc bool + dedupe bool log zerolog.Logger } // NewImageStore returns a new image store backed by a file storage. -func NewImageStore(rootDir string, log zlog.Logger) *ImageStore { +func NewImageStore(rootDir string, gc bool, dedupe bool, log zlog.Logger) *ImageStore { if _, err := os.Stat(rootDir); os.IsNotExist(err) { if err := os.MkdirAll(rootDir, 0700); err != nil { log.Error().Err(err).Str("rootDir", rootDir).Msg("unable to create root dir") @@ -54,10 +56,15 @@ func NewImageStore(rootDir string, log zlog.Logger) *ImageStore { rootDir: rootDir, lock: &sync.RWMutex{}, blobUploads: make(map[string]BlobUpload), - cache: NewCache(rootDir, "cache", log), + gc: gc, + dedupe: dedupe, log: log.With().Caller().Logger(), } + if dedupe { + is.cache = NewCache(rootDir, "cache", log) + } + return is } @@ -467,14 +474,16 @@ func (is *ImageStore) PutImageManifest(repo string, reference string, mediaType return "", err } - oci, err := umoci.OpenLayout(dir) - if err != nil { - return "", err - } - defer oci.Close() + if is.gc { + oci, err := umoci.OpenLayout(dir) + if err != nil { + return "", err + } + defer oci.Close() - if err := oci.GC(context.Background()); err != nil { - return "", err + if err := oci.GC(context.Background()); err != nil { + return "", err + } } return desc.Digest.String(), nil @@ -541,14 +550,16 @@ func (is *ImageStore) DeleteImageManifest(repo string, reference string) error { return err } - oci, err := umoci.OpenLayout(dir) - if err != nil { - return err - } - defer oci.Close() + if is.gc { + oci, err := umoci.OpenLayout(dir) + if err != nil { + return err + } + defer oci.Close() - if err := oci.GC(context.Background()); err != nil { - return err + if err := oci.GC(context.Background()); err != nil { + return err + } } p := path.Join(dir, "blobs", digest.Algorithm().String(), digest.Encoded()) @@ -733,15 +744,21 @@ func (is *ImageStore) FinishBlobUpload(repo string, uuid string, body io.Reader, ensureDir(dir, is.log) dst := is.BlobPath(repo, dstDigest) - if is.cache != nil { + if is.dedupe && is.cache != nil { if err := is.DedupeBlob(src, dstDigest, dst); err != nil { is.log.Error().Err(err).Str("src", src).Str("dstDigest", dstDigest.String()). Str("dst", dst).Msg("unable to dedupe blob") return err } + } else { + if err := os.Rename(src, dst); err != nil { + is.log.Error().Err(err).Str("src", src).Str("dstDigest", dstDigest.String()). + Str("dst", dst).Msg("unable to finish blob") + return err + } } - return err + return nil } // FullBlobUpload handles a full blob upload, and no partial session is created @@ -792,15 +809,21 @@ func (is *ImageStore) FullBlobUpload(repo string, body io.Reader, digest string) ensureDir(dir, is.log) dst := is.BlobPath(repo, dstDigest) - if is.cache != nil { + if is.dedupe && is.cache != nil { if err := is.DedupeBlob(src, dstDigest, dst); err != nil { is.log.Error().Err(err).Str("src", src).Str("dstDigest", dstDigest.String()). Str("dst", dst).Msg("unable to dedupe blob") return "", -1, err } + } else { + if err := os.Rename(src, dst); err != nil { + is.log.Error().Err(err).Str("src", src).Str("dstDigest", dstDigest.String()). + Str("dst", dst).Msg("unable to finish blob") + return "", -1, err + } } - return uuid, n, err + return uuid, n, nil } // nolint (interfacer) diff --git a/pkg/storage/storage_test.go b/pkg/storage/storage_test.go index 6fd62dae..07f2a80e 100644 --- a/pkg/storage/storage_test.go +++ b/pkg/storage/storage_test.go @@ -27,7 +27,7 @@ func TestAPIs(t *testing.T) { defer os.RemoveAll(dir) - il := storage.NewImageStore(dir, log.Logger{Logger: zerolog.New(os.Stdout)}) + il := storage.NewImageStore(dir, true, true, log.Logger{Logger: zerolog.New(os.Stdout)}) Convey("Repo layout", t, func(c C) { repoName := "test" @@ -404,7 +404,7 @@ func TestDedupe(t *testing.T) { } defer os.RemoveAll(dir) - is := storage.NewImageStore(dir, log.Logger{Logger: zerolog.New(os.Stdout)}) + is := storage.NewImageStore(dir, true, true, log.Logger{Logger: zerolog.New(os.Stdout)}) So(is.DedupeBlob("", "", ""), ShouldNotBeNil) }) @@ -419,8 +419,8 @@ func TestNegativeCases(t *testing.T) { } os.RemoveAll(dir) - So(storage.NewImageStore(dir, log.Logger{Logger: zerolog.New(os.Stdout)}), ShouldNotBeNil) - So(storage.NewImageStore("/deadBEEF", log.Logger{Logger: zerolog.New(os.Stdout)}), ShouldBeNil) + So(storage.NewImageStore(dir, true, true, log.Logger{Logger: zerolog.New(os.Stdout)}), ShouldNotBeNil) + So(storage.NewImageStore("/deadBEEF", true, true, log.Logger{Logger: zerolog.New(os.Stdout)}), ShouldBeNil) }) Convey("Invalid init repo", t, func(c C) { @@ -429,7 +429,7 @@ func TestNegativeCases(t *testing.T) { panic(err) } defer os.RemoveAll(dir) - il := storage.NewImageStore(dir, log.Logger{Logger: zerolog.New(os.Stdout)}) + il := storage.NewImageStore(dir, true, true, log.Logger{Logger: zerolog.New(os.Stdout)}) err = os.Chmod(dir, 0000) // remove all perms So(err, ShouldBeNil) So(func() { _ = il.InitRepo("test") }, ShouldPanic) @@ -441,7 +441,7 @@ func TestNegativeCases(t *testing.T) { panic(err) } defer os.RemoveAll(dir) - il := storage.NewImageStore(dir, log.Logger{Logger: zerolog.New(os.Stdout)}) + il := storage.NewImageStore(dir, true, true, log.Logger{Logger: zerolog.New(os.Stdout)}) So(il, ShouldNotBeNil) So(il.InitRepo("test"), ShouldBeNil) files, err := ioutil.ReadDir(path.Join(dir, "test")) @@ -472,7 +472,7 @@ func TestNegativeCases(t *testing.T) { panic(err) } defer os.RemoveAll(dir) - il = storage.NewImageStore(dir, log.Logger{Logger: zerolog.New(os.Stdout)}) + il = storage.NewImageStore(dir, true, true, log.Logger{Logger: zerolog.New(os.Stdout)}) So(il, ShouldNotBeNil) So(il.InitRepo("test"), ShouldBeNil) So(os.Remove(path.Join(dir, "test", "index.json")), ShouldBeNil) @@ -495,7 +495,7 @@ func TestNegativeCases(t *testing.T) { panic(err) } defer os.RemoveAll(dir) - il = storage.NewImageStore(dir, log.Logger{Logger: zerolog.New(os.Stdout)}) + il = storage.NewImageStore(dir, true, true, log.Logger{Logger: zerolog.New(os.Stdout)}) So(il, ShouldNotBeNil) So(il.InitRepo("test"), ShouldBeNil) So(os.Remove(path.Join(dir, "test", "index.json")), ShouldBeNil)