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 <chenyuzh@vmware.com>

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 %}

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()

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))
 				}
 			}
 		}

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

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

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,
+		BaseManager: cached.NewBaseManager(cached.ResourceTypeArtifact),
-		client:     func() libcache.Cache { return libcache.Default() },
+		delegator:   m,
-		keyBuilder: cached.NewObjectKey(cached.ResourceTypeArtifact),
+		keyBuilder:  cached.NewObjectKey(cached.ResourceTypeArtifact),
-		lifetime:   time.Duration(config.CacheExpireHours()) * time.Hour,
+		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
-}

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.cachedManager = NewManager(m.artMgr)
-		m.artMgr,
+	m.cachedManager.(*Manager).WithCacheClient(m.cache)
-	)
-	m.cachedManager.(*Manager).client = func() cache.Cache { return m.cache }
 	m.ctx = context.TODO()
 }
 

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
+}

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{})
+}

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

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.
+	*cached.BaseManager
-	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() *Manager {
 	return &Manager{
-		client:     func() libcache.Cache { return libcache.Default() },
+		BaseManager: cached.NewBaseManager(cached.ResourceTypeManifest),
-		keyBuilder: cached.NewObjectKey(cached.ResourceTypeManifest),
+		keyBuilder:  cached.NewObjectKey(cached.ResourceTypeManifest),
-		lifetime:   time.Duration(config.CacheExpireHours()) * time.Hour,
+		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) })
+	return retry.Retry(func() error { return m.CacheClient(ctx).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
 }

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"

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,
+		BaseManager: cached.NewBaseManager(cached.ResourceTypeProject),
-		client:     func() libcache.Cache { return libcache.Default() },
+		delegator:   m,
-		keyBuilder: cached.NewObjectKey(cached.ResourceTypeProject),
+		keyBuilder:  cached.NewObjectKey(cached.ResourceTypeProject),
-		lifetime:   time.Duration(config.CacheExpireHours()) * time.Hour,
+		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
-}

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.cachedManager = NewManager(m.projectMgr)
-		m.projectMgr,
+	m.cachedManager.(*Manager).WithCacheClient(m.cache)
-	)
-	m.cachedManager.(*Manager).client = func() cache.Cache { return m.cache }
 	m.ctx = context.TODO()
 }
 

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,
+		BaseManager: cached.NewBaseManager(cached.ResourceTypeProjectMeta),
-		client:     func() libcache.Cache { return libcache.Default() },
+		delegator:   m,
-		keyBuilder: cached.NewObjectKey(cached.ResourceTypeProjectMeta),
+		keyBuilder:  cached.NewObjectKey(cached.ResourceTypeProjectMeta),
-		lifetime:   time.Duration(config.CacheExpireHours()) * time.Hour,
+		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
 	}
-
+	// only cache when result has attributes
-	if err = m.client().Save(ctx, key, &result, m.lifetime); err != nil {
+	if len(result) > 0 {
-		// log error if save to cache failed
+		if err = m.CacheClient(ctx).Save(ctx, key, &result, m.lifetime); err != nil {
-		log.Debugf("save project metadata %v to cache error: %v", result, err)
+			// 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
-}

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.cachedManager = NewManager(m.projectMetaMgr)
-		m.projectMetaMgr,
+	m.cachedManager.(*Manager).WithCacheClient(m.cache)
-	)
-	m.cachedManager.(*Manager).client = func() cache.Cache { return 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)

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,
+		BaseManager: cached.NewBaseManager(cached.ResourceTypeRepository),
-		client:     func() libcache.Cache { return libcache.Default() },
+		delegator:   m,
-		keyBuilder: cached.NewObjectKey(cached.ResourceTypeRepository),
+		keyBuilder:  cached.NewObjectKey(cached.ResourceTypeRepository),
-		lifetime:   time.Duration(config.CacheExpireHours()) * time.Hour,
+		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
-}

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.cachedManager = NewManager(m.repoMgr)
-		m.repoMgr,
+	m.cachedManager.(*Manager).WithCacheClient(m.cache)
-	)
-	m.cachedManager.(*Manager).client = func() cache.Cache { return m.cache }
 	m.ctx = context.TODO()
 }
 

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() {

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

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)

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)