registry: allow to override the DOCKER_REGISTRY_URL and default to in… #14882

mfojtik · 2017-06-26T14:28:35Z

Attempt to fix: https://bugzilla.redhat.com/show_bug.cgi?id=1463499 and ublock the possiblity of using DNS for the registry.

mfojtik · 2017-06-26T14:28:41Z

[test]

mfojtik · 2017-06-26T14:29:25Z

@miminar @dmage this basically removes the hardcoded defaults from the image and allows us to change it. The defaulting is done when the registry initializes so this won't be backward compatibility issue.

mfojtik · 2017-06-26T14:30:14Z

pkg/dockerregistry/server/repositorymiddleware.go

+			return nil, fmt.Errorf("%s variable must be set when running outside Kubernetes cluster", DockerRegistryURLEnvVar)
+		}
 	}
+	context.GetLogger(ctx).Infof("Using %q as Docker Registry URL", registryAddr)


this will help with debuging :-)

mfojtik · 2017-06-26T14:33:57Z

@smarterclayton && others: why we have two variables here OPENSHIFT_DEFAULT_REGISTRY and this?

miminar · 2017-06-26T14:38:03Z

[testextended][extended:core(registry|imageapi|ImagePrune|ImageQuota)]

miminar · 2017-06-26T14:39:28Z

pkg/dockerregistry/server/repositorymiddleware.go

+		if len(os.Getenv("DOCKER_REGISTRY_SERVICE_HOST")) > 0 && len(os.GetEnv("DOCKER_REGISTRY_SERVICE_PORT")) > 0 {
+			registryAddr = os.Getenv("DOCKER_REGISTRY_SERVICE_HOST") + ":" + os.Getenv("DOCKER_REGISTRY_SERVICE_PORT")
+		} else {
+			return nil, fmt.Errorf("%s variable must be set when running outside Kubernetes cluster", DockerRegistryURLEnvVar)


mfojtik · 2017-06-26T14:47:40Z

@miminar made one more change. I deprecated the REGISTRY_DEFAULT_URL in favor of OPENSHIFT_DEFAULT_REGISTRY (i kept the old there for backward compatibility, but with warning). I also updated both Dockerfiles and tests.

sdodson · 2017-06-26T15:24:24Z

If for some reason OPENSHIFT_DEFAULT_REGISTRY is not set this will still work based on IP, correct? Just want to make sure we can back out any changes in the installer at the last minute and still expect the old behavior to work.

smarterclayton · 2017-06-26T15:52:36Z

pkg/dockerregistry/server/repositorymiddleware.go

+		context.GetLogger(ctx).Infof("DEPRECATED: %q is deprecated, use the %q instead", DockerRegistryURLEnvVar, OpenShiftDefaultRegistry)
+	}
+	if len(registryAddr) == 0 {
+		if len(os.Getenv("DOCKER_REGISTRY_SERVICE_HOST")) > 0 && len(os.GetEnv("DOCKER_REGISTRY_SERVICE_PORT")) > 0 {


This is somewhat challenging in the future - we don't want to call the service "docker-registry" forever. I expect both name of service and location of service to change in the future. Also, this is something that we typically inject, vs hardcoding. However, since it's a fallback, I'm ok with it. Leave a TODO "this may change"

sure, added todo. if this change we can still use the OPENSHIFT_DEFAULT_REGISTRY to set the hostname and the port as env variable.

…-cluster address

mfojtik · 2017-06-27T07:54:36Z

compilation failure... re-[test]-ing

openshift-bot · 2017-06-27T07:55:36Z

Evaluated for origin test up to 84a6d02

mfojtik · 2017-06-27T08:33:11Z

If for some reason OPENSHIFT_DEFAULT_REGISTRY is not set this will still work based on IP, correct? Just want to make sure we can back out any changes in the installer at the last minute and still expect the old behavior to work.

Yes, this has a fallback so nothing should break. If that variable is set for registry then the registry should create image streams with that URL (which should fix the BZ?). I think there might be a change needed on ansible side where we set that var for the registry (in registry DC).

So we have to set it on two places (for master and for registry). With this PR both vars should have the same name for the sake of consistency.

mfojtik · 2017-06-27T08:58:09Z

@miminar extended test passed :-)

openshift-bot · 2017-06-27T09:50:04Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2680/) (Base Commit: 247631a) (PR Branch Commit: 84a6d02)

miminar · 2017-06-27T09:50:36Z

extended test passed :-)

because they don't utilize the dns name? 😄

mfojtik · 2017-06-27T09:59:43Z

@miminar but it proves it does not break anything ;-) i'm going to run some tests locally

openshift-bot · 2017-06-27T13:23:32Z

Evaluated for origin testextended up to 84a6d02

openshift-bot · 2017-06-27T14:16:26Z

continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/760/) (Base Commit: bbb9647) (PR Branch Commit: 84a6d02) (Extended Tests: core(registry|imageapi|ImagePrune|ImageQuota))

miminar · 2017-06-27T14:32:31Z

Ha .. I told you that extended tests need to be updated before merging #13428:

