Merge branch 'make-cache-configurable' into 'master'

Make timeouts for ZIP configurable

See merge request gitlab-org/gitlab-pages!385

3 files changed, 61 insertions, 17 deletions

diff --git a/internal/vfs/zip/archive.go b/internal/vfs/zip/archive.go
index ba15af20..47295dc0 100644
--- a/internal/vfs/zip/archive.go
+++ b/internal/vfs/zip/archive.go
@@ -24,9 +24,6 @@ import (
 const (
 	dirPrefix      = "public/"
 	maxSymlinkSize = 256
-
-	// DefaultOpenTimeout to request an archive and read its contents the first time
-	DefaultOpenTimeout = 30 * time.Second
 )
 
 var (
diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go
index d3c5cf9b..f176a1d6 100644
--- a/internal/vfs/zip/vfs.go
+++ b/internal/vfs/zip/vfs.go
@@ -19,6 +19,7 @@ const (
 	defaultCacheExpirationInterval = time.Minute
 	defaultCacheCleanupInterval    = time.Minute / 2
 	defaultCacheRefreshInterval    = time.Minute / 2
+	defaultOpenTimeout             = time.Minute / 2
 
 	// we assume that each item costs around 100 bytes
 	// this gives around 5MB of raw memory needed without acceleration structures
@@ -40,26 +41,72 @@ type zipVFS struct {
 	cache     *cache.Cache
 	cacheLock sync.Mutex
 
+	openTimeout             time.Duration
+	cacheExpirationInterval time.Duration
+	cacheRefreshInterval    time.Duration
+	cacheCleanupInterval    time.Duration
+
 	dataOffsetCache *lruCache
 	readlinkCache   *lruCache
 
 	archiveCount int64
 }
 
+// Option function allows to override default values
+type Option func(*zipVFS)
+
+// WithCacheRefreshInterval when used it can override defaultCacheRefreshInterval
+func WithCacheRefreshInterval(interval time.Duration) Option {
+	return func(vfs *zipVFS) {
+		vfs.cacheRefreshInterval = interval
+	}
+}
+
+// WithCacheExpirationInterval when used it can override defaultCacheExpirationInterval
+func WithCacheExpirationInterval(interval time.Duration) Option {
+	return func(vfs *zipVFS) {
+		vfs.cacheExpirationInterval = interval
+	}
+}
+
+// WithCacheCleanupInterval when used it can override defaultCacheCleanupInterval
+func WithCacheCleanupInterval(interval time.Duration) Option {
+	return func(vfs *zipVFS) {
+		vfs.cacheCleanupInterval = interval
+	}
+}
+
+// WithOpenTimeout when used it can override openTimeout
+func WithOpenTimeout(interval time.Duration) Option {
+	return func(vfs *zipVFS) {
+		vfs.openTimeout = interval
+	}
+}
+
 // New creates a zipVFS instance that can be used by a serving request
-func New() vfs.VFS {
+func New(options ...Option) vfs.VFS {
 	zipVFS := &zipVFS{
-		cache:           cache.New(defaultCacheExpirationInterval, defaultCacheCleanupInterval),
-		dataOffsetCache: newLruCache("data-offset", defaultDataOffsetItems, defaultDataOffsetExpirationInterval),
-		readlinkCache:   newLruCache("readlink", defaultReadlinkItems, defaultReadlinkExpirationInterval),
+		cacheExpirationInterval: defaultCacheExpirationInterval,
+		cacheRefreshInterval:    defaultCacheRefreshInterval,
+		cacheCleanupInterval:    defaultCacheCleanupInterval,
+		openTimeout:             defaultOpenTimeout,
+	}
+
+	for _, option := range options {
+		option(zipVFS)
 	}
 
+	zipVFS.cache = cache.New(zipVFS.cacheExpirationInterval, zipVFS.cacheCleanupInterval)
 	zipVFS.cache.OnEvicted(func(s string, i interface{}) {
 		metrics.ZipCachedEntries.WithLabelValues("archive").Dec()
 
 		i.(*zipArchive).onEvicted()
 	})
 
+	// TODO: To be removed with https://gitlab.com/gitlab-org/gitlab-pages/-/issues/480
+	zipVFS.dataOffsetCache = newLruCache("data-offset", defaultDataOffsetItems, defaultDataOffsetExpirationInterval)
+	zipVFS.readlinkCache = newLruCache("readlink", defaultReadlinkItems, defaultReadlinkExpirationInterval)
+
 	return zipVFS
 }
 
@@ -112,12 +159,12 @@ func (fs *zipVFS) findOrCreateArchive(ctx context.Context, path string) (*zipArc
 		metrics.ZipCacheRequests.WithLabelValues("archive", "hit").Inc()
 
 		// TODO: do not refreshed errored archives https://gitlab.com/gitlab-org/gitlab-pages/-/merge_requests/351
-		if time.Until(expiry) < defaultCacheRefreshInterval {
+		if time.Until(expiry) < fs.cacheRefreshInterval {
 			// refresh item
 			fs.cache.SetDefault(path, archive)
 		}
 	} else {
-		archive = newArchive(fs, path, DefaultOpenTimeout)
+		archive = newArchive(fs, path, fs.openTimeout)
 
 		// We call delete to ensure that expired item
 		// is properly evicted as there's a bug in a cache library:
@@ -126,7 +173,7 @@ func (fs *zipVFS) findOrCreateArchive(ctx context.Context, path string) (*zipArc
 
 		// if adding the archive to the cache fails it means it's already been added before
 		// this is done to find concurrent additions.
-		if fs.cache.Add(path, archive, cache.DefaultExpiration) != nil {
+		if fs.cache.Add(path, archive, fs.cacheExpirationInterval) != nil {
 			return nil, errAlreadyCached
 		}
 
diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go
index d4730524..878e015a 100644
--- a/internal/vfs/zip/vfs_test.go
+++ b/internal/vfs/zip/vfs_test.go
@@ -101,29 +101,29 @@ func TestVFSFindOrOpenArchiveConcurrentAccess(t *testing.T) {
 	}, time.Second, time.Nanosecond)
 }
 
-func TestVFSFindOrCreateArchiveCacheEvict(t *testing.T) {
+func TestVFSArchiveCacheEvict(t *testing.T) {
 	testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil)
 	defer cleanup()
 
 	path := testServerURL + "/public.zip"
 
-	vfs := New().(*zipVFS)
+	vfs := New(
+		WithCacheExpirationInterval(time.Nanosecond),
+	).(*zipVFS)
 
 	archivesMetric := metrics.ZipCachedEntries.WithLabelValues("archive")
 	archivesCount := testutil.ToFloat64(archivesMetric)
 
 	// create a new archive and increase counters
-	archive, err := vfs.findOrOpenArchive(context.Background(), path)
+	archive, err := vfs.Root(context.Background(), path)
 	require.NoError(t, err)
 	require.NotNil(t, archive)
 
-	// inject into cache to be "expired"
-	// (we could as well wait `defaultCacheExpirationInterval`)
-	vfs.cache.Set(path, archive, time.Nanosecond)
+	// wait for archive to expire
 	time.Sleep(time.Nanosecond)
 
 	// a new object is created
-	archive2, err := vfs.findOrOpenArchive(context.Background(), path)
+	archive2, err := vfs.Root(context.Background(), path)
 	require.NoError(t, err)
 	require.NotNil(t, archive2)
 	require.NotEqual(t, archive, archive2, "a different archive is returned")