From 0d42ba2744e479e1bbc1ca6823b3530292f9f1f0 Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Sat, 11 Oct 2025 00:30:29 +0300 Subject: [PATCH] feat: the default retention delay is not the GC delay (#3447) Most users don't make the difference between retention deleting untagged manifests vs GC deleting other blobs. This causes confusion since the GC delay and the retention delay (used for untagged manifests and orphan referrers) have different defaults, and are set separately in the zot configuration. Most users don't configrue retention policies, and they still expect untagged manifests to be deleted at GC time. With this change, if retention delay is not specified in the config file, the value used is the GC delay. If GC delay is also unspecified in the config file, the default GC delay is used for both. Signed-off-by: Andrei Aaron --- pkg/api/config/config.go | 4 +- pkg/cli/server/root.go | 8 +- pkg/cli/server/root_test.go | 231 ++++++++++++++++++++++++++++- pkg/storage/constants/constants.go | 1 - pkg/storage/gc/gc_internal_test.go | 10 +- pkg/storage/gc/gc_test.go | 32 ++-- pkg/storage/local/local_test.go | 2 +- pkg/storage/storage_test.go | 4 +- 8 files changed, 261 insertions(+), 31 deletions(-) diff --git a/pkg/api/config/config.go b/pkg/api/config/config.go index 62f9f963..9b0efc7c 100644 --- a/pkg/api/config/config.go +++ b/pkg/api/config/config.go @@ -290,9 +290,7 @@ func New() *Config { GC: true, GCDelay: storageConstants.DefaultGCDelay, GCInterval: storageConstants.DefaultGCInterval, - Retention: ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, - }, + Retention: ImageRetention{}, }, }, HTTP: HTTPConfig{Address: "127.0.0.1", Port: "8080", Auth: &AuthConfig{FailDelay: 0}}, diff --git a/pkg/cli/server/root.go b/pkg/cli/server/root.go index 040a1dff..a1d1e7ef 100644 --- a/pkg/cli/server/root.go +++ b/pkg/cli/server/root.go @@ -724,6 +724,10 @@ func applyDefaultValues(config *config.Config, viperInstance *viper.Viper, logge if viperInstance.Get("storage::gcinterval") == nil { config.Storage.GCInterval = 0 } + } else if !viperInstance.IsSet("storage::retention::delay") { + // if GC is enabled, retentionDelay is set to gcDelay by default + // it could be default gcDelay or the custom value set in the config file + config.Storage.Retention.Delay = config.Storage.GCDelay } // apply deleteUntagged default @@ -802,7 +806,9 @@ func applyDefaultValues(config *config.Config, viperInstance *viper.Viper, logge // and retentionDelay is not set, it is set to default value if !viperInstance.IsSet("storage::subpaths::" + name + "::retention::delay") { - storageConfig.Retention.Delay = storageConstants.DefaultRetentionDelay + // retentionDelay is set to gcDelay by default + // it could be default gcDelay or the custom value set in the config file + storageConfig.Retention.Delay = storageConfig.GCDelay } // and gcInterval is not set, it is set to default value diff --git a/pkg/cli/server/root_test.go b/pkg/cli/server/root_test.go index 308dbc44..f49d8562 100644 --- a/pkg/cli/server/root_test.go +++ b/pkg/cli/server/root_test.go @@ -657,7 +657,7 @@ storage: "cve": { "updateInterval": "2h" } - }, + }, "ui": { "enable": true } @@ -693,7 +693,7 @@ storage: "cve": { "updateInterval": "2h" } - }, + }, "ui": { "enable": true } @@ -2735,3 +2735,230 @@ func runCLIWithConfig(tempDir string, config string) (string, error) { return logFile.Name(), nil } + +func TestRetentionDelayDefaults(t *testing.T) { + Convey("Test retention delay defaults to GC delay", t, func() { + Convey("When retention delay is not specified, it should default to GC delay", func() { + config := config.New() + tempDir := t.TempDir() + + // Config with GC enabled but no retention delay specified + content := []byte(`{ + "storage": { + "rootDirectory": "/tmp/zot", + "gc": true, + "gcDelay": "2h" + }, + "http": { + "address": "127.0.0.1", + "port": "8080" + } + }`) + tmpfile := path.Join(tempDir, "config.json") + err := os.WriteFile(tmpfile, content, 0o0600) + So(err, ShouldBeNil) + + err = cli.LoadConfiguration(config, tmpfile) + So(err, ShouldBeNil) + + // Verify GC delay is set correctly + So(config.Storage.GCDelay, ShouldEqual, 2*time.Hour) + // Verify retention delay defaults to GC delay + So(config.Storage.Retention.Delay, ShouldEqual, 2*time.Hour) + }) + + Convey("When retention delay is explicitly specified, it should use that value", func() { + config := config.New() + tempDir := t.TempDir() + + // Config with explicit retention delay + content := []byte(`{ + "storage": { + "rootDirectory": "/tmp/zot", + "gc": true, + "gcDelay": "2h", + "retention": { + "delay": "3h" + } + }, + "http": { + "address": "127.0.0.1", + "port": "8080" + } + }`) + tmpfile := path.Join(tempDir, "config.json") + err := os.WriteFile(tmpfile, content, 0o0600) + So(err, ShouldBeNil) + + err = cli.LoadConfiguration(config, tmpfile) + So(err, ShouldBeNil) + + // Verify GC delay is set correctly + So(config.Storage.GCDelay, ShouldEqual, 2*time.Hour) + // Verify retention delay uses explicit value + So(config.Storage.Retention.Delay, ShouldEqual, 3*time.Hour) + }) + + Convey("When GC is disabled, retention delay should be 0", func() { + config := config.New() + tempDir := t.TempDir() + + // Config with GC disabled + content := []byte(`{ + "storage": { + "rootDirectory": "/tmp/zot", + "gc": false + }, + "http": { + "address": "127.0.0.1", + "port": "8080" + } + }`) + tmpfile := path.Join(tempDir, "config.json") + err := os.WriteFile(tmpfile, content, 0o0600) + So(err, ShouldBeNil) + + err = cli.LoadConfiguration(config, tmpfile) + So(err, ShouldBeNil) + + // Verify GC delay is 0 when GC is disabled + So(config.Storage.GCDelay, ShouldEqual, 0) + // Verify retention delay is 0 when GC is disabled + So(config.Storage.Retention.Delay, ShouldEqual, 0) + }) + + Convey("When GC delay is not specified, retention delay should default to default GC delay", func() { + config := config.New() + tempDir := t.TempDir() + + // Config with GC enabled but no gcDelay specified + content := []byte(`{ + "storage": { + "rootDirectory": "/tmp/zot", + "gc": true + }, + "http": { + "address": "127.0.0.1", + "port": "8080" + } + }`) + tmpfile := path.Join(tempDir, "config.json") + err := os.WriteFile(tmpfile, content, 0o0600) + So(err, ShouldBeNil) + + err = cli.LoadConfiguration(config, tmpfile) + So(err, ShouldBeNil) + + // Verify GC delay defaults to default value + So(config.Storage.GCDelay, ShouldEqual, storageConstants.DefaultGCDelay) + // Verify retention delay defaults to default GC delay + So(config.Storage.Retention.Delay, ShouldEqual, storageConstants.DefaultGCDelay) + }) + }) + + Convey("Test subpath retention delay defaults to subpath GC delay", t, func() { + Convey("When subpath retention delay is not specified, it should default to subpath GC delay", func() { + config := config.New() + tempDir := t.TempDir() + + // Config with subpath GC enabled but no retention delay specified + content := []byte(`{ + "storage": { + "rootDirectory": "/tmp/zot", + "subPaths": { + "/a": { + "rootDirectory": "/tmp/zot-a", + "gc": true, + "gcDelay": "30m" + } + } + }, + "http": { + "address": "127.0.0.1", + "port": "8080" + } + }`) + tmpfile := path.Join(tempDir, "config.json") + err := os.WriteFile(tmpfile, content, 0o0600) + So(err, ShouldBeNil) + + err = cli.LoadConfiguration(config, tmpfile) + So(err, ShouldBeNil) + + // Verify subpath GC delay is set correctly + So(config.Storage.SubPaths["/a"].GCDelay, ShouldEqual, 30*time.Minute) + // Verify subpath retention delay defaults to subpath GC delay + So(config.Storage.SubPaths["/a"].Retention.Delay, ShouldEqual, 30*time.Minute) + }) + + Convey("When subpath retention delay is explicitly specified, it should use that value", func() { + config := config.New() + tempDir := t.TempDir() + + // Config with explicit subpath retention delay + content := []byte(`{ + "storage": { + "rootDirectory": "/tmp/zot", + "subPaths": { + "/a": { + "rootDirectory": "/tmp/zot-a", + "gc": true, + "gcDelay": "30m", + "retention": { + "delay": "45m" + } + } + } + }, + "http": { + "address": "127.0.0.1", + "port": "8080" + } + }`) + tmpfile := path.Join(tempDir, "config.json") + err := os.WriteFile(tmpfile, content, 0o0600) + So(err, ShouldBeNil) + + err = cli.LoadConfiguration(config, tmpfile) + So(err, ShouldBeNil) + + // Verify subpath GC delay is set correctly + So(config.Storage.SubPaths["/a"].GCDelay, ShouldEqual, 30*time.Minute) + // Verify subpath retention delay uses explicit value + So(config.Storage.SubPaths["/a"].Retention.Delay, ShouldEqual, 45*time.Minute) + }) + + Convey("When subpath GC is not specified, retention delay should default to default GC delay", func() { + config := config.New() + tempDir := t.TempDir() + + // Config with subpath but no GC settings + content := []byte(`{ + "storage": { + "rootDirectory": "/tmp/zot", + "subPaths": { + "/a": { + "rootDirectory": "/tmp/zot-a", + "gc": true + } + } + }, + "http": { + "address": "127.0.0.1", + "port": "8080" + } + }`) + tmpfile := path.Join(tempDir, "config.json") + err := os.WriteFile(tmpfile, content, 0o0600) + So(err, ShouldBeNil) + + err = cli.LoadConfiguration(config, tmpfile) + So(err, ShouldBeNil) + + // Verify subpath GC delay defaults to default value + So(config.Storage.SubPaths["/a"].GCDelay, ShouldEqual, storageConstants.DefaultGCDelay) + // Verify subpath retention delay defaults to default GC delay + So(config.Storage.SubPaths["/a"].Retention.Delay, ShouldEqual, storageConstants.DefaultGCDelay) + }) + }) +} diff --git a/pkg/storage/constants/constants.go b/pkg/storage/constants/constants.go index 0aba30a2..14533c28 100644 --- a/pkg/storage/constants/constants.go +++ b/pkg/storage/constants/constants.go @@ -22,7 +22,6 @@ const ( RedisDriverName = "redis" RedisLocksBucket = "locks" DefaultGCDelay = 1 * time.Hour - DefaultRetentionDelay = 24 * time.Hour DefaultGCInterval = 1 * time.Hour S3StorageDriverName = "s3" LocalStorageDriverName = "local" diff --git a/pkg/storage/gc/gc_internal_test.go b/pkg/storage/gc/gc_internal_test.go index a104ba09..d92f4947 100644 --- a/pkg/storage/gc/gc_internal_test.go +++ b/pkg/storage/gc/gc_internal_test.go @@ -52,7 +52,7 @@ func TestGarbageCollectManifestErrors(t *testing.T) { gc := NewGarbageCollect(imgStore, mocks.MetaDBMock{}, Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -176,7 +176,7 @@ func TestGarbageCollectIndexErrors(t *testing.T) { gc := NewGarbageCollect(imgStore, mocks.MetaDBMock{}, Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -291,7 +291,7 @@ func TestGarbageCollectWithMockedImageStore(t *testing.T) { gcOptions := Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -338,7 +338,7 @@ func TestGarbageCollectWithMockedImageStore(t *testing.T) { gcOptions := Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -366,7 +366,7 @@ func TestGarbageCollectWithMockedImageStore(t *testing.T) { gcOptions := Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, diff --git a/pkg/storage/gc/gc_test.go b/pkg/storage/gc/gc_test.go index ac59e85b..aa89e903 100644 --- a/pkg/storage/gc/gc_test.go +++ b/pkg/storage/gc/gc_test.go @@ -346,7 +346,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -765,7 +765,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -831,7 +831,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -894,7 +894,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -933,7 +933,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -971,7 +971,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -1009,7 +1009,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -1049,7 +1049,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -1134,7 +1134,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -1215,7 +1215,7 @@ func TestGarbageCollectAndRetentionMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: gcDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -1935,7 +1935,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -2282,7 +2282,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -2342,7 +2342,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -2397,7 +2397,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -2434,7 +2434,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -2539,7 +2539,7 @@ func TestGarbageCollectAndRetentionNoMetaDB(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, metaDB, gc.Options{ Delay: gcDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, diff --git a/pkg/storage/local/local_test.go b/pkg/storage/local/local_test.go index acd67712..00067558 100644 --- a/pkg/storage/local/local_test.go +++ b/pkg/storage/local/local_test.go @@ -48,7 +48,7 @@ const ( var trueVal bool = true //nolint: gochecknoglobals var DeleteReferrers = config.ImageRetention{ //nolint: gochecknoglobals - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, diff --git a/pkg/storage/storage_test.go b/pkg/storage/storage_test.go index c6d26486..3ee7f463 100644 --- a/pkg/storage/storage_test.go +++ b/pkg/storage/storage_test.go @@ -48,7 +48,7 @@ import ( var trueVal bool = true //nolint: gochecknoglobals var DeleteReferrers = config.ImageRetention{ //nolint: gochecknoglobals - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"}, @@ -1812,7 +1812,7 @@ func TestGarbageCollectImageManifest(t *testing.T) { gc := gc.NewGarbageCollect(imgStore, mocks.MetaDBMock{}, gc.Options{ Delay: storageConstants.DefaultGCDelay, ImageRetention: config.ImageRetention{ - Delay: storageConstants.DefaultRetentionDelay, + Delay: storageConstants.DefaultGCDelay, Policies: []config.RetentionPolicy{ { Repositories: []string{"**"},