• Failure [18.035 seconds]
[Feature:ImagePrune] Image prune
/openshifttmp/openshift/build-rpm-release/tito/rpmbuild-originMSAd32/BUILD/origin-3.6.0/_output/local/go/src/github.com/openshift/origin/test/extended/images/prune.go:117
  of schema 1
  /openshifttmp/openshift/build-rpm-release/tito/rpmbuild-originMSAd32/BUILD/origin-3.6.0/_output/local/go/src/github.com/openshift/origin/test/extended/images/prune.go:75
    should prune old image [It]
    /openshifttmp/openshift/build-rpm-release/tito/rpmbuild-originMSAd32/BUILD/origin-3.6.0/_output/local/go/src/github.com/openshift/origin/test/extended/images/prune.go:74

    Expected
        <string>: application/vnd.docker.distribution.manifest.v2+json
    to equal
        <string>: application/vnd.docker.distribution.manifest.v1+json

Update: should be fixed in #14509

mfojtik · 2017-06-30T09:24:34Z

@miminar tested locally and it seems to work :-) i can see the docker-registry DNS in image streams after build/etc.

guess we are safe to merge?

mfojtik · 2017-06-30T11:52:35Z

@smarterclayton @bparees @jim-minter THIS is causing the new-app flake. See: https://gist.github.com/mfojtik/7720890674d2e7927ee00b3b25e090ca

We need this in and updated playbook to set this variable for our registry cc / @sdodson

[merge][severity:blocker]

0xmichalis · 2017-06-30T11:54:25Z

Good catch! Do we need #14509 before this is merged?

openshift-bot · 2017-06-30T11:55:29Z

Evaluated for origin merge up to 84a6d02

mfojtik · 2017-06-30T11:56:19Z

@Kargakis i don't think so

sdodson · 2017-06-30T12:11:51Z

@mfojtik just to be clear, this is all I should be setting, correct?
OPENSHIFT_DEFAULT_REGISTRY=docker-registry.default.svc:5000

mfojtik · 2017-06-30T12:14:51Z

@sdodson after this PR, yes (you have to set it for docker-registry DC, so the pod runs with this var set). You can then check the docker-registry pod log if it runs with proper registry url.

openshift-bot · 2017-06-30T12:31:37Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1185/) (Base Commit: faae9ac) (PR Branch Commit: 84a6d02) (Extended Tests: blocker) (Image: devenv-rhel7_6413)

mfojtik commented Jun 26, 2017

View reviewed changes

miminar reviewed Jun 26, 2017

View reviewed changes

mfojtik force-pushed the registry-url branch 2 times, most recently from 552dc6e to 29dc955 Compare June 26, 2017 14:46

smarterclayton reviewed Jun 26, 2017

View reviewed changes

registry: allow to override the DOCKER_REGISTRY_URL and default to in…

84a6d02

…-cluster address

mfojtik force-pushed the registry-url branch from 29dc955 to 84a6d02 Compare June 27, 2017 07:53

openshift-bot merged commit 025772c into openshift:master Jun 30, 2017

mfojtik mentioned this pull request Jun 30, 2017

Extended.[builds][Conformance] oc new-app should succeed with a --name of 58 characters #14043

Closed

dmage mentioned this pull request Oct 9, 2017

Remove test variable from docker-registry Dockerfile #14608

Closed

mfojtik deleted the registry-url branch September 5, 2018 21:08

registry: allow to override the DOCKER_REGISTRY_URL and default to in… #14882

registry: allow to override the DOCKER_REGISTRY_URL and default to in… #14882

Uh oh!

Conversation

mfojtik commented Jun 26, 2017

Uh oh!

mfojtik commented Jun 26, 2017

Uh oh!

mfojtik commented Jun 26, 2017

Uh oh!

mfojtik Jun 26, 2017

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Jun 26, 2017

Uh oh!

miminar commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miminar Jun 26, 2017

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Jun 26, 2017

Uh oh!

sdodson commented Jun 26, 2017

Uh oh!

smarterclayton Jun 26, 2017

Choose a reason for hiding this comment

Uh oh!

mfojtik Jun 27, 2017

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Jun 27, 2017

Uh oh!

openshift-bot commented Jun 27, 2017

Uh oh!

mfojtik commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfojtik commented Jun 27, 2017

Uh oh!

openshift-bot commented Jun 27, 2017

Uh oh!

miminar commented Jun 27, 2017

Uh oh!

mfojtik commented Jun 27, 2017

Uh oh!

openshift-bot commented Jun 27, 2017

Uh oh!

openshift-bot commented Jun 27, 2017

Uh oh!

miminar commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfojtik commented Jun 30, 2017

Uh oh!

mfojtik commented Jun 30, 2017

Uh oh!

0xmichalis commented Jun 30, 2017

Uh oh!

openshift-bot commented Jun 30, 2017

Uh oh!

mfojtik commented Jun 30, 2017

Uh oh!

sdodson commented Jun 30, 2017

Uh oh!

mfojtik commented Jun 30, 2017

Uh oh!

openshift-bot commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

miminar commented Jun 26, 2017 •

edited

Loading

mfojtik commented Jun 27, 2017 •

edited

Loading

miminar commented Jun 27, 2017 •

edited

Loading

openshift-bot commented Jun 30, 2017 •

edited

Loading