Store the container-machine mapping predictably #13858

metlos · 2019-07-16T10:11:20Z

What does this PR do?

Store the container-machine mapping using a pair of annotations with a
predictable name length to prevent breaking the 63 character limit on the
k8s annotation names.

What issues does this PR fix or reference?

#13303

predictable name length to prevent breaking the 63 character limit on the k8s annotation names. Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

tsmaeder · 2019-07-16T11:29:26Z

What makes the machine name size predictable? To me it looks like it's still a function of the name of the container.

metlos · 2019-07-16T12:12:20Z

It's not the machine name that has a predictable size - as you correctly note, it is a function of the container name.

Before, we stored the mapping from the container name to the machine name using a single annotation that included the container name in its key. E.g.:

org.eclipse.che.container.vscode-kubernetes-tools.machine_name=blah

As you see above, the length of the annotation key is unpredictable and depends on the name of the container.

This PR changes the mapping into 2 annotations:

che.container.1.name=vscode-kubernetes-tools
che.container.1.machine=blah

And contains additional logic to increment the "1" in the above names for additional mappings if the pod contains more containers.

The length of the annotation keys is thus predictable and it is hard to imagine it going past the 63 character limit because that would require there to be more than 10^43 containers in a single pod.

Also the PR changes the code in all the places that used to read the machine name from the annotations directly to delegate that to the methods in Names so that the above more complex logic is applied.

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

metlos · 2019-07-16T13:22:32Z

ci-test

che-bot · 2019-07-16T16:36:47Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:13858
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

amisevsk

I'm personally not a huge fan of this approach -- it adds a fair bit of complexity and looks like a hack at the end of the day.

If the machine name annotation is never used, then why don't we remove it? It makes more sense to add the logic for how this annotating is done once something actually needs to use it.

Alternatively, changing MACHINE_NAME_ANNOTATION_FORMAT to org.eclipse.che.machine-name/%s would solve the problem for any container with a name less than 63 characters since everything before / is a prefix.

Added a few optional suggestions.

@davidfestal Just to double check but does this affect your operator work?

amisevsk · 2019-07-17T13:51:14Z

...ures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Names.java

