fix GC jobs upgrade issue (#11365)

Fixes #11313
Fixes #11275

1, Add more details log in GC job
2, Add type assertion for the upgrading case, the delete_untagged parameter is introduced from v2.0
3, Add UT

Signed-off-by: wang yan <wangyan@vmware.com>

src/jobservice/job/impl/gc/garbage_collection.go

@@ -31,6 +31,25 @@ import (
 	"github.com/goharbor/harbor/src/registryctl/client"
 )
 
+var (
+	regCtlInit = registryctl.Init
+
+	getReadOnly = func(cfgMgr *config.CfgManager) (bool, error) {
+		if err := cfgMgr.Load(); err != nil {
+			return false, err
+		}
+		return cfgMgr.Get(common.ReadOnly).GetBool(), nil
+	}
+
+	setReadOnly = func(cfgMgr *config.CfgManager, switcher bool) error {
+		cfg := map[string]interface{}{
+			common.ReadOnly: switcher,
+		}
+		cfgMgr.UpdateConfig(cfg)
+		return cfgMgr.Save()
+	}
+)
+
 const (
 	dialConnectionTimeout = 30 * time.Second
 	dialReadTimeout       = time.Minute + 10*time.Second
@@ -88,15 +107,15 @@ func (gc *GarbageCollector) Run(ctx job.Context, params job.Parameters) error {
 	if err := gc.init(ctx, params); err != nil {
 		return err
 	}
-	readOnlyCur, err := gc.getReadOnly()
+	readOnlyCur, err := getReadOnly(gc.cfgMgr)
 	if err != nil {
 		return err
 	}
 	if readOnlyCur != true {
-		if err := gc.setReadOnly(true); err != nil {
+		if err := setReadOnly(gc.cfgMgr, true); err != nil {
 			return err
 		}
-		defer gc.setReadOnly(readOnlyCur)
+		defer setReadOnly(gc.cfgMgr, readOnlyCur)
 	}
 	gc.logger.Infof("start to run gc in job.")
 	if err := gc.deleteCandidates(ctx); err != nil {
@@ -116,7 +135,7 @@ func (gc *GarbageCollector) Run(ctx job.Context, params job.Parameters) error {
 }
 
 func (gc *GarbageCollector) init(ctx job.Context, params job.Parameters) error {
-	registryctl.Init()
+	regCtlInit()
 	gc.registryCtlClient = registryctl.RegistryCtlClient
 	gc.logger = ctx.GetLogger()
 	gc.artCtl = artifact.Ctl
@@ -139,29 +158,16 @@ func (gc *GarbageCollector) init(ctx job.Context, params job.Parameters) error {
 	gc.redisURL = params["redis_url_reg"].(string)
 
 	// default is to delete the untagged artifact
-	if params["delete_untagged"] == "" {
-		gc.deleteUntagged = true
-	} else {
-		gc.deleteUntagged = params["delete_untagged"].(bool)
+	gc.deleteUntagged = true
+	deleteUntagged, exist := params["delete_untagged"]
+	if exist {
+		if untagged, ok := deleteUntagged.(bool); ok && !untagged {
+			gc.deleteUntagged = untagged
+		}
 	}
 	return nil
 }
 
-func (gc *GarbageCollector) getReadOnly() (bool, error) {
-	if err := gc.cfgMgr.Load(); err != nil {
-		return false, err
-	}
-	return gc.cfgMgr.Get(common.ReadOnly).GetBool(), nil
-}
-
-func (gc *GarbageCollector) setReadOnly(switcher bool) error {
-	cfg := map[string]interface{}{
-		common.ReadOnly: switcher,
-	}
-	gc.cfgMgr.UpdateConfig(cfg)
-	return gc.cfgMgr.Save()
-}
-
 // cleanCache is to clean the registry cache for GC.
 // To do this is because the issue https://github.com/docker/distribution/issues/2094
 func (gc *GarbageCollector) cleanCache() error {
@@ -220,6 +226,8 @@ func (gc *GarbageCollector) deleteCandidates(ctx job.Context) error {
 			return err
 		}
 		for _, art := range untagged {
+			gc.logger.Infof("delete the untagged artifact: ProjectID:(%d)-RepositoryName(%s)-MediaType:(%s)-Digest:(%s)",
+				art.ProjectID, art.RepositoryName, art.ManifestMediaType, art.Digest)
 			if err := gc.artCtl.Delete(ctx.SystemContext(), art.ID); err != nil {
 				// the failure ones can be GCed by the next execution
 				gc.logger.Errorf("failed to delete untagged:%d artifact in DB, error, %v", art.ID, err)
@@ -233,6 +241,8 @@ func (gc *GarbageCollector) deleteCandidates(ctx job.Context) error {
 		return err
 	}
 	for _, art := range required {
+		gc.logger.Infof("delete the manifest with registry v2 API: RepositoryName(%s)-MediaType:(%s)-Digest:(%s)",
+			art.RepositoryName, art.ManifestMediaType, art.Digest)
 		if err := deleteManifest(art.RepositoryName, art.Digest); err != nil {
 			return fmt.Errorf("failed to delete manifest, %s:%s with error: %v", art.RepositoryName, art.Digest, err)
 		}

src/jobservice/job/impl/gc/garbage_collection_test.go

@@ -1,21 +1,142 @@
 package gc
 
 import (
-	"github.com/stretchr/testify/assert"
+	"github.com/goharbor/harbor/src/common/config"
+	commom_regctl "github.com/goharbor/harbor/src/common/registryctl"
+	"github.com/goharbor/harbor/src/pkg/artifact"
+	"github.com/goharbor/harbor/src/pkg/artifactrash/model"
+	artifacttesting "github.com/goharbor/harbor/src/testing/controller/artifact"
+	mockjobservice "github.com/goharbor/harbor/src/testing/jobservice"
+	"github.com/goharbor/harbor/src/testing/mock"
+	trashtesting "github.com/goharbor/harbor/src/testing/pkg/artifactrash"
+	"github.com/goharbor/harbor/src/testing/registryctl"
+	"github.com/stretchr/testify/suite"
 	"testing"
 )
 
-func TestMaxFails(t *testing.T) {
-	rep := &GarbageCollector{}
-	assert.Equal(t, uint(1), rep.MaxFails())
+type gcTestSuite struct {
+	suite.Suite
+	artifactCtl       *artifacttesting.Controller
+	artrashMgr        *trashtesting.FakeManager
+	registryCtlClient *registryctl.Mockclient
+
+	regCtlInit  func()
+	setReadOnly func(cfgMgr *config.CfgManager, switcher bool) error
+	getReadOnly func(cfgMgr *config.CfgManager) (bool, error)
 }
 
-func TestShouldRetry(t *testing.T) {
-	rep := &GarbageCollector{}
-	assert.False(t, rep.ShouldRetry())
+func (suite *gcTestSuite) SetupTest() {
+	suite.artifactCtl = &artifacttesting.Controller{}
+	suite.artrashMgr = &trashtesting.FakeManager{}
+	suite.registryCtlClient = &registryctl.Mockclient{}
+
+	regCtlInit = func() { commom_regctl.RegistryCtlClient = suite.registryCtlClient }
+	setReadOnly = func(cfgMgr *config.CfgManager, switcher bool) error { return nil }
+	getReadOnly = func(cfgMgr *config.CfgManager) (bool, error) { return true, nil }
 }
 
-func TestValidate(t *testing.T) {
-	rep := &GarbageCollector{}
-	assert.Nil(t, rep.Validate(nil))
+func (suite *gcTestSuite) TestMaxFails() {
+	gc := &GarbageCollector{}
+	suite.Equal(uint(1), gc.MaxFails())
+}
+
+func (suite *gcTestSuite) TestShouldRetry() {
+	gc := &GarbageCollector{}
+	suite.False(gc.ShouldRetry())
+}
+
+func (suite *gcTestSuite) TestValidate() {
+	gc := &GarbageCollector{}
+	suite.Nil(gc.Validate(nil))
+}
+
+func (suite *gcTestSuite) TestDeleteCandidates() {
+	ctx := &mockjobservice.MockJobContext{}
+	logger := &mockjobservice.MockJobLogger{}
+	ctx.On("GetLogger").Return(logger)
+
+	suite.artrashMgr.On("Flush").Return(nil)
+	suite.artifactCtl.On("List").Return([]*artifact.Artifact{
+		{
+			ID:           1,
+			RepositoryID: 1,
+		},
+	}, nil)
+	suite.artifactCtl.On("Delete").Return(nil)
+	suite.artrashMgr.On("Filter").Return([]model.ArtifactTrash{}, nil)
+
+	gc := &GarbageCollector{
+		artCtl:     suite.artifactCtl,
+		artrashMgr: suite.artrashMgr,
+	}
+	suite.Nil(gc.deleteCandidates(ctx))
+}
+
+func (suite *gcTestSuite) TestInit() {
+	ctx := &mockjobservice.MockJobContext{}
+	logger := &mockjobservice.MockJobLogger{}
+	mock.OnAnything(ctx, "Get").Return("core url", true)
+	ctx.On("GetLogger").Return(logger)
+
+	gc := &GarbageCollector{}
+	params := map[string]interface{}{
+		"delete_untagged": true,
+		"redis_url_reg":   "redis url",
+	}
+	suite.Nil(gc.init(ctx, params))
+	suite.True(gc.deleteUntagged)
+
+	params = map[string]interface{}{
+		"delete_untagged": "unsupported",
+		"redis_url_reg":   "redis url",
+	}
+	suite.Nil(gc.init(ctx, params))
+	suite.True(gc.deleteUntagged)
+
+	params = map[string]interface{}{
+		"delete_untagged": false,
+		"redis_url_reg":   "redis url",
+	}
+	suite.Nil(gc.init(ctx, params))
+	suite.False(gc.deleteUntagged)
+
+	params = map[string]interface{}{
+		"redis_url_reg": "redis url",
+	}
+	suite.Nil(gc.init(ctx, params))
+	suite.True(gc.deleteUntagged)
+}
+
+func (suite *gcTestSuite) TestRun() {
+	ctx := &mockjobservice.MockJobContext{}
+	logger := &mockjobservice.MockJobLogger{}
+	ctx.On("GetLogger").Return(logger)
+	mock.OnAnything(ctx, "Get").Return("core url", true)
+
+	suite.artrashMgr.On("Flush").Return(nil)
+	suite.artifactCtl.On("List").Return([]*artifact.Artifact{
+		{
+			ID:           1,
+			RepositoryID: 1,
+		},
+	}, nil)
+	suite.artifactCtl.On("Delete").Return(nil)
+	suite.artrashMgr.On("Filter").Return([]model.ArtifactTrash{}, nil)
+
+	gc := &GarbageCollector{
+		artCtl:     suite.artifactCtl,
+		artrashMgr: suite.artrashMgr,
+		cfgMgr:     config.NewInMemoryManager(),
+	}
+	params := map[string]interface{}{
+		"delete_untagged": false,
+		// ToDo add a redis testing pkg, we do have a 'localhost' redis server in UT
+		"redis_url_reg": "redis://localhost:6379",
+	}
+
+	suite.Nil(gc.Run(ctx, params))
+}
+
+func TestGCTestSuite(t *testing.T) {
+	suite.Run(t, &gcTestSuite{})
 }

src/testing/registryctl/client.go

@@ -0,0 +1,24 @@
+package registryctl
+
+import (
+	"github.com/goharbor/harbor/src/registryctl/api"
+	"github.com/stretchr/testify/mock"
+)
+
+type Mockclient struct {
+	mock.Mock
+}
+
+// Health ...
+func (c *Mockclient) Health() error {
+	return nil
+}
+
+// StartGC ...
+func (c *Mockclient) StartGC() (*api.GCResult, error) {
+	result := &api.GCResult{
+		Status: true,
+		Msg:    "this is a mock client",
+	}
+	return result, nil
+}