From 1a1ce634ccc1ae000bff7f8be6942e68c3b05591 Mon Sep 17 00:00:00 2001 From: Chenyu Zhang Date: Wed, 6 Jul 2022 16:11:53 +0800 Subject: [PATCH] Fix the process of cache layer (#17010) fix: fix cache layer issues (#16995,#16997,#16996,#17038) 1. Load config and initialize cache layer in jobservice(for GC) 2. Cache artifact by digest the key should contains repository name 3. Repository cache cleanup error when update 4. Skip save cache when request ctx in transaction Signed-off-by: chlins --- .../prepare/templates/jobservice/env.jinja | 6 + src/jobservice/main.go | 18 +++ src/lib/cache/memory/memory.go | 2 +- src/lib/cache/redis/redis.go | 5 +- src/lib/orm/tx.go | 12 ++ src/pkg/cached/artifact/redis/manager.go | 72 ++-------- src/pkg/cached/artifact/redis/manager_test.go | 6 +- src/pkg/cached/base_manager.go | 135 ++++++++++++++++++ src/pkg/cached/base_manager_test.go | 96 +++++++++++++ src/pkg/cached/manager.go | 3 + src/pkg/cached/manifest/redis/manager.go | 60 ++------ src/pkg/cached/manifest/redis/manager_test.go | 3 +- src/pkg/cached/project/redis/manager.go | 64 ++------- src/pkg/cached/project/redis/manager_test.go | 6 +- .../cached/project_metadata/redis/manager.go | 81 +++-------- .../project_metadata/redis/manager_test.go | 7 +- src/pkg/cached/repository/redis/manager.go | 68 ++------- .../cached/repository/redis/manager_test.go | 6 +- .../middleware/transaction/transaction.go | 9 +- .../transaction/transaction_test.go | 2 +- src/server/v2.0/handler/repository.go | 2 + .../cached/manifest/redis/cached_manager.go | 18 +++ 22 files changed, 381 insertions(+), 300 deletions(-) create mode 100644 src/pkg/cached/base_manager.go create mode 100644 src/pkg/cached/base_manager_test.go diff --git a/make/photon/prepare/templates/jobservice/env.jinja b/make/photon/prepare/templates/jobservice/env.jinja index 30951f85c..31e039ff2 100644 --- a/make/photon/prepare/templates/jobservice/env.jinja +++ b/make/photon/prepare/templates/jobservice/env.jinja @@ -47,3 +47,9 @@ TRACE_OTEL_TIMEOUT={{ trace.otel.timeout }} TRACE_OTEL_INSECURE={{ trace.otel.insecure }} {% endif %} {% endif %} + +{% if cache.enabled %} +_REDIS_URL_CORE={{redis_url_core}} +CACHE_ENABLED=true +CACHE_EXPIRE_HOURS={{ cache.expire_hours }} +{% endif %} \ No newline at end of file diff --git a/src/jobservice/main.go b/src/jobservice/main.go index e9670d9d8..1a9609e10 100644 --- a/src/jobservice/main.go +++ b/src/jobservice/main.go @@ -19,6 +19,8 @@ import ( "errors" "flag" "fmt" + "net/url" + "os" "github.com/goharbor/harbor/src/common" "github.com/goharbor/harbor/src/jobservice/common/utils" @@ -27,6 +29,7 @@ import ( "github.com/goharbor/harbor/src/jobservice/job/impl" "github.com/goharbor/harbor/src/jobservice/logger" "github.com/goharbor/harbor/src/jobservice/runtime" + "github.com/goharbor/harbor/src/lib/cache" cfgLib "github.com/goharbor/harbor/src/lib/config" tracelib "github.com/goharbor/harbor/src/lib/trace" _ "github.com/goharbor/harbor/src/pkg/config/inmemory" @@ -39,6 +42,21 @@ func main() { panic(fmt.Sprintf("failed to load configuration, error: %v", err)) } + // init cache if cache layer enabled + // gc needs to delete artifact by artifact manager, but the artifact cache store in + // core redis db so here require core redis url and init default cache. + if cfgLib.CacheEnabled() { + cacheURL := os.Getenv("_REDIS_URL_CORE") + u, err := url.Parse(cacheURL) + if err != nil { + panic("bad _REDIS_URL_CORE") + } + + if err = cache.Initialize(u.Scheme, cacheURL); err != nil { + panic(fmt.Sprintf("failed to initialize cache: %v", err)) + } + } + // Get parameters configPath := flag.String("c", "", "Specify the yaml config file path") flag.Parse() diff --git a/src/lib/cache/memory/memory.go b/src/lib/cache/memory/memory.go index 37d1e12ec..6f65f2368 100644 --- a/src/lib/cache/memory/memory.go +++ b/src/lib/cache/memory/memory.go @@ -130,7 +130,7 @@ func (c *Cache) Keys(ctx context.Context, prefixes ...string) ([]string, error) } else { for _, p := range prefixes { if strings.HasPrefix(ks, c.opts.Key(p)) { - keys = append(keys, ks) + keys = append(keys, strings.TrimPrefix(ks, c.opts.Prefix)) } } } diff --git a/src/lib/cache/redis/redis.go b/src/lib/cache/redis/redis.go index a40bc9e91..703ce5f15 100644 --- a/src/lib/cache/redis/redis.go +++ b/src/lib/cache/redis/redis.go @@ -18,6 +18,7 @@ import ( "context" "fmt" "net/url" + "strings" "time" "github.com/go-redis/redis/v8" @@ -105,7 +106,9 @@ func (c *Cache) Keys(ctx context.Context, prefixes ...string) ([]string, error) return nil, err } - keys = append(keys, cmd.Val()...) + for _, k := range cmd.Val() { + keys = append(keys, strings.TrimPrefix(k, c.opts.Prefix)) + } } return keys, nil diff --git a/src/lib/orm/tx.go b/src/lib/orm/tx.go index e0ddbfb8a..0bfb69ce6 100644 --- a/src/lib/orm/tx.go +++ b/src/lib/orm/tx.go @@ -15,6 +15,7 @@ package orm import ( + "context" "encoding/hex" "fmt" @@ -22,6 +23,17 @@ import ( "github.com/google/uuid" ) +type CommittedKey struct{} + +// HasCommittedKey checks whether exist committed key in context. +func HasCommittedKey(ctx context.Context) bool { + if value := ctx.Value(CommittedKey{}); value != nil { + return true + } + + return false +} + // ormerTx transaction which support savepoint type ormerTx struct { orm.Ormer diff --git a/src/pkg/cached/artifact/redis/manager.go b/src/pkg/cached/artifact/redis/manager.go index 909a990fa..c5371a3a7 100644 --- a/src/pkg/cached/artifact/redis/manager.go +++ b/src/pkg/cached/artifact/redis/manager.go @@ -18,9 +18,7 @@ import ( "context" "time" - libcache "github.com/goharbor/harbor/src/lib/cache" "github.com/goharbor/harbor/src/lib/config" - "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/log" "github.com/goharbor/harbor/src/lib/q" "github.com/goharbor/harbor/src/lib/retry" @@ -38,25 +36,24 @@ type CachedManager interface { cached.Manager } -// Manager is the cached Manager implemented by redis. +// Manager is the cached manager implemented by redis. type Manager struct { + *cached.BaseManager // delegator delegates the raw crud to DAO. delegator artifact.Manager - // client returns the redis cache client. - client func() libcache.Cache // keyBuilder builds cache object key. keyBuilder *cached.ObjectKey // lifetime is the cache life time. lifetime time.Duration } -// NewManager returns the redis cache Manager. +// NewManager returns the redis cache manager. func NewManager(m artifact.Manager) *Manager { return &Manager{ - delegator: m, - client: func() libcache.Cache { return libcache.Default() }, - keyBuilder: cached.NewObjectKey(cached.ResourceTypeArtifact), - lifetime: time.Duration(config.CacheExpireHours()) * time.Hour, + BaseManager: cached.NewBaseManager(cached.ResourceTypeArtifact), + delegator: m, + keyBuilder: cached.NewObjectKey(cached.ResourceTypeArtifact), + lifetime: time.Duration(config.CacheExpireHours()) * time.Hour, } } @@ -87,7 +84,7 @@ func (m *Manager) Get(ctx context.Context, id int64) (*artifact.Artifact, error) } art := &artifact.Artifact{} - if err = m.client().Fetch(ctx, key, art); err == nil { + if err = m.CacheClient(ctx).Fetch(ctx, key, art); err == nil { return art, nil } @@ -98,7 +95,7 @@ func (m *Manager) Get(ctx context.Context, id int64) (*artifact.Artifact, error) return nil, err } - if err = m.client().Save(ctx, key, art, m.lifetime); err != nil { + if err = m.CacheClient(ctx).Save(ctx, key, art, m.lifetime); err != nil { // log error if save to cache failed log.Debugf("save artifact %s to cache error: %v", art.String(), err) } @@ -107,13 +104,13 @@ func (m *Manager) Get(ctx context.Context, id int64) (*artifact.Artifact, error) } func (m *Manager) GetByDigest(ctx context.Context, repository, digest string) (*artifact.Artifact, error) { - key, err := m.keyBuilder.Format("digest", digest) + key, err := m.keyBuilder.Format("repository", repository, "digest", digest) if err != nil { return nil, err } art := &artifact.Artifact{} - if err = m.client().Fetch(ctx, key, art); err == nil { + if err = m.CacheClient(ctx).Fetch(ctx, key, art); err == nil { return art, nil } @@ -122,7 +119,7 @@ func (m *Manager) GetByDigest(ctx context.Context, repository, digest string) (* return nil, err } - if err = m.client().Save(ctx, key, art, m.lifetime); err != nil { + if err = m.CacheClient(ctx).Save(ctx, key, art, m.lifetime); err != nil { // log error if save to cache failed log.Debugf("save artifact %s to cache error: %v", art.String(), err) } @@ -176,17 +173,17 @@ func (m *Manager) cleanUp(ctx context.Context, art *artifact.Artifact) { log.Errorf("format artifact id key error: %v", err) } else { // retry to avoid dirty data - if err = retry.Retry(func() error { return m.client().Delete(ctx, idIdx) }); err != nil { + if err = retry.Retry(func() error { return m.CacheClient(ctx).Delete(ctx, idIdx) }); err != nil { log.Errorf("delete artifact cache key %s error: %v", idIdx, err) } } // clean index by digest - digestIdx, err := m.keyBuilder.Format("digest", art.Digest) + digestIdx, err := m.keyBuilder.Format("repository", art.RepositoryName, "digest", art.Digest) if err != nil { log.Errorf("format artifact digest key error: %v", err) } else { - if err = retry.Retry(func() error { return m.client().Delete(ctx, digestIdx) }); err != nil { + if err = retry.Retry(func() error { return m.CacheClient(ctx).Delete(ctx, digestIdx) }); err != nil { log.Errorf("delete artifact cache key %s error: %v", digestIdx, err) } } @@ -216,42 +213,3 @@ func (m *Manager) refreshCache(ctx context.Context, art *artifact.Artifact) { log.Errorf("refresh cache by artifact digest %s error: %v", art.Digest, err) } } - -func (m *Manager) ResourceType(ctx context.Context) string { - return cached.ResourceTypeArtifact -} - -func (m *Manager) CountCache(ctx context.Context) (int64, error) { - // prefix is resource type - keys, err := m.client().Keys(ctx, m.ResourceType(ctx)) - if err != nil { - return 0, err - } - - return int64(len(keys)), nil -} - -func (m *Manager) DeleteCache(ctx context.Context, key string) error { - return m.client().Delete(ctx, key) -} - -func (m *Manager) FlushAll(ctx context.Context) error { - // prefix is resource type - keys, err := m.client().Keys(ctx, m.ResourceType(ctx)) - if err != nil { - return err - } - - var errs errors.Errors - for _, key := range keys { - if err = m.client().Delete(ctx, key); err != nil { - errs = append(errs, err) - } - } - - if errs.Len() > 0 { - return errs - } - - return nil -} diff --git a/src/pkg/cached/artifact/redis/manager_test.go b/src/pkg/cached/artifact/redis/manager_test.go index 20f283e9a..007673d4e 100644 --- a/src/pkg/cached/artifact/redis/manager_test.go +++ b/src/pkg/cached/artifact/redis/manager_test.go @@ -39,10 +39,8 @@ type managerTestSuite struct { func (m *managerTestSuite) SetupTest() { m.artMgr = &testArt.Manager{} m.cache = &testcache.Cache{} - m.cachedManager = NewManager( - m.artMgr, - ) - m.cachedManager.(*Manager).client = func() cache.Cache { return m.cache } + m.cachedManager = NewManager(m.artMgr) + m.cachedManager.(*Manager).WithCacheClient(m.cache) m.ctx = context.TODO() } diff --git a/src/pkg/cached/base_manager.go b/src/pkg/cached/base_manager.go new file mode 100644 index 000000000..fd0c801ab --- /dev/null +++ b/src/pkg/cached/base_manager.go @@ -0,0 +1,135 @@ +// Copyright Project Harbor Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cached + +import ( + "context" + "time" + + "github.com/goharbor/harbor/src/lib/cache" + "github.com/goharbor/harbor/src/lib/errors" + "github.com/goharbor/harbor/src/lib/orm" +) + +// innerCache is the default cache client, +// actually it is a wrapper for cache.Default(). +var innerCache cache.Cache = &cacheClient{} + +// cacheClient is a interceptor for cache.Default, in order to implement specific +// case for cache layer. +type cacheClient struct{} + +func (*cacheClient) Contains(ctx context.Context, key string) bool { + return cache.Default().Contains(ctx, key) +} + +func (*cacheClient) Delete(ctx context.Context, key string) error { + return cache.Default().Delete(ctx, key) +} + +func (*cacheClient) Fetch(ctx context.Context, key string, value interface{}) error { + return cache.Default().Fetch(ctx, key, value) +} + +func (*cacheClient) Ping(ctx context.Context) error { + return cache.Default().Ping(ctx) +} + +func (*cacheClient) Save(ctx context.Context, key string, value interface{}, expiration ...time.Duration) error { + // intercept here + // it should ignore save cache if this request is wrapped by orm.Transaction, + // because if tx rollback, we can not rollback cache, + // identify whether in transaction by checking the commitedKey in context. + // commitedKey is a context value which be injected in the transaction middleware. + if orm.HasCommittedKey(ctx) { + return nil + } + + return cache.Default().Save(ctx, key, value, expiration...) +} + +func (*cacheClient) Keys(ctx context.Context, prefixes ...string) ([]string, error) { + return cache.Default().Keys(ctx, prefixes...) +} + +var _ Manager = &BaseManager{} + +// BaseManager is the base manager for cache and implement the cache manager interface. +type BaseManager struct { + resourceType string + cacheClient cache.Cache +} + +// NewBaseManager returns a instance of base manager. +func NewBaseManager(resourceType string) *BaseManager { + return &BaseManager{ + resourceType: resourceType, + cacheClient: innerCache, + } +} + +// WithCacheClient can override the default cache client. +func (bm *BaseManager) WithCacheClient(cc cache.Cache) *BaseManager { + bm.cacheClient = cc + return bm +} + +// CacheClient returns the cache client. +func (bm *BaseManager) CacheClient(ctx context.Context) cache.Cache { + return bm.cacheClient +} + +// ResourceType returns the resource type. +func (bm *BaseManager) ResourceType(ctx context.Context) string { + return bm.resourceType +} + +// CountCache returns current this resource occupied cache count. +func (bm *BaseManager) CountCache(ctx context.Context) (int64, error) { + // prefix is resource type + keys, err := bm.CacheClient(ctx).Keys(ctx, bm.ResourceType(ctx)) + if err != nil { + return 0, err + } + + return int64(len(keys)), nil +} + +// DeleteCache deletes specific cache by key. +func (bm *BaseManager) DeleteCache(ctx context.Context, key string) error { + return bm.CacheClient(ctx).Delete(ctx, key) +} + +// 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)) + if err != nil { + return err + } + + var errs errors.Errors + for _, key := range keys { + if err = bm.CacheClient(ctx).Delete(ctx, key); err != nil { + errs = append(errs, err) + } + } + + if errs.Len() > 0 { + return errs + } + + return nil +} diff --git a/src/pkg/cached/base_manager_test.go b/src/pkg/cached/base_manager_test.go new file mode 100644 index 000000000..13aaa03c4 --- /dev/null +++ b/src/pkg/cached/base_manager_test.go @@ -0,0 +1,96 @@ +// Copyright Project Harbor Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cached + +import ( + "context" + "testing" + "time" + + "github.com/goharbor/harbor/src/lib/orm" + testcache "github.com/goharbor/harbor/src/testing/lib/cache" + "github.com/goharbor/harbor/src/testing/mock" + "github.com/stretchr/testify/suite" +) + +var testResourceType = "resource-test" + +type testCache struct { + *testcache.Cache +} + +func (tc *testCache) Save(ctx context.Context, key string, value interface{}, expiration ...time.Duration) error { + if orm.HasCommittedKey(ctx) { + return nil + } + + return tc.Cache.Save(ctx, key, value, expiration...) +} + +type baseManagerTestSuite struct { + suite.Suite + cache *testCache + mgr *BaseManager +} + +func (m *baseManagerTestSuite) SetupTest() { + m.cache = &testCache{Cache: &testcache.Cache{}} + m.mgr = NewBaseManager(testResourceType).WithCacheClient(m.cache) +} + +func (m *baseManagerTestSuite) TestSave() { + // normal ctx, should call cache.Save + m.cache.On("Save", mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() + ctx := context.TODO() + err := m.mgr.CacheClient(ctx).Save(ctx, "key", "value") + m.NoError(err) + m.cache.AssertCalled(m.T(), "Save", mock.Anything, mock.Anything, mock.Anything) + + // ctx in transaction, should skip call cache.Save + m.cache.On("Save", mock.Anything, mock.Anything, mock.Anything).Panic("should not be called") + ctx = context.WithValue(ctx, orm.CommittedKey{}, true) + err = m.mgr.CacheClient(ctx).Save(ctx, "key", "value") + m.NoError(err) + m.cache.AssertNumberOfCalls(m.T(), "Save", 1) +} + +func (m *baseManagerTestSuite) TestResourceType() { + m.Equal(testResourceType, m.mgr.ResourceType(context.TODO())) +} + +func (m *baseManagerTestSuite) TestCountCache() { + m.cache.On("Keys", mock.Anything, testResourceType).Return([]string{"k1", "k2"}, nil).Once() + c, err := m.mgr.CountCache(context.TODO()) + m.NoError(err) + m.Equal(int64(2), c) +} + +func (m *baseManagerTestSuite) TestDeleteCache() { + m.cache.On("Delete", mock.Anything, "k1").Return(nil).Once() + err := m.mgr.DeleteCache(context.TODO(), "k1") + m.NoError(err) +} + +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() + err := m.mgr.FlushAll(context.TODO()) + m.NoError(err) +} + +func TestBaseManager(t *testing.T) { + suite.Run(t, &baseManagerTestSuite{}) +} diff --git a/src/pkg/cached/manager.go b/src/pkg/cached/manager.go index 96601e375..d2a0b7140 100644 --- a/src/pkg/cached/manager.go +++ b/src/pkg/cached/manager.go @@ -18,6 +18,7 @@ import ( "context" "fmt" + "github.com/goharbor/harbor/src/lib/cache" "github.com/goharbor/harbor/src/lib/errors" ) @@ -38,6 +39,8 @@ const ( // Manager is the interface for resource cache manager. // Provides common interfaces for admin to view and manage resource cache. type Manager interface { + // CacheClient returns the cache client. + CacheClient(ctx context.Context) cache.Cache // ResourceType returns the resource type. // eg. artifact、project、tag、repository ResourceType(ctx context.Context) string diff --git a/src/pkg/cached/manifest/redis/manager.go b/src/pkg/cached/manifest/redis/manager.go index a4554b2f8..b0e767e88 100644 --- a/src/pkg/cached/manifest/redis/manager.go +++ b/src/pkg/cached/manifest/redis/manager.go @@ -18,9 +18,7 @@ import ( "context" "time" - libcache "github.com/goharbor/harbor/src/lib/cache" "github.com/goharbor/harbor/src/lib/config" - "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/retry" "github.com/goharbor/harbor/src/pkg/cached" ) @@ -45,22 +43,21 @@ type CachedManager interface { cached.Manager } -// Manager is the cached Manager implemented by redis. +// Manager is the cached manager implemented by redis. type Manager struct { - // client returns the redis cache client. - client func() libcache.Cache + *cached.BaseManager // keyBuilder builds cache object key. keyBuilder *cached.ObjectKey // lifetime is the cache life time. lifetime time.Duration } -// NewManager returns the redis cache Manager. +// NewManager returns the redis cache manager. func NewManager() *Manager { return &Manager{ - client: func() libcache.Cache { return libcache.Default() }, - keyBuilder: cached.NewObjectKey(cached.ResourceTypeManifest), - lifetime: time.Duration(config.CacheExpireHours()) * time.Hour, + BaseManager: cached.NewBaseManager(cached.ResourceTypeManifest), + keyBuilder: cached.NewObjectKey(cached.ResourceTypeManifest), + lifetime: time.Duration(config.CacheExpireHours()) * time.Hour, } } @@ -70,7 +67,7 @@ func (m *Manager) Save(ctx context.Context, digest string, manifest []byte) erro return err } - return m.client().Save(ctx, key, manifest, m.lifetime) + return m.CacheClient(ctx).Save(ctx, key, manifest, m.lifetime) } func (m *Manager) Get(ctx context.Context, digest string) ([]byte, error) { @@ -80,7 +77,7 @@ func (m *Manager) Get(ctx context.Context, digest string) ([]byte, error) { } var manifest []byte - if err = m.client().Fetch(ctx, key, &manifest); err == nil { + if err = m.CacheClient(ctx).Fetch(ctx, key, &manifest); err == nil { return manifest, nil } @@ -93,44 +90,5 @@ func (m *Manager) Delete(ctx context.Context, digest string) error { return err } - return retry.Retry(func() error { return m.client().Delete(ctx, key) }) -} - -func (m *Manager) ResourceType(ctx context.Context) string { - return cached.ResourceTypeManifest -} - -func (m *Manager) CountCache(ctx context.Context) (int64, error) { - // prefix is resource type - keys, err := m.client().Keys(ctx, m.ResourceType(ctx)) - if err != nil { - return 0, err - } - - return int64(len(keys)), nil -} - -func (m *Manager) DeleteCache(ctx context.Context, key string) error { - return m.client().Delete(ctx, key) -} - -func (m *Manager) FlushAll(ctx context.Context) error { - // prefix is resource type - keys, err := m.client().Keys(ctx, m.ResourceType(ctx)) - if err != nil { - return err - } - - var errs errors.Errors - for _, key := range keys { - if err = m.client().Delete(ctx, key); err != nil { - errs = append(errs, err) - } - } - - if errs.Len() > 0 { - return errs - } - - return nil + return retry.Retry(func() error { return m.CacheClient(ctx).Delete(ctx, key) }) } diff --git a/src/pkg/cached/manifest/redis/manager_test.go b/src/pkg/cached/manifest/redis/manager_test.go index b76cd63f6..a43389b0f 100644 --- a/src/pkg/cached/manifest/redis/manager_test.go +++ b/src/pkg/cached/manifest/redis/manager_test.go @@ -19,7 +19,6 @@ import ( "fmt" "testing" - "github.com/goharbor/harbor/src/lib/cache" testcache "github.com/goharbor/harbor/src/testing/lib/cache" "github.com/goharbor/harbor/src/testing/mock" "github.com/stretchr/testify/suite" @@ -38,7 +37,7 @@ type managerTestSuite struct { func (m *managerTestSuite) SetupTest() { m.cache = &testcache.Cache{} m.cachedManager = NewManager() - m.cachedManager.(*Manager).client = func() cache.Cache { return m.cache } + m.cachedManager.(*Manager).WithCacheClient(m.cache) m.ctx = context.TODO() m.digest = "sha256:52f431d980baa76878329b68ddb69cb124c25efa6e206d8b0bd797a828f0528e" diff --git a/src/pkg/cached/project/redis/manager.go b/src/pkg/cached/project/redis/manager.go index 9efc8308d..8ca1617f5 100644 --- a/src/pkg/cached/project/redis/manager.go +++ b/src/pkg/cached/project/redis/manager.go @@ -19,9 +19,7 @@ import ( "time" "github.com/goharbor/harbor/src/common/utils" - libcache "github.com/goharbor/harbor/src/lib/cache" "github.com/goharbor/harbor/src/lib/config" - "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/log" "github.com/goharbor/harbor/src/lib/q" "github.com/goharbor/harbor/src/lib/retry" @@ -40,25 +38,24 @@ type CachedManager interface { cached.Manager } -// Manager is the cached Manager implemented by redis. +// Manager is the cached manager implemented by redis. type Manager struct { + *cached.BaseManager // delegator delegates the raw crud to DAO. delegator project.Manager - // client returns the redis cache client. - client func() libcache.Cache // keyBuilder builds cache object key. keyBuilder *cached.ObjectKey // lifetime is the cache life time. lifetime time.Duration } -// NewManager returns the redis cache Manager. +// NewManager returns the redis cache manager. func NewManager(m project.Manager) *Manager { return &Manager{ - delegator: m, - client: func() libcache.Cache { return libcache.Default() }, - keyBuilder: cached.NewObjectKey(cached.ResourceTypeProject), - lifetime: time.Duration(config.CacheExpireHours()) * time.Hour, + BaseManager: cached.NewBaseManager(cached.ResourceTypeProject), + delegator: m, + keyBuilder: cached.NewObjectKey(cached.ResourceTypeProject), + lifetime: time.Duration(config.CacheExpireHours()) * time.Hour, } } @@ -119,7 +116,7 @@ func (m *Manager) Get(ctx context.Context, idOrName interface{}) (*models.Projec } p := &models.Project{} - if err = m.client().Fetch(ctx, key, p); err == nil { + if err = m.CacheClient(ctx).Fetch(ctx, key, p); err == nil { return p, nil } @@ -130,7 +127,7 @@ func (m *Manager) Get(ctx context.Context, idOrName interface{}) (*models.Projec return nil, err } - if err = m.client().Save(ctx, key, p, m.lifetime); err != nil { + if err = m.CacheClient(ctx).Save(ctx, key, p, m.lifetime); err != nil { // log error if save to cache failed log.Debugf("save project %s to cache error: %v", p.Name, err) } @@ -146,7 +143,7 @@ func (m *Manager) cleanUp(ctx context.Context, p *models.Project) { log.Errorf("format project id key error: %v", err) } else { // retry to avoid dirty data - if err = retry.Retry(func() error { return m.client().Delete(ctx, idIdx) }); err != nil { + if err = retry.Retry(func() error { return m.CacheClient(ctx).Delete(ctx, idIdx) }); err != nil { log.Errorf("delete project cache key %s error: %v", idIdx, err) } } @@ -156,47 +153,8 @@ func (m *Manager) cleanUp(ctx context.Context, p *models.Project) { if err != nil { log.Errorf("format project name key error: %v", err) } else { - if err = retry.Retry(func() error { return m.client().Delete(ctx, nameIdx) }); err != nil { + if err = retry.Retry(func() error { return m.CacheClient(ctx).Delete(ctx, nameIdx) }); err != nil { log.Errorf("delete project cache key %s error: %v", nameIdx, err) } } } - -func (m *Manager) ResourceType(ctx context.Context) string { - return cached.ResourceTypeProject -} - -func (m *Manager) CountCache(ctx context.Context) (int64, error) { - // prefix is resource type - keys, err := m.client().Keys(ctx, m.ResourceType(ctx)) - if err != nil { - return 0, err - } - - return int64(len(keys)), nil -} - -func (m *Manager) DeleteCache(ctx context.Context, key string) error { - return m.client().Delete(ctx, key) -} - -func (m *Manager) FlushAll(ctx context.Context) error { - // prefix is resource type - keys, err := m.client().Keys(ctx, m.ResourceType(ctx)) - if err != nil { - return err - } - - var errs errors.Errors - for _, key := range keys { - if err = m.client().Delete(ctx, key); err != nil { - errs = append(errs, err) - } - } - - if errs.Len() > 0 { - return errs - } - - return nil -} diff --git a/src/pkg/cached/project/redis/manager_test.go b/src/pkg/cached/project/redis/manager_test.go index c546bc52e..f60e4f279 100644 --- a/src/pkg/cached/project/redis/manager_test.go +++ b/src/pkg/cached/project/redis/manager_test.go @@ -40,10 +40,8 @@ type managerTestSuite struct { func (m *managerTestSuite) SetupTest() { m.projectMgr = &testProject.Manager{} m.cache = &testcache.Cache{} - m.cachedManager = NewManager( - m.projectMgr, - ) - m.cachedManager.(*Manager).client = func() cache.Cache { return m.cache } + m.cachedManager = NewManager(m.projectMgr) + m.cachedManager.(*Manager).WithCacheClient(m.cache) m.ctx = context.TODO() } diff --git a/src/pkg/cached/project_metadata/redis/manager.go b/src/pkg/cached/project_metadata/redis/manager.go index 8b95365c1..07658296c 100644 --- a/src/pkg/cached/project_metadata/redis/manager.go +++ b/src/pkg/cached/project_metadata/redis/manager.go @@ -19,9 +19,7 @@ import ( "strings" "time" - libcache "github.com/goharbor/harbor/src/lib/cache" "github.com/goharbor/harbor/src/lib/config" - "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/log" "github.com/goharbor/harbor/src/lib/retry" "github.com/goharbor/harbor/src/pkg/cached" @@ -39,30 +37,34 @@ type CachedManager interface { cached.Manager } -// Manager is the cached Manager implemented by redis. +// Manager is the cached manager implemented by redis. type Manager struct { + *cached.BaseManager // delegator delegates the raw crud to DAO. delegator metadata.Manager - // client returns the redis cache client. - client func() libcache.Cache // keyBuilder builds cache object key. keyBuilder *cached.ObjectKey // lifetime is the cache life time. lifetime time.Duration } -// NewManager returns the redis cache Manager. +// NewManager returns the redis cache manager. func NewManager(m metadata.Manager) *Manager { return &Manager{ - delegator: m, - client: func() libcache.Cache { return libcache.Default() }, - keyBuilder: cached.NewObjectKey(cached.ResourceTypeProjectMeta), - lifetime: time.Duration(config.CacheExpireHours()) * time.Hour, + BaseManager: cached.NewBaseManager(cached.ResourceTypeProjectMeta), + delegator: m, + keyBuilder: cached.NewObjectKey(cached.ResourceTypeProjectMeta), + lifetime: time.Duration(config.CacheExpireHours()) * time.Hour, } } func (m *Manager) Add(ctx context.Context, projectID int64, meta map[string]string) error { - return m.delegator.Add(ctx, projectID, meta) + if err := m.delegator.Add(ctx, projectID, meta); err != nil { + return err + } + // should cleanup cache when add metadata to project + m.cleanUp(ctx, projectID) + return nil } func (m *Manager) List(ctx context.Context, name string, value string) ([]*models.ProjectMetadata, error) { @@ -76,7 +78,7 @@ func (m *Manager) Get(ctx context.Context, projectID int64, meta ...string) (map } result := make(map[string]string) - if err = m.client().Fetch(ctx, key, &result); err == nil { + if err = m.CacheClient(ctx).Fetch(ctx, key, &result); err == nil { return result, nil } @@ -86,10 +88,12 @@ func (m *Manager) Get(ctx context.Context, projectID int64, meta ...string) (map if err != nil { return nil, err } - - if err = m.client().Save(ctx, key, &result, m.lifetime); err != nil { - // log error if save to cache failed - log.Debugf("save project metadata %v to cache error: %v", result, err) + // only cache when result has attributes + if len(result) > 0 { + if err = m.CacheClient(ctx).Save(ctx, key, &result, m.lifetime); err != nil { + // log error if save to cache failed + log.Debugf("save project metadata %v to cache error: %v", result, err) + } } return result, nil @@ -115,13 +119,13 @@ func (m *Manager) Update(ctx context.Context, projectID int64, meta map[string]s return err } // lookup all keys with projectID prefix - keys, err := m.client().Keys(ctx, prefix) + keys, err := m.CacheClient(ctx).Keys(ctx, prefix) if err != nil { return err } for _, key := range keys { - if err = retry.Retry(func() error { return m.client().Delete(ctx, key) }); err != nil { + 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) } } @@ -136,47 +140,8 @@ func (m *Manager) cleanUp(ctx context.Context, projectID int64, meta ...string) log.Errorf("format project metadata key error: %v", err) } else { // retry to avoid dirty data - if err = retry.Retry(func() error { return m.client().Delete(ctx, key) }); err != nil { + 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) } } } - -func (m *Manager) ResourceType(ctx context.Context) string { - return cached.ResourceTypeProjectMeta -} - -func (m *Manager) CountCache(ctx context.Context) (int64, error) { - // prefix is resource type - keys, err := m.client().Keys(ctx, m.ResourceType(ctx)) - if err != nil { - return 0, err - } - - return int64(len(keys)), nil -} - -func (m *Manager) DeleteCache(ctx context.Context, key string) error { - return m.client().Delete(ctx, key) -} - -func (m *Manager) FlushAll(ctx context.Context) error { - // prefix is resource type - keys, err := m.client().Keys(ctx, m.ResourceType(ctx)) - if err != nil { - return err - } - - var errs errors.Errors - for _, key := range keys { - if err = m.client().Delete(ctx, key); err != nil { - errs = append(errs, err) - } - } - - if errs.Len() > 0 { - return errs - } - - return nil -} diff --git a/src/pkg/cached/project_metadata/redis/manager_test.go b/src/pkg/cached/project_metadata/redis/manager_test.go index 44ee7ca2d..bfa739c54 100644 --- a/src/pkg/cached/project_metadata/redis/manager_test.go +++ b/src/pkg/cached/project_metadata/redis/manager_test.go @@ -39,14 +39,13 @@ type managerTestSuite struct { func (m *managerTestSuite) SetupTest() { m.projectMetaMgr = &testProjectMeta.Manager{} m.cache = &testcache.Cache{} - m.cachedManager = NewManager( - m.projectMetaMgr, - ) - m.cachedManager.(*Manager).client = func() cache.Cache { return m.cache } + m.cachedManager = NewManager(m.projectMetaMgr) + m.cachedManager.(*Manager).WithCacheClient(m.cache) m.ctx = context.TODO() } func (m *managerTestSuite) TestAdd() { + m.cache.On("Delete", mock.Anything, mock.Anything).Return(nil).Once() m.projectMetaMgr.On("Add", mock.Anything, mock.Anything, mock.Anything).Return(nil) err := m.cachedManager.Add(m.ctx, 1, map[string]string{}) m.NoError(err) diff --git a/src/pkg/cached/repository/redis/manager.go b/src/pkg/cached/repository/redis/manager.go index 50b502188..18f0546b5 100644 --- a/src/pkg/cached/repository/redis/manager.go +++ b/src/pkg/cached/repository/redis/manager.go @@ -18,9 +18,7 @@ import ( "context" "time" - libcache "github.com/goharbor/harbor/src/lib/cache" "github.com/goharbor/harbor/src/lib/config" - "github.com/goharbor/harbor/src/lib/errors" "github.com/goharbor/harbor/src/lib/log" "github.com/goharbor/harbor/src/lib/q" "github.com/goharbor/harbor/src/lib/retry" @@ -39,25 +37,24 @@ type CachedManager interface { cached.Manager } -// Manager is the cached Manager implemented by redis. +// Manager is the cached manager implemented by redis. type Manager struct { + *cached.BaseManager // delegator delegates the raw crud to DAO. delegator repository.Manager - // client returns the redis cache client. - client func() libcache.Cache // keyBuilder builds cache object key. keyBuilder *cached.ObjectKey // lifetime is the cache life time. lifetime time.Duration } -// NewManager returns the redis cache Manager. +// NewManager returns the redis cache manager. func NewManager(m repository.Manager) *Manager { return &Manager{ - delegator: m, - client: func() libcache.Cache { return libcache.Default() }, - keyBuilder: cached.NewObjectKey(cached.ResourceTypeRepository), - lifetime: time.Duration(config.CacheExpireHours()) * time.Hour, + BaseManager: cached.NewBaseManager(cached.ResourceTypeRepository), + delegator: m, + keyBuilder: cached.NewObjectKey(cached.ResourceTypeRepository), + lifetime: time.Duration(config.CacheExpireHours()) * time.Hour, } } @@ -84,7 +81,7 @@ func (m *Manager) Get(ctx context.Context, id int64) (*model.RepoRecord, error) } repo := &model.RepoRecord{} - if err = m.client().Fetch(ctx, key, repo); err == nil { + if err = m.CacheClient(ctx).Fetch(ctx, key, repo); err == nil { return repo, nil } @@ -95,7 +92,7 @@ func (m *Manager) Get(ctx context.Context, id int64) (*model.RepoRecord, error) return nil, err } - if err = m.client().Save(ctx, key, repo, m.lifetime); err != nil { + if err = m.CacheClient(ctx).Save(ctx, key, repo, m.lifetime); err != nil { // log error if save to cache failed log.Debugf("save repository %s to cache error: %v", repo.Name, err) } @@ -110,7 +107,7 @@ func (m *Manager) GetByName(ctx context.Context, name string) (*model.RepoRecord } repo := &model.RepoRecord{} - if err = m.client().Fetch(ctx, key, repo); err == nil { + if err = m.CacheClient(ctx).Fetch(ctx, key, repo); err == nil { return repo, nil } @@ -119,7 +116,7 @@ func (m *Manager) GetByName(ctx context.Context, name string) (*model.RepoRecord return nil, err } - if err = m.client().Save(ctx, key, repo, m.lifetime); err != nil { + if err = m.CacheClient(ctx).Save(ctx, key, repo, m.lifetime); err != nil { // log error if save to cache failed log.Debugf("save repository %s to cache error: %v", repo.Name, err) } @@ -173,7 +170,7 @@ func (m *Manager) cleanUp(ctx context.Context, repo *model.RepoRecord) { log.Errorf("format repository id key error: %v", err) } else { // retry to avoid dirty data - if err = retry.Retry(func() error { return m.client().Delete(ctx, idIdx) }); err != nil { + if err = retry.Retry(func() error { return m.CacheClient(ctx).Delete(ctx, idIdx) }); err != nil { log.Errorf("delete repository cache key %s error: %v", idIdx, err) } } @@ -183,7 +180,7 @@ func (m *Manager) cleanUp(ctx context.Context, repo *model.RepoRecord) { if err != nil { log.Errorf("format repository name key error: %v", err) } else { - if err = retry.Retry(func() error { return m.client().Delete(ctx, nameIdx) }); err != nil { + if err = retry.Retry(func() error { return m.CacheClient(ctx).Delete(ctx, nameIdx) }); err != nil { log.Errorf("delete repository cache key %s error: %v", nameIdx, err) } } @@ -213,42 +210,3 @@ func (m *Manager) refreshCache(ctx context.Context, repo *model.RepoRecord) { log.Errorf("refresh cache by repository name %s error: %v", repo.Name, err) } } - -func (m *Manager) ResourceType(ctx context.Context) string { - return cached.ResourceTypeRepository -} - -func (m *Manager) CountCache(ctx context.Context) (int64, error) { - // prefix is resource type - keys, err := m.client().Keys(ctx, m.ResourceType(ctx)) - if err != nil { - return 0, err - } - - return int64(len(keys)), nil -} - -func (m *Manager) DeleteCache(ctx context.Context, key string) error { - return m.client().Delete(ctx, key) -} - -func (m *Manager) FlushAll(ctx context.Context) error { - // prefix is resource type - keys, err := m.client().Keys(ctx, m.ResourceType(ctx)) - if err != nil { - return err - } - - var errs errors.Errors - for _, key := range keys { - if err = m.client().Delete(ctx, key); err != nil { - errs = append(errs, err) - } - } - - if errs.Len() > 0 { - return errs - } - - return nil -} diff --git a/src/pkg/cached/repository/redis/manager_test.go b/src/pkg/cached/repository/redis/manager_test.go index f458588fd..85c8a93e3 100644 --- a/src/pkg/cached/repository/redis/manager_test.go +++ b/src/pkg/cached/repository/redis/manager_test.go @@ -38,10 +38,8 @@ type managerTestSuite struct { func (m *managerTestSuite) SetupTest() { m.repoMgr = &testRepo.Manager{} m.cache = &testcache.Cache{} - m.cachedManager = NewManager( - m.repoMgr, - ) - m.cachedManager.(*Manager).client = func() cache.Cache { return m.cache } + m.cachedManager = NewManager(m.repoMgr) + m.cachedManager.(*Manager).WithCacheClient(m.cache) m.ctx = context.TODO() } diff --git a/src/server/middleware/transaction/transaction.go b/src/server/middleware/transaction/transaction.go index d3846b2cb..03b28261f 100644 --- a/src/server/middleware/transaction/transaction.go +++ b/src/server/middleware/transaction/transaction.go @@ -18,9 +18,10 @@ import ( "context" "errors" "fmt" - lib_http "github.com/goharbor/harbor/src/lib/http" "net/http" + lib_http "github.com/goharbor/harbor/src/lib/http" + "github.com/goharbor/harbor/src/lib" "github.com/goharbor/harbor/src/lib/log" "github.com/goharbor/harbor/src/lib/orm" @@ -31,12 +32,10 @@ var ( errNonSuccess = errors.New("non success status code") ) -type committedKey struct{} - // MustCommit mark http.Request as committed so that transaction // middleware ignore the status code of the response and commit transaction for this request func MustCommit(r *http.Request) error { - committed, ok := r.Context().Value(committedKey{}).(*bool) + committed, ok := r.Context().Value(orm.CommittedKey{}).(*bool) if !ok { return fmt.Errorf("%s URL %s is not committable, please enable transaction middleware for it", r.Method, r.URL.Path) } @@ -58,7 +57,7 @@ func Middleware(skippers ...middleware.Skipper) func(http.Handler) http.Handler h := func(ctx context.Context) error { committed := new(bool) // default false, not must commit - cc := context.WithValue(ctx, committedKey{}, committed) + cc := context.WithValue(ctx, orm.CommittedKey{}, committed) next.ServeHTTP(res, r.WithContext(cc)) if !(*committed) && !res.Success() { diff --git a/src/server/middleware/transaction/transaction_test.go b/src/server/middleware/transaction/transaction_test.go index b59129f05..078bc4c38 100644 --- a/src/server/middleware/transaction/transaction_test.go +++ b/src/server/middleware/transaction/transaction_test.go @@ -148,7 +148,7 @@ func TestMustCommit(t *testing.T) { } ctx := context.Background() - committableCtx := context.WithValue(ctx, committedKey{}, new(bool)) + committableCtx := context.WithValue(ctx, orm.CommittedKey{}, new(bool)) type args struct { r *http.Request diff --git a/src/server/v2.0/handler/repository.go b/src/server/v2.0/handler/repository.go index 34dc26e5a..6e5b5151c 100644 --- a/src/server/v2.0/handler/repository.go +++ b/src/server/v2.0/handler/repository.go @@ -17,6 +17,7 @@ package handler import ( "context" "fmt" + "github.com/goharbor/harbor/src/common/security/robot" robotCtr "github.com/goharbor/harbor/src/controller/robot" pkgModels "github.com/goharbor/harbor/src/pkg/project/models" @@ -233,6 +234,7 @@ func (r *repositoryAPI) UpdateRepository(ctx context.Context, params operation.U } if err := r.repoCtl.Update(ctx, &repomodel.RepoRecord{ RepositoryID: repository.RepositoryID, + Name: repository.Name, Description: params.Repository.Description, }, "Description"); err != nil { return r.SendError(ctx, err) diff --git a/src/testing/pkg/cached/manifest/redis/cached_manager.go b/src/testing/pkg/cached/manifest/redis/cached_manager.go index 72c592a0e..235377aa2 100644 --- a/src/testing/pkg/cached/manifest/redis/cached_manager.go +++ b/src/testing/pkg/cached/manifest/redis/cached_manager.go @@ -5,6 +5,8 @@ package redis import ( context "context" + cache "github.com/goharbor/harbor/src/lib/cache" + mock "github.com/stretchr/testify/mock" ) @@ -13,6 +15,22 @@ type CachedManager struct { mock.Mock } +// CacheClient provides a mock function with given fields: ctx +func (_m *CachedManager) CacheClient(ctx context.Context) cache.Cache { + ret := _m.Called(ctx) + + var r0 cache.Cache + if rf, ok := ret.Get(0).(func(context.Context) cache.Cache); ok { + r0 = rf(ctx) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(cache.Cache) + } + } + + return r0 +} + // CountCache provides a mock function with given fields: ctx func (_m *CachedManager) CountCache(ctx context.Context) (int64, error) { ret := _m.Called(ctx)