Add an ability to try Workspace.Next features #9774

garagatyi · 2018-05-22T14:31:04Z

This PR is designed to be reviewed in a commit by commit manner.

It is in WIP state because not all the unit tests are finished.

What does this PR do?

Add an ability to try Workspace.Next features.
To try it out user needs to add an attribute to a workspace with name features and a list of features as a value.
An example of value: "org.eclipse.che.che-theia-hello/0.0.1,org.eclipse.che.che-theia-ssh/0.0.1"
This PR contains code to use Workspace.Next with the kubernetes recipe only.
Support of other recipes should be added additionally.

What issues does this PR fix or reference?

Release Notes

Docs PR

sleshchenko

In general, looks good to me 👍
I'll clone your changes locally and review in details.

sleshchenko · 2018-05-23T07:11:41Z

...-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java

+   * Should be set/read from {@link Workspace#getAttributes}.
+   *
+   * <p/>Value is comma separated list of features in a format:
+   * '< feature1Name >/< feature1Version >,< feature2Name >/< feature2Version >'


Why is / symbol chosen as a separator of name and version? Wouldn't : symbol more conventional?

It is better to ask @skabashnyuk - it is Feature API that declares format.

The format is still under discussion. I do like : Fill free to comment. #9895

Looks like examples specified by the shared link contain : as separator but Feature API is implemented with /

sleshchenko · 2018-05-23T07:19:43Z

...src/main/java/org/eclipse/che/api/workspace/server/wsnext/WorkspaceNextObjectsRetriever.java

+        LOG.error(
+            "Response doesn't contain any data, though it was expected to contain some.");
+        throw new IOException(
+            "Internal server error. Unexpected response body received.");


I would say that empty response body would be more accurate message.

Does message Response contains an empty body, though it was expected to contain data looks better to you?

If the message will contain that this response is from "Feature API" then it would be even more clear and helpful.

sleshchenko · 2018-05-23T07:26:37Z

deploy/kubernetes/kubectl/che-kubernetes.yaml

@@ -270,6 +270,8 @@ items:
              configMapKeyRef:
                key: CHE_INFRA_KUBERNETES_SERVER__STRATEGY
                name: che
+          - name: CHE_WORKSPACE_FEATURE_API
+            value: "http://feature-api-service:3000"


Wouldn't it better to move a value to the config map?

Agree, will update this

sleshchenko

Good job. Please ping me one more time when PR won't be WIP.

sleshchenko · 2018-05-23T11:47:33Z

...src/main/java/org/eclipse/che/api/workspace/server/wsnext/WorkspaceNextObjectsRetriever.java

+            "Features format is illegal. Problematic feature entry:" + feature);
+      }
+      String key = featureAndVersion[0] + '/' + featureAndVersion[1];
+      if (featuresNameVersion.containsKey(key)) {


I guess you can use feature here instead of contacting separated values.

sleshchenko · 2018-05-23T11:57:57Z

...src/main/java/org/eclipse/che/api/workspace/server/wsnext/WorkspaceNextObjectsRetriever.java

+          String parameterKey =
+              serviceName + "/" + serviceVersion + "/${" + cheServiceParameter.getName() + "}";
+          parameters.putIfAbsent(parameterKey, new ArrayList<>());
+          List<String> cheServiceParameters = parameters.get(parameterKey);


parameters.putIfAbsent(parameterKey, new ArrayList<>()); List<String> cheServiceParameters = parameters.get(parameterKey);

May be rewritten in Java 8 style

List<String> cheServiceParameters = parameters.computeIfAbsent(parameterKey, (key) -> new ArrayList<>());

sleshchenko · 2018-05-23T11:59:19Z

...src/main/java/org/eclipse/che/api/workspace/server/wsnext/WorkspaceNextObjectsRetriever.java

+
+  /**
+   * Returns map where key is concatenation of service name, version and parameter name separated by slash symbol.
+   * < serviceName/serviceVersion/parameter > to < parameterValue >


Looks like it's outdated format since the format is < serviceName/serviceVersion/${parameter} >. Am I right?

yes, thanks for pointing it out

sleshchenko · 2018-05-23T12:03:46Z

...src/main/java/org/eclipse/che/api/workspace/server/wsnext/WorkspaceNextObjectsRetriever.java

+                  String envVarValue = envVar.getValue();
+                  String parameterKey = serviceKey + '/' + envVarValue;
+                  List<String> remove = parameters.remove(parameterKey);
+                  if (remove != null) {


Just idea to discuss, maybe we should set empty value if there is not the corresponding parameter value in a feature?

maybe, I'm not sure here

sleshchenko · 2018-05-23T12:06:39Z

...src/main/java/org/eclipse/che/api/workspace/server/wsnext/WorkspaceNextObjectsRetriever.java

+      // for now we match whole env variable value against '${<parameter name>}'
+      service.getSpec()
+          .getContainers()
+          .forEach(container -> container.getEnv()


Maybe it would be better(maybe not =)) to rewrite it in the following manner

service.getSpec() .getContainers() .stream() .flatMap(c -> c.getEnv().stream()) .forEach(envVar -> {...});

Right now this code throws an exception, so I can't do like that. Actually, this code looked like that before)

sleshchenko · 2018-05-23T12:30:45Z

...orkspace/src/main/java/org/eclipse/che/api/workspace/server/wsnext/WorkspaceNextApplier.java

+/**
+ * Applies Workspace.Next configuration to an internal runtime object that represents workspace
+ * runtime configuration on an infrastructure level.
+ *


I think it may be helpful to add a note that implementations must be bound with map binder and maybe add an example of binding.

Not sure that class should know what DI container is used and declare it. I'll think about it.

sleshchenko · 2018-05-23T12:33:02Z

deploy/kubernetes/kubectl/che-kubernetes.yaml

@@ -270,6 +270,8 @@ items:
              configMapKeyRef:
                key: CHE_INFRA_KUBERNETES_SERVER__STRATEGY
                name: che
+          - name: CHE_WORKSPACE_FEATURE_API


Maybe it makes sense to add the same configuration into OS deployment

I didn't check workspace next on OS infra, so don't want to add it for the time

sleshchenko · 2018-05-23T12:37:00Z

...g/eclipse/che/workspace/infrastructure/kubernetes/wsnext/KubernetesWorkspaceNextApplier.java

+  @Override
+  public void apply(InternalEnvironment internalEnvironment, Collection<CheService> cheServices)
+      throws InfrastructureException {
+    KubernetesEnvironment kubernetesEnvironment = (KubernetesEnvironment) internalEnvironment;


It makes sense to move this line lower since it is not needed to cast environment if Che services list is empty

ur right, thanks

sleshchenko · 2018-05-23T12:39:41Z

...g/eclipse/che/workspace/infrastructure/kubernetes/wsnext/KubernetesWorkspaceNextApplier.java

+    io.fabric8.kubernetes.api.model.Container container =
+        new ContainerBuilder()
+            .withImage(image)
+            .withName(Names.generateName(""))


Maybe it makes sense to add some meaningful prefix like "tooling"

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Add missing override annotations. Cleanup the code. Use existing getters instead of a field to prevent bugs with inheritance. Move public methods to the beggining of classes. Fix incorrect javadocs and misleading parameters names. Add a comment that explains why a compiler warning should be ignored. Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

…l environment Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi · 2018-06-04T12:08:14Z

@sleshchenko I've updated PR with fixes, unit tests and rebased it against master to resolve conflicts. Please, take a look one more time.

@l0rd @skabashnyuk Looks like this PR is not successful in acquiring new code reviewers, so I'll merge it as soon as @sleshchenko approves it unless other reviews appear.

sleshchenko

Good job 👍

sleshchenko · 2018-06-04T13:01:08Z

...src/main/java/org/eclipse/che/api/workspace/server/wsnext/WorkspaceNextObjectsRetriever.java

+    }
+  }
+
+  private void validateFeature(CheFeature feature, String name, String version)


Just proposal. Maybe it would be better to move validation private methods to new Validator class to make this class a bit smaller.

I would not want to create a separate class for that. At least for now.

sleshchenko · 2018-06-04T13:03:11Z

...src/main/java/org/eclipse/che/api/workspace/server/wsnext/WorkspaceNextObjectsRetriever.java

+    }
+  }
+
+  private void requireEqual(String version, String version1, String error, String... errorArgs)


I guess you used Refactor functionality and it's why the arguments are named version and version1. Am I right?

Looks like, I don't really remember, to be honest.

sleshchenko · 2018-06-04T13:11:39Z

...g/eclipse/che/workspace/infrastructure/kubernetes/wsnext/KubernetesWorkspaceNextApplier.java

+      return;
+    }
+    KubernetesEnvironment kubernetesEnvironment = (KubernetesEnvironment) internalEnvironment;
+    Map<String, Pod> pods = ((KubernetesEnvironment) internalEnvironment).getPods();


Looks like you don't need to cast it for the second time and you can reuse kubernetesEnvironment

sleshchenko · 2018-06-04T13:15:33Z

...lipse/che/workspace/infrastructure/kubernetes/wsnext/KubernetesWorkspaceNextApplierTest.java

+
+
+  @Mock
+  Pod pod;


Looks like unformatted. Please don't forget to execute mvn fmt:format before merge.
Also, I would make these variable private. It doesn't really matter while it is test class but still.

sleshchenko · 2018-06-04T13:16:35Z

deploy/kubernetes/kubectl/wsnext/feature-api.yaml

+      app: feature-api
+    name: feature-api
+  data:
+    CHE_REGISTRY_UPDATE_INTERVAL: "60"


Is it value in seconds?

sleshchenko · 2018-06-04T13:24:02Z

deploy/kubernetes/kubectl/wsnext/feature-api.yaml

@@ -0,0 +1,85 @@
+---


It would be nice to have few words in Deploy Che.md about deploying feature-api

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi · 2018-06-04T14:18:26Z

ci-build

codenvy-ci · 2018-06-04T18:41:15Z

Build # 4548 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4548/ to view the results.

l0rd

Sorry my review comes really late. That's a great PR and the code is clean and easy to read/review but I think we should still discuss if a Container should have a list of Servers.

l0rd · 2018-06-05T01:00:00Z

deploy/kubernetes/kubectl/Deploy Che.md

@@ -33,3 +33,7 @@ and clean redeploy of Che.
 - Create namespace `che`: `kubectl create namespace che`
 - Deploy Che: `kubectl --namespace=che apply -f che-kubernetes.yaml`
 - Check Che pod status until it become `Running`: `kubectl get --namespace=che pods`
+
+### Workspace Next:
+There is a file `wsnext/feature-api.yaml` which contains kubesrb service needed to test Workspace Next on kubernetes infrastructure.


You may want to s/kubesrb/kubesrv/

l0rd · 2018-06-05T01:15:42Z

...api-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsnext/model/Container.java

+  private List<EnvVar> env = new ArrayList<EnvVar>();
+  private ResourceRequirements resources = null;
+  private List<Command> commands = new ArrayList<Command>();
+  private List<Server> servers = new ArrayList<Server>();


I don't remember that Container had a list of servers as an attribute in the original model. And I don't think that we should do that:

A CheService that has a list of Container and each Container has a list of Servers is something difficult to understand (what's the difference between a CheService and a Server?)

Moreover one of workspace.next goals was to extract the list servers that originally ran in on container into multiple containers (sidecars). So it doesn't make sense to have a list of Server for each Container.

I would suggest to move higher (at CheService level) the attributes of the Server.

I clearly see servers field in one of the model versions, in particular, #9292 (comment).

what's the difference between a CheService and a Server

As we discussed before usage of Service word is confusing and brings misunderstanding like this one.

Moreover one of workspace.next goals was to extract the list servers that originally ran in on container into multiple containers (sidecars). So it doesn't make sense to have a list of Server for each Container.

It make sense. We should, probably, discuss it with @skabashnyuk.

I would suggest to move higher (at CheService level) the attributes of the Server.

I don't own Workspace.Next model, so I would discuss it with Sergii first. In any case I think it makes sense to do that in a separate PR.

l0rd · 2018-06-05T01:17:51Z

...api-workspace/src/main/java/org/eclipse/che/api/workspace/server/wsnext/model/Container.java

+import java.util.List;
+import java.util.Objects;
+
+public class Container {


Wouldn't it be more consistent to call it CheContainer (as CheService or CheFeature)? In the issue where the model is described it is called Container but that's weird. WDYT?

Maybe. I reused already generated files by Swagger model because I don't want to bring new changes that I would have to redo after regeneration of the model classes.
So, I believe, it is rather a suggestion for @skabashnyuk

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi · 2018-06-05T07:02:30Z

ci-build

codenvy-ci · 2018-06-05T13:58:38Z

Build # 4549 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4549/ to view the results.

riuvshin · 2018-06-05T14:00:04Z

ci-build

codenvy-ci · 2018-06-05T14:45:09Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4550/

garagatyi requested review from l0rd, davidfestal, ibuziuk, skabashnyuk, sleshchenko and a user May 22, 2018 14:31

garagatyi requested a review from riuvshin as a code owner May 22, 2018 14:31

benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label May 22, 2018

sleshchenko reviewed May 23, 2018

View reviewed changes

This was referenced May 30, 2018

Load Theia in Workspace.Next workspaces #9870

Closed

Make Workspace.Next work with Dockerimage recipe type #9871

Closed

Oleksandr Garagatyi added 7 commits June 4, 2018 14:48

Cleanup code of unrecoverable events in k8s/OS infras

95d2bfb

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Fix incorrect address in kubects deployment

5dc9479

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Remove useless throwing declaration

0591fb8

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

CHE-8265: Add Workspace.Next model objects

20da213

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

CHE-8265: Add a component to retrieve Workspace.Next objects

ab85152

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

CHE-8265: Add an interface to apply Workspace.Next objects to interna…

e010499

…l environment Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi force-pushed the wsnext_infra_clean branch from 0c95ef6 to b6cef3c Compare June 4, 2018 12:02

garagatyi changed the title ~~[WIP] Add an ability to try Workspace.Next features~~ Add an ability to try Workspace.Next features Jun 4, 2018

sleshchenko approved these changes Jun 4, 2018

View reviewed changes

benoitf added the kind/enhancement A feature request - must adhere to the feature request template. label Jun 4, 2018

garagatyi requested a review from vparfonov as a code owner June 4, 2018 14:17

CHE-8265: Apply workspace next features in WorkspaceRuntimes

f6b639d

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi force-pushed the wsnext_infra_clean branch from 4be4dfb to 8c4be34 Compare June 4, 2018 14:17

l0rd reviewed Jun 5, 2018

View reviewed changes

Oleksandr Garagatyi added 2 commits June 5, 2018 10:02

CHE-8265: Add Workspace.Next objects applier for k8s recipe

b6104fb

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

CHE-8265: Add an ability to try Workspace.Next on k8s deployment

2fcdcf1

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi force-pushed the wsnext_infra_clean branch from 2caf07a to 2fcdcf1 Compare June 5, 2018 07:02

garagatyi merged commit ddb86c4 into eclipse-che:master Jun 5, 2018

garagatyi deleted the wsnext_infra_clean branch June 5, 2018 17:40

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jun 5, 2018

benoitf added this to the 6.7.0 milestone Jun 5, 2018

l0rd mentioned this pull request Jun 7, 2018

Workspace.next - CheService model adjustment #9964

Closed

Add an ability to try Workspace.Next features #9774

Add an ability to try Workspace.Next features #9774

Conversation

garagatyi commented May 22, 2018 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

sleshchenko left a comment • edited Loading

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

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

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

garagatyi commented Jun 4, 2018

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

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

garagatyi commented Jun 4, 2018

codenvy-ci commented Jun 4, 2018

l0rd 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

garagatyi commented Jun 5, 2018

codenvy-ci commented Jun 5, 2018

riuvshin commented Jun 5, 2018

codenvy-ci commented Jun 5, 2018

garagatyi commented May 22, 2018 •

edited

Loading

sleshchenko left a comment •

edited

Loading