Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rebased docker/distribution #5404

Merged
merged 17 commits into from
Dec 18, 2015
Merged

Rebased docker/distribution #5404

merged 17 commits into from
Dec 18, 2015

Conversation

miminar
Copy link

@miminar miminar commented Oct 26, 2015

Resolves #4339
Resolves #4672
Partially addresses #5900 (this PR just reenables liveness probe).

Benefits:

Notable features by Docker:

@miminar
Copy link
Author

miminar commented Oct 26, 2015

Ping @Kargakis, @soltysh, @ncdc

@liggitt
Copy link
Contributor

liggitt commented Oct 26, 2015

this is post 1.1, right?

@deads2k
Copy link
Contributor

deads2k commented Oct 26, 2015

Can you summarize the additional benefit we'd get by bumping levels?

@miminar
Copy link
Author

miminar commented Oct 27, 2015

@deads2k most notably swift support. There are customers asking for this.

@pweil- shall we push this before 1.1?

@0xmichalis
Copy link
Contributor

@pweil- shall we push this before 1.1?

Hm, still a big change for the registry...

@soltysh
Copy link
Contributor

soltysh commented Oct 27, 2015

What about removing code.google.com/p/go-uuid/uuid that @Kargakis brought up in #4339. I see docker is using github.com/docker/distribution/uuid we k8s uses github.com/pborman/uuid. Without removing we have 3 packages in our godeps doing the same/similar thing. We could at least get rid of the first one.

@0xmichalis
Copy link
Contributor

What about removing code.google.com/p/go-uuid/uuid that @Kargakis brought up in #4339. I see docker is using github.com/docker/distribution/uuid we k8s uses github.com/pborman/uuid. Without removing we have 3 packages in our godeps doing the same/similar thing. We could at least get rid of the first one.

We have a couple of places in Origin where we use the code.google package, we could replace those for start.

pkg/cmd/server/origin/auth.go
13: "code.google.com/p/go-uuid/uuid"

pkg/cmd/server/origin/auth_config.go
8:  "code.google.com/p/go-uuid/uuid"

pkg/auth/server/csrf/cookie.go
6:  "code.google.com/p/go-uuid/uuid"

pkg/auth/server/csrf/session.go
8:  "code.google.com/p/go-uuid/uuid"

pkg/generate/app/app.go
13: "code.google.com/p/go-uuid/uuid"

@deads2k
Copy link
Contributor

deads2k commented Oct 27, 2015

@smarterclayton This looks like a really big change to bring in now. Is the swift support worth it?

@soltysh
Copy link
Contributor

soltysh commented Oct 27, 2015

