OpenShift spi initial implementation #5716

sleshchenko · 2017-07-18T09:26:22Z

What does this PR do?

Minor fixes:

Fix location of agent binaries
Add missed server configs to InstallerDto
Fix construction of output channel

Changes related to spi reworking to make it reusable by other implementations:

Move installer and output endpoints to api-workspace module
Rework Bootstrapping to be able to reuse it for different spi implementations

Add initial state for openshift implementation of spi

What issues does this PR fix or reference?

#5686

benoitf · 2017-07-18T09:35:07Z

agents/exec/src/main/resources/org.eclipse.che.exec.script.sh

-    AGENT_BINARIES_URI=$(echo ${AGENT_BINARIES_URI} | sed 's/https/http/g')
-
-    # use wget
-    WGET_SPIDER="wget --spider"


why wget is removed ?

benoitf · 2017-07-18T09:35:35Z

agents/terminal/src/main/resources/org.eclipse.che.terminal.script.sh

-    AGENT_BINARIES_URI=$(echo ${AGENT_BINARIES_URI} | sed 's/https/http/g')
-
-    # use wget
-    WGET_SPIDER="wget --spider"


why wget is removed ?

note: support was added as on alpine curl is not provided by default while wget is there

Thanks for comment. I copied/pasted it from openshift-connector branch. I'll revise all changes in installer scripts and notify you.

@sleshchenko note: we're now using openshift-connector-wip branch of che

These changes were redundant. I reverted all changes in installers scripts. Thanks for this catch.

benoitf · 2017-07-18T09:37:05Z

agents/exec/src/main/resources/org.eclipse.che.exec.script.sh

-
-test "$(id -u)" = 0 || SUDO="sudo -E"
+command -v curl >/dev/null 2>&1 || { PACKAGES=${PACKAGES}" curl"; }
+test "$(id -u)" = 0 || test -f ${HOME}/is_arbitrary_user || SUDO="sudo -E"


why do we have a test on is_arbitrary_user ?

benoitf · 2017-07-18T09:40:07Z

ide/che-core-ide-stacks/src/main/resources/stacks.json

