From f6c0608e22504023ab8d2ed766ff9175823dcf2f Mon Sep 17 00:00:00 2001 From: Wang Yan Date: Wed, 1 Apr 2020 11:53:41 +0800 Subject: [PATCH] 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 --- .../job/impl/gc/garbage_collection.go | 56 ++++--- .../job/impl/gc/garbage_collection_test.go | 141 ++++++++++++++++-- src/testing/registryctl/client.go | 24 +++ 3 files changed, 188 insertions(+), 33 deletions(-) create mode 100644 src/testing/registryctl/client.go diff --git a/src/jobservice/job/impl/gc/garbage_collection.go b/src/jobservice/job/impl/gc/garbage_collection.go index 93811ab65..7561aab0d 100644 --- a/src/jobservice/job/impl/gc/garbage_collection.go +++ b/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) } diff --git a/src/jobservice/job/impl/gc/garbage_collection_test.go b/src/jobservice/job/impl/gc/garbage_collection_test.go index f5286e342..f66871e5e 100644 --- a/src/jobservice/job/impl/gc/garbage_collection_test.go +++ b/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 = ®istryctl.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{}) } diff --git a/src/testing/registryctl/client.go b/src/testing/registryctl/client.go new file mode 100644 index 000000000..cc532c66d --- /dev/null +++ b/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 +}