From b688e576eb627f6ee7214d6de3cb53fcf0b0e84e Mon Sep 17 00:00:00 2001 From: Andreea-Lupu Date: Tue, 11 Jul 2023 20:39:47 +0300 Subject: [PATCH] fix: remove inline GC and set a default value of gc interval - remove inline GC - add a default value of GC interval - run the GC periodically by default with the default value if no interval provided - generate GC tasks with a random delay(0-30s) between - add IsReady() method to scheduler.TaskGenerator interface Signed-off-by: Andreea-Lupu --- examples/config-conformance.json | 4 +- pkg/api/config/config.go | 5 +- pkg/api/controller.go | 4 +- pkg/api/controller_test.go | 61 +++------ pkg/cli/root.go | 25 +++- pkg/debug/gqlplayground/gqlplayground.go | 3 +- pkg/extensions/extension_mgmt.go | 7 + pkg/extensions/extension_scrub.go | 4 + pkg/extensions/extension_search.go | 4 + pkg/extensions/extensions_test.go | 11 ++ pkg/extensions/scrub/scrub_test.go | 3 + pkg/extensions/sync/sync.go | 4 + pkg/extensions/sync/sync_test.go | 6 + pkg/scheduler/README.md | 8 +- pkg/scheduler/scheduler.go | 5 + pkg/scheduler/scheduler_test.go | 8 ++ pkg/storage/common/common.go | 82 +++++++++++ pkg/storage/common/common_test.go | 30 ---- pkg/storage/constants/constants.go | 1 + pkg/storage/local/local.go | 62 +-------- pkg/storage/local/local_test.go | 167 ++++++++++++++++++++++- test/blackbox/scrub.bats | 4 +- 22 files changed, 357 insertions(+), 151 deletions(-) diff --git a/examples/config-conformance.json b/examples/config-conformance.json index 175aa9c28..f29364b47 100644 --- a/examples/config-conformance.json +++ b/examples/config-conformance.json @@ -2,8 +2,8 @@ "distSpecVersion": "1.1.0-dev", "storage": { "rootDirectory": "/tmp/zot", - "gc": false, - "dedupe": false + "gc": true, + "dedupe": true }, "http": { "address": "0.0.0.0", diff --git a/pkg/api/config/config.go b/pkg/api/config/config.go index 9d07e6fb7..7809747e1 100644 --- a/pkg/api/config/config.go +++ b/pkg/api/config/config.go @@ -184,7 +184,10 @@ func New() *Config { ReleaseTag: ReleaseTag, BinaryType: BinaryType, Storage: GlobalStorageConfig{ - StorageConfig: StorageConfig{GC: true, GCDelay: storageConstants.DefaultGCDelay, Dedupe: true}, + StorageConfig: StorageConfig{ + GC: true, GCDelay: storageConstants.DefaultGCDelay, + GCInterval: storageConstants.DefaultGCInterval, Dedupe: true, + }, }, HTTP: HTTPConfig{Address: "127.0.0.1", Port: "8080", Auth: &AuthConfig{FailDelay: 0}}, Log: &LogConfig{Level: "debug"}, diff --git a/pkg/api/controller.go b/pkg/api/controller.go index 38728b3d0..bbceea4de 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -324,7 +324,7 @@ func (c *Controller) StartBackgroundTasks(reloadCtx context.Context) { taskScheduler.RunScheduler(reloadCtx) // Enable running garbage-collect periodically for DefaultStore - if c.Config.Storage.GC && c.Config.Storage.GCInterval != 0 { + if c.Config.Storage.GC { c.StoreController.DefaultStore.RunGCPeriodically(c.Config.Storage.GCInterval, taskScheduler) } @@ -340,7 +340,7 @@ func (c *Controller) StartBackgroundTasks(reloadCtx context.Context) { if c.Config.Storage.SubPaths != nil { for route, storageConfig := range c.Config.Storage.SubPaths { // Enable running garbage-collect periodically for subImageStore - if storageConfig.GC && storageConfig.GCInterval != 0 { + if storageConfig.GC { c.StoreController.SubStore[route].RunGCPeriodically(storageConfig.GCInterval, taskScheduler) } diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 1162c85ed..f5da71e7d 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -4503,6 +4503,7 @@ func TestCrossRepoMount(t *testing.T) { cm.StopServer() ctlr.Config.Storage.Dedupe = true + ctlr.Config.Storage.GC = false ctlr.Config.Storage.RootDirectory = newDir cm = test.NewControllerManager(ctlr) //nolint: varnamelen cm.StartAndWait(port) @@ -7352,48 +7353,6 @@ func TestInjectTooManyOpenFiles(t *testing.T) { So(resp.StatusCode, ShouldEqual, http.StatusCreated) } }) - Convey("code coverage: error inside PutImageManifest method of img store (umoci.OpenLayout error)", func() { - injected := inject.InjectFailure(3) - - request, _ := http.NewRequestWithContext(context.TODO(), http.MethodPut, baseURL, bytes.NewReader(content)) - request = mux.SetURLVars(request, map[string]string{"name": "repotest", "reference": "1.0"}) - request.Header.Set("Content-Type", "application/vnd.oci.image.manifest.v1+json") - response := httptest.NewRecorder() - - rthdlr.UpdateManifest(response, request) - - resp := response.Result() - defer resp.Body.Close() - - So(resp, ShouldNotBeNil) - - if injected { - So(resp.StatusCode, ShouldEqual, http.StatusInternalServerError) - } else { - So(resp.StatusCode, ShouldEqual, http.StatusCreated) - } - }) - Convey("code coverage: error inside PutImageManifest method of img store (oci.GC)", func() { - injected := inject.InjectFailure(4) - - request, _ := http.NewRequestWithContext(context.TODO(), http.MethodPut, baseURL, bytes.NewReader(content)) - request = mux.SetURLVars(request, map[string]string{"name": "repotest", "reference": "1.0"}) - request.Header.Set("Content-Type", "application/vnd.oci.image.manifest.v1+json") - response := httptest.NewRecorder() - - rthdlr.UpdateManifest(response, request) - - resp := response.Result() - defer resp.Body.Close() - - So(resp, ShouldNotBeNil) - - if injected { - So(resp.StatusCode, ShouldEqual, http.StatusInternalServerError) - } else { - So(resp.StatusCode, ShouldEqual, http.StatusCreated) - } - }) Convey("when index.json is not in json format", func() { resp, err = resty.R().SetHeader("Content-Type", "application/vnd.oci.image.manifest.v1+json"). SetBody(content).Put(baseURL + "/v2/repotest/manifests/v1.0") @@ -7436,6 +7395,8 @@ func TestGCSignaturesAndUntaggedManifests(t *testing.T) { ctlr.Config.Storage.GC = true ctlr.Config.Storage.GCDelay = 1 * time.Millisecond + ctlr.Config.Storage.Dedupe = false + test.CopyTestFiles("../../test/data/zot-test", path.Join(dir, repoName)) cm := test.NewControllerManager(ctlr) @@ -7519,6 +7480,9 @@ func TestGCSignaturesAndUntaggedManifests(t *testing.T) { img := test.CreateRandomImage() err = test.UploadImage(img, baseURL, repoName, img.DigestStr()) + So(err, ShouldBeNil) + + err = ctlr.StoreController.DefaultStore.RunGCRepo(repoName) So(err, ShouldNotBeNil) err = os.Chmod(path.Join(dir, repoName, "blobs", "sha256", refs.Manifests[0].Digest.Encoded()), 0o755) @@ -7530,6 +7494,9 @@ func TestGCSignaturesAndUntaggedManifests(t *testing.T) { So(err, ShouldBeNil) err = test.UploadImage(img, baseURL, repoName, tag) + So(err, ShouldBeNil) + + err = ctlr.StoreController.DefaultStore.RunGCRepo(repoName) So(err, ShouldNotBeNil) err = os.WriteFile(path.Join(dir, repoName, "blobs", "sha256", refs.Manifests[0].Digest.Encoded()), content, 0o600) @@ -7568,6 +7535,9 @@ func TestGCSignaturesAndUntaggedManifests(t *testing.T) { So(err, ShouldBeNil) newManifestDigest := godigest.FromBytes(manifestBuf) + err = ctlr.StoreController.DefaultStore.RunGCRepo(repoName) + So(err, ShouldBeNil) + // both signatures should be gc'ed resp, err = resty.R().Get(baseURL + fmt.Sprintf("/v2/%s/manifests/%s", repoName, cosignTag)) So(err, ShouldBeNil) @@ -7658,6 +7628,9 @@ func TestGCSignaturesAndUntaggedManifests(t *testing.T) { So(err, ShouldBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusCreated) + err = ctlr.StoreController.DefaultStore.RunGCRepo(repoName) + So(err, ShouldBeNil) + resp, err = resty.R().SetHeader("Content-Type", ispec.MediaTypeImageIndex). Get(baseURL + fmt.Sprintf("/v2/%s/manifests/latest", repoName)) So(err, ShouldBeNil) @@ -7741,9 +7714,9 @@ func TestPeriodicGC(t *testing.T) { data, err := os.ReadFile(logFile.Name()) So(err, ShouldBeNil) - // periodic GC is not enabled for default store + // periodic GC is enabled by default for default store with a default interval So(string(data), ShouldContainSubstring, - "\"GCDelay\":3600000000000,\"GCInterval\":0,\"") + "\"GCDelay\":3600000000000,\"GCInterval\":3600000000000,\"") // periodic GC is enabled for sub store So(string(data), ShouldContainSubstring, fmt.Sprintf("\"SubPaths\":{\"/a\":{\"RootDirectory\":\"%s\",\"Dedupe\":false,\"RemoteCache\":false,\"GC\":true,\"Commit\":false,\"GCDelay\":1000000000,\"GCInterval\":86400000000000", subDir)) //nolint:lll // gofumpt conflicts with lll diff --git a/pkg/cli/root.go b/pkg/cli/root.go index 3a9d65724..cfde9a448 100644 --- a/pkg/cli/root.go +++ b/pkg/cli/root.go @@ -609,8 +609,14 @@ func applyDefaultValues(config *config.Config, viperInstance *viper.Viper) { } } - if !config.Storage.GC && viperInstance.Get("storage::gcdelay") == nil { - config.Storage.GCDelay = 0 + if !config.Storage.GC { + if viperInstance.Get("storage::gcdelay") == nil { + config.Storage.GCDelay = 0 + } + + if viperInstance.Get("storage::gcinterval") == nil { + config.Storage.GCInterval = 0 + } } // cache settings @@ -657,9 +663,18 @@ func applyDefaultValues(config *config.Config, viperInstance *viper.Viper) { } } - // if gc is enabled and gcDelay is not set, it is set to default value - if storageConfig.GC && !viperInstance.IsSet("storage::subpaths::"+name+"::gcdelay") { - storageConfig.GCDelay = storageConstants.DefaultGCDelay + // if gc is enabled + if storageConfig.GC { + // and gcDelay is not set, it is set to default value + if !viperInstance.IsSet("storage::subpaths::" + name + "::gcdelay") { + storageConfig.GCDelay = storageConstants.DefaultGCDelay + } + + // and gcInterval is not set, it is set to default value + if !viperInstance.IsSet("storage::subpaths::" + name + "::gcinterval") { + storageConfig.GCInterval = storageConstants.DefaultGCInterval + } + config.Storage.SubPaths[name] = storageConfig } } diff --git a/pkg/debug/gqlplayground/gqlplayground.go b/pkg/debug/gqlplayground/gqlplayground.go index 3ce638eab..5b9a0987d 100644 --- a/pkg/debug/gqlplayground/gqlplayground.go +++ b/pkg/debug/gqlplayground/gqlplayground.go @@ -22,9 +22,8 @@ var playgroundHTML embed.FS // SetupGQLPlaygroundRoutes ... func SetupGQLPlaygroundRoutes(conf *config.Config, router *mux.Router, - storeController storage.StoreController, l log.Logger, + storeController storage.StoreController, log log.Logger, ) { - log := log.Logger{Logger: l.With().Caller().Timestamp().Logger()} log.Info().Msg("setting up graphql playground route") templ, err := template.ParseFS(playgroundHTML, "index.html.tmpl") diff --git a/pkg/extensions/extension_mgmt.go b/pkg/extensions/extension_mgmt.go index 7d365643a..d91c545c8 100644 --- a/pkg/extensions/extension_mgmt.go +++ b/pkg/extensions/extension_mgmt.go @@ -280,6 +280,7 @@ func (gen *taskGeneratorSigValidity) Next() (scheduler.Task, error) { gen.repoIndex++ if gen.repoIndex >= len(gen.repos) { + gen.log.Info().Msg("finished generating tasks for updating signatures validity") gen.done = true return nil, nil @@ -293,6 +294,8 @@ func (gen *taskGeneratorSigValidity) IsDone() bool { } func (gen *taskGeneratorSigValidity) Reset() { + gen.log.Info().Msg("reset task generator for updating signatures validity") + gen.done = false gen.repoIndex = -1 ctx := context.Background() @@ -305,6 +308,10 @@ func (gen *taskGeneratorSigValidity) Reset() { gen.repos = repos } +func (gen *taskGeneratorSigValidity) IsReady() bool { + return true +} + type validityTask struct { metaDB mTypes.MetaDB repo mTypes.RepoMetadata diff --git a/pkg/extensions/extension_scrub.go b/pkg/extensions/extension_scrub.go index 94bb2435d..2997a23d5 100644 --- a/pkg/extensions/extension_scrub.go +++ b/pkg/extensions/extension_scrub.go @@ -79,6 +79,10 @@ func (gen *taskGenerator) IsDone() bool { return gen.done } +func (gen *taskGenerator) IsReady() bool { + return true +} + func (gen *taskGenerator) Reset() { gen.lastRepo = "" gen.done = false diff --git a/pkg/extensions/extension_search.go b/pkg/extensions/extension_search.go index 359d648ca..f14b5288f 100644 --- a/pkg/extensions/extension_search.go +++ b/pkg/extensions/extension_search.go @@ -108,6 +108,10 @@ func (gen *TrivyTaskGenerator) IsDone() bool { return status == done } +func (gen *TrivyTaskGenerator) IsReady() bool { + return true +} + func (gen *TrivyTaskGenerator) Reset() { gen.lock.Lock() gen.status = pending diff --git a/pkg/extensions/extensions_test.go b/pkg/extensions/extensions_test.go index 57075f94d..ced1fa6f8 100644 --- a/pkg/extensions/extensions_test.go +++ b/pkg/extensions/extensions_test.go @@ -684,6 +684,7 @@ func TestMgmtExtension(t *testing.T) { ) So(err, ShouldBeNil) + conf.Storage.GC = false conf.Extensions = &extconf.ExtensionConfig{} conf.Extensions.Search = &extconf.SearchConfig{} conf.Extensions.Search.Enable = &defaultValue @@ -786,6 +787,16 @@ func TestMgmtExtension(t *testing.T) { time.Second) So(err, ShouldBeNil) So(found, ShouldBeTrue) + + found, err = test.ReadLogFileAndSearchString(logFile.Name(), + "finished generating tasks for updating signatures validity", 10*time.Second) + So(err, ShouldBeNil) + So(found, ShouldBeTrue) + + found, err = test.ReadLogFileAndSearchString(logFile.Name(), "reset task generator for updating signatures validity", + 10*time.Second) + So(err, ShouldBeNil) + So(found, ShouldBeTrue) }) } diff --git a/pkg/extensions/scrub/scrub_test.go b/pkg/extensions/scrub/scrub_test.go index e47130404..db55d7a66 100644 --- a/pkg/extensions/scrub/scrub_test.go +++ b/pkg/extensions/scrub/scrub_test.go @@ -46,6 +46,7 @@ func TestScrubExtension(t *testing.T) { conf.Storage.RootDirectory = dir conf.Storage.Dedupe = false + conf.Storage.GC = false substore := config.StorageConfig{RootDirectory: subdir} conf.Storage.SubPaths = map[string]config.StorageConfig{"/a": substore} @@ -89,6 +90,7 @@ func TestScrubExtension(t *testing.T) { conf.Storage.RootDirectory = dir conf.Storage.Dedupe = false + conf.Storage.GC = false conf.Log.Output = logFile.Name() trueValue := true @@ -137,6 +139,7 @@ func TestScrubExtension(t *testing.T) { conf.Storage.RootDirectory = dir conf.Storage.Dedupe = false + conf.Storage.GC = false conf.Log.Output = logFile.Name() trueValue := true diff --git a/pkg/extensions/sync/sync.go b/pkg/extensions/sync/sync.go index 5d7c71856..bfd672b1b 100644 --- a/pkg/extensions/sync/sync.go +++ b/pkg/extensions/sync/sync.go @@ -114,6 +114,10 @@ func (gen *TaskGenerator) IsDone() bool { return gen.done } +func (gen *TaskGenerator) IsReady() bool { + return true +} + func (gen *TaskGenerator) Reset() { gen.lastRepo = "" gen.Service.ResetCatalog() diff --git a/pkg/extensions/sync/sync_test.go b/pkg/extensions/sync/sync_test.go index cfcb60200..c0e2486a1 100644 --- a/pkg/extensions/sync/sync_test.go +++ b/pkg/extensions/sync/sync_test.go @@ -131,6 +131,7 @@ func makeUpstreamServer( } srcConfig.HTTP.Port = srcPort + srcConfig.Storage.GC = false srcDir := t.TempDir() @@ -1663,6 +1664,7 @@ func TestPermsDenied(t *testing.T) { destDir := t.TempDir() + destConfig.Storage.GC = false destConfig.Storage.RootDirectory = destDir destConfig.Extensions = &extconf.ExtensionConfig{} @@ -3038,6 +3040,8 @@ func TestSubPaths(t *testing.T) { srcConfig.HTTP.Port = srcPort + srcConfig.Storage.GC = false + srcDir := t.TempDir() subpath := "/subpath" @@ -4506,6 +4510,7 @@ func TestOnDemandRetryGoroutine(t *testing.T) { srcBaseURL := test.GetBaseURL(srcPort) srcConfig.HTTP.Port = srcPort + srcConfig.Storage.GC = false srcDir := t.TempDir() @@ -4712,6 +4717,7 @@ func TestOnDemandMultipleImage(t *testing.T) { srcBaseURL := test.GetBaseURL(srcPort) srcConfig.HTTP.Port = srcPort + srcConfig.Storage.GC = false srcDir := t.TempDir() diff --git a/pkg/scheduler/README.md b/pkg/scheduler/README.md index 9779f148d..2b6d9096f 100644 --- a/pkg/scheduler/README.md +++ b/pkg/scheduler/README.md @@ -1,7 +1,7 @@ # How to submit a Generator to the scheduler ## What is a generator and how should it be implemented? -In order to create a new generator (which will generate new tasks one by one) and add it to the scheduler there are 3 methods which should be implemented: +In order to create a new generator (which will generate new tasks one by one) and add it to the scheduler there are 4 methods which should be implemented: 1. ***GenerateTask() (Task, error)*** ``` This method should implement the logic for generating a new task. @@ -12,7 +12,11 @@ In order to create a new generator (which will generate new tasks one by one) an ``` This method should return true after the generator finished all the work and has no more tasks to generate. ``` -3. ***Reset()*** +3. ***IsReady() bool*** + ``` + This method should return true if the generator is ready to generate a new task and should be used when it is needed to generate tasks with some delay between. + ``` +4. ***Reset()*** ``` When this method is called the generator should reset to its initial state. After the generator is reset, it will generate new tasks as if it hadn't been used before. diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go index 6acfdfc13..de574da9c 100644 --- a/pkg/scheduler/scheduler.go +++ b/pkg/scheduler/scheduler.go @@ -284,6 +284,7 @@ const ( type TaskGenerator interface { Next() (Task, error) IsDone() bool + IsReady() bool Reset() } @@ -351,6 +352,10 @@ func (gen *generator) getState() state { } } + if !gen.taskGenerator.IsReady() { + return waiting + } + return ready } diff --git a/pkg/scheduler/scheduler_test.go b/pkg/scheduler/scheduler_test.go index d2112fc42..c74c5c092 100644 --- a/pkg/scheduler/scheduler_test.go +++ b/pkg/scheduler/scheduler_test.go @@ -56,6 +56,10 @@ func (g *generator) IsDone() bool { return g.done } +func (g *generator) IsReady() bool { + return true +} + func (g *generator) Reset() { g.done = false g.step = 0 @@ -79,6 +83,10 @@ func (g *shortGenerator) IsDone() bool { return g.done } +func (g *shortGenerator) IsReady() bool { + return true +} + func (g *shortGenerator) Reset() { g.done = true g.step = 0 diff --git a/pkg/storage/common/common.go b/pkg/storage/common/common.go index ea73c5dc0..4e020e647 100644 --- a/pkg/storage/common/common.go +++ b/pkg/storage/common/common.go @@ -4,8 +4,11 @@ import ( "bytes" "encoding/json" "errors" + "io" + "math/rand" "path" "strings" + "time" notreg "github.com/notaryproject/notation-go/registry" godigest "github.com/opencontainers/go-digest" @@ -853,6 +856,10 @@ func (gen *DedupeTaskGenerator) IsDone() bool { return gen.done } +func (gen *DedupeTaskGenerator) IsReady() bool { + return true +} + func (gen *DedupeTaskGenerator) Reset() { gen.lastDigests = []godigest.Digest{} gen.duplicateBlobs = []string{} @@ -886,3 +893,78 @@ func (dt *dedupeTask) DoWork() error { return err } + +/* + GCTaskGenerator takes all repositories found in the storage.imagestore + +and it will execute garbage collection for each repository by creating a task +for each repository and pushing it to the task scheduler. +*/ +type GCTaskGenerator struct { + ImgStore storageTypes.ImageStore + lastRepo string + nextRun time.Time + done bool + rand *rand.Rand +} + +func (gen *GCTaskGenerator) getRandomDelay() int { + maxDelay := 30 + + return gen.rand.Intn(maxDelay) +} + +func (gen *GCTaskGenerator) Next() (scheduler.Task, error) { + if gen.lastRepo == "" && gen.nextRun.IsZero() { + gen.rand = rand.New(rand.NewSource(time.Now().UTC().UnixNano())) //nolint: gosec + } + + delay := gen.getRandomDelay() + + gen.nextRun = time.Now().Add(time.Duration(delay) * time.Second) + + repo, err := gen.ImgStore.GetNextRepository(gen.lastRepo) + + if err != nil && !errors.Is(err, io.EOF) { + return nil, err + } + + if repo == "" { + gen.done = true + + return nil, nil + } + + gen.lastRepo = repo + + return NewGCTask(gen.ImgStore, repo), nil +} + +func (gen *GCTaskGenerator) IsDone() bool { + return gen.done +} + +func (gen *GCTaskGenerator) IsReady() bool { + return time.Now().After(gen.nextRun) +} + +func (gen *GCTaskGenerator) Reset() { + gen.lastRepo = "" + gen.done = false + gen.nextRun = time.Time{} +} + +type gcTask struct { + imgStore storageTypes.ImageStore + repo string +} + +func NewGCTask(imgStore storageTypes.ImageStore, repo string, +) *gcTask { + return &gcTask{imgStore, repo} +} + +func (gct *gcTask) DoWork() error { + // run task + return gct.imgStore.RunGCRepo(gct.repo) +} diff --git a/pkg/storage/common/common_test.go b/pkg/storage/common/common_test.go index 76ae36eca..622bd698f 100644 --- a/pkg/storage/common/common_test.go +++ b/pkg/storage/common/common_test.go @@ -77,36 +77,6 @@ func TestValidateManifest(t *testing.T) { So(err, ShouldNotBeNil) }) - Convey("bad config blob", func() { - manifest := ispec.Manifest{ - Config: ispec.Descriptor{ - MediaType: ispec.MediaTypeImageConfig, - Digest: cdigest, - Size: int64(len(cblob)), - }, - Layers: []ispec.Descriptor{ - { - MediaType: ispec.MediaTypeImageLayer, - Digest: digest, - Size: int64(len(content)), - }, - }, - } - - manifest.SchemaVersion = 2 - - configBlobPath := imgStore.BlobPath("test", cdigest) - - err := os.WriteFile(configBlobPath, []byte("bad config blob"), 0o000) - So(err, ShouldBeNil) - - body, err := json.Marshal(manifest) - So(err, ShouldBeNil) - - _, _, err = imgStore.PutImageManifest("test", "1.0", ispec.MediaTypeImageManifest, body) - So(err, ShouldNotBeNil) - }) - Convey("manifest with non-distributable layers", func() { content := []byte("this blob doesn't exist") digest := godigest.FromBytes(content) diff --git a/pkg/storage/constants/constants.go b/pkg/storage/constants/constants.go index 2a7df48a8..55ea6b038 100644 --- a/pkg/storage/constants/constants.go +++ b/pkg/storage/constants/constants.go @@ -20,5 +20,6 @@ const ( BoltdbName = "cache" DynamoDBDriverName = "dynamodb" DefaultGCDelay = 1 * time.Hour + DefaultGCInterval = 1 * time.Hour S3StorageDriverName = "s3" ) diff --git a/pkg/storage/local/local.go b/pkg/storage/local/local.go index 74707952b..32698cd53 100644 --- a/pkg/storage/local/local.go +++ b/pkg/storage/local/local.go @@ -593,12 +593,6 @@ func (is *ImageStoreLocal) PutImageManifest(repo, reference, mediaType string, / return "", "", err } - if is.gc { - if err := is.garbageCollect(dir, repo); err != nil { - return "", "", err - } - } - return desc.Digest, subjectDigest, nil } @@ -650,12 +644,6 @@ func (is *ImageStoreLocal) DeleteImageManifest(repo, reference string, detectCol return err } - if is.gc { - if err := is.garbageCollect(dir, repo); err != nil { - return err - } - } - // Delete blob only when blob digest not present in manifest entry. // e.g. 1.0.1 & 1.0.2 have same blob digest so if we delete 1.0.1, blob should not be removed. toDelete := true @@ -1812,58 +1800,12 @@ func (is *ImageStoreLocal) RunGCRepo(repo string) error { } func (is *ImageStoreLocal) RunGCPeriodically(interval time.Duration, sch *scheduler.Scheduler) { - generator := &taskGenerator{ - imgStore: is, + generator := &common.GCTaskGenerator{ + ImgStore: is, } sch.SubmitGenerator(generator, interval, scheduler.MediumPriority) } -type taskGenerator struct { - imgStore *ImageStoreLocal - lastRepo string - done bool -} - -func (gen *taskGenerator) Next() (scheduler.Task, error) { - repo, err := gen.imgStore.GetNextRepository(gen.lastRepo) - - if err != nil && !errors.Is(err, io.EOF) { - return nil, err - } - - if repo == "" { - gen.done = true - - return nil, nil - } - - gen.lastRepo = repo - - return newGCTask(gen.imgStore, repo), nil -} - -func (gen *taskGenerator) IsDone() bool { - return gen.done -} - -func (gen *taskGenerator) Reset() { - gen.lastRepo = "" - gen.done = false -} - -type gcTask struct { - imgStore *ImageStoreLocal - repo string -} - -func newGCTask(imgStore *ImageStoreLocal, repo string) *gcTask { - return &gcTask{imgStore, repo} -} - -func (gcT *gcTask) DoWork() error { - return gcT.imgStore.RunGCRepo(gcT.repo) -} - func (is *ImageStoreLocal) GetNextDigestWithBlobPaths(lastDigests []godigest.Digest, ) (godigest.Digest, []string, error) { var lockLatency time.Time diff --git a/pkg/storage/local/local_test.go b/pkg/storage/local/local_test.go index 4b4b3e03c..24e5560cd 100644 --- a/pkg/storage/local/local_test.go +++ b/pkg/storage/local/local_test.go @@ -2040,6 +2040,98 @@ func TestInjectWriteFile(t *testing.T) { }) } +func TestGCInjectFailure(t *testing.T) { + Convey("code coverage: error inside garbageCollect method of img store", t, func() { + dir := t.TempDir() + logFile, _ := os.CreateTemp("", "zot-log*.txt") + + defer os.Remove(logFile.Name()) // clean up + + log := log.NewLogger("debug", logFile.Name()) + metrics := monitoring.NewMetricsServer(false, log) + cacheDriver, _ := storage.Create("boltdb", cache.BoltDBDriverParameters{ + RootDir: dir, + Name: "cache", + UseRelPaths: true, + }, log) + imgStore := local.NewImageStore(dir, true, storageConstants.DefaultGCDelay, + true, true, log, metrics, nil, cacheDriver) + repoName := "test-gc" + + upload, err := imgStore.NewBlobUpload(repoName) + So(err, ShouldBeNil) + So(upload, ShouldNotBeEmpty) + + content := []byte("test-data1") + buf := bytes.NewBuffer(content) + buflen := buf.Len() + bdigest := godigest.FromBytes(content) + + blob, err := imgStore.PutBlobChunk(repoName, upload, 0, int64(buflen), buf) + So(err, ShouldBeNil) + So(blob, ShouldEqual, buflen) + + err = imgStore.FinishBlobUpload(repoName, upload, buf, bdigest) + So(err, ShouldBeNil) + + annotationsMap := make(map[string]string) + annotationsMap[ispec.AnnotationRefName] = tag + + cblob, cdigest := test.GetRandomImageConfig() + _, clen, err := imgStore.FullBlobUpload(repoName, bytes.NewReader(cblob), cdigest) + So(err, ShouldBeNil) + So(clen, ShouldEqual, len(cblob)) + hasBlob, _, err := imgStore.CheckBlob(repoName, cdigest) + So(err, ShouldBeNil) + So(hasBlob, ShouldEqual, true) + + manifest := ispec.Manifest{ + Config: ispec.Descriptor{ + MediaType: "application/vnd.oci.image.config.v1+json", + Digest: cdigest, + Size: int64(len(cblob)), + }, + Layers: []ispec.Descriptor{ + { + MediaType: "application/vnd.oci.image.layer.v1.tar", + Digest: bdigest, + Size: int64(buflen), + }, + }, + Annotations: annotationsMap, + } + + manifest.SchemaVersion = 2 + manifestBuf, err := json.Marshal(manifest) + So(err, ShouldBeNil) + + _, _, err = imgStore.PutImageManifest(repoName, tag, ispec.MediaTypeImageManifest, manifestBuf) + So(err, ShouldBeNil) + + // umoci.OpenLayout error + injected := inject.InjectFailure(0) + + err = imgStore.RunGCRepo(repoName) + + if injected { + So(err, ShouldNotBeNil) + } else { + So(err, ShouldBeNil) + } + + // oci.GC + injected = inject.InjectFailure(1) + + err = imgStore.RunGCRepo(repoName) + + if injected { + So(err, ShouldNotBeNil) + } else { + So(err, ShouldBeNil) + } + }) +} + func TestGarbageCollect(t *testing.T) { Convey("Repo layout", t, func(c C) { dir := t.TempDir() @@ -2108,6 +2200,9 @@ func TestGarbageCollect(t *testing.T) { _, _, err = imgStore.PutImageManifest(repoName, tag, ispec.MediaTypeImageManifest, manifestBuf) So(err, ShouldBeNil) + err = imgStore.RunGCRepo(repoName) + So(err, ShouldBeNil) + hasBlob, _, err = imgStore.CheckBlob(repoName, bdigest) So(err, ShouldBeNil) So(hasBlob, ShouldEqual, true) @@ -2115,6 +2210,9 @@ func TestGarbageCollect(t *testing.T) { err = imgStore.DeleteImageManifest(repoName, digest.String(), false) So(err, ShouldBeNil) + err = imgStore.RunGCRepo(repoName) + So(err, ShouldBeNil) + hasBlob, _, err = imgStore.CheckBlob(repoName, bdigest) So(err, ShouldBeNil) So(hasBlob, ShouldEqual, true) @@ -2201,6 +2299,9 @@ func TestGarbageCollect(t *testing.T) { _, _, err = imgStore.PutImageManifest(repoName, tag, ispec.MediaTypeImageManifest, manifestBuf) So(err, ShouldBeNil) + err = imgStore.RunGCRepo(repoName) + So(err, ShouldBeNil) + hasBlob, _, err = imgStore.CheckBlob(repoName, odigest) So(err, ShouldNotBeNil) So(hasBlob, ShouldEqual, false) @@ -2223,6 +2324,9 @@ func TestGarbageCollect(t *testing.T) { err = imgStore.DeleteImageManifest(repoName, digest.String(), false) So(err, ShouldBeNil) + err = imgStore.RunGCRepo(repoName) + So(err, ShouldBeNil) + hasBlob, _, err = imgStore.CheckBlob(repoName, bdigest) So(err, ShouldNotBeNil) So(hasBlob, ShouldEqual, false) @@ -2360,7 +2464,7 @@ func TestGarbageCollect(t *testing.T) { So(err, ShouldBeNil) So(hasBlob, ShouldEqual, true) - // immediately upload any other image to second repo which should invoke GC inline, but expect layers to persist + // immediately upload any other image to second repo and run GC, but expect layers to persist upload, err = imgStore.NewBlobUpload(repo2Name) So(err, ShouldBeNil) @@ -2413,6 +2517,9 @@ func TestGarbageCollect(t *testing.T) { _, _, err = imgStore.PutImageManifest(repo2Name, tag, ispec.MediaTypeImageManifest, manifestBuf) So(err, ShouldBeNil) + err = imgStore.RunGCRepo(repo2Name) + So(err, ShouldBeNil) + // original blob should exist hasBlob, _, err = imgStore.CheckBlob(repo2Name, tdigest) @@ -2494,6 +2601,64 @@ func TestGarbageCollectForImageStore(t *testing.T) { fmt.Sprintf("error while running GC for %s", path.Join(imgStore.RootDir(), repoName))) So(os.Chmod(path.Join(dir, repoName, "index.json"), 0o755), ShouldBeNil) }) + + Convey("Garbage collect - the manifest which the reference points to can be found", func() { + logFile, _ := os.CreateTemp("", "zot-log*.txt") + + defer os.Remove(logFile.Name()) // clean up + + log := log.NewLogger("debug", logFile.Name()) + metrics := monitoring.NewMetricsServer(false, log) + cacheDriver, _ := storage.Create("boltdb", cache.BoltDBDriverParameters{ + RootDir: dir, + Name: "cache", + UseRelPaths: true, + }, log) + imgStore := local.NewImageStore(dir, true, 1*time.Second, true, true, log, metrics, nil, cacheDriver) + repoName := "gc-sig" + + storeController := storage.StoreController{DefaultStore: imgStore} + img := test.CreateRandomImage() + + err := test.WriteImageToFileSystem(img, repoName, "tag1", storeController) + So(err, ShouldBeNil) + + // add fake signature for tag1 + cosignTag, err := test.GetCosignSignatureTagForManifest(img.Manifest) + So(err, ShouldBeNil) + + cosignSig := test.CreateRandomImage() + So(err, ShouldBeNil) + + err = test.WriteImageToFileSystem(cosignSig, repoName, cosignTag, storeController) + So(err, ShouldBeNil) + + // add sbom + manifestBlob, err := json.Marshal(img.Manifest) + So(err, ShouldBeNil) + + manifestDigest := godigest.FromBytes(manifestBlob) + sbomTag := fmt.Sprintf("sha256-%s.%s", manifestDigest.Encoded(), "sbom") + So(err, ShouldBeNil) + + sbomImg := test.CreateRandomImage() + So(err, ShouldBeNil) + + err = test.WriteImageToFileSystem(sbomImg, repoName, sbomTag, storeController) + So(err, ShouldBeNil) + + // add fake signature for tag1 + notationSig := test.CreateImageWith(). + RandomLayers(1, 10). + ArtifactConfig("application/vnd.cncf.notary.signature"). + Subject(img.DescriptorRef()).Build() + + err = test.WriteImageToFileSystem(notationSig, repoName, "notation", storeController) + So(err, ShouldBeNil) + + err = imgStore.RunGCRepo(repoName) + So(err, ShouldBeNil) + }) }) } diff --git a/test/blackbox/scrub.bats b/test/blackbox/scrub.bats index 01f8f1113..a2c639d6f 100644 --- a/test/blackbox/scrub.bats +++ b/test/blackbox/scrub.bats @@ -61,7 +61,7 @@ function teardown() { wait_zot_reachable "http://127.0.0.1:8080/v2/_catalog" # wait for scrub to be done and logs to get populated - run sleep 10s + run sleep 15s run not_affected [ "$status" -eq 0 ] [ $(echo "${lines[0]}" ) = 'true' ] @@ -76,7 +76,7 @@ function teardown() { wait_zot_reachable "http://127.0.0.1:8080/v2/_catalog" # wait for scrub to be done and logs to get populated - run sleep 10s + run sleep 15s run affected [ "$status" -eq 0 ] [ $(echo "${lines[0]}" ) = 'true' ]