+              }
+            }
+          },
+          "recipe": {


AFAIK right now in OpenShift Connector there is a deployment generator of workspace definitions. Here you expect that users have to provide specific data for OSIO ?

I don't think it's a bad idea to introduce a new recipe type openshift.

But yes the approach we had with the current OpenShift connector was to be able to reuse current "docker image" recipes. It makes sense to run a centos_jdk8 Docker image on OpenShift so we didn't feel the need to support the openshift deployment descriptions (yaml or json).

The only drawback with your approach is that you will need to duplicate all the stacks in stacks.json if you want them to run on openshift.

To make the things clear - the format is not "dedicated", it is "native" for K8s, OS, which I consider as a huge advantage of Che6 SPI.
Showing ability to support it is one of the main goal of initial implementation.
The problem you are talking about can be solved with more than one way:

defining another environment, i.e. one for Docker, another for OSIO (we can imagine some more for example for Local computer, no docker, no containers), so one or another will work depending on available infrastructure

adding one more infrastructure type which works on OS but support some other, simplified format (docker image, dockerfile etc)

extending this infrastructure implementation making it support more than one recipe type (like in Docker impl) - possible by design but I personally would not prefer this way

And, of course they can be combined.

@benoitf As it is an initial implementation it doesn't have to cover all the use cases. Implementing k8s here seems more interesting since it was quite impossible in the master.
And points 2 and 3 (GA, yes, it's by design) from Gennady's comment would allow using env types from the master in Openshift infra.

I'm just saying that che is a 'portable workspace' so I was expecting I could use my workspace on all platforms whatever is their native stuff. It's like when I use docker I don't care if it's windows, linux or MacOS as native platform.
If I need to rewrite the data of my workspace then it's not anymore 'portable'

benoitf · 2017-07-18T09:44:21Z

...c/main/java/org/eclipse/che/workspace/infrastructure/openshift/OpenshiftInternalRuntime.java

+                    }
+                }
+            }
+        } catch (RuntimeException ignored) {


silent catch ?

benoitf · 2017-07-18T09:48:12Z

...c/main/java/org/eclipse/che/workspace/infrastructure/openshift/OpenshiftInternalRuntime.java

+                                      .get();
+
+                if (actualPod == null) {
+                    throw new InternalInfrastructureException("Can't find creted pod");


FYI typo on exception

benoitf · 2017-07-18T09:48:48Z

...c/main/java/org/eclipse/che/workspace/infrastructure/openshift/OpenshiftInternalRuntime.java

+                    throw new InternalInfrastructureException("Can't find creted pod");
+                }
+                String status = actualPod.getStatus().getPhase();
+                LOG.info("POD " + actualPod.getMetadata().getName() + " has status " + status);


no + usage in logger

benoitf

I may be wrong but it looks like odd for me that the recipe format is a dedicated format. Like if I write a simple dockerfile from eclipse che it won't work on OSIO ?

I don't see usage of persistent volume, env variables, but as it seems to be "initial" implementation, there is a lot of TODO
It looks like that some code is coming from the current openshift connector implementation in master but copyright header/author is changed.

codenvy-ci · 2017-07-18T11:42:36Z

Build # 3089 - FAILED

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

garagatyi

Good job! Since it is an initial step I believe it is a good start!

garagatyi · 2017-07-18T14:46:08Z

...g/eclipse/che/workspace/infrastructure/openshift/environment/OpenshiftEnvironmentParser.java

+            return environmentRecipe.getContent();
+        } else {
+            try {
+                return recipeDownloader.getRecipe(environmentRecipe.getLocation());


I know that it is similar to Docker infra impl. But it should use recipe retrieved by RuntimeContext

@garagatyi Do you mean that parsing and provisioning of the environment should be done by RuntimeContext instead of RuntimeInfrastructure?

I mean that downloading of environment recipe is done on RuntimeContext.

I just don't understand how to implement it. As docker infra does: context should have already parsed environment, so we need to download recipe for parsing before context creation.
WDYT should we do in this case? Should we move parsing of recipe and provisioning to context? Or maybe now leave it as it is and rework later along with docker infra?

Docker infra is a separate implementation. Your implementation doesn't have to do things in the same way as it does. Downloading in docker infra was implemented long time ago. Then we realized that it should use recipe downloaded in context, but haven't changed.

Doesn't have but for me, the responsibility of infrastructure and context is not clear enough. So I implemented in the same way as docker infrastructure does. So I'd leave it as it is. And rework it later when runtime infrastructure won't do estimation (that require recipe downloading) and it will define when validation and estimation should be done.

codenvy-ci · 2017-07-18T14:52:22Z

Build # 3095 - FAILED

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

codenvy-ci · 2017-07-18T15:54:53Z

Build # 3098 - FAILED

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

gazarenkov · 2017-07-19T07:00:50Z

...rc/main/java/org/eclipse/che/workspace/infrastructure/openshift/OpenshiftRuntimeContext.java

+            return UriBuilder.fromUri(apiURI)
+                             .scheme("https".equals(apiURI.getScheme()) ? "wss" : "ws")
+                             .replacePath(apiURI.getPath().replace("/api", ""))
+                             .path(OUTPUT_WEBSOCKET_ENDPOINT_BASE)


Let's avoid URI transformation providing ws:// URI directly instead (in general case it can be different than http:// )

Introduced new property che.websocket.endpoint.base and added using it while construction of output channel

gazarenkov · 2017-07-19T07:03:45Z

...a/org/eclipse/che/workspace/infrastructure/openshift/bootstrapper/OpenshiftBootstrapper.java

+            URL wsmasterEndpoint = new URL(workspaceMasterEndpoint);
+            this.downloadBootstrapperLink = wsmasterEndpoint.getProtocol() + "://" + wsmasterEndpoint.getHost() + ":" +
+                                            wsmasterEndpoint.getPort() +
+                                            "/agent-binaries/linux_amd64/bootstrapper/bootstrapper";


Let's avoid modifying URL. It is temporary is not it?

Added dedicated property che.infra.openshift.bootstrapper_binary_url.

Maybe later we will inject bootstrapper binary in other way but now I have no idea how we can implement it without execution command for downloading like curl or wget.

codenvy-ci · 2017-07-19T12:31:48Z

Build # 3107 - FAILED

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

sleshchenko · 2017-07-19T15:38:44Z

@benoitf

Like if I write a simple dockerfile from eclipse che it won't work on OSIO ?

You're right. It won't work. Mb later we will support dockerfile/composefile but not in this initial implementation of openshift spi. See #5716 (comment)

I don't see usage of persistent volume, env variables

There are few [hard coded env variables](https://github.com/eclipse/che/pull/5716/files#diff-
dc6c75e34aa896716a466e0b3bf1cf98R40), it will be fixed in #5690
For using persistent volume there is another issue #5691

It looks like that some code is coming from the current openshift connector implementation in master but copyright header/author is changed.

It this comment actual after refactoring? If yes, then please notify me.

And please take a look my PR again after refactoring.

codenvy-ci · 2017-07-19T18:43:05Z

Build # 3115 - FAILED

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

l0rd · 2017-07-19T22:26:32Z

I think that supporting recipes of type openshift is definitely a good thing. So I really like the work you are doing and that is something that we haven't implemented in the OpenShift connector and it's cool that you have thought about that. But I think that before merging this to master you should add the capability to run different kind of recipes on OpenShift too. Not only openshift recipes but at least also already built docker images recipes. I'm sure that is something you have already think about but let me note here a couple of reasons why this is important:

Portable stacks: Until now a user was able to use the same stack definition without worrying about what was the underlying Che infrastructure (docker, swarm, openshift, k8s etc...). And that is good because I want to be able to use the same runtime Dockerfile on my laptop, for the CI and in production. If we having different stacks for different environment we loose all the benefits of using Docker containers.
Don't break OSIO: OSIO Che will be broken if you don't support running already-built docker images recipes. Even if you can map every Docker stack to an OpenShift stack there are some details that need to be deduced at runtime and cannot be hardcoded in stacks.json. For example workspace routes and paths of workspace persistent volume claims.

skabashnyuk

@l0rd I agree with all your points. As we mention the main purpose of this PR is to prove that Che6 SPI is capable of providing native capabilities for Openshift. I understand your concerns about backward compatibility with the state that we have in the master branch, and as @gazarenkov says #5716 (comment) we have a couple of different options to solve this.

…ntations

codenvy-ci · 2017-07-20T12:53:28Z

Build # 3124 - FAILED

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

sleshchenko added the target/branch Indicates that a PR will be merged into a branch other than master. label Jul 18, 2017

sleshchenko self-assigned this Jul 18, 2017

sleshchenko requested review from garagatyi, voievodin and akorneta July 18, 2017 09:26

sleshchenko requested review from ashumilova, benoitf, riuvshin, skabashnyuk, tolusha and vparfonov as code owners July 18, 2017 09:26

benoitf reviewed Jul 18, 2017

View reviewed changes

sleshchenko force-pushed the spi-openshift branch from 14a4ac5 to f1dba42 Compare July 18, 2017 12:27

sleshchenko changed the title ~~OpenShift spi initial implementation~~ [WIP] OpenShift spi initial implementation Jul 18, 2017

garagatyi approved these changes Jul 18, 2017

View reviewed changes

gazarenkov reviewed Jul 19, 2017

View reviewed changes

sleshchenko force-pushed the spi-openshift branch 2 times, most recently from 41e93d1 to 193f18a Compare July 19, 2017 10:08

akorneta approved these changes Jul 19, 2017

View reviewed changes

sleshchenko force-pushed the spi-openshift branch 3 times, most recently from ecdbe88 to ae116b1 Compare July 19, 2017 15:28

sleshchenko changed the title ~~[WIP] OpenShift spi initial implementation~~ OpenShift spi initial implementation Jul 20, 2017

skabashnyuk approved these changes Jul 20, 2017

View reviewed changes

sleshchenko force-pushed the spi-openshift branch from ae116b1 to bfb2969 Compare July 20, 2017 09:03

sleshchenko added 5 commits July 20, 2017 15:34

Fix location of agent binaries

c9b689d

Add missed server configs to InstallerDto

dbfebfd

Move installer and output endpoints to api-workspace module

c806b80

Rework Bootstrapping to be able to reuse it for different spi impleme…

0cee791

…ntations

Fix construction of output channel

8480c21

OpenShift initial implementation

a0a51f6

sleshchenko force-pushed the spi-openshift branch from bfb2969 to a0a51f6 Compare July 20, 2017 13:54

sleshchenko merged commit 1fa5248 into spi Jul 20, 2017

sleshchenko deleted the spi-openshift branch July 20, 2017 13:58

skabashnyuk added this to the 6.0.0-M1 milestone Oct 19, 2017

OpenShift spi initial implementation #5716

OpenShift spi initial implementation #5716

Conversation

sleshchenko commented Jul 18, 2017 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

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 Jul 18, 2017 • 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

benoitf left a comment • edited Loading

Choose a reason for hiding this comment

codenvy-ci commented Jul 18, 2017

garagatyi 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

codenvy-ci commented Jul 18, 2017

codenvy-ci commented Jul 18, 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

codenvy-ci commented Jul 19, 2017

sleshchenko commented Jul 19, 2017 • edited Loading

codenvy-ci commented Jul 19, 2017

l0rd commented Jul 19, 2017

skabashnyuk left a comment

Choose a reason for hiding this comment

codenvy-ci commented Jul 20, 2017

sleshchenko commented Jul 18, 2017 •

edited

Loading

benoitf Jul 18, 2017 •

edited

Loading

benoitf left a comment •

edited

Loading

sleshchenko commented Jul 19, 2017 •

edited

Loading