Skip to content

Commit 0d42ba2

Browse files
andaaronandaaron
authored
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>
1 parent 466cbc3 commit 0d42ba2

8 files changed

Lines changed: 261 additions & 31 deletions

File tree

‎pkg/api/config/config.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -290,9 +290,7 @@ func New() *Config {
290290
GC: true,
291291
GCDelay: storageConstants.DefaultGCDelay,
292292
GCInterval: storageConstants.DefaultGCInterval,
293-
Retention: ImageRetention{
294-
Delay: storageConstants.DefaultRetentionDelay,
295-
},
293+
Retention: ImageRetention{},
296294
},
297295
},
298296
HTTP: HTTPConfig{Address: "127.0.0.1", Port: "8080", Auth: &AuthConfig{FailDelay: 0}},

‎pkg/cli/server/root.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -724,6 +724,10 @@ func applyDefaultValues(config *config.Config, viperInstance *viper.Viper, logge
724724
if viperInstance.Get("storage::gcinterval") == nil {
725725
config.Storage.GCInterval = 0
726726
}
727+
} else if !viperInstance.IsSet("storage::retention::delay") {
728+
// if GC is enabled, retentionDelay is set to gcDelay by default
729+
// it could be default gcDelay or the custom value set in the config file
730+
config.Storage.Retention.Delay = config.Storage.GCDelay
727731
}
728732

