fix artifact trash filter issue (#11575)

fixes #11533

GC jobs will use the filter results to call registry API to delete manifest.

In the current imple, the filter function in some case does not return the deleted artifact as it's using digest as the filter condition.

Like: If one artifact is deleted, but there is another project/repo has a image with same digest with the deleted one, filter func will
not mark the deleted artifact as candidate. It results in, GC job does not call API to remove the manifest.

To fix it, update the filter to use both digest and repository name to filter candidate.

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

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

@@ -215,10 +215,14 @@ func (gc *GarbageCollector) cleanCache() error {
 // 1, required part, the artifacts were removed from Harbor.
 // 2, optional part, the untagged artifacts.
 func (gc *GarbageCollector) deleteCandidates(ctx job.Context) error {
+	if os.Getenv("UTTEST") == "true" {
+		gc.logger = ctx.GetLogger()
+	}
 	// default is not to clean trash
 	flushTrash := false
 	defer func() {
 		if flushTrash {
+			gc.logger.Info("flush artifact trash")
 			if err := gc.artrashMgr.Flush(ctx.SystemContext()); err != nil {
 				gc.logger.Errorf("failed to flush artifact trash: %v", err)
 			}
@@ -235,14 +239,17 @@ func (gc *GarbageCollector) deleteCandidates(ctx job.Context) error {
 		if err != nil {
 			return err
 		}
+		gc.logger.Info("start to delete untagged artifact.")
 		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)
+				continue
 			}
+			gc.logger.Infof("delete the untagged artifact: ProjectID:(%d)-RepositoryName(%s)-MediaType:(%s)-Digest:(%s)",
+				art.ProjectID, art.RepositoryName, art.ManifestMediaType, art.Digest)
 		}
+		gc.logger.Info("end to delete untagged artifact.")
 	}
 
 	// handle the trash
@@ -250,13 +257,15 @@ func (gc *GarbageCollector) deleteCandidates(ctx job.Context) error {
 	if err != nil {
 		return err
 	}
+	gc.logger.Info("required candidate: %+v", required)
 	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)
 		}
+		gc.logger.Infof("delete the manifest with registry v2 API: RepositoryName(%s)-MediaType:(%s)-Digest:(%s)",
+			art.RepositoryName, art.ManifestMediaType, art.Digest)
 	}
+	gc.logger.Info("end to delete required artifact.")
 	flushTrash = true
 	return nil
 }

src/pkg/artifactrash/dao/dao.go

@@ -63,7 +63,7 @@ func (d *dao) Delete(ctx context.Context, id int64) (err error) {
 	return nil
 }
 
-// Filter ...
+// Filter the results are: all of records in artifact_trash excludes the records in artifact with same repo and digest.
 func (d *dao) Filter(ctx context.Context) (arts []model.ArtifactTrash, err error) {
 	var deletedAfs []model.ArtifactTrash
 	ormer, err := orm.FromContext(ctx)
@@ -71,12 +71,13 @@ func (d *dao) Filter(ctx context.Context) (arts []model.ArtifactTrash, err error
 		return deletedAfs, err
 	}
 
-	sql := `SELECT * FROM artifact_trash where artifact_trash.digest NOT IN (select digest from artifact)`
+	sql := `SELECT aft.* FROM artifact_trash AS aft LEFT JOIN artifact af ON (aft.repository_name=af.repository_name AND aft.digest=af.digest) WHERE (af.digest IS NULL AND af.repository_name IS NULL)`
 
 	_, err = ormer.Raw(sql).QueryRows(&deletedAfs)
 	if err != nil {
 		return deletedAfs, err
 	}
+
 	return deletedAfs, nil
 }
 

src/pkg/artifactrash/dao/dao_test.go

@@ -87,6 +87,70 @@ func (d *daoTestSuite) TestFilter() {
 	afs, err := d.dao.Filter(d.ctx)
 	d.Require().Nil(err)
 	d.Require().Equal(afs[0].Digest, "1234")
+
+	// clean it in GC
+	err = d.dao.Flush(d.ctx)
+	d.Require().Nil(err)
+
+	// push hello-world to projecta
+	art1 := &artdao.Artifact{
+		Type:              "image",
+		ManifestMediaType: v1.MediaTypeImageManifest,
+		ProjectID:         11,
+		RepositoryID:      11,
+		RepositoryName:    "projectA/hello-world",
+		Digest:            "sha256:hello-world",
+	}
+	_, err = d.afDao.Create(d.ctx, art1)
+	d.Require().Nil(err)
+
+	// push hello-world to projectb
+	art2 := &artdao.Artifact{
+		Type:              "image",
+		ManifestMediaType: v1.MediaTypeImageManifest,
+		ProjectID:         12,
+		RepositoryID:      12,
+		RepositoryName:    "projectB/hello-world",
+		Digest:            "sha256:hello-world",
+	}
+	_, err = d.afDao.Create(d.ctx, art2)
+	d.Require().Nil(err)
+
+	// remove hello-world to projectA
+	err = d.afDao.Delete(d.ctx, art1.ID)
+	d.Require().Nil(err)
+
+	aft2 := &model.ArtifactTrash{
+		ManifestMediaType: v1.MediaTypeImageManifest,
+		RepositoryName:    "projectA/hello-world",
+		Digest:            "sha256:hello-world",
+	}
+	_, err = d.dao.Create(d.ctx, aft2)
+	d.Require().Nil(err)
+
+	// filter results should contain projectA hello-world
+	afs1, err := d.dao.Filter(d.ctx)
+	d.Require().Nil(err)
+	d.Require().Equal(afs1[0].Digest, "sha256:hello-world")
+	d.Require().Equal(afs1[0].RepositoryName, "projectA/hello-world")
+
+	// push hello-world again to projecta
+	art3 := &artdao.Artifact{
+		Type:              "image",
+		ManifestMediaType: v1.MediaTypeImageManifest,
+		ProjectID:         11,
+		RepositoryID:      13,
+		RepositoryName:    "projectA/hello-world",
+		Digest:            "sha256:hello-world",
+	}
+	_, err = d.afDao.Create(d.ctx, art3)
+	d.Require().Nil(err)
+
+	// filter results should contain nothing
+	afs2, err := d.dao.Filter(d.ctx)
+	d.Require().Nil(err)
+	d.Require().Equal(0, len(afs2))
+
 }
 
 func (d *daoTestSuite) TestFlush() {