CHE-26: Initial OpenShift integration #3798

ibuziuk · 2017-01-18T15:58:09Z

What does this PR do?

Initial OpenshiftConnector implementation

What issues does this PR fix or reference?

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

NOTE: Do not merge until PR with io.fabric8 dependencies is applied - eclipse-che/che-dependencies#23

New behavior

Provides a possibility to use openshift io.fabric8 API for container management

# Which implementation of DockerConnector to use in managing containers. In general,
# the base implementation of DockerConnector is appropriate, but OpenShiftConnector
# is necessary for deploying Che on OpenShift. Options:
#     - 'default'   : Use DockerConnector
#     - 'openshift' : use OpenShiftConnector
che.docker.connector=default

Tests written?

Yes

Changelog and Release Note Information

Changelog

Add OpenShiftConnector to support io.fabric8 API for container management.

Release Notes:
[no release notes since this is part of a work in progress]

Docs Pull Request:

Docs issue is here, but docs won't be created until #2847 is complete.

codenvy-ci · 2017-01-18T16:00:00Z

Can one of the admins verify this patch?

ibuziuk · 2017-01-18T16:00:02Z

@l0rd @amisevsk could you please verify? Just do not forget to build che-dependencies from https://issues.jboss.org/browse/CHE-26 first

TylerJewell · 2017-01-18T16:04:07Z

FYI, we are implementing a new maintenance policy for PRs where each PR will need to have completed changelog, docs linked PR, and release notes included within it before merge. We are documenting this now and how maintainers + PR submitters are to work through these items. But as this is significant, wanted to give this heads up.

gorkem · 2017-01-18T19:27:58Z

@TylerJewell Since this is the initial implementation and we will have a few more iterations until this is completed. Perhaps we can open an issue and for the missing docs and assign it to @ibuziuk or @l0rd.

TylerJewell · 2017-01-18T20:02:52Z

Like larger projects, we have moved all of the docs into a separate repository. It seems essential so that the core repository doesn't end up with a lot of large image or animated gif files that complicates cloning and fetching with git. So, we have moved everything into github.com/codenvy/che-docs as a repository and it will be moved into github.com/eclipse/che-docs as soon as the repository has been created and CQ passed, which should be this week.

But in the mean time, we ask that every core PR have a matching che-docs PR linked into it, and then both PRs are merged at the same time. If you'd like a separate issue for just that work, that makes sense.

ibuziuk · 2017-01-18T22:09:56Z

@TylerJewell @gorkem I would be glad to work on documentation part

TylerJewell · 2017-01-19T01:29:15Z

Thanks, @gorkem and @ibuziuk for being flexible with us as we work to get our documentation processes better refined. I know they are in a state of flux right now, so having ambiguous rules that must be passed in order to get something merged is not good for anyone.

The basic rules of what we are thinking for docs is the following three things provided within the PR:

A single line to be used in the Changelog
If a new feature, then a cross-link PR to github.com/codenvy/che-docs with the docs improvements and suggestions.
A section that includes what we should include in the Release Notes for the next release. This can be small or long markdown. We think of release notes as a marketing tool.

In this case for the documentation, I think there are probably a few pages of the docs that should be updated:

The configuration page of the setup section, about how to set this property.
Then the user pages that we currently have talking about OpenShift. We have dedicated pages that currently cover how to use Che to integrate with OpenShift, but we should add another page dedicated to how to deploy Che onto OpenShift.

benoitf · 2017-01-19T15:44:13Z

ci-build

ibuziuk · 2017-01-19T17:13:57Z

Issue for documenting the process of deployment Che on OpenShift has been created https://github.com/codenvy/che-docs/issues/76

codenvy-ci · 2017-01-19T18:16:36Z

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

benoitf · 2017-01-19T22:38:31Z

...main/java/org/eclipse/che/plugin/docker/machine/cleaner/DockerAbandonedResourcesCleaner.java

        this.nameGenerator = nameGenerator;
-        this.runtimes = workspaceRuntimes;
+		this.runtimes = workspaceRuntimes;


indent looks like incorrect

benoitf · 2017-01-19T22:39:34Z

...lugin-docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/DockerInstance.java

@@ -104,7 +105,7 @@
    private final MachineRuntimeInfoImpl                      machineRuntime;

    @Inject