729733
// apply deleteUntagged default
@@ -802,7 +806,9 @@ func applyDefaultValues(config *config.Config, viperInstance *viper.Viper, logge
802806

803807
// and retentionDelay is not set, it is set to default value
804808
if !viperInstance.IsSet("storage::subpaths::" + name + "::retention::delay") {
805-
storageConfig.Retention.Delay = storageConstants.DefaultRetentionDelay
809+
// retentionDelay is set to gcDelay by default
810+
// it could be default gcDelay or the custom value set in the config file
811+
storageConfig.Retention.Delay = storageConfig.GCDelay
806812
}
807813

808814
// and gcInterval is not set, it is set to default value

‎pkg/cli/server/root_test.go‎

Lines changed: 229 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -657,7 +657,7 @@ storage:
657657
"cve": {
658658
"updateInterval": "2h"
659659
}
660-
},
660+
},
661661
"ui": {
662662
"enable": true
663663
}
@@ -693,7 +693,7 @@ storage:
693693
"cve": {
694694
"updateInterval": "2h"
695695
}
696-
},
696+
},
697697
"ui": {
698698
"enable": true
699699
}
@@ -2735,3 +2735,230 @@ func runCLIWithConfig(tempDir string, config string) (string, error) {
27352735

27362736
return logFile.Name(), nil
27372737
}
2738+
2739+
func TestRetentionDelayDefaults(t *testing.T) {
2740+
Convey("Test retention delay defaults to GC delay", t, func() {
2741+
Convey("When retention delay is not specified, it should default to GC delay", func() {
2742+
config := config.New()
2743+
tempDir := t.TempDir()
2744+
2745+
// Config with GC enabled but no retention delay specified
2746+
content := []byte(`{
2747+
"storage": {
2748+
"rootDirectory": "/tmp/zot",
2749+
"gc": true,
2750+
"gcDelay": "2h"
2751+
},
2752+
"http": {
2753+
"address": "127.0.0.1",
2754+
"port": "8080"
2755+
}
2756+
}`)
2757+
tmpfile := path.Join(tempDir, "config.json")
2758+
err := os.WriteFile(tmpfile, content, 0o0600)
2759+
So(err, ShouldBeNil)
2760+
2761+
err = cli.LoadConfiguration(config, tmpfile)
2762+
So(err, ShouldBeNil)
2763+
2764+
// Verify GC delay is set correctly
2765+
So(config.Storage.GCDelay, ShouldEqual, 2*time.Hour)
2766+
// Verify retention delay defaults to GC delay
2767+
So(config.Storage.Retention.Delay, ShouldEqual, 2*time.Hour)
2768+
})
2769+
2770+
Convey("When retention delay is explicitly specified, it should use that value", func() {
2771+
config := config.New()
2772+
tempDir := t.TempDir()
2773+
2774+
// Config with explicit retention delay
2775+
content := []byte(`{
2776+
"storage": {
2777+
"rootDirectory": "/tmp/zot",
2778+
"gc": true,
2779+
"gcDelay": "2h",
2780+
"retention": {
2781+
"delay": "3h"
2782+
}
2783+
},
2784+
"http": {
2785+
"address": "127.0.0.1",
2786+
"port": "8080"
2787+
}
2788+
}`)
2789+
tmpfile := path.Join(tempDir, "config.json")
2790+
err := os.WriteFile(tmpfile, content, 0o0600)
2791+
So(err, ShouldBeNil)
2792+
2793+
err = cli.LoadConfiguration(config, tmpfile)
2794+
So(err, ShouldBeNil)
2795+
2796+
// Verify GC delay is set correctly
2797+
So(config.Storage.GCDelay, ShouldEqual, 2*time.Hour)
2798+
// Verify retention delay uses explicit value
2799+
So(config.Storage.Retention.Delay, ShouldEqual, 3*time.Hour)
2800+
})
2801+
2802+
Convey("When GC is disabled, retention delay should be 0", func() {
2803+
config := config.New()
2804+
tempDir := t.TempDir()
2805+
2806+
// Config with GC disabled
2807+
content := []byte(`{
2808+
"storage": {
2809+
"rootDirectory": "/tmp/zot",
2810+
"gc": false
2811+
},
2812+
"http": {
2813+
"address": "127.0.0.1",
2814+
"port": "8080"
2815+
}
2816+
}`)
2817+
tmpfile := path.Join(tempDir, "config.json")
2818+
err := os.WriteFile(tmpfile, content, 0o0600)
2819+
So(err, ShouldBeNil)
2820+
2821+
err = cli.LoadConfiguration(config, tmpfile)
2822+
So(err, ShouldBeNil)
2823+
2824+
// Verify GC delay is 0 when GC is disabled
2825+
So(config.Storage.GCDelay, ShouldEqual, 0)
2826+
// Verify retention delay is 0 when GC is disabled
2827+
So(config.Storage.Retention.Delay, ShouldEqual, 0)
2828+
})
2829+
2830+
Convey("When GC delay is not specified, retention delay should default to default GC delay", func() {
2831+
config := config.New()
2832+
tempDir := t.TempDir()
2833+
2834+
// Config with GC enabled but no gcDelay specified
2835+
content := []byte(`{
2836+
"storage": {
2837+
"rootDirectory": "/tmp/zot",
2838+
"gc": true
2839+
},
2840+
"http": {
2841+
"address": "127.0.0.1",
2842+
"port": "8080"
2843+
}
2844+
}`)
2845+
tmpfile := path.Join(tempDir, "config.json")
2846+
err := os.WriteFile(tmpfile, content, 0o0600)
2847+
So(err, ShouldBeNil)
2848+
2849+
err = cli.LoadConfiguration(config, tmpfile)
2850+
So(err, ShouldBeNil)
2851+
2852+
// Verify GC delay defaults to default value
2853+
So(config.Storage.GCDelay, ShouldEqual, storageConstants.DefaultGCDelay)
2854+
// Verify retention delay defaults to default GC delay
2855+
So(config.Storage.Retention.Delay, ShouldEqual, storageConstants.DefaultGCDelay)
2856+
})
2857+
})
2858+
2859+
Convey("Test subpath retention delay defaults to subpath GC delay", t, func() {
2860+
Convey("When subpath retention delay is not specified, it should default to subpath GC delay", func() {
2861+
config := config.New()
2862+
tempDir := t.TempDir()
2863+
2864+
// Config with subpath GC enabled but no retention delay specified
2865+
content := []byte(`{
2866+
"storage": {
2867+
"rootDirectory": "/tmp/zot",
2868+
"subPaths": {
2869+
"/a": {
2870+
"rootDirectory": "/tmp/zot-a",
2871+
"gc": true,
2872+
"gcDelay": "30m"
2873+
}
2874+
}
2875+
},
2876+
"http": {
2877+
"address": "127.0.0.1",
2878+
"port": "8080"
2879+
}
2880+
}`)
2881+
tmpfile := path.Join(tempDir, "config.json")
2882+
err := os.WriteFile(tmpfile, content, 0o0600)
2883+
So(err, ShouldBeNil)
2884+
2885+
err = cli.LoadConfiguration(config, tmpfile)
2886+
So(err, ShouldBeNil)
2887+
2888+
// Verify subpath GC delay is set correctly
2889+
So(config.Storage.SubPaths["/a"].GCDelay, ShouldEqual, 30*time.Minute)
2890+
// Verify subpath retention delay defaults to subpath GC delay
2891+
So(config.Storage.SubPaths["/a"].Retention.Delay, ShouldEqual, 30*time.Minute)
2892+
})
2893+
2894+
Convey("When subpath retention delay is explicitly specified, it should use that value", func() {
2895+
config := config.New()
2896+
tempDir := t.TempDir()
2897+
2898+
// Config with explicit subpath retention delay
2899+
content := []byte(`{
2900+
"storage": {
2901+
"rootDirectory": "/tmp/zot",
2902+
"subPaths": {
2903+
"/a": {
2904+
"rootDirectory": "/tmp/zot-a",
2905+
"gc": true,
2906+
"gcDelay": "30m",
2907+
"retention": {
2908+
"delay": "45m"
2909+
}
2910+
}
2911+
}
2912+
},
2913+
"http": {
2914+
"address": "127.0.0.1",
2915+
"port": "8080"
2916+
}
2917+
}`)
2918+
tmpfile := path.Join(tempDir, "config.json")
2919+
err := os.WriteFile(tmpfile, content, 0o0600)
2920+
So(err, ShouldBeNil)
2921+
2922+
err = cli.LoadConfiguration(config, tmpfile)
2923+
So(err, ShouldBeNil)
2924+
2925+
// Verify subpath GC delay is set correctly
2926+
So(config.Storage.SubPaths["/a"].GCDelay, ShouldEqual, 30*time.Minute)
2927+
// Verify subpath retention delay uses explicit value
2928+
So(config.Storage.SubPaths["/a"].Retention.Delay, ShouldEqual, 45*time.Minute)
2929+
})
2930+
2931+
Convey("When subpath GC is not specified, retention delay should default to default GC delay", func() {
2932+
config := config.New()
2933+
tempDir := t.TempDir()
2934+
2935+
// Config with subpath but no GC settings
2936+
content := []byte(`{
2937+
"storage": {
2938+
"rootDirectory": "/tmp/zot",
2939+
"subPaths": {
2940+
"/a": {
2941+
"rootDirectory": "/tmp/zot-a",
2942+
"gc": true
2943+
}
2944+
}
2945+
},
2946+
"http": {
2947+
"address": "127.0.0.1",
2948+
"port": "8080"
2949+
}
2950+
}`)
2951+
tmpfile := path.Join(tempDir, "config.json")
2952+
err := os.WriteFile(tmpfile, content, 0o0600)
2953+
So(err, ShouldBeNil)
2954+
2955+
err = cli.LoadConfiguration(config, tmpfile)
2956+
So(err, ShouldBeNil)
2957+
2958+
// Verify subpath GC delay defaults to default value
2959+
So(config.Storage.SubPaths["/a"].GCDelay, ShouldEqual, storageConstants.DefaultGCDelay)
2960+
// Verify subpath retention delay defaults to default GC delay
2961+
So(config.Storage.SubPaths["/a"].Retention.Delay, ShouldEqual, storageConstants.DefaultGCDelay)
2962+
})
2963+
})
2964+
}

