Registry catalog #87

miminar · 2018-05-22T17:05:30Z

Implements the upstream repository catalog API call:

GET /v2/_catalog

Returned is a list of repository names as json:

200 OK
Content-Type: application/json
{ "repositories": [ <name>, ... ] }

Pagination is supported by mapping the last returned repository name to an opaque continue token received from master API and using it on the next invocation.

Task: https://trello.com/c/AZINw5qI

Associated origin PR to allow registry to list image stream: openshift/origin#19879

Documentation: openshift/openshift-docs#10102

TODO:

- tests
- use user token
- app holds just a lru cache

miminar · 2018-05-22T17:06:58Z

@dmage, @legionus PTAL

@bparees FYI

/test

miminar · 2018-05-22T17:16:19Z

pkg/dockerregistry/server/catalog.go

+) error {
+	var start string
+
+	osClient, err := r.client.Client()


Probably user's token should be used instead. The registry cannot list imagestreams ATM, but we can be sure that user can thanks to the SAR check.

Otherwise list imagestreams needs to be added to system:registry cluster role.

using the user's token seems reasonable to me...

miminar · 2018-05-22T17:20:26Z

pkg/dockerregistry/server/app.go

+	return r.registry.Repository(ctx, name)
+}
+
+func (r *registry) Repositories(ctx context.Context, repos []string, last string) (n int, err error) {


Also a good candidate for metrics. Suitable as a follow-up after this and #79 are merged.

miminar · 2018-05-22T17:21:55Z

pkg/dockerregistry/server/app.go

+	repositoryEnumerator RepositoryEnumerator
+}
+
+type registry struct {


Note to myself: Maybe worth its own file (to complement already existing registry_test.go).

miminar · 2018-05-22T17:24:25Z

pkg/dockerregistry/server/app.go

@@ -55,16 +57,44 @@ type App struct {

 	// cache is a shared cache of digests and descriptors.
 	cache cache.DigestCache
+
+	// repositoryEnumerator allows to enumerate repository names known to registry
+	repositoryEnumerator RepositoryEnumerator


This could be just the lru cache of opaque blobs. Putting on TODO.

bparees · 2018-05-22T20:20:58Z

pkg/dockerregistry/server/catalog.go

+	last string,
+) (n int, err error) {
+	if len(repos) == 0 {
+		// Client explicitely requested 0 results. Returning nil for error seems more appropriate but this is


bparees · 2018-05-22T20:21:58Z

pkg/dockerregistry/server/catalog.go

+		// Client explicitely requested 0 results. Returning nil for error seems more appropriate but this is
+		// more in line with upstream does. Returning nil actually makes the upstream code panic.
+		// TODO: patch upstream?  /_catalog?n=0  results in 500
+		return 0, errors.New("no space in slice")


can we at least return a better error than "no space in slice", like "client requested zero entries"?

If we'll use this API somewhere else, the message about the client will provide misleading information.

still seems like there is a more informative message we can provide/bubble up. it's pretty much a self-inflicted+correctable error, so the error text should make that clear.. "no space in slice" makes it sound like an internal error where we ran out of memory and nothing can be done w/o changing code.

bparees · 2018-05-22T20:27:55Z

pkg/dockerregistry/server/catalog.go

+) error {
+	var start string
+
+	osClient, err := r.client.Client()


using the user's token seems reasonable to me...

dmage · 2018-05-23T08:20:03Z

pkg/dockerregistry/server/catalog.go

+		name := fmt.Sprintf("%s/%s", is.Namespace, is.Name)
+		if len(last) > 0 && name <= last {
+			if !warned {
+				context.GetLogger(ctx).Warnf("pagination not working -> filtering the full image stream list")


What should I do when I see this warning in the log?

Check if you run the registry against different version of master API. The message seemed too long already but I can add the suggestion.

Can this situation be handled by enumerateImageStreams, so that we don't need to spread this hack by the code?

Sure. I'll rework it.

dmage · 2018-05-23T08:22:14Z

pkg/dockerregistry/server/catalog.go

+	case nil:
+		err = io.EOF
+	default:
+		context.GetLogger(ctx).Errorf("failed to enumerate repositories: %v", err)


Do we need this? I would expect the upstream code to log this error.

You are right, the returned error is logged, I've removed the logging statement.

dmage · 2018-05-23T09:11:08Z

pkg/dockerregistry/server/catalog.go

+		Continue: start,
+	})
+
+	if err != nil {


The master may respond with 410 ResourceExpired, that should be treated as an indication to repeat the query without Continue.

Good point, I'll handle the case.

dmage · 2018-05-23T09:14:16Z

pkg/dockerregistry/server/catalog.go

+	}
+
+	if len(last) > 0 {
+		if c, ok := r.cache.Get(last); !ok {


Can we use this token twice?

Yes, from what I understand, the same token can be returned to two different clients or to two different requests.

miminar · 2018-05-24T14:26:03Z

Addressed most of the comments.

bparees · 2018-05-24T20:22:39Z

pkg/dockerregistry/server/catalog.go

+
+	if len(last) > 0 {
+		if c, ok := r.cache.Get(last); !ok {
+			context.GetLogger(ctx).Warnf("failed to find opaque continue token for last repository=%q -> requesting the full image stream list instead of %d items", last, limit)


should set warned=true?

Not necessary, but it will add to legibility and robustness.

miminar · 2018-05-30T10:28:07Z

Added unit tests and squashed.

To sustain registry catalog API implemented in openshift/image-registry#87 Not really necessary since registry will attempt to use user's token. But a fall-back to registry client depends on this policy. Signed-off-by: Michal Minář <miminar@redhat.com>

dmage · 2018-06-07T14:30:13Z

My main concern that it will perform poorly behind load balancers without sticky sessions. And as I can see, the optimistic way when we have an opaque token is not tested.

If we are fine with slow requests, then LGTM.

bparees · 2018-06-07T14:54:58Z

My main concern that it will perform poorly behind load balancers without sticky sessions.

specifically the pagination you mean?

miminar · 2018-06-07T15:11:33Z

Our docker-registry's service has sessionAffinity=ClientIP by default. We can only hope that clients will not be too dumm to ask for all.

miminar · 2018-06-21T06:35:58Z

Rebased.

miminar · 2018-06-21T08:00:41Z

Re: ci/prow/e2e - Job failed. - Looks like broken environment:

failed to build "172.30.42.94:5000/e2e-test-prune-images-hnpr6/prune-schema2-all-images-false:latest" image: dial unix /var/run/docker.sock: connect: no such file or directory

/retest

miminar · 2018-06-21T09:18:56Z

@openshift/sig-continuous-infrastructure to rescue:

failed to build "172.30.74.83:5000/e2e-test-prune-images-dt99r/prune:latest" image: dial unix /var/run/docker.sock: connect: no such file or directory

smarterclayton · 2018-06-21T15:28:40Z

Does test-prune-images assume it has access to the docker socket on the host where the e2e test runs? IF so it should be [local] and excluded, or fixed to not assume that.

bparees · 2018-06-21T17:10:53Z

yes it looks like hardprune.go is building an image locally to push into the registry in order to tightly control the structure of the image being produced.

bparees · 2018-06-21T17:11:14Z

@smarterclayton if we mark it [local], under what conditions will it still get run?

bparees · 2018-06-21T17:16:52Z

(marking it local here: openshift/origin#20061)

openshift-docker · 2018-06-21T22:18:40Z

When run as part of a single node installation. Extended tests shouldn’t depend on local access to the node. Use a privileged pod if you need to check host stuff (and remember, you would fail on crio). On Jun 21, 2018, at 1:11 PM, Ben Parees <notifications@github.com> wrote: @smarterclayton <https://github.com/smarterclayton> if we mark it [local], under what conditions will it still get run? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#87 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG7fCsqIs5yIAelUtbJ-NepnV2cErGXxks5t-9O2gaJpZM4UJC8y> .

bparees · 2018-06-21T22:35:59Z

When run as part of a single node installation.

@smarterclayton right, so does any of our PR testing on either origin or image-registry exercise the extended tests on a single node installation? That is my fundamental question here... once i tag the test as [local] will we still know if something breaks it.

Extended tests shouldn’t depend on local access to the node. Use a
privileged pod if you need to check host stuff (and remember, you would
fail on crio).

I get it, trying to evaluate how urgent it is that we fix this so the test is being executed again.

smarterclayton · 2018-06-21T22:44:52Z

On Jun 21, 2018, at 6:36 PM, Ben Parees <notifications@github.com> wrote: When run as part of a single node installation. @smarterclayton <https://github.com/smarterclayton> right, so does any of our PR testing on either origin or image-registry exercise the extended tests on a single node installation? That is my fundamental question here... once i tag the test as [local] will we still know if something breaks it. The master AWS job currently tests it. We will probably have a single node oc cluster up test that may be able to use this in the future. Extended tests shouldn’t depend on local access to the node. Use a privileged pod if you need to check host stuff (and remember, you would fail on crio). I get it, trying to evaluate how urgent it is that we fix this so the test is being executed again. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#87 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_px_VOU6ofXwAcDl_ehv1oq1pMKIpks5t_B_WgaJpZM4UJC8y> .

bparees · 2018-06-22T02:41:45Z

The master AWS job currently tests it. We will probably have a single node
oc cluster up test that may be able to use this in the future.

ok, good enough, thanks.

miminar · 2018-06-22T06:48:34Z

/retest

legionus · 2018-06-25T14:54:54Z

/retest

legionus

/lgtm

Implements the upstream repository catalog API call: GET /v2/_catalog Returned is a list of repository names as json: 200 OK Content-Type: application/json { "repositories": [ <name>, ... ] } Pagination is supported by mapping the last returned repository name to the opaque continue token received from master API and using it on the next invocation. Signed-off-by: Michal Minář <miminar@redhat.com>

miminar · 2018-06-26T05:52:09Z

Rebased.

legionus · 2018-06-26T08:55:20Z

/lgtm
/retest

openshift-ci-robot · 2018-06-26T08:56:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: legionus, miminar

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [legionus,miminar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

miminar · 2018-06-26T10:18:34Z

/retest

To sustain registry catalog API implemented in openshift/image-registry#87 Not really necessary since registry will attempt to use user's token. But a fall-back to registry client depends on this policy. Signed-off-by: Michal Minář <miminar@redhat.com>

openshift-ci-robot requested review from mfojtik and soltysh May 22, 2018 17:05

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 22, 2018

miminar changed the title ~~Registry catalog~~ [WIP] Registry catalog May 22, 2018

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 May 22, 2018

miminar commented May 22, 2018

View reviewed changes

bparees reviewed May 22, 2018

View reviewed changes

dmage reviewed May 23, 2018

View reviewed changes

bparees reviewed May 24, 2018

View reviewed changes

miminar force-pushed the repository-catalog branch from ee5335b to ef613c0 Compare May 30, 2018 10:19

miminar changed the title ~~[WIP] Registry catalog~~ Registry catalog May 30, 2018

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 May 30, 2018

miminar force-pushed the repository-catalog branch from ef613c0 to 7fef2aa Compare May 30, 2018 10:34

miminar mentioned this pull request May 30, 2018

registry can now list image streams openshift/origin#19879

Merged

miminar force-pushed the repository-catalog branch 2 times, most recently from f2f9660 to 78f5f9e Compare May 30, 2018 12:03

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2018

miminar force-pushed the repository-catalog branch from 78f5f9e to 439bfe0 Compare June 21, 2018 06:34

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2018

miminar mentioned this pull request Jun 22, 2018

Make registry tests using docker.socket dockerless openshift/origin#20079

Open

legionus approved these changes Jun 25, 2018

View reviewed changes

openshift-ci-robot assigned legionus Jun 25, 2018

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

miminar force-pushed the repository-catalog branch from 439bfe0 to 5561421 Compare June 26, 2018 05:51

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2018

openshift-merge-robot merged commit 746afe4 into openshift:master Jun 26, 2018

Registry catalog #87

Registry catalog #87

Conversation

miminar commented May 22, 2018 • edited

miminar commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miminar commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miminar commented May 30, 2018

dmage commented Jun 7, 2018

bparees commented Jun 7, 2018

miminar commented Jun 7, 2018

miminar commented Jun 21, 2018

miminar commented Jun 21, 2018

miminar commented Jun 21, 2018

smarterclayton commented Jun 21, 2018

bparees commented Jun 21, 2018

bparees commented Jun 21, 2018

bparees commented Jun 21, 2018

openshift-docker commented Jun 21, 2018 via email

bparees commented Jun 21, 2018

smarterclayton commented Jun 21, 2018 via email

bparees commented Jun 22, 2018

miminar commented Jun 22, 2018

legionus commented Jun 25, 2018

legionus left a comment

Choose a reason for hiding this comment

miminar commented Jun 26, 2018

legionus commented Jun 26, 2018

openshift-ci-robot commented Jun 26, 2018

miminar commented Jun 26, 2018

miminar commented May 22, 2018 •

edited