-    public DockerInstance(DockerConnector docker,
+    public DockerInstance(DockerConnectorProvider dockerProvider,


do we need guice Providers vs named connector ?

Not sure I got the comment :( We do need to inject provider because now it is in charge of providing suitable connector (DockerConnector / OpenShiftConnector) based on che.docker_connector.provider property

@benoitf BTW, similar approach is used for obtaining ServerEvaluationStrategy (default / docker-local) - https://github.com/eclipse/che/blob/master/plugins/plugin-docker/che-plugin-docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/local/LocalDockerModule.java#L52
And I think it is relevant to also use MapBinder for DockerConnector use-case (default / openshift)

@ibuziuk I agree

I agree with Florent that it would look prettier if we get rid of injecting provider. There is always a way to reduce distraction. But up to you.

@garagatyi For initial implementation I would rather leave it this way. The usage of DockerConnectorProvider will be changed once OpenShiftConnector is decoupled from DockerConnector and there will be a good time for rethinking the whole approach

benoitf · 2017-01-19T22:41:09Z

plugins/plugin-docker/che-plugin-openshift-client/pom.xml

+    <build>
+        <resources>
+            <resource>
+                <directory>src/main/java</directory>


do we need to specify this directory as a resource item ?

benoitf · 2017-01-19T22:44:07Z

...enshift-client/src/main/java/org/eclipse/che/plugin/openshift/client/OpenShiftConnector.java

+    private final KubernetesContainer kubernetesContainer;
+    private final KubernetesService kubernetesService;
+    private final String             cheOpenShiftProjectName;
+    private final String             cheOpenShiftServiceAccount;


I'm not sure indent of previous lines are compliant with code style

benoitf · 2017-01-19T22:44:56Z

...enshift-client/src/main/java/org/eclipse/che/plugin/openshift/client/OpenShiftConnector.java

+                              @Named("che.openshift.username") String openShiftUserName,
+                              @Named("che.openshift.password") String openShiftUserPassword,
+                              @Named("che.openshift.project") String cheOpenShiftProjectName,
+                              @Named("che.openshift.serviceaccountname") String cheOpenShiftServiceAccount) {


is there a reason why some of attributes for che.* are starting with che and others by openShift ?

fixed. All attributes are now starting with openShift.*

benoitf · 2017-01-19T23:03:08Z

...c/main/java/org/eclipse/che/plugin/openshift/client/kubernetes/KubernetesLabelConverter.java

+                LOG.warn("Expected CreateContainerParams label key " + entry.getKey() +
+                         " to start with " + CHE_SERVER_LABEL_PREFIX);
+            } else if (key.contains(".") || key.contains("_") || value.contains("_")) {
+                LOG.error(String.format("Cannot convert label %s to DNS Name: "


do not use format and +

benoitf · 2017-01-19T23:03:22Z

...c/main/java/org/eclipse/che/plugin/openshift/client/kubernetes/KubernetesLabelConverter.java

+            value = String.format(CHE_SERVER_LABEL_PADDING, value);
+
+            if (!key.matches(KUBERNETES_ANNOTATION_REGEX) || !value.matches(KUBERNETES_ANNOTATION_REGEX)) {
+                LOG.error(String.format("Could not convert label %s into Kubernetes annotation: "


do not use + and format

benoitf · 2017-01-19T23:03:35Z

...ient/src/main/java/org/eclipse/che/plugin/openshift/client/kubernetes/KubernetesService.java

+import io.fabric8.kubernetes.api.model.IntOrString;
+import io.fabric8.kubernetes.api.model.ServicePort;
+
+public class KubernetesService {


javadoc is missing

benoitf · 2017-01-19T23:03:52Z

...ient/src/main/java/org/eclipse/che/plugin/openshift/client/kubernetes/KubernetesService.java

+public class KubernetesService {
+
+    public List<ServicePort> getServicePortsFrom(Set<String> exposedPorts) {
+        List<ServicePort> servicePorts = new ArrayList<>();


size is known so we can define it

benoitf · 2017-01-19T23:04:34Z

plugins/plugin-maven/maven-server/maven-server-impl/src/test/resources/EffectivePom/pom.xml

@@ -1214,23 +1214,6 @@
                                </configuration>
                            </execution>
                            <execution>
-                                <id>distribution-management-used</id>


why is it removed ?

ibuziuk · 2017-01-20T09:01:04Z

@benoitf thank you for review. Working on improvements..

ibuziuk · 2017-01-20T18:14:28Z

ci-build

most of the comments were addressed

benoitf · 2017-01-23T13:13:03Z

ci-build

codenvy-ci · 2017-01-23T16:15:08Z

Build # 1737 - FAILED

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

ibuziuk · 2017-01-23T16:55:54Z

@l0rd could you please push the build ? it seems to be working only for committers. There is one regression test failure, but all tests pass on my machine

ibuziuk · 2017-01-23T16:56:34Z

@garagatyi @gazarenkov guys, could you please review the PR ?

benoitf · 2017-01-23T17:05:23Z

ci-build

garagatyi · 2017-01-23T17:05:28Z

ci-build

garagatyi · 2017-01-24T14:38:23Z

Guys how do you want to merge this PR? Are you going to do merge with rebase or squash or just merge?

garagatyi · 2017-01-24T14:39:36Z

@gazarenkov @benoitf Can you take one more look?

ibuziuk · 2017-01-24T14:53:44Z

@garagatyi thanks for review. I would rather opt for "just merge" as there are commits from different authors in the PR. @l0rd WDYT ?

garagatyi · 2017-01-24T15:14:40Z

Maybe you can try to rebase with newest master at the time you will merge and if there will be no conflicts then merge this PR with Github's merge with rebase? @l0rd WDYT?

ibuziuk · 2017-01-24T16:23:56Z

@garagatyi after some discussion it was decided to squash last three commits and use "Rebase and Merge" strategy for applying the PR. Also, there is some problem with ip-validation - all commits have "Signed-off-by" section but the check still fails

benoitf · 2017-01-24T16:28:40Z

@ibuziuk sometimes the script is failing

l0rd · 2017-01-24T16:33:44Z

@garagatyi Yes we would like to keep the 3 significant commit and squash last two commits. Then rebase + merge as soon as soon as review is complete.

ibuziuk · 2017-01-24T16:33:52Z

@benoitf oh, ok
@garagatyi yes, we have also decided to do it this way. PR has been rebased on top of master

benoitf

@gazarenkov @benoitf Can you take one more look?

I give a new look at it
I've commented with some errors reported by FindBugs or SonarLint
OpenShift connector class is tested at:
7% method, 6% lines covered

while DockerConnector is tested at
100% methods, 95% line covered

Note: the kubernetes module is better tested: 100% classes, 89% lines covered

benoitf · 2017-01-26T13:20:38Z

...enshift-client/src/main/java/org/eclipse/che/plugin/openshift/client/OpenShiftConnector.java

+            openShiftClient.resource(replicaSet).delete();
+        }
+
+        if (pod != null) {


using SonarLint, I have the warning that this variable is always non null

benoitf · 2017-01-26T13:21:55Z

...enshift-client/src/main/java/org/eclipse/che/plugin/openshift/client/OpenShiftConnector.java

+
+        String containerID = waitAndRetrieveContainerID(deploymentName);
+        if (containerID == null) {
+            throw new RuntimeException("Failed to get the ID of the container running in the OpenShift pod");


sonarlint hint is to subclass this exception

Using such generic exceptions as Error, RuntimeException, Throwable, and Exception prevents calling methods from handling true, system-generated exceptions differently than application-generated errors. Noncompliant Code Example public void foo(String bar) throws Throwable { // Noncompliant throw new RuntimeException("My Message"); // Noncompliant } Compliant Solution public void foo(String bar) { throw new MyOwnRuntimeException("My Message"); }

benoitf · 2017-01-26T13:57:58Z

...c/main/java/org/eclipse/che/plugin/openshift/client/kubernetes/KubernetesLabelConverter.java

+     */
+    public Map<String, String> labelsToNames(Map<String, String> labels) {
+        Map<String, String> names = new HashMap<>();
+        for (Map.Entry<String, String> entry : labels.entrySet()) {


SonarLint is flagging this loop as error

Loops should not contain more than a single "break" or "continue" statement squid:S135 Restricting the number of break and continue statements in a loop is done in the interest of good structured programming. One break and continue statement is acceptable in a loop, since it facilitates optimal coding. If there is more than one, the code should be refactored to increase readability.

the method has been refactored

benoitf · 2017-01-26T13:58:44Z

...enshift-client/src/main/java/org/eclipse/che/plugin/openshift/client/OpenShiftConnector.java

+    }
+
+    private Pod getChePodByContainerId(String containerId) throws IOException {
+        Map<String, String> podLabels = new HashMap<>();


findbugs shows that podLabels is initialized but never used in that method

Signed-off-by: Mario Loriedo <[email protected]>

Adds class so that choice between DockerConnector and OpenShiftConnector can be made via setting a property in che.properties. The provider is injected instead of DockerConnector, and provides the appropriate implementation through a get() method. Signed-off-by: Angel Misevski <[email protected]>

…resources (KubernetesContainer, KubernetesService, KubernetesEnvVar) Signed-off-by: Ilya Buziuk <[email protected]>

ibuziuk · 2017-01-26T18:59:21Z

@benoitf thanks again for reviewing. PR has been updated with fixes and rebased on the current master branch. In terms of test coverage there are several reasons why OpenShiiftConnector is not well-covered in comparison with kubernetes package:

kubernetes package is better tested because most of it's functionality is coupled with String manipulations (adjusting labels, ports, env. vars from / to kubernetes API). However, for OpenshiftConnector meaningful integration tests against minishift cluster are required and it simply can not be properly tested via unit tests
For now OpenShiftConnector extends DockerConnector and there are a bunch of methods that are (will be) either empty of throw UnsupportedOperationException e.g. Network create / remove / update. Once SPI for Che runtime management is defined I believe the code will be much cleaner and with better test coverage.

Basically, this PR is just initial minimal OpenShift integration piece and most of the code together with tests will be changed / refactored once Service Provider Interface is defined.

benoitf · 2017-01-26T23:47:53Z

@ibuziuk ok thanks for the fixes.
about tests I will wait the service provider interface then.

riuvshin · 2017-01-27T23:12:57Z

hey guys we have problems with compilation of https://github.com/codenvy/codenvy project seems due to those changes. @garagatyi @benoitf can you guys please take a look

01:03:40 /home/codenvy/workspace/codenvy-ci-master/plugins/plugin-hosted/codenvy-machine-hosted/src/main/java/com/codenvy/machine/HostedDockerInstance.java:61: error: incompatible types: DockerConnector cannot be converted to DockerConnectorProvider
01:03:40         super(docker,
01:03:40               ^

more info:

https://ci.codenvycorp.com/job/codenvy-ci-master/1293/console

amisevsk · 2017-01-27T23:22:20Z

Hi @riuvshin.

One of the changes this PR makes is to inject DockerConnectorProvider instead of DockerConnector. This is done because OpenShiftConnector is a replacement for DockerConnector when running Che on OpenShift.

This means changing the method signature for constructors using DockerConnector to use DockerConnectorProvider instead, and calling DockerConnectorProvider.get() to get the DockerConnector instance. To accommodate this change in Codenvy, classes calling these constructors will have to be changed.

I'll be available later to explain the required fixes in more detail.

garagatyi · 2017-01-27T23:46:03Z

@riuvshin We missed this as consequence of missing QA cycle.

benoitf · 2017-01-27T23:53:18Z

I've created a PR codenvy/codenvy#1636
you may commit on my branch if it does not fix all issues or do whatever you want with that PR

Update code based on CHE #3798 updates

amisevsk · 2017-01-28T04:52:25Z

If it helps, the classes in the main codebase that are affected are KeysInjector, CgroupOOMDetector, DockerInstance, DockerInstanceProvider, DockerInstanceStopDetector, DockerProcess, MachineProviderImpl, DockerAbandonedResourcesCleaner, and LocalDockerModule.

Any classes calling these constructors directly would need to be modified, as in @benoitf's PR.

ibuziuk force-pushed the CHE-26 branch from 686380f to 595c68d Compare January 18, 2017 23:03

l0rd mentioned this pull request Jan 19, 2017

Packaging Che to run on OpenShift #2847

Closed

14 tasks

ibuziuk force-pushed the CHE-26 branch from 595c68d to b9e40c7 Compare January 19, 2017 22:41

benoitf previously requested changes Jan 19, 2017

View reviewed changes

TylerJewell requested review from garagatyi and gazarenkov January 20, 2017 00:09

ibuziuk force-pushed the CHE-26 branch 2 times, most recently from b8ccae9 to 0275d07 Compare January 20, 2017 17:54

benoitf assigned ibuziuk Jan 23, 2017

benoitf added the kind/enhancement A feature request - must adhere to the feature request template. label Jan 23, 2017

ibuziuk force-pushed the CHE-26 branch from 0275d07 to ae89269 Compare January 23, 2017 16:58

ibuziuk force-pushed the CHE-26 branch from 0752eb4 to 0c1aa37 Compare January 24, 2017 16:17

gazarenkov approved these changes Jan 26, 2017

View reviewed changes

benoitf reviewed Jan 26, 2017

View reviewed changes

l0rd and others added 3 commits January 26, 2017 18:58

Added an OpenShift connector for container creation

b691089

Signed-off-by: Mario Loriedo <[email protected]>

CHE-58: Refactoring. Introducing new classes for handling Kubernetes …

b84013d

…resources (KubernetesContainer, KubernetesService, KubernetesEnvVar) Signed-off-by: Ilya Buziuk <[email protected]>

ibuziuk force-pushed the CHE-26 branch from 0c1aa37 to b84013d Compare January 26, 2017 18:24

benoitf approved these changes Jan 26, 2017

View reviewed changes

ibuziuk mentioned this pull request Jan 27, 2017

Switching to eclipse/che master branch kbsingh/build-run-che#14

Merged

l0rd merged commit 1ec641a into eclipse-che:master Jan 27, 2017

benoitf added this to the 5.2.0 milestone Jan 27, 2017

benoitf mentioned this pull request Jan 27, 2017

Update code based on CHE #3798 updates codenvy/codenvy#1636

Merged

garagatyi pushed a commit to codenvy/codenvy that referenced this pull request Jan 28, 2017

Merge pull request #1636 from eclipse-che/che#3798

adaf220

Update code based on CHE #3798 updates

ibuziuk mentioned this pull request Jan 31, 2017

Need to document the process of deployment Che on OpenShift #3817

Closed

5 tasks

JamesDrummond mentioned this pull request Feb 6, 2017

5.2 Milestone Overview #3976

Closed

9 tasks

CHE-26: Initial OpenShift integration #3798

CHE-26: Initial OpenShift integration #3798

Conversation

ibuziuk commented Jan 18, 2017 • edited by JamesDrummond Loading

What does this PR do?

What issues does this PR fix or reference?

New behavior

Tests written?

Changelog and Release Note Information

Changelog

Docs Pull Request:

codenvy-ci commented Jan 18, 2017

ibuziuk commented Jan 18, 2017

TylerJewell commented Jan 18, 2017

gorkem commented Jan 18, 2017

TylerJewell commented Jan 18, 2017

ibuziuk commented Jan 18, 2017

TylerJewell commented Jan 19, 2017

benoitf commented Jan 19, 2017

ibuziuk commented Jan 19, 2017 • edited Loading

codenvy-ci commented Jan 19, 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

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

ibuziuk commented Jan 20, 2017

ibuziuk commented Jan 20, 2017

benoitf commented Jan 23, 2017

codenvy-ci commented Jan 23, 2017

ibuziuk commented Jan 23, 2017

ibuziuk commented Jan 23, 2017

benoitf commented Jan 23, 2017

garagatyi commented Jan 23, 2017

garagatyi commented Jan 24, 2017

garagatyi commented Jan 24, 2017

ibuziuk commented Jan 24, 2017

garagatyi commented Jan 24, 2017

ibuziuk commented Jan 24, 2017

benoitf commented Jan 24, 2017

l0rd commented Jan 24, 2017

ibuziuk commented Jan 24, 2017

benoitf 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

ibuziuk commented Jan 26, 2017

benoitf commented Jan 26, 2017

riuvshin commented Jan 27, 2017 • edited Loading

amisevsk commented Jan 27, 2017

garagatyi commented Jan 27, 2017

benoitf commented Jan 27, 2017

amisevsk commented Jan 28, 2017

ibuziuk commented Jan 18, 2017 •

edited by JamesDrummond

Loading

ibuziuk commented Jan 19, 2017 •

edited

Loading

benoitf left a comment •

edited

Loading

riuvshin commented Jan 27, 2017 •

edited

Loading