From 54cabcc8ea107974cf58c093016cab51d773e5c7 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 27 Oct 2020 16:28:47 +1100 Subject: [PATCH 1/3] Do not refresh errored archives Remove to-do and update test --- internal/vfs/zip/archive.go | 17 +++++++++++----- internal/vfs/zip/vfs.go | 9 +++++---- internal/vfs/zip/vfs_test.go | 39 ++++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 9 deletions(-) diff --git a/internal/vfs/zip/archive.go b/internal/vfs/zip/archive.go index 47295dc0d..df1757644 100644 --- a/internal/vfs/zip/archive.go +++ b/internal/vfs/zip/archive.go @@ -68,11 +68,8 @@ func newArchive(fs *zipVFS, path string, openTimeout time.Duration) *zipArchive func (a *zipArchive) openArchive(parentCtx context.Context) (err error) { // return early if openArchive was done already in a concurrent request - select { - case <-a.done: - return a.err - - default: + if ok, err := a.openStatus(); ok { + return err } ctx, cancel := context.WithTimeout(parentCtx, a.openTimeout) @@ -283,3 +280,13 @@ func (a *zipArchive) Readlink(ctx context.Context, name string) (string, error) func (a *zipArchive) onEvicted() { metrics.ZipArchiveEntriesCached.Sub(float64(len(a.files))) } + +func (a *zipArchive) openStatus() (bool, error) { + select { + case <-a.done: + return true, a.err + + default: + return false, nil + } +} diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index f176a1d66..3b69d1e9e 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -158,10 +158,11 @@ func (fs *zipVFS) findOrCreateArchive(ctx context.Context, path string) (*zipArc if found { 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) < fs.cacheRefreshInterval { - // refresh item - fs.cache.SetDefault(path, archive) + if opened, err := archive.(*zipArchive).openStatus(); opened && err == nil { + if time.Until(expiry) < fs.cacheRefreshInterval { + // refresh item that has been opened successfully + fs.cache.SetDefault(path, archive) + } } } else { archive = newArchive(fs, path, fs.openTimeout) diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go index 878e015aa..2c3e89493 100644 --- a/internal/vfs/zip/vfs_test.go +++ b/internal/vfs/zip/vfs_test.go @@ -131,3 +131,42 @@ func TestVFSArchiveCacheEvict(t *testing.T) { archivesCountEnd := testutil.ToFloat64(archivesMetric) require.Equal(t, float64(1), archivesCountEnd-archivesCount, "all expired archives are evicted") } + +func TestVFSFindOrCreateArchiveErrored(t *testing.T) { + testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil) + defer cleanup() + + path := testServerURL + "/unknown.zip" + + vfs := New().(*zipVFS) + + archivesMetric := metrics.ZipCachedEntries.WithLabelValues("archive") + archivesCount := testutil.ToFloat64(archivesMetric) + + // create a new archive and increase counters + archive, err := vfs.findOrOpenArchive(context.Background(), path) + require.Error(t, err) + require.Nil(t, archive) + + item, exp1, found := vfs.cache.GetWithExpiration(path) + require.True(t, found) + + // item has been opened and has an error + opened, err1 := item.(*zipArchive).openStatus() + require.True(t, opened) + require.Error(t, err1) + require.Equal(t, err, err1, "same error as findOrOpenArchive") + + // should return errored archive + archive2, err2 := vfs.findOrOpenArchive(context.Background(), path) + require.Error(t, err2) + require.Nil(t, archive2) + require.Equal(t, err1, err2, "same error for the same archive") + + _, exp2, found := vfs.cache.GetWithExpiration(path) + require.True(t, found) + require.Equal(t, exp1, exp2, "archive has not been refreshed") + + archivesCountEnd := testutil.ToFloat64(archivesMetric) + require.Equal(t, float64(1), archivesCountEnd-archivesCount, "only one archive cached") +} -- GitLab From 47a11efd46435fb28c3d5243c5419b0d9dc9c1fe Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 28 Oct 2020 16:33:09 +1100 Subject: [PATCH 2/3] Update test with configurable refresh remove commented test --- internal/vfs/zip/vfs_test.go | 77 ++++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 26 deletions(-) diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go index 2c3e89493..90023144d 100644 --- a/internal/vfs/zip/vfs_test.go +++ b/internal/vfs/zip/vfs_test.go @@ -132,41 +132,66 @@ func TestVFSArchiveCacheEvict(t *testing.T) { require.Equal(t, float64(1), archivesCountEnd-archivesCount, "all expired archives are evicted") } -func TestVFSFindOrCreateArchiveErrored(t *testing.T) { +func TestVFSFindOrOpenArchiveRefresh(t *testing.T) { testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil) defer cleanup() - path := testServerURL + "/unknown.zip" + vfs := New( + // using defaultCacheExpirationInterval ensures refresh would be called + WithCacheRefreshInterval(defaultCacheExpirationInterval), + ).(*zipVFS) - vfs := New().(*zipVFS) + tests := map[string]struct { + path string + expectOpenError bool + expectEqualExpiry bool + }{ + "successful_refresh": {path: "/public.zip", expectOpenError: false, expectEqualExpiry: false}, + "errored_archive": {path: "/unknown.zip", expectOpenError: true, expectEqualExpiry: true}, + } - archivesMetric := metrics.ZipCachedEntries.WithLabelValues("archive") - archivesCount := testutil.ToFloat64(archivesMetric) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + archivesMetric := metrics.ZipCachedEntries.WithLabelValues("archive") + archivesCount := testutil.ToFloat64(archivesMetric) + + path := testServerURL + test.path + + // create a new archive and increase counters + archive1, err1 := vfs.findOrOpenArchive(context.Background(), path) + if test.expectOpenError { + require.Error(t, err1) + require.Nil(t, archive1) + } else { + require.NoError(t, err1) + } - // create a new archive and increase counters - archive, err := vfs.findOrOpenArchive(context.Background(), path) - require.Error(t, err) - require.Nil(t, archive) + item1, exp1, found := vfs.cache.GetWithExpiration(path) + require.True(t, found) - item, exp1, found := vfs.cache.GetWithExpiration(path) - require.True(t, found) + // should return errored archive + archive2, err2 := vfs.findOrOpenArchive(context.Background(), path) + if test.expectOpenError { + require.Error(t, err2) + require.Nil(t, archive2) + } else { + require.NoError(t, err2) + } - // item has been opened and has an error - opened, err1 := item.(*zipArchive).openStatus() - require.True(t, opened) - require.Error(t, err1) - require.Equal(t, err, err1, "same error as findOrOpenArchive") + require.Equal(t, err1, err2, "same error for the same archive") - // should return errored archive - archive2, err2 := vfs.findOrOpenArchive(context.Background(), path) - require.Error(t, err2) - require.Nil(t, archive2) - require.Equal(t, err1, err2, "same error for the same archive") + item2, exp2, found := vfs.cache.GetWithExpiration(path) + require.True(t, found) + require.Equal(t, item1, item2, "same item is returned") - _, exp2, found := vfs.cache.GetWithExpiration(path) - require.True(t, found) - require.Equal(t, exp1, exp2, "archive has not been refreshed") + if test.expectEqualExpiry { + require.True(t, exp1.Equal(exp2), "archive has not been refreshed") + } else { + require.True(t, exp2.After(exp1), "exp2 should be after exp1 due to refresh") + } - archivesCountEnd := testutil.ToFloat64(archivesMetric) - require.Equal(t, float64(1), archivesCountEnd-archivesCount, "only one archive cached") + archivesCountEnd := testutil.ToFloat64(archivesMetric) + require.Equal(t, float64(1), archivesCountEnd-archivesCount, "only one archive cached") + }) + } } -- GitLab From b44f7c856739ed3155e8667d700d4209a01f5ada Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Thu, 29 Oct 2020 12:18:51 +1100 Subject: [PATCH 3/3] Refactor cache refresh tests --- internal/vfs/zip/vfs_test.go | 184 ++++++++++++++++++++--------------- 1 file changed, 106 insertions(+), 78 deletions(-) diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go index 90023144d..8a5e77a8d 100644 --- a/internal/vfs/zip/vfs_test.go +++ b/internal/vfs/zip/vfs_test.go @@ -101,97 +101,125 @@ func TestVFSFindOrOpenArchiveConcurrentAccess(t *testing.T) { }, time.Second, time.Nanosecond) } -func TestVFSArchiveCacheEvict(t *testing.T) { - testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil) - defer cleanup() - - path := testServerURL + "/public.zip" - - 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.Root(context.Background(), path) - require.NoError(t, err) - require.NotNil(t, archive) - - // wait for archive to expire - time.Sleep(time.Nanosecond) - - // a new object is created - 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") - - archivesCountEnd := testutil.ToFloat64(archivesMetric) - require.Equal(t, float64(1), archivesCountEnd-archivesCount, "all expired archives are evicted") -} - func TestVFSFindOrOpenArchiveRefresh(t *testing.T) { testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil) defer cleanup() - vfs := New( - // using defaultCacheExpirationInterval ensures refresh would be called - WithCacheRefreshInterval(defaultCacheExpirationInterval), - ).(*zipVFS) + // It should be large enough to not have flaky executions + const expiryInterval = 10 * time.Millisecond tests := map[string]struct { - path string - expectOpenError bool - expectEqualExpiry bool + path string + expirationInterval time.Duration + refreshInterval time.Duration + + expectNewArchive bool + expectOpenError bool + expectArchiveRefreshed bool }{ - "successful_refresh": {path: "/public.zip", expectOpenError: false, expectEqualExpiry: false}, - "errored_archive": {path: "/unknown.zip", expectOpenError: true, expectEqualExpiry: true}, + "after cache expiry of successful open a new archive is returned": { + path: "/public.zip", + expirationInterval: expiryInterval, + expectNewArchive: true, + expectOpenError: false, + }, + "after cache expiry of errored open a new archive is returned": { + path: "/unknown.zip", + expirationInterval: expiryInterval, + expectNewArchive: true, + expectOpenError: true, + }, + "subsequent open during refresh interval does refresh archive": { + path: "/public.zip", + expirationInterval: time.Second, + refreshInterval: time.Second, // refresh always + expectNewArchive: false, + expectOpenError: false, + expectArchiveRefreshed: true, + }, + "subsequent open before refresh interval does not refresh archive": { + path: "/public.zip", + expirationInterval: time.Second, + refreshInterval: time.Millisecond, // very short interval should not refresh + expectNewArchive: false, + expectOpenError: false, + expectArchiveRefreshed: false, + }, + "subsequent open of errored archive during refresh interval does not refresh": { + path: "/unknown.zip", + expirationInterval: time.Second, + refreshInterval: time.Second, // refresh always (if not error) + expectNewArchive: false, + expectOpenError: true, + expectArchiveRefreshed: false, + }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - archivesMetric := metrics.ZipCachedEntries.WithLabelValues("archive") - archivesCount := testutil.ToFloat64(archivesMetric) - - path := testServerURL + test.path - - // create a new archive and increase counters - archive1, err1 := vfs.findOrOpenArchive(context.Background(), path) - if test.expectOpenError { - require.Error(t, err1) - require.Nil(t, archive1) - } else { - require.NoError(t, err1) - } - - item1, exp1, found := vfs.cache.GetWithExpiration(path) - require.True(t, found) - - // should return errored archive - archive2, err2 := vfs.findOrOpenArchive(context.Background(), path) - if test.expectOpenError { - require.Error(t, err2) - require.Nil(t, archive2) - } else { - require.NoError(t, err2) - } + withExpectedArchiveCount(t, 1, func(t *testing.T) { + vfs := New( + WithCacheExpirationInterval(test.expirationInterval), + WithCacheRefreshInterval(test.refreshInterval), + ).(*zipVFS) + + path := testServerURL + test.path + + // create a new archive and increase counters + archive1, err1 := vfs.findOrOpenArchive(context.Background(), path) + if test.expectOpenError { + require.Error(t, err1) + require.Nil(t, archive1) + } else { + require.NoError(t, err1) + } + + item1, exp1, found := vfs.cache.GetWithExpiration(path) + require.True(t, found) + + // give some time to for timeouts to fire + time.Sleep(expiryInterval) + + if test.expectNewArchive { + // should return a new archive + archive2, err2 := vfs.findOrOpenArchive(context.Background(), path) + if test.expectOpenError { + require.Error(t, err2) + require.Nil(t, archive2) + } else { + require.NoError(t, err2) + require.NotEqual(t, archive1, archive2, "a new archive should be returned") + } + return + } + + // should return exactly the same archive + archive2, err2 := vfs.findOrOpenArchive(context.Background(), path) + require.Equal(t, archive1, archive2, "same archive is returned") + require.Equal(t, err1, err2, "same error for the same archive") + + item2, exp2, found := vfs.cache.GetWithExpiration(path) + require.True(t, found) + require.Equal(t, item1, item2, "same item is returned") + + if test.expectArchiveRefreshed { + require.Greater(t, exp2.UnixNano(), exp1.UnixNano(), "archive should be refreshed") + } else { + require.Equal(t, exp1.UnixNano(), exp2.UnixNano(), "archive has not been refreshed") + } + }) + }) + } +} - require.Equal(t, err1, err2, "same error for the same archive") +func withExpectedArchiveCount(t *testing.T, archiveCount int, fn func(t *testing.T)) { + t.Helper() - item2, exp2, found := vfs.cache.GetWithExpiration(path) - require.True(t, found) - require.Equal(t, item1, item2, "same item is returned") + archivesMetric := metrics.ZipCachedEntries.WithLabelValues("archive") + archivesCount := testutil.ToFloat64(archivesMetric) - if test.expectEqualExpiry { - require.True(t, exp1.Equal(exp2), "archive has not been refreshed") - } else { - require.True(t, exp2.After(exp1), "exp2 should be after exp1 due to refresh") - } + fn(t) - archivesCountEnd := testutil.ToFloat64(archivesMetric) - require.Equal(t, float64(1), archivesCountEnd-archivesCount, "only one archive cached") - }) - } + archivesCountEnd := testutil.ToFloat64(archivesMetric) + require.Equal(t, float64(archiveCount), archivesCountEnd-archivesCount, "exact number of archives is cached") } -- GitLab