From 0db58b5ce378aad7106ea1c0d04fa50e3b5ad54e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Tue, 27 Oct 2020 11:24:47 +0100 Subject: [PATCH 1/2] Make timeouts for ZIP configurable --- internal/vfs/zip/archive.go | 3 -- internal/vfs/zip/vfs.go | 61 +++++++++++++++++++++++++++++++----- internal/vfs/zip/vfs_test.go | 14 ++++----- 3 files changed, 61 insertions(+), 17 deletions(-) diff --git a/internal/vfs/zip/archive.go b/internal/vfs/zip/archive.go index ba15af200..47295dc0d 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 78a77e1cb..0685c5d93 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 + defaultOpenTimeout 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 + } +} + +// WithDefaultOpenTimeout when used it can override defaultOpenTimeout +func WithDefaultOpenTimeout(interval time.Duration) Option { + return func(vfs *zipVFS) { + vfs.defaultOpenTimeout = 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, + defaultOpenTimeout: 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.defaultOpenTimeout) // 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 c12e49cd1..a9737a432 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") -- GitLab From 1177c38011786e08c9685ac61a379364c6a930ed Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 28 Oct 2020 14:21:09 +1100 Subject: [PATCH 2/2] Rename openTiemout --- internal/vfs/zip/vfs.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index 0685c5d93..fe1522606 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -40,7 +40,7 @@ type zipVFS struct { cache *cache.Cache cacheLock sync.Mutex - defaultOpenTimeout time.Duration + openTimeout time.Duration cacheExpirationInterval time.Duration cacheRefreshInterval time.Duration cacheCleanupInterval time.Duration @@ -75,10 +75,10 @@ func WithCacheCleanupInterval(interval time.Duration) Option { } } -// WithDefaultOpenTimeout when used it can override defaultOpenTimeout -func WithDefaultOpenTimeout(interval time.Duration) Option { +// WithOpenTimeout when used it can override openTimeout +func WithOpenTimeout(interval time.Duration) Option { return func(vfs *zipVFS) { - vfs.defaultOpenTimeout = interval + vfs.openTimeout = interval } } @@ -88,7 +88,7 @@ func New(options ...Option) vfs.VFS { cacheExpirationInterval: defaultCacheExpirationInterval, cacheRefreshInterval: defaultCacheRefreshInterval, cacheCleanupInterval: defaultCacheCleanupInterval, - defaultOpenTimeout: defaultOpenTimeout, + openTimeout: defaultOpenTimeout, } for _, option := range options { @@ -158,7 +158,7 @@ func (fs *zipVFS) findOrCreateArchive(ctx context.Context, path string) (*zipArc fs.cache.SetDefault(path, archive) } } else { - archive = newArchive(fs, path, fs.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: -- GitLab