Dockerregistry hard prune fix layer links by legionus · Pull Request #17020 · openshift/origin

legionus · 2017-10-24T12:49:49Z

bparees · 2017-10-24T15:48:46Z

/lgtm

dmage · 2017-10-24T16:01:49Z

/hold
/test extended_image_registry

dmage · 2017-10-24T16:18:36Z

pkg/dockerregistry/server/prune/prune.go

+	logger := context.GetLogger(ctx)
+	vacuum := storage.NewVacuum(ctx, r.storageDriver)
+
+	logger.Debugln("Removing %s repository", r.name)


dmage · 2017-10-24T16:19:40Z

pkg/dockerregistry/server/prune/prune.go

+	}
+
 	err = repositoryEnumerator.Enumerate(ctx, func(repoName string) error {
+		if !dryRun {


This dryRun is confusing, after Enumerate you call these functions unconditionally.

i think it's ok because the pruner's never get values set if it's dryrun.

but i agree that some comments would go a long way in this logic.

legionus · 2017-10-24T17:21:44Z

After discussion with Oleg and Michael, we realized that in its current form, this PR is not good enough.
I'm reworking it.

legionus · 2017-10-25T10:22:43Z

@bparees @miminar please review again

legionus · 2017-10-25T10:24:55Z

/test extended_image_registry

miminar

Mostly nits.

miminar · 2017-10-25T10:29:17Z

pkg/dockerregistry/server/prune/prune.go

+	gc.repoName = repoName
+}
+
+func (gc *garbageColletor) SetManifestLink(svc distribution.ManifestService, repoName string, dgst digest.Digest) {


nit: I'd prefer add instead of set to make it more aligned with real-world garbage collectors.

By marking an object for garbage collection, you don't want to clear the mark from any other marked object.

i think Set is appropriate here because it's not appending the value to anything, it's replacing it.

it's replacing it.

And that's the problem. Those attributes holding objects marked for deletion should be arrays or set.

Requiring a user to call Collect() each time when he marks a single object for deletion (otherwise the garbage collector forgets earlier object) suggests broken design and makes the gb unusable for generic use.

But as I said, it's a nit since the gb is not exported.

miminar · 2017-10-25T10:29:21Z

pkg/dockerregistry/server/prune/prune.go

+	return nil
+}
+
+type garbageColletor struct {


also typo. should be garbageCollector.

miminar · 2017-10-25T10:30:12Z

pkg/dockerregistry/server/prune/prune.go

-				return fmt.Errorf("failed to delete the manifest link %s@%s: %v", repoName, dgst, err)
-			}
-
+			gc.SetManifestLink(manifestService, repoName, dgst)


Put here a comment why the deletion needs to be delayed.

miminar · 2017-10-25T10:30:26Z

pkg/dockerregistry/server/prune/prune.go

-			// We cannot delete the repository at this point, because it would break Enumerate.
-			reposToDelete = append(reposToDelete, repoName)
-
+			gc.SetRepository(repoName)


Explain why the deletion needs to be delayed.

yes please doc the whole logic of how this deferred deletion works (and why it is necessary).

bparees · 2017-10-25T11:36:54Z

pkg/dockerregistry/server/prune/prune.go

+	logger := context.GetLogger(ctx)
+	vacuum := storage.NewVacuum(ctx, p.StorageDriver)
+
+	logger.Debugln("Removing %s repository", reponame)


bparees · 2017-10-25T11:37:37Z

pkg/dockerregistry/server/prune/prune.go

+func (p *RegistryPruner) DeleteManifestLink(ctx context.Context, svc distribution.ManifestService, reponame string, dgst digest.Digest) error {
+	logger := context.GetLogger(ctx)
+
+	logger.Printf("Deleting manifest link: %s@%s", reponame, dgst)


not sure how you've made the decision between printf and debugf for the various Delete* functions, but it seems inconsistent.

bparees · 2017-10-25T11:38:25Z

pkg/dockerregistry/server/prune/prune.go

+}
+
+func (p *RegistryPruner) DeleteBlob(ctx context.Context, dgst digest.Digest) error {
+	vacuum := storage.NewVacuum(ctx, p.StorageDriver)


why no logging for this one?

The vacuum logs this operation on its own.

bparees · 2017-10-25T11:39:18Z

pkg/dockerregistry/server/prune/prune.go

+	gc.repoName = repoName
+}
+
+func (gc *garbageColletor) SetManifestLink(svc distribution.ManifestService, repoName string, dgst digest.Digest) {


i think Set is appropriate here because it's not appending the value to anything, it's replacing it.

bparees · 2017-10-25T11:39:57Z

pkg/dockerregistry/server/prune/prune.go

+	return nil
+}
+
+type garbageColletor struct {


also typo. should be garbageCollector.

bparees · 2017-10-25T11:44:15Z

pkg/dockerregistry/server/prune/prune.go

-			// We cannot delete the repository at this point, because it would break Enumerate.
-			reposToDelete = append(reposToDelete, repoName)
-
+			gc.SetRepository(repoName)


yes please doc the whole logic of how this deferred deletion works (and why it is necessary).

legionus · 2017-10-25T12:22:18Z

@bparees @miminar comments addressed.

legionus · 2017-10-25T12:32:44Z

/test extended_image_registry

bparees · 2017-10-25T12:39:21Z

/lgtm

bparees · 2017-10-25T12:39:58Z

@legionus i guess you can just remove the hold once the registry test passes.

Signed-off-by: Gladkov Alexey <agladkov@redhat.com>

legionus · 2017-10-25T14:55:56Z

/test extended_image_registry

legionus · 2017-10-25T16:12:29Z

/retest

openshift-ci-robot · 2017-10-25T16:47:59Z

@legionus: you cannot LGTM your own PR.

Details

In response to this:

/lgtm
/unhold

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

legionus · 2017-10-25T16:50:42Z

/hold cancel

dmage · 2017-10-25T16:51:27Z

/lgtm

openshift-merge-robot · 2017-10-25T16:51:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, dmage, legionus

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cmd/dockerregistry/OWNERS~~ [bparees,dmage,legionus]
~~pkg/dockerregistry/OWNERS~~ [bparees,dmage,legionus]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-10-25T17:44:10Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2017-10-25T21:01:00Z

Automatic merge from submit-queue (batch tested with PRs 17020, 17026, 17000, 17010).

This commits allows to deploy the RDO registry with OpenShift 3.7. It currently uses the "rdo-test" branch because there is still one unmerged pull request upstream that hasn't merged yet upstream. Delta from OpenShift 3.5 that is interesting to us: - Significant improvements for registry and image pruning see [1][2][3][4] - docker-registry can now use reencrypt routes [5] - Metrics and logging were deployed by default in 3.5, this is no longer the case in 3.7, avoiding an unnecessary impact on performance. [6] - We're now deploying a persistent volume for the docker-registry service on the local filesystem. [1]: openshift/origin#13671 [2]: openshift/origin#16717 [3]: openshift/origin#16656 [4]: openshift/origin#17020 [5]: openshift/origin#14249 [6]: openshift/openshift-ansible@660bafe Change-Id: I5c364a1aab883b6af061051bf190ce857bf2e1f9

legionus added the component/imageregistry label Oct 24, 2017

legionus self-assigned this Oct 24, 2017

legionus requested a review from dmage October 24, 2017 12:49

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2017

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 24, 2017

legionus requested a review from bparees October 24, 2017 13:55

openshift-ci-robot assigned bparees Oct 24, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2017

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2017

dmage reviewed Oct 24, 2017

View reviewed changes

legionus changed the title ~~Dockerregistry hard prune fix layer links~~ [WIP] Dockerregistry hard prune fix layer links Oct 24, 2017

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 24, 2017

legionus force-pushed the dockerregistry-hard-prune-fix-layer-links branch from f25315b to 465ad83 Compare October 25, 2017 10:11

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2017

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 25, 2017

legionus force-pushed the dockerregistry-hard-prune-fix-layer-links branch from 465ad83 to eed4633 Compare October 25, 2017 10:21

legionus changed the title ~~[WIP] Dockerregistry hard prune fix layer links~~ Dockerregistry hard prune fix layer links Oct 25, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2017

miminar suggested changes Oct 25, 2017

View reviewed changes

bparees requested changes Oct 25, 2017

View reviewed changes

legionus force-pushed the dockerregistry-hard-prune-fix-layer-links branch from eed4633 to 0684af4 Compare October 25, 2017 12:20

bparees approved these changes Oct 25, 2017

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2017

legionus force-pushed the dockerregistry-hard-prune-fix-layer-links branch from 0684af4 to 79afcb3 Compare October 25, 2017 14:31

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2017

Refactor hard prune

9421ecc

Signed-off-by: Gladkov Alexey <agladkov@redhat.com>

legionus force-pushed the dockerregistry-hard-prune-fix-layer-links branch from 79afcb3 to 9421ecc Compare October 25, 2017 14:50

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2017

openshift-ci-robot assigned dmage Oct 25, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2017

bparees added the kind/bug Categorizes issue or PR as related to a bug. label Oct 25, 2017

openshift-merge-robot merged commit e0032c4 into openshift:master Oct 25, 2017

Conversation

legionus commented Oct 24, 2017

Uh oh!

bparees commented Oct 24, 2017

Uh oh!

dmage commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legionus commented Oct 24, 2017

Uh oh!

legionus commented Oct 25, 2017

Uh oh!

legionus commented Oct 25, 2017

Uh oh!

miminar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legionus commented Oct 25, 2017

Uh oh!

legionus commented Oct 25, 2017

Uh oh!

bparees commented Oct 25, 2017

Uh oh!

bparees commented Oct 25, 2017

Uh oh!

legionus commented Oct 25, 2017

Uh oh!

legionus commented Oct 25, 2017

Uh oh!

openshift-ci-robot commented Oct 25, 2017

Uh oh!

legionus commented Oct 25, 2017

Uh oh!

dmage commented Oct 25, 2017

Uh oh!

openshift-merge-robot commented Oct 25, 2017

Uh oh!

openshift-merge-robot commented Oct 25, 2017

Uh oh!

openshift-merge-robot commented Oct 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects