From d091ea1bb9d69299e6aed023dce1ac1d683aa6c8 Mon Sep 17 00:00:00 2001 From: chlins Date: Sat, 17 Jun 2023 20:55:42 +0800 Subject: [PATCH] refactor: migrate the redis command keys to scan Refine the cache interface, migrate the Keys to Scan, change the redis underlying keys command to scan. Signed-off-by: chlins --- src/go.mod | 1 - src/lib/cache/cache.go | 13 +- src/lib/cache/cache_test.go | 13 +- src/lib/cache/helper_test.go | 9 +- src/lib/cache/memory/memory.go | 58 ++++++-- src/lib/cache/memory/memory_test.go | 71 ++++++--- src/lib/cache/mock_cache_test.go | 133 +++++++++++++++++ src/lib/cache/redis/redis.go | 50 ++++--- src/lib/cache/redis/redis_test.go | 70 ++++++--- src/pkg/cached/artifact/redis/manager_test.go | 10 +- src/pkg/cached/base_manager.go | 19 ++- src/pkg/cached/base_manager_test.go | 13 +- src/pkg/cached/manifest/redis/manager_test.go | 10 +- src/pkg/cached/project/redis/manager_test.go | 10 +- .../cached/project_metadata/redis/manager.go | 8 +- .../project_metadata/redis/manager_test.go | 10 +- .../cached/repository/redis/manager_test.go | 10 +- src/pkg/task/dao/execution.go | 7 +- src/pkg/task/dao/execution_test.go | 4 +- src/testing/lib/cache/cache.go | 64 ++++----- src/testing/lib/cache/iterator.go | 57 ++++++++ src/testing/lib/lib.go | 1 + src/testing/lib/libcache/cache.go | 136 ++++++++++++++++++ 23 files changed, 615 insertions(+), 162 deletions(-) create mode 100644 src/lib/cache/mock_cache_test.go create mode 100644 src/testing/lib/cache/iterator.go create mode 100644 src/testing/lib/libcache/cache.go diff --git a/src/go.mod b/src/go.mod index b12401187fc..04d4529db35 100644 --- a/src/go.mod +++ b/src/go.mod @@ -47,7 +47,6 @@ require ( github.com/opencontainers/go-digest v1.0.0 github.com/opencontainers/image-spec v1.1.0-rc2.0.20221005185240-3a7f492d3f1b github.com/pkg/errors v0.9.1 - github.com/prometheus/client_golang v1.14.0 github.com/robfig/cron/v3 v3.0.0 github.com/spf13/viper v1.8.1 diff --git a/src/lib/cache/cache.go b/src/lib/cache/cache.go index aa23efaaa64..85f1b921e6d 100644 --- a/src/lib/cache/cache.go +++ b/src/lib/cache/cache.go @@ -40,6 +40,14 @@ var ( ErrNotFound = errors.New("key not found") ) +// Iterator returns the ScanIterator +type Iterator interface { + Next(ctx context.Context) bool + Val() string +} + +//go:generate mockery --name Cache --output . --outpkg cache --filename mock_cache_test.go --structname mockCache --inpackage + // Cache cache interface type Cache interface { // Contains returns true if key exists @@ -57,8 +65,9 @@ type Cache interface { // Save cache the value by key Save(ctx context.Context, key string, value interface{}, expiration ...time.Duration) error - // Keys returns the key matched by prefixes - Keys(ctx context.Context, prefixes ...string) ([]string, error) + // Scan scans the keys matched by match string + // NOTICE: memory cache does not support use wildcard, compared by strings.Contains + Scan(ctx context.Context, match string) (Iterator, error) } var ( diff --git a/src/lib/cache/cache_test.go b/src/lib/cache/cache_test.go index 3024d8dafb3..72e4e6120c8 100644 --- a/src/lib/cache/cache_test.go +++ b/src/lib/cache/cache_test.go @@ -18,10 +18,9 @@ import ( "fmt" "testing" + "github.com/goharbor/harbor/src/lib/retry" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" - - cachetesting "github.com/goharbor/harbor/src/testing/lib/cache" - "github.com/goharbor/harbor/src/testing/mock" ) type CacheTestSuite struct { @@ -30,7 +29,7 @@ type CacheTestSuite struct { func (suite *CacheTestSuite) SetupSuite() { Register("mock", func(opts Options) (Cache, error) { - return &cachetesting.Cache{}, nil + return &mockCache{}, nil }) } @@ -62,8 +61,8 @@ func (suite *CacheTestSuite) TestInitialize() { { Register("cache", func(opts Options) (Cache, error) { - c := &cachetesting.Cache{} - c.On("Ping", mock.Anything).Return(fmt.Errorf("oops")) + c := &mockCache{} + c.On("Ping", mock.Anything).Return(retry.Abort(fmt.Errorf("oops"))) return c, nil }) @@ -75,7 +74,7 @@ func (suite *CacheTestSuite) TestInitialize() { { Register("cache", func(opts Options) (Cache, error) { - c := &cachetesting.Cache{} + c := &mockCache{} c.On("Ping", mock.Anything).Return(nil) return c, nil diff --git a/src/lib/cache/helper_test.go b/src/lib/cache/helper_test.go index c9ccf9f6fa2..1aed1d719c6 100644 --- a/src/lib/cache/helper_test.go +++ b/src/lib/cache/helper_test.go @@ -23,7 +23,6 @@ import ( "github.com/stretchr/testify/suite" - cachetesting "github.com/goharbor/harbor/src/testing/lib/cache" "github.com/goharbor/harbor/src/testing/mock" ) @@ -42,7 +41,7 @@ func (suite *FetchOrSaveTestSuite) SetupSuite() { } func (suite *FetchOrSaveTestSuite) TestFetchInternalError() { - c := &cachetesting.Cache{} + c := &mockCache{} mock.OnAnything(c, "Fetch").Return(fmt.Errorf("oops")) @@ -55,7 +54,7 @@ func (suite *FetchOrSaveTestSuite) TestFetchInternalError() { } func (suite *FetchOrSaveTestSuite) TestBuildError() { - c := &cachetesting.Cache{} + c := &mockCache{} mock.OnAnything(c, "Fetch").Return(ErrNotFound) @@ -68,7 +67,7 @@ func (suite *FetchOrSaveTestSuite) TestBuildError() { } func (suite *FetchOrSaveTestSuite) TestSaveError() { - c := &cachetesting.Cache{} + c := &mockCache{} mock.OnAnything(c, "Fetch").Return(ErrNotFound) mock.OnAnything(c, "Save").Return(fmt.Errorf("oops")) @@ -83,7 +82,7 @@ func (suite *FetchOrSaveTestSuite) TestSaveError() { } func (suite *FetchOrSaveTestSuite) TestSaveCalledOnlyOneTime() { - c := &cachetesting.Cache{} + c := &mockCache{} var data sync.Map diff --git a/src/lib/cache/memory/memory.go b/src/lib/cache/memory/memory.go index 6f65f23686d..e80757259d0 100644 --- a/src/lib/cache/memory/memory.go +++ b/src/lib/cache/memory/memory.go @@ -117,27 +117,55 @@ func (c *Cache) Save(ctx context.Context, key string, value interface{}, expirat return nil } -// Keys returns the key matched by prefixes. -func (c *Cache) Keys(ctx context.Context, prefixes ...string) ([]string, error) { - // if no prefix, means match all keys. - matchAll := len(prefixes) == 0 - // range map to get all keys - keys := make([]string, 0) +// Scan scans the keys matched by match string +func (c *Cache) Scan(ctx context.Context, match string) (cache.Iterator, error) { + var keys []string c.storage.Range(func(k, v interface{}) bool { - ks := k.(string) - if matchAll { - keys = append(keys, ks) - } else { - for _, p := range prefixes { - if strings.HasPrefix(ks, c.opts.Key(p)) { - keys = append(keys, strings.TrimPrefix(ks, c.opts.Prefix)) - } + matched := true + if match != "" { + matched = strings.Contains(k.(string), match) + } + + if matched { + if v.(*entry).isExpirated() { + c.storage.Delete(k) + } else { + keys = append(keys, strings.TrimPrefix(k.(string), c.opts.Prefix)) } } return true }) - return keys, nil + return &ScanIterator{keys: keys}, nil +} + +// ScanIterator is a ScanIterator for memory cache +type ScanIterator struct { + mu sync.Mutex + pos int + keys []string +} + +// Next checks whether has the next element +func (i *ScanIterator) Next(ctx context.Context) bool { + i.mu.Lock() + defer i.mu.Unlock() + + i.pos++ + return i.pos <= len(i.keys) +} + +// Val returns the key +func (i *ScanIterator) Val() string { + i.mu.Lock() + defer i.mu.Unlock() + + var val string + if i.pos <= len(i.keys) { + val = i.keys[i.pos-1] + } + + return val } // New returns memory cache diff --git a/src/lib/cache/memory/memory_test.go b/src/lib/cache/memory/memory_test.go index 9d6c6119de1..883045b5cc6 100644 --- a/src/lib/cache/memory/memory_test.go +++ b/src/lib/cache/memory/memory_test.go @@ -16,6 +16,7 @@ package memory import ( "context" + "fmt" "testing" "time" @@ -109,28 +110,54 @@ func (suite *CacheTestSuite) TestPing() { suite.NoError(suite.cache.Ping(suite.ctx)) } -func (suite *CacheTestSuite) TestKeys() { - key1 := "p1" - key2 := "p2" - - var err error - err = suite.cache.Save(suite.ctx, key1, "hello, p1") - suite.Nil(err) - err = suite.cache.Save(suite.ctx, key2, "hello, p2") - suite.Nil(err) - - // should match all - keys, err := suite.cache.Keys(suite.ctx, "p") - suite.Nil(err) - suite.ElementsMatch([]string{"p1", "p2"}, keys) - // only get p1 - keys, err = suite.cache.Keys(suite.ctx, key1) - suite.Nil(err) - suite.Equal([]string{"p1"}, keys) - // only get p2 - keys, err = suite.cache.Keys(suite.ctx, key2) - suite.Nil(err) - suite.Equal([]string{"p2"}, keys) +func (suite *CacheTestSuite) TestScan() { + seed := func(n int) { + for i := 0; i < n; i++ { + key := fmt.Sprintf("test-scan-%d", i) + err := suite.cache.Save(suite.ctx, key, "") + suite.NoError(err) + } + } + clean := func(n int) { + for i := 0; i < n; i++ { + key := fmt.Sprintf("test-scan-%d", i) + err := suite.cache.Delete(suite.ctx, key) + suite.NoError(err) + } + } + { + // no match should return all keys + expect := []string{"test-scan-0", "test-scan-1", "test-scan-2"} + // seed data + seed(3) + // test scan + iter, err := suite.cache.Scan(suite.ctx, "") + suite.NoError(err) + got := []string{} + for iter.Next(suite.ctx) { + got = append(got, iter.Val()) + } + suite.ElementsMatch(expect, got) + // clean up + clean(3) + } + + { + // with match should return matched keys + expect := []string{"test-scan-1", "test-scan-10"} + // seed data + seed(11) + // test scan + iter, err := suite.cache.Scan(suite.ctx, "test-scan-1") + suite.NoError(err) + got := []string{} + for iter.Next(suite.ctx) { + got = append(got, iter.Val()) + } + suite.ElementsMatch(expect, got) + // clean up + clean(11) + } } func TestCacheTestSuite(t *testing.T) { diff --git a/src/lib/cache/mock_cache_test.go b/src/lib/cache/mock_cache_test.go new file mode 100644 index 00000000000..e8e0b3574a1 --- /dev/null +++ b/src/lib/cache/mock_cache_test.go @@ -0,0 +1,133 @@ +// Code generated by mockery v2.22.1. DO NOT EDIT. + +package cache + +import ( + context "context" + time "time" + + mock "github.com/stretchr/testify/mock" +) + +// mockCache is an autogenerated mock type for the Cache type +type mockCache struct { + mock.Mock +} + +// Contains provides a mock function with given fields: ctx, key +func (_m *mockCache) Contains(ctx context.Context, key string) bool { + ret := _m.Called(ctx, key) + + var r0 bool + if rf, ok := ret.Get(0).(func(context.Context, string) bool); ok { + r0 = rf(ctx, key) + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} + +// Delete provides a mock function with given fields: ctx, key +func (_m *mockCache) Delete(ctx context.Context, key string) error { + ret := _m.Called(ctx, key) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { + r0 = rf(ctx, key) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Fetch provides a mock function with given fields: ctx, key, value +func (_m *mockCache) Fetch(ctx context.Context, key string, value interface{}) error { + ret := _m.Called(ctx, key, value) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, interface{}) error); ok { + r0 = rf(ctx, key, value) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Ping provides a mock function with given fields: ctx +func (_m *mockCache) Ping(ctx context.Context) error { + ret := _m.Called(ctx) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context) error); ok { + r0 = rf(ctx) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Save provides a mock function with given fields: ctx, key, value, expiration +func (_m *mockCache) Save(ctx context.Context, key string, value interface{}, expiration ...time.Duration) error { + _va := make([]interface{}, len(expiration)) + for _i := range expiration { + _va[_i] = expiration[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, key, value) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, interface{}, ...time.Duration) error); ok { + r0 = rf(ctx, key, value, expiration...) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Scan provides a mock function with given fields: ctx, match +func (_m *mockCache) Scan(ctx context.Context, match string) (Iterator, error) { + ret := _m.Called(ctx, match) + + var r0 Iterator + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string) (Iterator, error)); ok { + return rf(ctx, match) + } + if rf, ok := ret.Get(0).(func(context.Context, string) Iterator); ok { + r0 = rf(ctx, match) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(Iterator) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = rf(ctx, match) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +type mockConstructorTestingTnewMockCache interface { + mock.TestingT + Cleanup(func()) +} + +// newMockCache creates a new instance of mockCache. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +func newMockCache(t mockConstructorTestingTnewMockCache) *mockCache { + mock := &mockCache{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/src/lib/cache/redis/redis.go b/src/lib/cache/redis/redis.go index 3e7d5e433aa..a72b05e9c87 100644 --- a/src/lib/cache/redis/redis.go +++ b/src/lib/cache/redis/redis.go @@ -25,6 +25,7 @@ import ( "github.com/goharbor/harbor/src/lib/cache" "github.com/goharbor/harbor/src/lib/errors" + "github.com/goharbor/harbor/src/lib/log" ) var _ cache.Cache = (*Cache)(nil) @@ -89,30 +90,41 @@ func (c *Cache) Save(ctx context.Context, key string, value interface{}, expirat return c.Client.Set(ctx, c.opts.Key(key), data, exp).Err() } -// Keys returns the key matched by prefixes. -func (c *Cache) Keys(ctx context.Context, prefixes ...string) ([]string, error) { - patterns := make([]string, 0, len(prefixes)) - if len(prefixes) == 0 { - patterns = append(patterns, "*") - } else { - for _, p := range prefixes { - patterns = append(patterns, c.opts.Key(p)+"*") - } +// Scan scans the keys matched by match string +func (c *Cache) Scan(ctx context.Context, match string) (cache.Iterator, error) { + // the cursor and count are used for scan from redis, do not expose to outside + // by performance concern. + // cursor should start from 0 + cursor := uint64(0) + count := int64(1000) + match = fmt.Sprintf("%s*%s*", c.opts.Prefix, match) + iter := c.Client.Scan(ctx, cursor, match, count).Iterator() + if iter.Err() != nil { + return nil, iter.Err() } - keys := make([]string, 0) - for _, pattern := range patterns { - cmd := c.Client.Keys(ctx, pattern) - if err := cmd.Err(); err != nil { - return nil, err - } + return &ScanIterator{iter: iter, prefix: c.opts.Prefix}, nil +} - for _, k := range cmd.Val() { - keys = append(keys, strings.TrimPrefix(k, c.opts.Prefix)) - } +// ScanIterator is a wrapper for redis ScanIterator +type ScanIterator struct { + iter *redis.ScanIterator + prefix string +} + +// Next check whether has the next element +func (i *ScanIterator) Next(ctx context.Context) bool { + hasNext := i.iter.Next(ctx) + if !hasNext && i.iter.Err() != nil { + log.Errorf("error occurred when scan redis: %v", i.iter.Err()) } - return keys, nil + return hasNext +} + +// Val returns the key +func (i *ScanIterator) Val() string { + return strings.TrimPrefix(i.iter.Val(), i.prefix) } // New returns redis cache diff --git a/src/lib/cache/redis/redis_test.go b/src/lib/cache/redis/redis_test.go index cd8a67ea5f7..1170543aab1 100644 --- a/src/lib/cache/redis/redis_test.go +++ b/src/lib/cache/redis/redis_test.go @@ -110,28 +110,54 @@ func (suite *CacheTestSuite) TestPing() { suite.NoError(suite.cache.Ping(suite.ctx)) } -func (suite *CacheTestSuite) TestKeys() { - key1 := "p1" - key2 := "p2" - - var err error - err = suite.cache.Save(suite.ctx, key1, "hello, p1") - suite.Nil(err) - err = suite.cache.Save(suite.ctx, key2, "hello, p2") - suite.Nil(err) - - // should match all - keys, err := suite.cache.Keys(suite.ctx, "p") - suite.Nil(err) - suite.ElementsMatch([]string{"p1", "p2"}, keys) - // only get p1 - keys, err = suite.cache.Keys(suite.ctx, key1) - suite.Nil(err) - suite.Equal([]string{"p1"}, keys) - // only get p2 - keys, err = suite.cache.Keys(suite.ctx, key2) - suite.Nil(err) - suite.Equal([]string{"p2"}, keys) +func (suite *CacheTestSuite) TestScan() { + seed := func(n int) { + for i := 0; i < n; i++ { + key := fmt.Sprintf("test-scan-%d", i) + err := suite.cache.Save(suite.ctx, key, "") + suite.NoError(err) + } + } + clean := func(n int) { + for i := 0; i < n; i++ { + key := fmt.Sprintf("test-scan-%d", i) + err := suite.cache.Delete(suite.ctx, key) + suite.NoError(err) + } + } + { + // no match should return all keys + expect := []string{"test-scan-0", "test-scan-1", "test-scan-2"} + // seed data + seed(3) + // test scan + iter, err := suite.cache.Scan(suite.ctx, "") + suite.NoError(err) + got := []string{} + for iter.Next(suite.ctx) { + got = append(got, iter.Val()) + } + suite.ElementsMatch(expect, got) + // clean up + clean(3) + } + + { + // with match should return matched keys + expect := []string{"test-scan-1", "test-scan-10"} + // seed data + seed(11) + // test scan + iter, err := suite.cache.Scan(suite.ctx, "*test-scan-1*") + suite.NoError(err) + got := []string{} + for iter.Next(suite.ctx) { + got = append(got, iter.Val()) + } + suite.ElementsMatch(expect, got) + // clean up + clean(11) + } } func TestCacheTestSuite(t *testing.T) { diff --git a/src/pkg/cached/artifact/redis/manager_test.go b/src/pkg/cached/artifact/redis/manager_test.go index b3aca32faaa..a46accc060b 100644 --- a/src/pkg/cached/artifact/redis/manager_test.go +++ b/src/pkg/cached/artifact/redis/manager_test.go @@ -34,12 +34,14 @@ type managerTestSuite struct { cachedManager CachedManager artMgr *testArt.Manager cache *testcache.Cache + iterator *testcache.Iterator ctx context.Context } func (m *managerTestSuite) SetupTest() { m.artMgr = &testArt.Manager{} m.cache = &testcache.Cache{} + m.iterator = &testcache.Iterator{} m.cachedManager = NewManager(m.artMgr) m.cachedManager.(*Manager).WithCacheClient(m.cache) m.ctx = context.TODO() @@ -177,10 +179,11 @@ func (m *managerTestSuite) TestResourceType() { } func (m *managerTestSuite) TestCountCache() { - m.cache.On("Keys", mock.Anything, mock.Anything).Return([]string{"1"}, nil).Once() + m.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.iterator, nil).Once() c, err := m.cachedManager.CountCache(m.ctx) m.NoError(err) - m.Equal(int64(1), c) + m.Equal(int64(0), c) } func (m *managerTestSuite) TestDeleteCache() { @@ -190,7 +193,8 @@ func (m *managerTestSuite) TestDeleteCache() { } func (m *managerTestSuite) TestFlushAll() { - m.cache.On("Keys", mock.Anything, mock.Anything).Return([]string{"1"}, nil).Once() + m.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.iterator, nil).Once() m.cache.On("Delete", mock.Anything, mock.Anything).Return(nil).Once() err := m.cachedManager.FlushAll(m.ctx) m.NoError(err) diff --git a/src/pkg/cached/base_manager.go b/src/pkg/cached/base_manager.go index aa5cd8f4f8e..bc810372849 100644 --- a/src/pkg/cached/base_manager.go +++ b/src/pkg/cached/base_manager.go @@ -60,8 +60,8 @@ func (*cacheClient) Save(ctx context.Context, key string, value interface{}, exp return cache.Default().Save(ctx, key, value, expiration...) } -func (*cacheClient) Keys(ctx context.Context, prefixes ...string) ([]string, error) { - return cache.Default().Keys(ctx, prefixes...) +func (*cacheClient) Scan(ctx context.Context, match string) (cache.Iterator, error) { + return cache.Default().Scan(ctx, match) } var _ Manager = &BaseManager{} @@ -98,13 +98,18 @@ func (bm *BaseManager) ResourceType(ctx context.Context) string { // CountCache returns current this resource occupied cache count. func (bm *BaseManager) CountCache(ctx context.Context) (int64, error) { + var count int64 // prefix is resource type - keys, err := bm.CacheClient(ctx).Keys(ctx, bm.ResourceType(ctx)) + iter, err := bm.CacheClient(ctx).Scan(ctx, bm.ResourceType(ctx)) if err != nil { return 0, err } - return int64(len(keys)), nil + for iter.Next(ctx) { + count++ + } + + return count, nil } // DeleteCache deletes specific cache by key. @@ -115,14 +120,14 @@ func (bm *BaseManager) DeleteCache(ctx context.Context, key string) error { // FlushAll flush this resource's all cache. func (bm *BaseManager) FlushAll(ctx context.Context) error { // prefix is resource type - keys, err := bm.CacheClient(ctx).Keys(ctx, bm.ResourceType(ctx)) + iter, err := bm.CacheClient(ctx).Scan(ctx, bm.ResourceType(ctx)) if err != nil { return err } var errs errors.Errors - for _, key := range keys { - if err = bm.CacheClient(ctx).Delete(ctx, key); err != nil { + for iter.Next(ctx) { + if err = bm.CacheClient(ctx).Delete(ctx, iter.Val()); err != nil { errs = append(errs, err) } } diff --git a/src/pkg/cached/base_manager_test.go b/src/pkg/cached/base_manager_test.go index 7cdc76f2466..5db2613f9ea 100644 --- a/src/pkg/cached/base_manager_test.go +++ b/src/pkg/cached/base_manager_test.go @@ -30,6 +30,7 @@ var testResourceType = "resource-test" type testCache struct { *testcache.Cache + iterator *testcache.Iterator } func (tc *testCache) Save(ctx context.Context, key string, value interface{}, expiration ...time.Duration) error { @@ -47,7 +48,7 @@ type baseManagerTestSuite struct { } func (m *baseManagerTestSuite) SetupTest() { - m.cache = &testCache{Cache: &testcache.Cache{}} + m.cache = &testCache{Cache: &testcache.Cache{}, iterator: &testcache.Iterator{}} m.mgr = NewBaseManager(testResourceType).WithCacheClient(m.cache) } @@ -72,10 +73,11 @@ func (m *baseManagerTestSuite) TestResourceType() { } func (m *baseManagerTestSuite) TestCountCache() { - m.cache.On("Keys", mock.Anything, testResourceType).Return([]string{"k1", "k2"}, nil).Once() + m.cache.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.cache.iterator, nil).Once() c, err := m.mgr.CountCache(context.TODO()) m.NoError(err) - m.Equal(int64(2), c) + m.Equal(int64(0), c) } func (m *baseManagerTestSuite) TestDeleteCache() { @@ -85,9 +87,8 @@ func (m *baseManagerTestSuite) TestDeleteCache() { } func (m *baseManagerTestSuite) TestFlushAll() { - m.cache.On("Keys", mock.Anything, testResourceType).Return([]string{"k1", "k2"}, nil).Once() - m.cache.On("Delete", mock.Anything, "k1").Return(nil).Once() - m.cache.On("Delete", mock.Anything, "k2").Return(nil).Once() + m.cache.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.cache.iterator, nil).Once() err := m.mgr.FlushAll(context.TODO()) m.NoError(err) } diff --git a/src/pkg/cached/manifest/redis/manager_test.go b/src/pkg/cached/manifest/redis/manager_test.go index af2ff6a7d39..916bc7b9881 100644 --- a/src/pkg/cached/manifest/redis/manager_test.go +++ b/src/pkg/cached/manifest/redis/manager_test.go @@ -29,6 +29,7 @@ type managerTestSuite struct { suite.Suite cachedManager CachedManager cache *testcache.Cache + iterator *testcache.Iterator ctx context.Context digest string @@ -37,6 +38,7 @@ type managerTestSuite struct { func (m *managerTestSuite) SetupTest() { m.cache = &testcache.Cache{} + m.iterator = &testcache.Iterator{} m.cachedManager = NewManager() m.cachedManager.(*Manager).WithCacheClient(m.cache) m.ctx = context.TODO() @@ -69,10 +71,11 @@ func (m *managerTestSuite) TestResourceType() { } func (m *managerTestSuite) TestCountCache() { - m.cache.On("Keys", mock.Anything, mock.Anything).Return([]string{"1"}, nil).Once() + m.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.iterator, nil).Once() c, err := m.cachedManager.CountCache(m.ctx) m.NoError(err) - m.Equal(int64(1), c) + m.Equal(int64(0), c) } func (m *managerTestSuite) TestDeleteCache() { @@ -82,7 +85,8 @@ func (m *managerTestSuite) TestDeleteCache() { } func (m *managerTestSuite) TestFlushAll() { - m.cache.On("Keys", mock.Anything, mock.Anything).Return([]string{"1"}, nil).Once() + m.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.iterator, nil).Once() m.cache.On("Delete", mock.Anything, mock.Anything).Return(nil).Once() err := m.cachedManager.FlushAll(m.ctx) m.NoError(err) diff --git a/src/pkg/cached/project/redis/manager_test.go b/src/pkg/cached/project/redis/manager_test.go index e0630f92aac..fa9d8994b1f 100644 --- a/src/pkg/cached/project/redis/manager_test.go +++ b/src/pkg/cached/project/redis/manager_test.go @@ -34,12 +34,14 @@ type managerTestSuite struct { cachedManager CachedManager projectMgr *testProject.Manager cache *testcache.Cache + iterator *testcache.Iterator ctx context.Context } func (m *managerTestSuite) SetupTest() { m.projectMgr = &testProject.Manager{} m.cache = &testcache.Cache{} + m.iterator = &testcache.Iterator{} m.cachedManager = NewManager(m.projectMgr) m.cachedManager.(*Manager).WithCacheClient(m.cache) m.ctx = context.TODO() @@ -113,10 +115,11 @@ func (m *managerTestSuite) TestResourceType() { } func (m *managerTestSuite) TestCountCache() { - m.cache.On("Keys", mock.Anything, mock.Anything).Return([]string{"1"}, nil).Once() + m.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.iterator, nil).Once() c, err := m.cachedManager.CountCache(m.ctx) m.NoError(err) - m.Equal(int64(1), c) + m.Equal(int64(0), c) } func (m *managerTestSuite) TestDeleteCache() { @@ -126,7 +129,8 @@ func (m *managerTestSuite) TestDeleteCache() { } func (m *managerTestSuite) TestFlushAll() { - m.cache.On("Keys", mock.Anything, mock.Anything).Return([]string{"1"}, nil).Once() + m.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.iterator, nil).Once() m.cache.On("Delete", mock.Anything, mock.Anything).Return(nil).Once() err := m.cachedManager.FlushAll(m.ctx) m.NoError(err) diff --git a/src/pkg/cached/project_metadata/redis/manager.go b/src/pkg/cached/project_metadata/redis/manager.go index 07658296c08..1bf1ee6df07 100644 --- a/src/pkg/cached/project_metadata/redis/manager.go +++ b/src/pkg/cached/project_metadata/redis/manager.go @@ -119,14 +119,14 @@ func (m *Manager) Update(ctx context.Context, projectID int64, meta map[string]s return err } // lookup all keys with projectID prefix - keys, err := m.CacheClient(ctx).Keys(ctx, prefix) + iter, err := m.CacheClient(ctx).Scan(ctx, prefix) if err != nil { return err } - for _, key := range keys { - if err = retry.Retry(func() error { return m.CacheClient(ctx).Delete(ctx, key) }); err != nil { - log.Errorf("delete project metadata cache key %s error: %v", key, err) + for iter.Next(ctx) { + if err = retry.Retry(func() error { return m.CacheClient(ctx).Delete(ctx, iter.Val()) }); err != nil { + log.Errorf("delete project metadata cache key %s error: %v", iter.Val(), err) } } diff --git a/src/pkg/cached/project_metadata/redis/manager_test.go b/src/pkg/cached/project_metadata/redis/manager_test.go index 2a2172e1108..8315e406a07 100644 --- a/src/pkg/cached/project_metadata/redis/manager_test.go +++ b/src/pkg/cached/project_metadata/redis/manager_test.go @@ -33,12 +33,14 @@ type managerTestSuite struct { cachedManager CachedManager projectMetaMgr *testProjectMeta.Manager cache *testcache.Cache + iterator *testcache.Iterator ctx context.Context } func (m *managerTestSuite) SetupTest() { m.projectMetaMgr = &testProjectMeta.Manager{} m.cache = &testcache.Cache{} + m.iterator = &testcache.Iterator{} m.cachedManager = NewManager(m.projectMetaMgr) m.cachedManager.(*Manager).WithCacheClient(m.cache) m.ctx = context.TODO() @@ -98,10 +100,11 @@ func (m *managerTestSuite) TestResourceType() { } func (m *managerTestSuite) TestCountCache() { - m.cache.On("Keys", mock.Anything, mock.Anything).Return([]string{"1"}, nil).Once() + m.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.iterator, nil).Once() c, err := m.cachedManager.CountCache(m.ctx) m.NoError(err) - m.Equal(int64(1), c) + m.Equal(int64(0), c) } func (m *managerTestSuite) TestDeleteCache() { @@ -111,7 +114,8 @@ func (m *managerTestSuite) TestDeleteCache() { } func (m *managerTestSuite) TestFlushAll() { - m.cache.On("Keys", mock.Anything, mock.Anything).Return([]string{"1"}, nil).Once() + m.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.iterator, nil).Once() m.cache.On("Delete", mock.Anything, mock.Anything).Return(nil).Once() err := m.cachedManager.FlushAll(m.ctx) m.NoError(err) diff --git a/src/pkg/cached/repository/redis/manager_test.go b/src/pkg/cached/repository/redis/manager_test.go index 914ea5cce9d..f09f8e92c9d 100644 --- a/src/pkg/cached/repository/redis/manager_test.go +++ b/src/pkg/cached/repository/redis/manager_test.go @@ -33,12 +33,14 @@ type managerTestSuite struct { cachedManager CachedManager repoMgr *testRepo.Manager cache *testcache.Cache + iterator *testcache.Iterator ctx context.Context } func (m *managerTestSuite) SetupTest() { m.repoMgr = &testRepo.Manager{} m.cache = &testcache.Cache{} + m.iterator = &testcache.Iterator{} m.cachedManager = NewManager(m.repoMgr) m.cachedManager.(*Manager).WithCacheClient(m.cache) m.ctx = context.TODO() @@ -166,10 +168,11 @@ func (m *managerTestSuite) TestResourceType() { } func (m *managerTestSuite) TestCountCache() { - m.cache.On("Keys", mock.Anything, mock.Anything).Return([]string{"1"}, nil).Once() + m.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.iterator, nil).Once() c, err := m.cachedManager.CountCache(m.ctx) m.NoError(err) - m.Equal(int64(1), c) + m.Equal(int64(0), c) } func (m *managerTestSuite) TestDeleteCache() { @@ -179,7 +182,8 @@ func (m *managerTestSuite) TestDeleteCache() { } func (m *managerTestSuite) TestFlushAll() { - m.cache.On("Keys", mock.Anything, mock.Anything).Return([]string{"1"}, nil).Once() + m.iterator.On("Next", mock.Anything).Return(false).Once() + m.cache.On("Scan", mock.Anything, mock.Anything).Return(m.iterator, nil).Once() m.cache.On("Delete", mock.Anything, mock.Anything).Return(nil).Once() err := m.cachedManager.FlushAll(m.ctx) m.NoError(err) diff --git a/src/pkg/task/dao/execution.go b/src/pkg/task/dao/execution.go index 611dca1fcef..cd26e792e56 100644 --- a/src/pkg/task/dao/execution.go +++ b/src/pkg/task/dao/execution.go @@ -447,11 +447,16 @@ func (e *executionDAO) AsyncRefreshStatus(ctx context.Context, id int64, vendor // scanAndRefreshOutdateStatus scans the outdate execution status from redis and then refresh the status to db, // do not want to expose to external use so keep it as private. func scanAndRefreshOutdateStatus(ctx context.Context) { - keys, err := cache.Default().Keys(ctx, "execution:id:") + iter, err := cache.Default().Scan(ctx, "execution:id:*vendor:*status_outdate") if err != nil { log.Errorf("failed to scan the outdate executions, error: %v", err) return } + + var keys []string + for iter.Next(ctx) { + keys = append(keys, iter.Val()) + } // return earlier if no keys found which represents no outdate execution if len(keys) == 0 { log.Debug("skip to refresh, no outdate execution status found") diff --git a/src/pkg/task/dao/execution_test.go b/src/pkg/task/dao/execution_test.go index e826ac774e9..732286eabdc 100644 --- a/src/pkg/task/dao/execution_test.go +++ b/src/pkg/task/dao/execution_test.go @@ -22,7 +22,7 @@ import ( "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/jobservice/job" "github.com/goharbor/harbor/src/lib/cache" - _ "github.com/goharbor/harbor/src/lib/cache/memory" + _ "github.com/goharbor/harbor/src/lib/cache/redis" "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/orm" "github.com/goharbor/harbor/src/lib/q" @@ -45,7 +45,7 @@ func (e *executionDAOTestSuite) SetupSuite() { taskDAO: e.taskDao, } // initializes cache for testing - err := cache.Initialize(cache.Memory, "") + err := cache.Initialize(cache.Redis, "redis://localhost:6379/0") e.NoError(err) } diff --git a/src/testing/lib/cache/cache.go b/src/testing/lib/cache/cache.go index efac85b0a83..230d170832d 100644 --- a/src/testing/lib/cache/cache.go +++ b/src/testing/lib/cache/cache.go @@ -4,9 +4,12 @@ package cache import ( context "context" - time "time" + + cache "github.com/goharbor/harbor/src/lib/cache" mock "github.com/stretchr/testify/mock" + + time "time" ) // Cache is an autogenerated mock type for the Cache type @@ -56,39 +59,6 @@ func (_m *Cache) Fetch(ctx context.Context, key string, value interface{}) error return r0 } -// Keys provides a mock function with given fields: ctx, prefixes -func (_m *Cache) Keys(ctx context.Context, prefixes ...string) ([]string, error) { - _va := make([]interface{}, len(prefixes)) - for _i := range prefixes { - _va[_i] = prefixes[_i] - } - var _ca []interface{} - _ca = append(_ca, ctx) - _ca = append(_ca, _va...) - ret := _m.Called(_ca...) - - var r0 []string - var r1 error - if rf, ok := ret.Get(0).(func(context.Context, ...string) ([]string, error)); ok { - return rf(ctx, prefixes...) - } - if rf, ok := ret.Get(0).(func(context.Context, ...string) []string); ok { - r0 = rf(ctx, prefixes...) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).([]string) - } - } - - if rf, ok := ret.Get(1).(func(context.Context, ...string) error); ok { - r1 = rf(ctx, prefixes...) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - // Ping provides a mock function with given fields: ctx func (_m *Cache) Ping(ctx context.Context) error { ret := _m.Called(ctx) @@ -124,6 +94,32 @@ func (_m *Cache) Save(ctx context.Context, key string, value interface{}, expira return r0 } +// Scan provides a mock function with given fields: ctx, match +func (_m *Cache) Scan(ctx context.Context, match string) (cache.Iterator, error) { + ret := _m.Called(ctx, match) + + var r0 cache.Iterator + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string) (cache.Iterator, error)); ok { + return rf(ctx, match) + } + if rf, ok := ret.Get(0).(func(context.Context, string) cache.Iterator); ok { + r0 = rf(ctx, match) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(cache.Iterator) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = rf(ctx, match) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + type mockConstructorTestingTNewCache interface { mock.TestingT Cleanup(func()) diff --git a/src/testing/lib/cache/iterator.go b/src/testing/lib/cache/iterator.go new file mode 100644 index 00000000000..2dacab3f42a --- /dev/null +++ b/src/testing/lib/cache/iterator.go @@ -0,0 +1,57 @@ +// Code generated by mockery v2.22.1. DO NOT EDIT. + +package cache + +import ( + context "context" + + mock "github.com/stretchr/testify/mock" +) + +// Iterator is an autogenerated mock type for the Iterator type +type Iterator struct { + mock.Mock +} + +// Next provides a mock function with given fields: ctx +func (_m *Iterator) Next(ctx context.Context) bool { + ret := _m.Called(ctx) + + var r0 bool + if rf, ok := ret.Get(0).(func(context.Context) bool); ok { + r0 = rf(ctx) + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} + +// Val provides a mock function with given fields: +func (_m *Iterator) Val() string { + ret := _m.Called() + + var r0 string + if rf, ok := ret.Get(0).(func() string); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(string) + } + + return r0 +} + +type mockConstructorTestingTNewIterator interface { + mock.TestingT + Cleanup(func()) +} + +// NewIterator creates a new instance of Iterator. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +func NewIterator(t mockConstructorTestingTNewIterator) *Iterator { + mock := &Iterator{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/src/testing/lib/lib.go b/src/testing/lib/lib.go index 24eff32bd08..3b6f4a36a0e 100644 --- a/src/testing/lib/lib.go +++ b/src/testing/lib/lib.go @@ -16,4 +16,5 @@ package lib //go:generate mockery --case snake --dir ../../lib/orm --name Creator --output ./orm --outpkg orm //go:generate mockery --case snake --dir ../../lib/cache --name Cache --output ./cache --outpkg cache +//go:generate mockery --case snake --dir ../../lib/cache --name Iterator --output ./cache --outpkg cache //go:generate mockery --case snake --dir ../../lib/config --name Manager --output ./config --outpkg config diff --git a/src/testing/lib/libcache/cache.go b/src/testing/lib/libcache/cache.go new file mode 100644 index 00000000000..8eb215e52d9 --- /dev/null +++ b/src/testing/lib/libcache/cache.go @@ -0,0 +1,136 @@ +// Code generated by mockery v2.22.1. DO NOT EDIT. + +package libcache + +import ( + context "context" + + cache "github.com/goharbor/harbor/src/lib/cache" + + mock "github.com/stretchr/testify/mock" + + time "time" +) + +// Cache is an autogenerated mock type for the Cache type +type Cache struct { + mock.Mock +} + +// Contains provides a mock function with given fields: ctx, key +func (_m *Cache) Contains(ctx context.Context, key string) bool { + ret := _m.Called(ctx, key) + + var r0 bool + if rf, ok := ret.Get(0).(func(context.Context, string) bool); ok { + r0 = rf(ctx, key) + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} + +// Delete provides a mock function with given fields: ctx, key +func (_m *Cache) Delete(ctx context.Context, key string) error { + ret := _m.Called(ctx, key) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { + r0 = rf(ctx, key) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Fetch provides a mock function with given fields: ctx, key, value +func (_m *Cache) Fetch(ctx context.Context, key string, value interface{}) error { + ret := _m.Called(ctx, key, value) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, interface{}) error); ok { + r0 = rf(ctx, key, value) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Ping provides a mock function with given fields: ctx +func (_m *Cache) Ping(ctx context.Context) error { + ret := _m.Called(ctx) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context) error); ok { + r0 = rf(ctx) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Save provides a mock function with given fields: ctx, key, value, expiration +func (_m *Cache) Save(ctx context.Context, key string, value interface{}, expiration ...time.Duration) error { + _va := make([]interface{}, len(expiration)) + for _i := range expiration { + _va[_i] = expiration[_i] + } + var _ca []interface{} + _ca = append(_ca, ctx, key, value) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, interface{}, ...time.Duration) error); ok { + r0 = rf(ctx, key, value, expiration...) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Scan provides a mock function with given fields: ctx, match +func (_m *Cache) Scan(ctx context.Context, match string) (cache.Iterator, error) { + ret := _m.Called(ctx, match) + + var r0 cache.Iterator + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string) (cache.Iterator, error)); ok { + return rf(ctx, match) + } + if rf, ok := ret.Get(0).(func(context.Context, string) cache.Iterator); ok { + r0 = rf(ctx, match) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(cache.Iterator) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = rf(ctx, match) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +type mockConstructorTestingTNewCache interface { + mock.TestingT + Cleanup(func()) +} + +// NewCache creates a new instance of Cache. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +func NewCache(t mockConstructorTestingTNewCache) *Cache { + mock := &Cache{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +}