Factorize code of `ServerEvaluationStrategy` classes, to use the Custom strategy as the basis of other strategies #5366

davidfestal · 2017-06-13T14:53:47Z

What does this PR do?

This PR makes the CustomServerEvaluationStrategy the basis of all other strategies. This allows having the LocalDockerSinglePortStrategy simply use a custom template.

For the moment the properties in che.properties are not changed, and other strategies extend the custom strategy with specific settings. So this change should have no impact on existing configurations / installations.

What issues does this PR fix or reference?

https://issues.jboss.org/browse/CHE-277

codenvy-ci · 2017-06-13T14:54:02Z

Can one of the admins verify this patch?

davidfestal · 2017-06-13T14:54:27Z

@benoitf @l0rd could you review this PR please ?

benoitf · 2017-06-13T14:57:25Z

@davidfestal do you have checked https://github.com/codenvy/codenvy/blob/master/plugins/plugin-hosted/codenvy-machine-hosted/src/main/java/com/codenvy/machine/CodenvyDockerServerEvaluationStrategy.java is still compiling ?

benoitf · 2017-06-13T14:58:50Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

    /**
     * Default constructor
     */
    @Inject
-    public CustomServerEvaluationStrategy(@Nullable @Named("che.docker.ip") String cheDockerIp,
-                                          @Nullable @Named("che.docker.ip.external") String cheDockerIpExternal,
+    public CustomServerEvaluationStrategy(@Nullable @Named("che.docker.ip") String internalAddress,


maybe it's better to keep the name cheDockerIp / cheDockerIpExternal ?

@davidfestal I think this comment was not addressed.

benoitf · 2017-06-13T15:00:08Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

    }

+    @Override
+    protected Map<String, String> getInternalAddressesAndPorts(ContainerInfo containerInfo, String internalHost) {
+        String internalAddressContainer = containerInfo.getNetworkSettings().getIpAddress();


is it final objects ?

benoitf · 2017-06-13T15:01:09Z

@garagatyi ?
@davidfestal as long as it works for codenvy strategy, +1 for me

davidfestal · 2017-06-13T15:03:03Z

@davidfestal do you have checked https://github.com/codenvy/codenvy/blob/master/plugins/plugin-hosted/codenvy-machine-hosted/src/main/java/com/codenvy/machine/CodenvyDockerServerEvaluationStrategy.java is still compiling ?

Good point. I didn't check that. Let me have a look.

waiting codenvy check for strategy

sunix · 2017-06-13T14:59:00Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

@@ -96,13 +159,17 @@ public CustomServerEvaluationStrategy(@Nullable @Named("che.docker.ip") String c

        // create Rendering evaluation
        RenderingEvaluation renderingEvaluation = getOnlineRenderingEvaluation(containerInfo, internalHost);
-
+        


this should not be part of the PR

OK. Not hard to remove !

sunix · 2017-06-13T15:00:03Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

+
+        if (isNullOrEmpty(cheDockerCustomExternalTemplate)) {
+            return getExposedPortsToAddressPorts(renderingEvaluation.getExternalAddress(), ports, false);
+        } else {


Is this else necessary ?

Is this else necessary ?

Sure it isn't. Just for style ;-)
Let me change it.

sunix · 2017-06-13T15:18:00Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

+                    !isNullOrEmpty(gatewayAddressContainer) ?
+                    gatewayAddressContainer :
+                    this.internalHost;
+            } else {


is this else necessary ?

davidfestal · 2017-06-13T15:35:58Z

@sunix is it OK for you now ?

davidfestal · 2017-06-13T15:50:35Z

@benoitf after commit 70cb99b the file CodenvyDockerServerEvaluationStrategy.java should compile fine now (very simple to see it from the code since it directly extends ServerEvaluationStrategy). However I can checkout the codenvy repo and test it if you want.

garagatyi · 2017-06-14T07:26:12Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

+    /**
+     * Constructor to be called by derived strategies
+     */
+    public CustomServerEvaluationStrategy(@Nullable @Named("che.docker.ip") String internalAddress,


Can you align names of parameters of constructors? E.g. cheDockerIp == internalAddress

benoitf · 2017-06-14T08:16:34Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

@@ -302,7 +382,7 @@ protected void initPortMapping() {
        /**
         * Gets default external address.
         */
-        protected String getExternalAddress() {
+        public String getExternalAddress() {


do we need public there ?

yes, because I pulled up the method in the RenderingEvaluation interface, since it is now also useful in CustomServerEvaluationStrategy.getExternalAddressesAndPorts() (in case cheDockerCustomExternalTemplate is null)

davidfestal · 2017-06-14T09:06:41Z

@benoitf I checked the compilation of codenvy against this PR. it fails, but not due to this PR. in fact the codenvy build fails with the openshift-connector-rebased branch due to this change: ab740c6#diff-eae0d593d9d11b19a0c27f9e468d957bR73 :
The addition of the useHttpsForExternalUrls() abstract method.

I'll fix this anyway, so we won't have any surprise when merging openshift-connector-rebased to master.

benoitf · 2017-06-14T13:32:08Z

@davidfestal ok, waiting for the fix

Also what about the comments on using variable cheDockerIp instead of internalAddress and cheDockerIpExternal for the externalAddress fields ?
it seems you replaced it in custom server strategy with "old" names

benoitf · 2017-06-14T13:39:20Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

+    /**
+     * Used to store the address set by property {@code che.docker.ip}, if applicable.
+     */
+    protected String internalAddressProperty;


we should use cheDockerIp

The names used here are those that were used initially in the DefaultServerEvaluationStrategy.

all the other strategies had internalAddressProperty and externalAddressProperty That's why I had kept them.

yes but name of properties have been changed during that time so it's better to keep matching name between the name of the property and the value of the field

benoitf · 2017-06-14T13:39:34Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

+    /**
+     * Used to store the address set by property {@code che.docker.ip.external}. if applicable.
+     */
+    protected String externalAddressProperty;


and here cheDockerIpExternal

davidfestal · 2017-06-14T13:59:47Z

@benoitf I fixed the problem that produce a Codenvy build failure, and changed the names of the custom strategy attributes as requested.

benoitf · 2017-06-14T14:03:04Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

            globalPropertiesMap.put("externalAddress", externalAddress);
            globalPropertiesMap.put("externalIP", externalIP);
            globalPropertiesMap.put("workspaceId", getWorkspaceId());
+            globalPropertiesMap.put("workspaceIdWithoutPrefix", getWorkspaceId().replaceFirst("workspace",""));


could you move "workspace" as a class constant ?

this property should be also tested in unit test

sunix · 2017-06-14T14:07:08Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

@@ -99,10 +162,14 @@ public CustomServerEvaluationStrategy(@Nullable @Named("che.docker.ip") String c

        // get current ports
        Map<String, List<PortBinding>> ports = containerInfo.getNetworkSettings().getPorts();
-
+


could this change be removed (trailing space)

sunix · 2017-06-14T14:07:49Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

+        if (isNullOrEmpty(cheDockerCustomExternalTemplate)) {
+            return getExposedPortsToAddressPorts(renderingEvaluation.getExternalAddress(), ports, false);
+        }
+


could trailing space be removed ?

sunix · 2017-06-14T14:11:55Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

-                    .collect(Collectors.toMap(portKey -> portKey,
-                                              portKey -> renderingEvaluation.render(cheDockerCustomExternalTemplate, portKey)));
+                .collect(Collectors.toMap(portKey -> portKey,
+                                          portKey -> renderingEvaluation.render(cheDockerCustomExternalTemplate, portKey)));


if there is no change and just formatting, it would be better if these changes are not included in the PR. (if there is a conflict with another branch it can be a harder to merge). Formatting should be done separately to simplify merge IMO.

benoitf · 2017-06-14T14:17:52Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

            globalPropertiesMap.put("machineName", getMachineName());
            globalPropertiesMap.put("wildcardNipDomain", getWildcardNipDomain(externalAddress));
            globalPropertiesMap.put("wildcardXipDomain", getWildcardXipDomain(externalAddress));
            globalPropertiesMap.put("chePort", chePort);
+            globalPropertiesMap.put("isDevMachine", getIsDevMachine());


missing unit test

benoitf · 2017-06-14T14:18:15Z

...hine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java

@@ -333,12 +420,26 @@ public String render(String template, String port) {
                this.initialized = true;
            }
            ST stringTemplate = new ST(template);
-            globalPropertiesMap.forEach((key, value) -> stringTemplate.add(key, value));
+            globalPropertiesMap.forEach((key, value) -> stringTemplate.add(key, 
+                                                                           "isDevMachine".equals(key) ? 


"isDevMachine" should be a class constant ?

benoitf · 2017-06-14T15:18:31Z

.../src/test/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategyTest.java

+        Map<String, String> portMapping = this.customServerEvaluationStrategy.getExternalAddressesAndPorts(containerInfo, "localhost");
+
+        Assert.assertTrue(portMapping.containsKey("4401/tcp"));
+        Assert.assertEquals(portMapping.get("4401/tcp"), WORKSPACE_ID_VALUE.replaceFirst(CHE_WORKSPACE_ID_PREFIX, ""));


where do we check that we have value ABCDEFG ? for the workspaceId without prefix ?

benoitf · 2017-06-14T15:19:17Z

.../src/test/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategyTest.java

+        Map<String, String> portMapping = this.customServerEvaluationStrategy.getExternalAddressesAndPorts(containerInfo, "localhost");
+
+        Assert.assertTrue(portMapping.containsKey("4401/tcp"));
+        Assert.assertEquals(portMapping.get("4401/tcp"), WORKSPACE_ID_VALUE);


I don't see where we check the value of the template value ? if we get workspaceId or machineName ?

Signed-off-by: David Festal <dfestal@redhat.com>

benoitf · 2017-06-15T07:44:45Z

@davidfestal BTW why it isn't directly merged to master ?

Because the first idea was to have the new LocalDockerSinglePortServerEvaluationStrategy (existing only in openshift-connector and openshift-connector-rebased branches) reuse the new custom strategy. But finally, this required some more refactoring. However this started as a pure Openshift connector change.

@davidfestal ok this is because I think that merging into master will allow to decrease the work on merge operations into the openshift-connector-rebased branch, as BTW is being rebased on master.

davidfestal · 2017-06-15T07:59:53Z

@davidfestal ok this is because I think that merging into master will allow to decrease the work on merge operations into the openshift-connector-rebased branch, as BTW is being rebased on master.

Well, the problem is that it depends on previous changes made on the openshift-connector-branch, and is a prerequisite for PR #5110 , which also depends on previous openshift-connector-branch changes

slemeur

Ok for me as it is going to openshift-connector-rebased

sunix · 2017-06-15T08:35:16Z

...ava/org/eclipse/che/plugin/docker/machine/LocalDockerSinglePortServerEvaluationStrategy.java

-    private String getWorkspaceServerName(Map<String, String> labels, String portKey) {
-        ServerConfImpl serverConf = getServerConfImpl(portKey, labels, new HashMap<>());
-        if (serverConf == null) {
-            return "server-" + portKey.split("/",2)[0];


OK these lines have been removed, how do we deal with exposed ports without label ? we did a quick fix here #4970 while waiting for the rebase to work on a refactored and cleanner fix. Would preview urls works with non label exposed ports ?

Seems to me it is managed here:

https://github.com/davidfestal/che/blob/0f9a64170ee72077099a89f458ad557f1ae38380/plugins/plugin-docker/che-plugin-docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/CustomServerEvaluationStrategy.java#L384

Signed-off-by: David Festal <dfestal@redhat.com>

@fbenoit

…om strategy as the basis of other strategies (eclipse-che#5366) * Pull-up the local docker port management (use exposed ports) Signed-off-by: David Festal <dfestal@redhat.com> * Make all the strategies extend `CustomEvaluationStrategy` Signed-off-by: David Festal <dfestal@redhat.com> * Add a `workspaceIdWithoutPrefix` macro and use it for `single-port` This macro is based on the `workspaceId` macro, but without the `workspace` prefix. Signed-off-by: David Festal <dfestal@redhat.com> * Add the `isDevMachine` to allow conditions in the ST template. This is required to allow the `single-port` strategy to have a different url according to the type of machine. (see the work done for CHE-175 : Support multi-container workspaces on OpenShift) Signed-off-by: David Festal <dfestal@redhat.com> * Small fixes after comments from @fbenoit Signed-off-by: David Festal <dfestal@redhat.com> * Fix unnecessary space pointed out by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Remove unnecessary `else` as suggested by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Keep the method signatures compatible with the `condenvy` strategy Signed-off-by: David Festal <dfestal@redhat.com> * Align names of parameters of constructors (requested by @garagatyi) Signed-off-by: David Festal <dfestal@redhat.com> * Add a default implementation to avoid breaking the Codenvy build Signed-off-by: David Festal <dfestal@redhat.com> * Also rename the attributes Signed-off-by: David Festal <dfestal@redhat.com> * Use a constant for the `workspace` prefix string Signed-off-by: David Festal <dfestal@redhat.com> * Fix formatting as requested by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Use a constant for the `isDevMachine` macro name Signed-off-by: David Festal <dfestal@redhat.com> * Add unit tests for `workspaceIdWithoutPrefixè and `isDevMachine` macros Signed-off-by: David Festal <dfestal@redhat.com> * Another requested formatting fix Signed-off-by: David Festal <dfestal@redhat.com> * Make new tests clearer Signed-off-by: David Festal <dfestal@redhat.com> * yet another formatting request Signed-off-by: David Festal <dfestal@redhat.com> * Respect the original order of imports Signed-off-by: David Festal <dfestal@redhat.com> * remove unnecessary `toString()` Signed-off-by: David Festal <dfestal@redhat.com> * use a lowercase `S` in the `server-` prefix Signed-off-by: David Festal <dfestal@redhat.com>

@fbenoit

…om strategy as the basis of other strategies (eclipse-che#5366) * Pull-up the local docker port management (use exposed ports) Signed-off-by: David Festal <dfestal@redhat.com> * Make all the strategies extend `CustomEvaluationStrategy` Signed-off-by: David Festal <dfestal@redhat.com> * Add a `workspaceIdWithoutPrefix` macro and use it for `single-port` This macro is based on the `workspaceId` macro, but without the `workspace` prefix. Signed-off-by: David Festal <dfestal@redhat.com> * Add the `isDevMachine` to allow conditions in the ST template. This is required to allow the `single-port` strategy to have a different url according to the type of machine. (see the work done for CHE-175 : Support multi-container workspaces on OpenShift) Signed-off-by: David Festal <dfestal@redhat.com> * Small fixes after comments from @fbenoit Signed-off-by: David Festal <dfestal@redhat.com> * Fix unnecessary space pointed out by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Remove unnecessary `else` as suggested by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Keep the method signatures compatible with the `condenvy` strategy Signed-off-by: David Festal <dfestal@redhat.com> * Align names of parameters of constructors (requested by @garagatyi) Signed-off-by: David Festal <dfestal@redhat.com> * Add a default implementation to avoid breaking the Codenvy build Signed-off-by: David Festal <dfestal@redhat.com> * Also rename the attributes Signed-off-by: David Festal <dfestal@redhat.com> * Use a constant for the `workspace` prefix string Signed-off-by: David Festal <dfestal@redhat.com> * Fix formatting as requested by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Use a constant for the `isDevMachine` macro name Signed-off-by: David Festal <dfestal@redhat.com> * Add unit tests for `workspaceIdWithoutPrefixè and `isDevMachine` macros Signed-off-by: David Festal <dfestal@redhat.com> * Another requested formatting fix Signed-off-by: David Festal <dfestal@redhat.com> * Make new tests clearer Signed-off-by: David Festal <dfestal@redhat.com> * yet another formatting request Signed-off-by: David Festal <dfestal@redhat.com> * Respect the original order of imports Signed-off-by: David Festal <dfestal@redhat.com> * remove unnecessary `toString()` Signed-off-by: David Festal <dfestal@redhat.com> * use a lowercase `S` in the `server-` prefix Signed-off-by: David Festal <dfestal@redhat.com>

@fbenoit

…om strategy as the basis of other strategies (#5366) * Pull-up the local docker port management (use exposed ports) Signed-off-by: David Festal <dfestal@redhat.com> * Make all the strategies extend `CustomEvaluationStrategy` Signed-off-by: David Festal <dfestal@redhat.com> * Add a `workspaceIdWithoutPrefix` macro and use it for `single-port` This macro is based on the `workspaceId` macro, but without the `workspace` prefix. Signed-off-by: David Festal <dfestal@redhat.com> * Add the `isDevMachine` to allow conditions in the ST template. This is required to allow the `single-port` strategy to have a different url according to the type of machine. (see the work done for CHE-175 : Support multi-container workspaces on OpenShift) Signed-off-by: David Festal <dfestal@redhat.com> * Small fixes after comments from @fbenoit Signed-off-by: David Festal <dfestal@redhat.com> * Fix unnecessary space pointed out by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Remove unnecessary `else` as suggested by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Keep the method signatures compatible with the `condenvy` strategy Signed-off-by: David Festal <dfestal@redhat.com> * Align names of parameters of constructors (requested by @garagatyi) Signed-off-by: David Festal <dfestal@redhat.com> * Add a default implementation to avoid breaking the Codenvy build Signed-off-by: David Festal <dfestal@redhat.com> * Also rename the attributes Signed-off-by: David Festal <dfestal@redhat.com> * Use a constant for the `workspace` prefix string Signed-off-by: David Festal <dfestal@redhat.com> * Fix formatting as requested by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Use a constant for the `isDevMachine` macro name Signed-off-by: David Festal <dfestal@redhat.com> * Add unit tests for `workspaceIdWithoutPrefixè and `isDevMachine` macros Signed-off-by: David Festal <dfestal@redhat.com> * Another requested formatting fix Signed-off-by: David Festal <dfestal@redhat.com> * Make new tests clearer Signed-off-by: David Festal <dfestal@redhat.com> * yet another formatting request Signed-off-by: David Festal <dfestal@redhat.com> * Respect the original order of imports Signed-off-by: David Festal <dfestal@redhat.com> * remove unnecessary `toString()` Signed-off-by: David Festal <dfestal@redhat.com> * use a lowercase `S` in the `server-` prefix Signed-off-by: David Festal <dfestal@redhat.com>

@fbenoit

* CHE-4141 - Use Persistent Volumes Claims when creating workspaces Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com> * Implement getContainerLogs method in OpenShiftConnector Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com> * Implement createExec() and startExec() in OpenShiftConnector Add implementations of createExec() and startExec(). Since OpenShift does not separate the create and start steps, a holder class KubernetesExecHolder is necessary, to pass information between the call to createExec() (which just saves relevant information) and startExec(). Additionally, adds KubernetesOutputAdapter, which parses the output from OpenShift into LogMessages that can be handled by Che's MessageProcessor<LogMessage> class. Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Add implementation of getEvents() to avoid busy wait Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Update Dockerfile to avoid permissions issues Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * Che server and workpaces exposed on the same single TCP port (#4351) Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * Disabling usage of user account service in openshift-connector Signed-off-by: Sun Seng David Tan <sutan@redhat.com> * Update Docker Compose tests to fix test failure Updating to Jackson 2.7.7 causes tests in the docker compose plugin to fail. This is due to the fact that the tests expect empty values in dictionaries to be parsed as the empty string, whereas jackson 2.7.7 parses them as null (as specified by the yaml spec). Modifies the affected tests to explicitly use an empty string (i.e. "") instead of an empty value. Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Find an alternative to subPath in volumeMount Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com> * Setting rwx permissions for all on /data/ in case it's not mounted Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * Add support for resource limits when running on Openshift Add resource limits to workspace Pods when running on OpenShift. The memory limit is normally obtained from the API request to create the workspace, however it can be overridden via the property `che.openshift.workspace.memory.override`. The cpu limit used is determined by the property `che.openshift.workspace.cpu.limit`. In both cases, the value of the property is passed directly to OpenShift, so any valid quantity is acceptable (e.g. 150Mi, 1Gi, 1024, etc). Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Fix dockerImageConfig is null (since v1.5 of OpenShift API) Signed-off-by: Sun Seng David Tan <sutan@redhat.com> * Add Nullable annotation to che.docker.ip.external The property che.docker.ip.external can be null, but OpenShiftConnector does not include the annotation. This prevents Che from initialising if e.g. running on docker without the property set. Signed-off-by: Angel Misevski <amisevsk@redhat.com> * CHE-158 Adding TLS support for Workspace routes Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com> * Adding property to set requests for RAM Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * CHE-158 Using '-' instead of '.' for generating OpenShift route Urls Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com> * Fixing tests after changing Url generation logic Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com> * Redirect insecure HTTP requests to TLS endpoint Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * CHE-180: Creating and closing OpenShiftClient in every method of OpenshiftConnector Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com> * Update route naming to make it work on OSO Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * Rework PVC management on OpenShift - Change how subdirectories are created in pods to use a short, terminating job instead of a full deployment - Add OpenShiftWorkspaceFilesCleaner class to properly notice workspace deleted events - Add helper class to manage job pods. For creation, some effort is made to avoid attempting to create workspaces unnecessarily, but only exists in-memory - Workspace deletions are batched together so that removing workspaces directories can be done when server is idled, avoiding unnecessary PVC mounts - Add two new properties: che.openshift.jobs.image and che.openshift.jobs.memorylimit, which are used by OpenShiftPvcHelper to set up pods Current issues: - Since workspace directories are not deleted immediately, attempting to re-create a workspace with the same name will result in the previous instance's project to already be there. This should have a minor impact. - Memory for which workspace dirs have been created is not persisted, resulting in potentially unnecessary jobs - Openshift workspace files cleaner is included by overwriting binding in WsMasterModule instead of using a provider. This could be better, but OpenShift integration may be reaching a point where a custom module is a better solution. Signed-off-by: Angel Misevski <amisevsk@redhat.com> Signed-off-by: Sun Seng David Tan <sutan@redhat.com> * Delete ReplicaSets explicitly when shutting down a workspace Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Fix OpenShiftConnectorTest Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * Fix route server names if unknown should start with server-. https://issues.jboss.org/browse/CHE-230 Signed-off-by: Sun Seng David Tan <sutan@redhat.com> * Add property to control manual workspace dir creation in OpenShift Add property 'che.openshift.precreate.workspace.dirs'. If property is true, OpenShiftConnector will run a pod before launching workspaces to create a subpath in the workspace's persistent volume with correct permissions. If the property is false, this step is skipped. This is necessary as in older versions of OpenShift/Kubernetes, subpaths created as part of a volume mount are created with root permissions, and so cannot be modified by workspace pods. More recent versions fix this, creating subpath volumes with correct permissions, making the step above unnecessary. Signed-off-by: Angel Misevski <amisevsk@redhat.com> * CHE-102 - Idle detection of che-server and workspaces Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com> * Add and modify tests for OpenShift helper classes Add tests for the untested classes in openshift.client.kuberentes, and update existing tests where necessary. Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Recent changes required access to `/` which is impossible under OS Signed-off-by: David Festal <dfestal@redhat.com> * adapt che-server entrypoint.sh to environments without write permissions in '/' (#5344) * adapt che-server entrypoint.sh to environments without write permissions in '/' * CHE-280: Adding container's state info to the 'inspectContainer' API Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com> * Factorize code of `ServerEvaluationStrategy` classes, to use the Custom strategy as the basis of other strategies (#5366) * Pull-up the local docker port management (use exposed ports) Signed-off-by: David Festal <dfestal@redhat.com> * Make all the strategies extend `CustomEvaluationStrategy` Signed-off-by: David Festal <dfestal@redhat.com> * Add a `workspaceIdWithoutPrefix` macro and use it for `single-port` This macro is based on the `workspaceId` macro, but without the `workspace` prefix. Signed-off-by: David Festal <dfestal@redhat.com> * Add the `isDevMachine` to allow conditions in the ST template. This is required to allow the `single-port` strategy to have a different url according to the type of machine. (see the work done for CHE-175 : Support multi-container workspaces on OpenShift) Signed-off-by: David Festal <dfestal@redhat.com> * Small fixes after comments from @fbenoit Signed-off-by: David Festal <dfestal@redhat.com> * Fix unnecessary space pointed out by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Remove unnecessary `else` as suggested by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Keep the method signatures compatible with the `condenvy` strategy Signed-off-by: David Festal <dfestal@redhat.com> * Align names of parameters of constructors (requested by @garagatyi) Signed-off-by: David Festal <dfestal@redhat.com> * Add a default implementation to avoid breaking the Codenvy build Signed-off-by: David Festal <dfestal@redhat.com> * Also rename the attributes Signed-off-by: David Festal <dfestal@redhat.com> * Use a constant for the `workspace` prefix string Signed-off-by: David Festal <dfestal@redhat.com> * Fix formatting as requested by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Use a constant for the `isDevMachine` macro name Signed-off-by: David Festal <dfestal@redhat.com> * Add unit tests for `workspaceIdWithoutPrefixè and `isDevMachine` macros Signed-off-by: David Festal <dfestal@redhat.com> * Another requested formatting fix Signed-off-by: David Festal <dfestal@redhat.com> * Make new tests clearer Signed-off-by: David Festal <dfestal@redhat.com> * yet another formatting request Signed-off-by: David Festal <dfestal@redhat.com> * Respect the original order of imports Signed-off-by: David Festal <dfestal@redhat.com> * remove unnecessary `toString()` Signed-off-by: David Festal <dfestal@redhat.com> * use a lowercase `S` in the `server-` prefix Signed-off-by: David Festal <dfestal@redhat.com> * Multi-container workspace Support (#5110) * Fix 2 NPE that prevented using *non-dev* additional machines In the context of https://issues.jboss.org/browse/CHE-175 Signed-off-by: David Festal <dfestal@redhat.com> * Name openshift resources based on the machine name for non-dev machines This fixes https://issues.jboss.org/browse/CHE-259 and https://issues.jboss.org/browse/CHE-258 Signed-off-by: David Festal <dfestal@redhat.com> * Fix failing Traeffik tests... ... by: - adding the new `CHE_IS_DEV_MACHINE` env variable in tests - pulling up all the `CustomServerEvaluationStrategy` features in an abstract `BaseServerEvaluationStrategy` (which all other Che strategies extend) and have the `CustomServerEvaluationStrategy` class simply extend this `BaseServerEvaluationStrategy`. Signed-off-by: David Festal <dfestal@redhat.com> * Fix tests in the LocalDockerEvaluationStrategy... ... by correctly using the boolean attribute to manage the new use-case introduced by @fbenoit in master. Signed-off-by: David Festal <dfestal@redhat.com> * Replace OSIO-specific `single-port` strategy by `docker-local-custom` This fixes redhat-developer/rh-che#113 Signed-off-by: David Festal <dfestal@redhat.com>

@fbenoit

* CHE-4141 - Use Persistent Volumes Claims when creating workspaces Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com> * Implement getContainerLogs method in OpenShiftConnector Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com> * Implement createExec() and startExec() in OpenShiftConnector Add implementations of createExec() and startExec(). Since OpenShift does not separate the create and start steps, a holder class KubernetesExecHolder is necessary, to pass information between the call to createExec() (which just saves relevant information) and startExec(). Additionally, adds KubernetesOutputAdapter, which parses the output from OpenShift into LogMessages that can be handled by Che's MessageProcessor<LogMessage> class. Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Add implementation of getEvents() to avoid busy wait Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Update Dockerfile to avoid permissions issues Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * Che server and workpaces exposed on the same single TCP port (eclipse-che#4351) Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * Disabling usage of user account service in openshift-connector Signed-off-by: Sun Seng David Tan <sutan@redhat.com> * Update Docker Compose tests to fix test failure Updating to Jackson 2.7.7 causes tests in the docker compose plugin to fail. This is due to the fact that the tests expect empty values in dictionaries to be parsed as the empty string, whereas jackson 2.7.7 parses them as null (as specified by the yaml spec). Modifies the affected tests to explicitly use an empty string (i.e. "") instead of an empty value. Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Find an alternative to subPath in volumeMount Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com> * Setting rwx permissions for all on /data/ in case it's not mounted Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * Add support for resource limits when running on Openshift Add resource limits to workspace Pods when running on OpenShift. The memory limit is normally obtained from the API request to create the workspace, however it can be overridden via the property `che.openshift.workspace.memory.override`. The cpu limit used is determined by the property `che.openshift.workspace.cpu.limit`. In both cases, the value of the property is passed directly to OpenShift, so any valid quantity is acceptable (e.g. 150Mi, 1Gi, 1024, etc). Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Fix dockerImageConfig is null (since v1.5 of OpenShift API) Signed-off-by: Sun Seng David Tan <sutan@redhat.com> * Add Nullable annotation to che.docker.ip.external The property che.docker.ip.external can be null, but OpenShiftConnector does not include the annotation. This prevents Che from initialising if e.g. running on docker without the property set. Signed-off-by: Angel Misevski <amisevsk@redhat.com> * CHE-158 Adding TLS support for Workspace routes Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com> * Adding property to set requests for RAM Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * CHE-158 Using '-' instead of '.' for generating OpenShift route Urls Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com> * Fixing tests after changing Url generation logic Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com> * Redirect insecure HTTP requests to TLS endpoint Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * CHE-180: Creating and closing OpenShiftClient in every method of OpenshiftConnector Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com> * Update route naming to make it work on OSO Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * Rework PVC management on OpenShift - Change how subdirectories are created in pods to use a short, terminating job instead of a full deployment - Add OpenShiftWorkspaceFilesCleaner class to properly notice workspace deleted events - Add helper class to manage job pods. For creation, some effort is made to avoid attempting to create workspaces unnecessarily, but only exists in-memory - Workspace deletions are batched together so that removing workspaces directories can be done when server is idled, avoiding unnecessary PVC mounts - Add two new properties: che.openshift.jobs.image and che.openshift.jobs.memorylimit, which are used by OpenShiftPvcHelper to set up pods Current issues: - Since workspace directories are not deleted immediately, attempting to re-create a workspace with the same name will result in the previous instance's project to already be there. This should have a minor impact. - Memory for which workspace dirs have been created is not persisted, resulting in potentially unnecessary jobs - Openshift workspace files cleaner is included by overwriting binding in WsMasterModule instead of using a provider. This could be better, but OpenShift integration may be reaching a point where a custom module is a better solution. Signed-off-by: Angel Misevski <amisevsk@redhat.com> Signed-off-by: Sun Seng David Tan <sutan@redhat.com> * Delete ReplicaSets explicitly when shutting down a workspace Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Fix OpenShiftConnectorTest Signed-off-by: Mario Loriedo <mloriedo@redhat.com> * Fix route server names if unknown should start with server-. https://issues.jboss.org/browse/CHE-230 Signed-off-by: Sun Seng David Tan <sutan@redhat.com> * Add property to control manual workspace dir creation in OpenShift Add property 'che.openshift.precreate.workspace.dirs'. If property is true, OpenShiftConnector will run a pod before launching workspaces to create a subpath in the workspace's persistent volume with correct permissions. If the property is false, this step is skipped. This is necessary as in older versions of OpenShift/Kubernetes, subpaths created as part of a volume mount are created with root permissions, and so cannot be modified by workspace pods. More recent versions fix this, creating subpath volumes with correct permissions, making the step above unnecessary. Signed-off-by: Angel Misevski <amisevsk@redhat.com> * CHE-102 - Idle detection of che-server and workspaces Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com> * Add and modify tests for OpenShift helper classes Add tests for the untested classes in openshift.client.kuberentes, and update existing tests where necessary. Signed-off-by: Angel Misevski <amisevsk@redhat.com> * Recent changes required access to `/` which is impossible under OS Signed-off-by: David Festal <dfestal@redhat.com> * adapt che-server entrypoint.sh to environments without write permissions in '/' (eclipse-che#5344) * adapt che-server entrypoint.sh to environments without write permissions in '/' * CHE-280: Adding container's state info to the 'inspectContainer' API Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com> * Factorize code of `ServerEvaluationStrategy` classes, to use the Custom strategy as the basis of other strategies (eclipse-che#5366) * Pull-up the local docker port management (use exposed ports) Signed-off-by: David Festal <dfestal@redhat.com> * Make all the strategies extend `CustomEvaluationStrategy` Signed-off-by: David Festal <dfestal@redhat.com> * Add a `workspaceIdWithoutPrefix` macro and use it for `single-port` This macro is based on the `workspaceId` macro, but without the `workspace` prefix. Signed-off-by: David Festal <dfestal@redhat.com> * Add the `isDevMachine` to allow conditions in the ST template. This is required to allow the `single-port` strategy to have a different url according to the type of machine. (see the work done for CHE-175 : Support multi-container workspaces on OpenShift) Signed-off-by: David Festal <dfestal@redhat.com> * Small fixes after comments from @fbenoit Signed-off-by: David Festal <dfestal@redhat.com> * Fix unnecessary space pointed out by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Remove unnecessary `else` as suggested by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Keep the method signatures compatible with the `condenvy` strategy Signed-off-by: David Festal <dfestal@redhat.com> * Align names of parameters of constructors (requested by @garagatyi) Signed-off-by: David Festal <dfestal@redhat.com> * Add a default implementation to avoid breaking the Codenvy build Signed-off-by: David Festal <dfestal@redhat.com> * Also rename the attributes Signed-off-by: David Festal <dfestal@redhat.com> * Use a constant for the `workspace` prefix string Signed-off-by: David Festal <dfestal@redhat.com> * Fix formatting as requested by @sunix Signed-off-by: David Festal <dfestal@redhat.com> * Use a constant for the `isDevMachine` macro name Signed-off-by: David Festal <dfestal@redhat.com> * Add unit tests for `workspaceIdWithoutPrefixè and `isDevMachine` macros Signed-off-by: David Festal <dfestal@redhat.com> * Another requested formatting fix Signed-off-by: David Festal <dfestal@redhat.com> * Make new tests clearer Signed-off-by: David Festal <dfestal@redhat.com> * yet another formatting request Signed-off-by: David Festal <dfestal@redhat.com> * Respect the original order of imports Signed-off-by: David Festal <dfestal@redhat.com> * remove unnecessary `toString()` Signed-off-by: David Festal <dfestal@redhat.com> * use a lowercase `S` in the `server-` prefix Signed-off-by: David Festal <dfestal@redhat.com> * Multi-container workspace Support (eclipse-che#5110) * Fix 2 NPE that prevented using *non-dev* additional machines In the context of https://issues.jboss.org/browse/CHE-175 Signed-off-by: David Festal <dfestal@redhat.com> * Name openshift resources based on the machine name for non-dev machines This fixes https://issues.jboss.org/browse/CHE-259 and https://issues.jboss.org/browse/CHE-258 Signed-off-by: David Festal <dfestal@redhat.com> * Fix failing Traeffik tests... ... by: - adding the new `CHE_IS_DEV_MACHINE` env variable in tests - pulling up all the `CustomServerEvaluationStrategy` features in an abstract `BaseServerEvaluationStrategy` (which all other Che strategies extend) and have the `CustomServerEvaluationStrategy` class simply extend this `BaseServerEvaluationStrategy`. Signed-off-by: David Festal <dfestal@redhat.com> * Fix tests in the LocalDockerEvaluationStrategy... ... by correctly using the boolean attribute to manage the new use-case introduced by @fbenoit in master. Signed-off-by: David Festal <dfestal@redhat.com> * Replace OSIO-specific `single-port` strategy by `docker-local-custom` This fixes redhat-developer/rh-che#113 Signed-off-by: David Festal <dfestal@redhat.com>

benoitf previously approved these changes Jun 13, 2017

View reviewed changes

benoitf requested a review from garagatyi June 13, 2017 15:12

benoitf added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Jun 13, 2017

sunix requested changes Jun 13, 2017

View reviewed changes

sunix reviewed Jun 13, 2017

View reviewed changes

garagatyi approved these changes Jun 14, 2017

View reviewed changes

benoitf reviewed Jun 14, 2017

View reviewed changes

benoitf mentioned this pull request Jun 14, 2017

Expose Che server and workspaces through one single TCP port #4269

Closed

benoitf reviewed Jun 14, 2017

View reviewed changes

sunix requested changes Jun 14, 2017

View reviewed changes

benoitf reviewed Jun 14, 2017

View reviewed changes

benoitf assigned davidfestal Jun 14, 2017

benoitf approved these changes Jun 14, 2017

View reviewed changes

davidfestal added 15 commits June 14, 2017 18:58

Fix unnecessary space pointed out by @sunix

58c5609

Signed-off-by: David Festal <dfestal@redhat.com>

Remove unnecessary else as suggested by @sunix

c5a99e6

Signed-off-by: David Festal <dfestal@redhat.com>

Keep the method signatures compatible with the condenvy strategy

8addae5

Signed-off-by: David Festal <dfestal@redhat.com>

Align names of parameters of constructors (requested by @garagatyi)

0e87138

Signed-off-by: David Festal <dfestal@redhat.com>

Add a default implementation to avoid breaking the Codenvy build

97cc631

Signed-off-by: David Festal <dfestal@redhat.com>

Also rename the attributes

d2c538a

Signed-off-by: David Festal <dfestal@redhat.com>

Use a constant for the workspace prefix string

0644a37

Signed-off-by: David Festal <dfestal@redhat.com>

Fix formatting as requested by @sunix

1325df8

Signed-off-by: David Festal <dfestal@redhat.com>

Use a constant for the isDevMachine macro name

82ed118

Signed-off-by: David Festal <dfestal@redhat.com>

Add unit tests for workspaceIdWithoutPrefixè and isDevMachine` macros

bd07aa8

Signed-off-by: David Festal <dfestal@redhat.com>

Another requested formatting fix

385a108

Signed-off-by: David Festal <dfestal@redhat.com>

Make new tests clearer

5bf33e1

Signed-off-by: David Festal <dfestal@redhat.com>

yet another formatting request

ad629e3

Signed-off-by: David Festal <dfestal@redhat.com>

Respect the original order of imports

1154f57

Signed-off-by: David Festal <dfestal@redhat.com>

remove unnecessary toString()

b350718

Signed-off-by: David Festal <dfestal@redhat.com>

slemeur approved these changes Jun 15, 2017

View reviewed changes

sunix reviewed Jun 15, 2017

View reviewed changes

sunix approved these changes Jun 15, 2017

View reviewed changes

use a lowercase S in the server- prefix

769ef86

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal force-pushed the JBOSS-CHE-277 branch from 0f9a641 to 769ef86 Compare June 15, 2017 09:38

sunix merged commit 42f2903 into eclipse-che:openshift-connector-rebased Jun 15, 2017

sunix removed this from the 5.13.0 milestone Jun 15, 2017

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 Nov 2, 2017

		@@ -96,13 +159,17 @@ public CustomServerEvaluationStrategy(@Nullable @Named("che.docker.ip") String c

		// create Rendering evaluation
		RenderingEvaluation renderingEvaluation = getOnlineRenderingEvaluation(containerInfo, internalHost);

		@@ -99,10 +162,14 @@ public CustomServerEvaluationStrategy(@Nullable @Named("che.docker.ip") String c

		// get current ports
		Map<String, List<PortBinding>> ports = containerInfo.getNetworkSettings().getPorts();

Factorize code of ServerEvaluationStrategy classes, to use the Custom strategy as the basis of other strategies #5366

Factorize code of ServerEvaluationStrategy classes, to use the Custom strategy as the basis of other strategies #5366

Conversation

davidfestal commented Jun 13, 2017

What does this PR do?

What issues does this PR fix or reference?

codenvy-ci commented Jun 13, 2017

davidfestal commented Jun 13, 2017

benoitf commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf commented Jun 13, 2017

davidfestal commented Jun 13, 2017

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 Jun 13, 2017

davidfestal commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfestal commented Jun 14, 2017

benoitf commented Jun 14, 2017

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 Jun 14, 2017

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

benoitf commented Jun 15, 2017

davidfestal commented Jun 15, 2017

slemeur 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

Factorize code of `ServerEvaluationStrategy` classes, to use the Custom strategy as the basis of other strategies #5366

Factorize code of `ServerEvaluationStrategy` classes, to use the Custom strategy as the basis of other strategies #5366