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 <andreifdaaron@gmail.com>

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}},

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

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)
+		})
+	})
+}

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"

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{"**"},

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{"**"},

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{"**"},

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{"**"},