Fix bug where broker container name can exceed 63 characters #14964

amisevsk · 2019-10-23T14:13:48Z

What does this PR do?

Strips docker registry hostname when creating a container name for plugin brokers.

The container name for the plugin broker is derived from its docker image reference. When this reference contains a registry, it's possible for this name to exceed the 63-character limit imposed by Kubernetes,
e.g.

  my-internal-registry/my-organization/che-unified-plugin-broker:v0.20

results in a broker container named

  my-internal-registry-my-organization-che-unified-plugin-broker-v0-20

What issues does this PR fix or reference?

#14958

amisevsk · 2019-10-23T14:14:03Z

@tomgeorge Could you check that this fixes the issue?

amisevsk · 2019-10-23T14:16:12Z

Note that even with this commit, it's possible to encounter #14958 if the organization publishing the broker image has a name that exceeds 30 characters.

che-bot · 2019-10-23T14:28:53Z

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

build details
test report
logs and configs
"che-server" docker image: maxura/che-server:14964
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "crw-ci-test" to rerun the test.

che-bot · 2019-10-23T15:21:59Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:14964

amisevsk · 2019-10-23T16:08:46Z

ci-build

ibuziuk · 2019-10-23T16:10:34Z

...che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java

+    Matcher matcher = IMAGE_PATTERN.matcher(image);
+    String containerName;
+    if (matcher.matches()) {
+      containerName = String.format("%s/%s", matcher.group("org"), matcher.group("image"));
+    } else {
+      containerName = image;


will it be possible to extract this into method and cover with unit test + doc (the logic is not transparent IMO)

I've extracted the method (and simplified it to strip everything but image name) and added tests.

tomgeorge · 2019-10-23T20:13:17Z

containers will start with this fix

sleshchenko

LGTM
Please consider addressing Illya's comment

The container name for the plugin broker is derived from its docker image reference. When this reference contains a registry, it's possible for this name to exceed the 63-character limit imposed by Kubernetes, e.g. my-internal-registry/my-organization/che-unified-plugin-broker:v0.20 results in a broker container named my-internal-registry-my-organization-che-unified-plugin-broker-v0-20 This commit removes the registry hostname from the container name and keeps the conversion the same. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk · 2019-10-24T14:21:53Z

@sleshchenko Ilya and I discussed in standup that it's probably safer to just remove everything but image name (since a long organization name could still cause the issue) -- could you reapprove the new logic?

che-bot · 2019-10-24T14:31:09Z

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

build details
logs and configs
"che-server" docker image: maxura/che-server:14964

sleshchenko

still LGTM

che-bot · 2019-10-24T15:11:38Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:14964

ibuziuk · 2019-10-24T15:43:36Z

...che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java

+    // There's a chance the full image reference may be over the name limit of 63 chars
+    // so we need to remove registry hostname


@amisevsk this comment looks obsolete

or maybe it meant to be added to the generateContainerNameFromImageRef doc?

Moved docs to generateContainerNameFromImageRef

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

ibuziuk

👍 LGTM

che-bot · 2019-10-24T16:02:21Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

Build details
Test report
docker image: eclipseche/che-server:14964
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "ci-test" to rerun the test.

che-bot · 2019-10-24T16:03:53Z

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

build details
logs and configs
"che-server" docker image: maxura/che-server:14964

amisevsk · 2019-10-24T16:28:56Z

ci-test

amisevsk · 2019-10-24T16:29:12Z

ci-build

che-bot · 2019-10-24T16:34:40Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

Build details
Test report
docker image: eclipseche/che-server:14964
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "ci-test" to rerun the test.

ibuziuk · 2019-10-25T10:50:36Z

ci-build

ibuziuk · 2019-10-25T12:02:54Z

12:54:23 [ERROR] Failed to execute goal com.coveo:fmt-maven-plugin:2.5.1:check (default) on project infrastructure-kubernetes: Found 1 non-complying files, failing build -> [Help 1]
12:54:23 [ERROR] 
12:54:23 [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
12:54:23 [ERROR] Re-run Maven using the -X switch to enable full debug logging.
12:54:23 [ERROR] 
12:54:23 [ERROR] For more information about the errors and possible solutions, please read the following articles:
12:54:23 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
12:54:23 [ERROR] 
12:54:23 [ERROR] After correcting the problems, you can resume the build with the command
12:54:23 [ERROR]   mvn <goals> -rf :infrastructure-kubernetes

@amisevsk looks like formatting needs to be fixed ;)

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

che-bot · 2019-10-25T12:58:58Z

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

build details
logs and configs
"che-server" docker image: maxura/che-server:14964

benoitf · 2019-10-25T13:05:04Z

...workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactoryTest.java

+      {"very-long-registry-hostname-url.service/eclipse/image:tag", "image-tag"},
+      {"eclipse/che-unified-plugin-broker:v0.20", "che-unified-plugin-broker-v0-20"},
+      {"very-long-organization.name-eclipse-che/image:tag", "image-tag"},
+      {"very-long-registry-hostname-url.service/very-long-organization/image:tag", "image-tag"}


different images from different registries or users will have same name

It looks odd when you debug if there is no easy way to know from where it could come

You could always get the full image ref from the pod or env var though. There's no way to include registry and organization in the name, since it very easily goes over the 63 char limit (the length of just the broker pod name + version is 32 chars already).

I never talked on including the full registry or org name
but I think it's like some logger are doing for packages
instead of seeing org.apache.catalina.something you see o.a.c.something

In logger of Tomcat I see for example o.a.c.startup.VersionLoggerListener
so I know more from where is coming the VersionLoggerListener class than if only VersionLoggerListener was displayed

So I would say you could do something similar, it's for UX.

and of course I could go to openshift webconsole or somewhere else but it's definitely not UX

quay.io/my-organization/che-unified-plugin-broker:v0.20 --> q-m-che-unified-plugin-broker-v0-20

docker.io/my-organization/che-unified-plugin-broker:v0.20 --> d-m-che-unified-plugin-broker-v0-20

docker.io/other-organization/che-unified-plugin-broker:v0.20 --> d-o-che-unified-plugin-broker-v0-20

@benoitf this sounds like a relevant improvement, but is it really that valuable?

I think q-m-che-unified-broker-image-v0-20 and d-m-che-unified-plugin-broker-v0-20 are less readable than che-unified-plugin-broker-v0-20. Even with the truncation, it's an external reference (whereas logger trimming is referring to internal classes), so you'll still have to ask "what image is this/where is it hosted/what is it built from?" A single m character doesn't tell us anything real about the actual organization it's pushed to. I care more about what job the broker has (init vs unified), and what version it comes from (since e.g. v0.19 brokers don't support offline).

It seems i'm the only one to care UX or that it's just a hot fix of removing repo/org and not fixing the 63 limit

If we know that we can't reach more than 63 values should never return more than 63 characters

@amisevk. You may use other prefix it was an example to illustrate that you may care repo/org more than full image name as well

I won't block but to me it's not a fix of the issue. Just hot fix

Also speaking about UX I think issue is that we're giving each image name independently and not a set of images so the logic can work only on a limited data while with all images it could dedup in a better format all images

che-bot · 2019-10-25T13:46:16Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:14964

ibuziuk · 2019-10-25T13:49:02Z

@dmytro-ndp @vkuznyetsov @rhopp please provide QA approval

dmytro-ndp

E2E Java selenium tests against OCP and Happy path tests against kubernetes have passed.

benoitf · 2019-10-25T16:19:56Z

What happens for my image like:

https://hub.docker.com/r/florentbenoit/thisisaverylongcontainernamewithanimageitrytomakeimagenamelongerthan63characters/tags

$ docker pull florentbenoit/thisisaverylongcontainernamewithanimageitrytomakeimagenamelongerthan63characters:thisisaverylongtagwithanimageitrytomakeimagenamelongerthan63characters

(if this image is a broker)

ibuziuk · 2019-10-25T17:06:56Z

@benoitf isn't it an edge case?

benoitf · 2019-10-25T17:13:50Z

I don't know, issue is about 63characters limit but PR is just a workaround by stripping org/repo

ibuziuk · 2019-10-25T17:24:05Z

issue is about 63characters limit but PR is just a workaround by stripping org/repo

well, dropping the org,repo makes the possibility of hitting 63 chars limit utterly unlike. unless you use smth like #14964 (comment)
Do you insist on PR rework or is it good enough for 7.3.1 ?

ibuziuk · 2019-10-28T11:50:51Z

Merging to master. @benoitf @amisevsk we can create a separate issue for improving the generateContainerNameFromImageRef method

ibuziuk · 2019-10-28T15:09:29Z

@amisevsk could you please backport it to 7.3.x ?

amisevsk requested review from metlos and sleshchenko October 23, 2019 14:13

amisevsk requested review from l0rd, nickboldt and rhopp as code owners October 23, 2019 14:13

che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Oct 23, 2019

ibuziuk reviewed Oct 23, 2019

View reviewed changes

tomgeorge mentioned this pull request Oct 23, 2019

Set CHE_DEVFILE_REGISTRY_URL to allow airgap operation eclipse-che/che-operator#102

Merged

sleshchenko approved these changes Oct 24, 2019

View reviewed changes

l0rd mentioned this pull request Oct 24, 2019

Release Che v7.3.1 #14947

Closed

30 tasks

amisevsk added 2 commits October 24, 2019 10:18

Fixup: Extract name generation to method and add tests

fd98a46

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk force-pushed the broker-name-length branch from d7bfb3b to fd98a46 Compare October 24, 2019 14:19

sleshchenko approved these changes Oct 24, 2019

View reviewed changes

ibuziuk reviewed Oct 24, 2019

View reviewed changes

fixup: Move some documentation around

8790908

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

ibuziuk approved these changes Oct 24, 2019

View reviewed changes

fixup: formatting

edec4e9

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

benoitf reviewed Oct 25, 2019

View reviewed changes

ibuziuk requested review from dmytro-ndp and vkuznyetsov October 25, 2019 13:49

dmytro-ndp approved these changes Oct 25, 2019

View reviewed changes

ibuziuk merged commit 75b95d5 into eclipse-che:master Oct 28, 2019

che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Oct 28, 2019

che-bot added this to the 7.4.0 milestone Oct 28, 2019

amisevsk mentioned this pull request Oct 28, 2019

Fix bug where broker container name can exceed 63 characters #15004

Merged

		// There's a chance the full image reference may be over the name limit of 63 chars
		// so we need to remove registry hostname

Fix bug where broker container name can exceed 63 characters #14964

Fix bug where broker container name can exceed 63 characters #14964

Conversation

amisevsk commented Oct 23, 2019

What does this PR do?

What issues does this PR fix or reference?

amisevsk commented Oct 23, 2019

amisevsk commented Oct 23, 2019

che-bot commented Oct 23, 2019

che-bot commented Oct 23, 2019

amisevsk commented Oct 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomgeorge commented Oct 23, 2019

sleshchenko left a comment

Choose a reason for hiding this comment

amisevsk commented Oct 24, 2019

che-bot commented Oct 24, 2019

sleshchenko left a comment

Choose a reason for hiding this comment

che-bot commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuziuk left a comment

Choose a reason for hiding this comment

che-bot commented Oct 24, 2019

che-bot commented Oct 24, 2019

amisevsk commented Oct 24, 2019

amisevsk commented Oct 24, 2019

che-bot commented Oct 24, 2019

ibuziuk commented Oct 25, 2019

ibuziuk commented Oct 25, 2019

che-bot commented Oct 25, 2019

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

che-bot commented Oct 25, 2019

ibuziuk commented Oct 25, 2019

dmytro-ndp left a comment

Choose a reason for hiding this comment

benoitf commented Oct 25, 2019 • edited Loading

ibuziuk commented Oct 25, 2019

benoitf commented Oct 25, 2019

ibuziuk commented Oct 25, 2019

ibuziuk commented Oct 28, 2019

ibuziuk commented Oct 28, 2019

benoitf commented Oct 25, 2019 •

edited

Loading