+      return null;
+    }
+
+    for (Map.Entry<String, String> e : annotations.entrySet()) {


This could likely be made a lot simpler and more readable in a stream:

annotations.entrySet() .stream() .filter(e -> e.getKey().startsWith(CONTAINER_META_PREFIX)) .filter(e -> e.getKey().endsWith(CONTAINER_META_NAME_SUFFIX)) .filter(e -> containerName.equals(e.getValue())) .map(...) .collect(...)

amisevsk · 2019-07-17T13:55:36Z

...ures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Names.java

+
+      String index =
+          key.substring(
+              CONTAINER_META_PREFIX.length(), key.length() - CONTAINER_META_NAME_SUFFIX.length());


A cleaner way to do this would be to match it out using a regex based off of prefix and suffix.

Call me old school, but I fail to see how is that cleaner... If you insist, I can change it, but substring is IMHO both faster and functionally simpler than regex matching.

amisevsk · 2019-07-17T13:56:43Z

...ures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Names.java

+
+  private static final String CONTAINER_META_PREFIX = "che.container.";
+  private static final String CONTAINER_META_NAME_SUFFIX = ".name";
+  private static final String CONTAINER_META_MACHINE_SUFFIX = ".machine";


To me it looks better if you use

CONTAINER_META_NAME = "che.container.%s.name"; CONTAINER_META_MACHINE = "che.contianer.%s.machine";

an then String.format() the indexes in.

According to the casual Google search, format is at least an order of magnitude slower than concatenation. I would 100% agree with you if these were localizable strings, but they're not.

amisevsk · 2019-07-17T14:10:25Z

...ures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Names.java

+      return max;
+    }
+
+    for (String key : annotations.keySet()) {


Could also be a stream operation, something like:

annotations.keySet() .stream() .mapToInt() .max()

davidfestal · 2019-07-17T16:26:32Z

@davidfestal Just to double check but does this affect your operator work?

I also don't like the numbering very much. That means that you have to know the final number of containers at start, before generating the deployment annotations but obviously after doing brokering. In the Workspace CRD controller POC that's not the case for example. We add containers in the overall workspace deployment PodSpec as soon as a plugin component is found in the devfile and brokering is called on-the-fly in the controller itself for this component.

I really like better the alternate approach @amisevsk proposed:

changing MACHINE_NAME_ANNOTATION_FORMAT to org.eclipse.che.machine-name/%s

sleshchenko

Have you investigated the possibility of getting rid of a machine name in annotations?

Generally, I'm OK with the proposed solution since it solves an issue and looks like we would be able to get rid of this quite complex logic along with WorkspaceConfig format (maybe current Runtime model)

Comment about org.eclipse.che.machine-name/%s:
org.eclipse.che.machine-name/ has 29 characters and it would mean that we should limit machine name to 34 characters and everything that propagated as machine name, like alias of dockerimage component, also we should introduce algorithm of how we transform image of dockerimage component to machine name (now it does not have limit of characters). Machine name concept is a core concept and no one knows every place that is affected by this. I think it can be investigated more, and it's not equivalent to the current approach that allows using unlimited (quite long) machine name.

sleshchenko · 2019-07-18T07:55:49Z

...ures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Names.java

-    if (annotations != null
-        && (machineName = annotations.get(format(MACHINE_NAME_ANNOTATION_FMT, containerName)))
-            != null) {
+    if ((machineName = findMachineName(annotations, containerName)) != null) {


Please consider updating of java docs as well.
I don't see any java docs that would explain how we store machines names in annotations. I guess, previously java doc of MACHINE_NAME_ANNOTATION_FMT const has some short info.

sleshchenko · 2019-07-18T07:59:24Z

...ures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Names.java

@@ -98,4 +162,83 @@ public static String uniqueResourceName(String originalResourceName, String work
  public static String generateName(String prefix) {
    return NameGenerator.generate(prefix, GENERATED_PART_SIZE);
  }
+
+  private static String findMachineName(Map<String, String> annotations, String containerName) {


Please add @nullable

sleshchenko · 2019-07-18T08:00:46Z

...ures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Names.java

+    for (Map.Entry<String, String> e : annotations.entrySet()) {
+      String key = e.getKey();
+
+      if (!key.startsWith(CONTAINER_META_PREFIX)) {


I see quite the same logic in findContainerIndex method, maybe we can rewrite this method in the following way to reduce the same code:

@Nullable private static String findMachineName(Map<String, String> annotations, String containerName) { int index = findContainerIndex(annotations, containerName); if (index < 0) { return null; } String machineNameKey = CONTAINER_META_PREFIX + index + CONTAINER_META_MACHINE_SUFFIX; return annotations.get(machineNameKey); }

sleshchenko · 2019-07-18T08:12:14Z

...ures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/Names.java

+    }
+
+    for (String key : annotations.keySet()) {
+      if (!(key.startsWith(CONTAINER_META_PREFIX) && key.endsWith(CONTAINER_META_NAME_SUFFIX))) {


What do you think about using Pattern instead of startsWith+endsWith+substring?
Like the following
https://gist.github.com/sleshchenko/f2190306fdd7c9dc2cbb071bc22d64e0

metlos · 2019-07-18T12:24:49Z

tldr; Yes, it is a hack put in place to fix the current wrong behavior with minimal impact on the codebase. The hopes are the need for the mapping goes away once we change the k8s infra internals that are still implemented around the old domain model.

Have you investigated the possibility of getting rid of a machine name in annotations?

The Names.machineName(*) methods have 14 usages all over the k8s infrastructure (and 1 on openshift infra).

I looked at them while investigating this and, mostly, the mapping is used to map the old model of servers/installers/machines that we still use internally to the actual k8s deployment. As such, I decided to not try to get rid of this mapping because it would be too risky at this point in time.

changing MACHINE_NAME_ANNOTATION_FORMAT to org.eclipse.che.machine-name/%s

while this would improve the situation greatly, it still imposes a limit that we don't enforce anywhere. So I chose an approach that doesn't impose that limit again to keep the impact of the change to the minimum. If I merely switched to the format you suggest, I'd have to also modify all the places that result in definition of a machine name (devfile appliers, factory, workspace config, ...) and add the length validation.

That said, if we agree here that such validation wouldn't be needed (to limit the scope of the change), I am more than happy to apply that format. I like it better, too.

That means that you have to know the final number of containers at start, before generating the deployment annotations but obviously after doing brokering.

I believe that's not the case. At least in the current implementation, no outside caller knows anything about any numbering. The numbering is a mere implementation choice of the Names class used to "bind" the two annotations together.

davidfestal · 2019-07-18T13:15:44Z

That means that you have to know the final number of containers at start, before generating the deployment annotations but obviously after doing brokering.

I believe that's not the case. At least in the current implementation, no outside caller knows anything about any numbering. The numbering is a mere implementation choice of the Names class used to "bind" the two annotations together.

@metlos I'm speaking with the Workspace CRD Operator POC in mind: it creates the various Che workspace K8S resources without using the internal k8s infrastructure implementation. It uses its own logic implementation to provide compatible K8S resources. And in this context, using this type of numbering seems more a backward step than a forward one, and makes it more complicated.

That said, I just wanted to notify you, but that's not a blocker, I'll accommodate.

metlos · 2019-07-18T13:41:24Z

That said, I just wanted to notify you, but that's not a blocker, I'll accommodate.

Ok, I see what you mean now. You want to keep compatibility between workspaces created using the CRD and che server. I didn't take that into account actually. Not sure we need to be backwards compatible here, though. I assume you don't need these annotations in your implementation at all?

davidfestal · 2019-07-18T13:54:33Z

That said, I just wanted to notify you, but that's not a blocker, I'll accommodate.

Ok, I see what you mean now. You want to keep compatibility between workspaces created using the CRD and che server. I didn't take that into account actually. Not sure we need to be backwards compatible here, though?

For various reason, I think we should keep compatibility between both, at least for a certain amount of time. But as I said, I'll accommodate on the Workspace CRD side if you don't have any better solution for now on your side.

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

amisevsk · 2019-07-18T14:31:08Z

@sleshchenko For annotations, a up-to-256-char prefix is permitted (the / delimits this), so it would give the full 63 chars for the machine name. See: https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#syntax-and-character-set

I agree on it still imposing a limit we don't enforce, however.

…ions-too-long Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

metlos · 2019-07-19T15:30:49Z

I reviewed all the involved code again and I think we can reasonably assume that the container can always be < 63 chars. The plugins/editors already have that limitation in place (or rather they just trim the name to be < 63 chars) and for dockerimages I added a validation check that will fail the deployment of a devfile and will suggest to adding an alias for such component. The kubernetes/openshift components don't seem to be involved in machine naming at all (or at least they don't manifest themselves in the workspace pod annotations).

Thanks for the suggestion @amisevsk!

amisevsk

LGTM, thanks @metlos !

sleshchenko · 2019-07-22T07:48:59Z

.../che/workspace/infrastructure/kubernetes/devfile/DockerimageComponentToWorkspaceApplier.java

@@ -257,6 +257,14 @@ static String toMachineName(String imageName) throws DevfileException {
      return imageName;
    }

+    if (imageName.length() > Names.MAX_CONTAINER_NAME_LENGTH) {


Isn't it something that can be checked by JSON Schema?

Well, we can't really restrict the names of the images. What would have to happen is conditional check that would mark alias required if the length of the image name would be > 63. Maybe it is doable in the schema but we have to have the check in the code anyway. So for simplicity's sake I only did it in the code. I believe the PR is writable by contributors so please add the schema changes if required (I don't have access to my computer this week).

sleshchenko

LGTM

metlos · 2019-07-22T08:27:03Z

ci-test

che-bot · 2019-07-22T08:29:45Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:13858
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

metlos · 2019-07-22T08:41:52Z

ci-test

che-bot · 2019-07-22T12:02:57Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:13858
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2019-07-23T14:38:20Z

I've retest locally the same stack (go) that failed for ci and it works for me. Will try to rerun on ci, maybe it's like a random failure.

sleshchenko · 2019-07-23T14:38:25Z

ci-test

che-bot · 2019-07-23T15:28:30Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:13858
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

nickboldt

+0. Others have already +1'd and I'm clearing my PR review backlog. No opinion as I'm not SME here.

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

sleshchenko · 2019-07-24T07:48:26Z

ci-test

che-bot · 2019-07-24T08:33:57Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:13858
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

artaleks9 · 2019-07-24T10:11:52Z

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/1941/) doesn't show any regression against this Pull Request.

Store the container-machine mapping using a pair of annotations with a

f2e1b61

predictable name length to prevent breaking the 63 character limit on the k8s annotation names. Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

metlos requested review from amisevsk, l0rd, nickboldt, rhopp and sleshchenko as code owners July 16, 2019 10:11

Add a simple test for reading out the machine name from the annotations.

4fdc56a

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

amisevsk reviewed Jul 17, 2019

View reviewed changes

sleshchenko reviewed Jul 18, 2019

View reviewed changes

Replace for loops with streams. Make the code a little bit more DRY.

ca36b81

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

metlos added 3 commits July 19, 2019 10:12

Merge remote-tracking branch 'upstream/master' into bug/13303-annotat…

6508848

…ions-too-long Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

Simplify the impl by making sure we don't exceed the 63 char limit.

0fc49b4

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

Improve the error message wording a little bit.

3ddce7b

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

amisevsk approved these changes Jul 19, 2019

View reviewed changes

sleshchenko reviewed Jul 22, 2019

View reviewed changes

sleshchenko approved these changes Jul 22, 2019

View reviewed changes

davidfestal approved these changes Jul 22, 2019

View reviewed changes

nickboldt approved these changes Jul 23, 2019

View reviewed changes

Fix setting annotations for jwt-proxy pod

8e681cf

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

sleshchenko merged commit e598e22 into eclipse-che:master Jul 24, 2019

sleshchenko deleted the bug/13303-annotations-too-long branch July 24, 2019 10:59

Store the container-machine mapping predictably #13858

Store the container-machine mapping predictably #13858

Conversation

metlos commented Jul 16, 2019

What does this PR do?

What issues does this PR fix or reference?

tsmaeder commented Jul 16, 2019

metlos commented Jul 16, 2019

metlos commented Jul 16, 2019

che-bot commented Jul 16, 2019

amisevsk left a 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

davidfestal commented Jul 17, 2019

sleshchenko left a 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

metlos commented Jul 18, 2019

davidfestal commented Jul 18, 2019

metlos commented Jul 18, 2019 • edited Loading

davidfestal commented Jul 18, 2019

amisevsk commented Jul 18, 2019

metlos commented Jul 19, 2019

amisevsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleshchenko left a comment

Choose a reason for hiding this comment

metlos commented Jul 22, 2019

che-bot commented Jul 22, 2019

metlos commented Jul 22, 2019

che-bot commented Jul 22, 2019

sleshchenko commented Jul 23, 2019

sleshchenko commented Jul 23, 2019

che-bot commented Jul 23, 2019

nickboldt left a comment

Choose a reason for hiding this comment

sleshchenko commented Jul 24, 2019

che-bot commented Jul 24, 2019

artaleks9 commented Jul 24, 2019

metlos commented Jul 18, 2019 •

edited

Loading