diff --git a/internal/vfs/zip/archive.go b/internal/vfs/zip/archive.go index ba15af20044a34f3ed0686b7303b80aa8394de1b..47295dc0d714cadea6f562c06c66cb67dfa916ad 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 78a77e1cb374a3cd9ff20701c78756760522844c..fe1522606571586e53ae0ac0bec493cf6a79fcd2 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -18,6 +18,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 @@ -39,26 +40,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 } @@ -106,12 +153,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: @@ -120,7 +167,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 c12e49cd1490f6fbc4597d62c22a50efd3211216..a9737a432df60f7298155bfe0afc6a6a66ffa5d7 100644 --- a/internal/vfs/zip/vfs_test.go +++ b/internal/vfs/zip/vfs_test.go @@ -99,29 +99,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")