‎pkg/storage/constants/constants.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ const (
2222
RedisDriverName = "redis"
2323
RedisLocksBucket = "locks"
2424
DefaultGCDelay = 1 * time.Hour
25-
DefaultRetentionDelay = 24 * time.Hour
2625
DefaultGCInterval = 1 * time.Hour
2726
S3StorageDriverName = "s3"
2827
LocalStorageDriverName = "local"

‎pkg/storage/gc/gc_internal_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func TestGarbageCollectManifestErrors(t *testing.T) {
5252
gc := NewGarbageCollect(imgStore, mocks.MetaDBMock{}, Options{
5353
Delay: storageConstants.DefaultGCDelay,
5454
ImageRetention: config.ImageRetention{
55-
Delay: storageConstants.DefaultRetentionDelay,
55+
Delay: storageConstants.DefaultGCDelay,
5656
Policies: []config.RetentionPolicy{
5757
{
5858
Repositories: []string{"**"},
@@ -176,7 +176,7 @@ func TestGarbageCollectIndexErrors(t *testing.T) {
176176
gc := NewGarbageCollect(imgStore, mocks.MetaDBMock{}, Options{
177177
Delay: storageConstants.DefaultGCDelay,
178178
ImageRetention: config.ImageRetention{
179-
Delay: storageConstants.DefaultRetentionDelay,
179+
Delay: storageConstants.DefaultGCDelay,
180180
Policies: []config.RetentionPolicy{
181181
{
182182
Repositories: []string{"**"},
@@ -291,7 +291,7 @@ func TestGarbageCollectWithMockedImageStore(t *testing.T) {
291291
gcOptions := Options{
292292
Delay: storageConstants.DefaultGCDelay,
293293
ImageRetention: config.ImageRetention{
294-
Delay: storageConstants.DefaultRetentionDelay,
294+
Delay: storageConstants.DefaultGCDelay,
295295
Policies: []config.RetentionPolicy{
296296
{
297297
Repositories: []string{"**"},
@@ -338,7 +338,7 @@ func TestGarbageCollectWithMockedImageStore(t *testing.T) {
338338
gcOptions := Options{
339339
Delay: storageConstants.DefaultGCDelay,
340340
ImageRetention: config.ImageRetention{
341-
Delay: storageConstants.DefaultRetentionDelay,
341+
Delay: storageConstants.DefaultGCDelay,
342342
Policies: []config.RetentionPolicy{
343343
{
344344
Repositories: []string{"**"},
@@ -366,7 +366,7 @@ func TestGarbageCollectWithMockedImageStore(t *testing.T) {
366366
gcOptions := Options{
367367
Delay: storageConstants.DefaultGCDelay,
368368
ImageRetention: config.ImageRetention{
369-
Delay: storageConstants.DefaultRetentionDelay,
369+
Delay: storageConstants.DefaultGCDelay,
370370
Policies: []config.RetentionPolicy{
371371
{
372372
Repositories: []string{"**"},

0 commit comments

Comments
 (0)