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 <[email protected]>
project-zot · Aug 1, 2023 · b688e57 · b688e57
1 parent 42f9f78
commit b688e57
Show file tree

Hide file tree

Showing 22 changed files with 357 additions and 151 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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.