Additionally, is it possible to upgrade the docker/distribution without the biggest issue (#3333) addressed?

@pweil-
Copy link
Contributor

pweil- commented Oct 27, 2015

@pweil- shall we push this before 1.1?

I'd be worried about pulling it in at this stage in the release unless it is 100% required. I'd prefer we hang on to it for a 3.1.1 or 3.1.2 but I'll defer to @smarterclayton

@miminar
Copy link
Author

miminar commented Oct 27, 2015

What about removing code.google.com/p/go-uuid/uuid that @Kargakis brought up in #4339.

Will do.

Additionally, is it possible to upgrade the docker/distribution without the biggest issue (#3333) addressed?

Current pruning code will only be using a bit different api calls exported with still unmerged PR#1050.

@ncdc
Copy link
Contributor

ncdc commented Oct 27, 2015

I don't think this should be merged until post 3.1. I also don't think this should be merged until upstream 1050 is merged. We don't want to risk deviating from what ultimately goes in upstream. Also, I'm seeing a proposed restructuring of some of the top level interfaces (distribution/distribution#1105). It might be worth getting an ETA on when those will land so we don't have to change things twice (once now, once after that lands).

@miminar
Copy link
Author

miminar commented Oct 27, 2015

Yeah, that upstream restructuring PR would really breaks our API pruning PR into pieces. It'd be worth to get it upstream first.

@ncdc
Copy link
Contributor

ncdc commented Oct 27, 2015

@miminar can you find out the status of 1105? Do they have an ETA?

@smarterclayton
Copy link
Contributor

If we don't do this for 3.1 it is must have (as in, top priority) for
3.1.1.

On Tue, Oct 27, 2015 at 10:34 AM, Andy Goldstein [email protected]
wrote:

@miminar https://github.com/miminar can you find out the status of
1105? Do they have an ETA?

—
Reply to this email directly or view it on GitHub
#5404 (comment).

@miminar
Copy link
Author

miminar commented Oct 27, 2015

Their ETA is 1.10, which is not yet in their roadmap but based on their 2 months release cycle it could be the end of December. And luckily, according to today's irc discussion, they I'd like to get 1050 in before 1105.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2015
@soltysh soltysh mentioned this pull request Nov 16, 2015
Closed
@miminar
Copy link
Author

miminar commented Dec 11, 2015

Rebased to v2.2.0. I'll update this once more with two days old v2.2.1 - a bugfix release.

Reenabled liveness probe (issue #5900). I'd like to add registry's readiness probe once this is in.

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2015
@soltysh
Copy link
Contributor

soltysh commented Dec 11, 2015

I'd appreciate if the two commits regarding uuid package changes has those package names in it.

@soltysh
Copy link
Contributor

soltysh commented Dec 11, 2015

That last commit (regarding changes in OpenShift) is a bit lengthy, just by looking into the description. Could you split it into logical parts from the description?

@soltysh
Copy link
Contributor

soltysh commented Dec 11, 2015

[test]

@soltysh
Copy link
Contributor

soltysh commented Dec 11, 2015

The error from tests proves my argument about not updated test/end-to-end/core.sh:

[INFO] Verifying the docker-registry is up at 172.30.169.199:5000
ERROR: gave up waiting for http://172.30.169.199:5000/healthz
!!! Error in hack/../test/end-to-end/core.sh:313

@miminar
Copy link
Author

miminar commented Dec 11, 2015

Rebased to 2.2.1.

I'd appreciate if the two commits regarding uuid package changes has those package names in it.

fixed.

The error from tests proves my argument about not updated test/end-to-end/core.sh

yes, updated. Thanks for pointing that out.

@ncdc
Copy link
Contributor

ncdc commented Dec 16, 2015

Sorry, this slipped by me yesterday. It's been a bit of a crazy morning at home so far, so once I've finished eating, I'll take a look.

Michal Minar added 10 commits December 16, 2015 16:52
unbump(code.google.com/p/go-uuid/uuid): which is obsoleted
9ecc7cf 
Obsoleted by:

    github.com/pborman/uuid

Signed-off-by: Michal Minar <[email protected]>
Registry refactor: handle REGISTRY_CONFIGURATION_PATH
d714558 
Use this environment variable as a fallback for missing config argument.

Unset it to avoid a warning during config file parsing.

Signed-off-by: Michal Minar <[email protected]>
Refactor dockerregistry: adapt to upstream changes
f585b49 
Repository middleware now implements ManifestService.

Enable delete in registry's configuration file.

Signed-off-by: Michal Minar <[email protected]>
Refactor dockerregistry: use registry's own health check
6ac9396 
There are two endpoints for health checks implemented by upstream code:

- /         - no auth required
- /v2/      - requires authentication

Also based on upstream change, add a panic handler.

Signed-off-by: Michal Minar <[email protected]>
Refactor dockerregistry: unify logging style with upstream's codebase
ff330a7 
Configure logging the way, upstream does it.

Propagate context where the logging is needed.

Signed-off-by: Michal Minar <[email protected]>
Refactor dockerregistry: prevent a warning during startup
bb6781f 
`REGISTRY_URL` is used by our middleware code to prefix image names in
the registry. It's unknown to registry's configuration parser which
tries to match it against a config option. Its failed attempt results in
a warning:

    level=warning msg="Ignoring unrecognized environment variable REGISTRY_URL"

This patch renames it to `DOCKER_REGISTRY_URL` and thus prevents the
warning.

Signed-off-by: Michal Minar <[email protected]>
Refactor of OSO's code
c46d7ab 
Repository components 1 character long allowed.

Use new docker/distribution/registry/api/errcode package.

Signed-off-by: Michal Minar <[email protected]>
Reenabled registry dc's liveness probe
9ade4e6 
Reverts 12f4a35

Signed-off-by: Michal Minar <[email protected]>
Remove redundant admin routes
0e2465a 
Upstream registry allows for soft deletion of layers and manifests with
routes:

- `/v2/<name>/blobs/<digest>`
- `/v2/<name>/manifests/<digest`

which make 2 out of our 3 admin routes obsolete.

This patch removes following routes:

- `/admin/<name>/layers/<digest>`
- `/admin/<name>/manifests/<digest>`

Signed-off-by: Michal Minar <[email protected]>
Delete tag if its history is empty
0a574c7 
During a run of prune images, history of a tag may get emptied. When an
image with the same tag is re-pushed, the operation will fail because
the history list is not expected to be empty.

This patch checks the history list for non-emptiness before returning a
valid tag event and ensures that tag is deleted from ImageStream's
status.tags  when the last item has been removed from its history during
a prune.

Signed-off-by: Michal Minar <[email protected]>
@miminar
Copy link
Author

miminar commented Dec 16, 2015

Flake #6339. re-[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 0a574c7

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7878/)

@miminar
Copy link
Author

miminar commented Dec 17, 2015

I believe all the issues raised have been addressed. Is there anything else?

@ncdc
Copy link
Contributor

ncdc commented Dec 17, 2015

I got pulled away and didn't thoroughly review all the commits. If you
already have an ok, don't let me hold this up.

On Thursday, December 17, 2015, Michal Minar [email protected]
wrote:

I believe all the issues raised have been addressed. Is there anything
else?

—
Reply to this email directly or view it on GitHub
#5404 (comment).

@pweil-
Copy link
Contributor

pweil- commented Dec 17, 2015

[merge]

@pweil- pweil- mentioned this pull request Dec 17, 2015
Merged
@soltysh
Copy link
Contributor

soltysh commented Dec 17, 2015

I'm gonna regret it, but yay! ;)

@0xmichalis
Copy link
Contributor

flake #6065

[merge]

@soltysh
Copy link
Contributor

soltysh commented Dec 18, 2015

I think it's a flake from #6259.

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 0a574c7

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7878/) (Image: devenv-rhel7_2988)

@miminar
Copy link
Author

miminar commented Dec 18, 2015

The same "flake" #6259 for the second time. I'll try to rebase and debug locally. I probably saw the outdated failure.

openshift-bot pushed a commit that referenced this pull request Dec 18, 2015
Merge pull request #5404 from miminar/registry-rebase
7ac9d02 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 7ac9d02 into openshift:master Dec 18, 2015
This was referenced Dec 21, 2015
Closed
Closed
@miminar miminar deleted the registry-rebase branch January 24, 2016 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@miminar @liggitt @deads2k @0xmichalis @soltysh @pweil- @ncdc @smarterclayton @ironcladlou @openshift-bot