Replaced openshift-rest-client library with fabric8.kubernetes #3381

l0rd · 2016-12-14T13:34:02Z

Replace library (openshift-restclient)[https://github.com/openshift/openshift-restclient-java] with (fabric8-kubernetes)[https://github.com/fabric8io/kubernetes-client] to access the OpenShift API.

@ibuziuk and @amisevsk please review

Signed-off-by: Mario Loriedo mario.loriedo@gmail.com

codenvy-ci · 2016-12-14T13:39:02Z

Can one of the admins verify this patch?

benoitf · 2016-12-14T13:40:17Z

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

-                </exclusions>
+                <groupId>io.fabric8</groupId>
+                <artifactId>openshift-client</artifactId>
+                <version>${io.fabric8.openshift-client.version}</version>


block should be in https://github.com/eclipse/che-dependencies/blob/master/pom.xml

and each dependency needs to have a CQ
I see one (your request) which is https://dev.eclipse.org/ipzilla/show_bug.cgi?id=12209 but it's for version 1.4 while here I see 1.4.26

Thanks for your review @benoitf. Indeed this code is not ready to be merged to master yet.

Are you sure that a CQ for version 1.4 (which by the way doesn't exist) isn't valid for 1.4.* versions too?

@l0rd AFAIK each version need to be approved as it's based on the source code provided.

IMHO migrating from 1.4.25 to 1.4.26 requires a new CQ

also if I remember, even branches should have valid CQs (only code that is not in eclipse/che repository can use any code (for example your fork)

ibuziuk · 2016-12-14T18:25:33Z

...docker-client/src/test/java/org/eclipse/che/plugin/docker/client/OpenShiftConnectorTest.java

@@ -5,6 +5,9 @@
 import com.openshift.restclient.IResourceFactory;
 import com.openshift.restclient.model.IPort;
 import com.openshift.restclient.model.IServicePort;


Need to remove obsolete com.openshift.restclient.* imports

ibuziuk · 2016-12-14T19:29:21Z

@l0rd after switch to fabric8 I am not able to create any workspace. The problem seems to be coupled with "createOpenShiftService" method

ibuziuk · 2016-12-15T17:38:29Z

@lord here is the stacktrace:

org.eclipse.che.plugin.docker.machine.MachineProviderImpl.startService(MachineProviderImpl.java:276) ~[che-plugin-docker-machine-5.0.0-M8-SNAPSHOT.jar:5.0.0-M8-SNAPSHOT]
	... 11 common frames omitted
Caused by: javax.net.ssl.SSLPeerUnverifiedException: Hostname che.openshift.mini not verified:
    certificate: sha256/RW/0ZcaQ1MoE7yV6k1r0atg9d26hVQQ33FUId0j4ne0=
    DN: CN=127.0.0.1
    subjectAltNames: [127.0.0.1, 172.17.0.1, 172.30.0.1, 192.168.122.59, 192.168.42.195, kubernetes, kubernetes.default, kubernetes.default.svc, kubernetes.default.svc.cluster.local, localhost, openshift, openshift.default, openshift.default.svc, openshift.default.svc.cluster.local, 127.0.0.1, 172.17.0.1, 172.30.0.1, 192.168.122.59, 192.168.42.195]

it looks like che.openshift.mini hostname is forbidden to use

ibuziuk · 2016-12-15T18:58:13Z

@benoitf yes, we are going to open a CQ for openshift-client 1.4.26 version. In the meantime I was trying to setup a build without using "-Dskip-enforce". I have added openshift-client to che-dependencies, but when I run the build I have the following error:

[INFO] Add the following to your pom to correct the missing dependencies: 
[INFO] 
<dependency>
  <groupId>io.fabric8</groupId>
  <artifactId>kubernetes-model</artifactId>
  <version>1.0.63</version>
</dependency>
<dependency>
  <groupId>io.fabric8</groupId>
  <artifactId>kubernetes-client</artifactId>
  <version>1.4.26</version>
</dependency>

So, in order to make it work I had to add those dependencies to pom.xml and che-dependencies - ibuziuk/che-dependencies@228bb81

Basically, openshift-client depends on those libs:

[INFO] +- io.fabric8:openshift-client:jar:1.4.26:compile
[INFO] |  \- io.fabric8:kubernetes-client:jar:1.4.26:compile
[INFO] |     +- io.fabric8:kubernetes-model:jar:1.0.63:compile
[INFO] |     |  \- com.fasterxml.jackson.module:jackson-module-jaxb-annotations:jar:2.7.5:compile
[INFO] |     +- com.squareup.okhttp3:okhttp:jar:3.4.1:compile
[INFO] |     |  \- com.squareup.okio:okio:jar:1.9.0:compile
[INFO] |     +- com.squareup.okhttp3:logging-interceptor:jar:3.4.1:compile
[INFO] |     +- com.squareup.okhttp3:okhttp-ws:jar:3.4.1:compile
[INFO] |     +- org.slf4j:jul-to-slf4j:jar:1.7.6:compile
[INFO] |     +- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.5.0:compile
[INFO] |     +- com.fasterxml.jackson.core:jackson-databind:jar:2.5.0:compile
[INFO] |     |  \- com.fasterxml.jackson.core:jackson-annotations:jar:2.5.0:compile
[INFO] |     +- com.fasterxml.jackson.core:jackson-core:jar:2.5.0:compile
[INFO] |     +- io.fabric8:zjsonpatch:jar:0.2.3:compile
[INFO] |     \- com.github.mifmif:generex:jar:1.0.1:compile
[INFO] |        \- dk.brics.automaton:automaton:jar:1.11-8:compile

Am I correct that opening CQs for kubernetes-client & kubernetes-model is not required and they can be added once CQ for openshift-client 1.4.26 will be approved ?

amisevsk · 2016-12-15T20:38:22Z

@ibuziuk @l0rd I don't seem to have any issue creating a workspace.

amisevsk

The changes seem good and the fabric8.kubernetes api seems to be a lot easier to read and work with. However there is a weird issue with stopping workspaces where deletion does not seem to be complete. I'll have to look into it more and test it on another machine later.

amisevsk · 2016-12-15T20:39:40Z

...gin-docker-client/src/main/java/org/eclipse/che/plugin/docker/client/OpenShiftConnector.java

+import io.fabric8.kubernetes.api.model.PodSpecBuilder;
+import io.fabric8.kubernetes.api.model.Probe;
+import io.fabric8.kubernetes.api.model.ProbeBuilder;
+import io.fabric8.kubernetes.api.model.ReplicationController;


ReplicationController is an unused dependency.

amisevsk · 2016-12-15T20:57:44Z

...docker-client/src/test/java/org/eclipse/che/plugin/docker/client/OpenShiftConnectorTest.java


        // Then
-        List<String> keysAndValues = env.keySet().stream().map(k -> k + "=" + env.get(k)).collect(Collectors.toList());
+        List<String> keysAndValues = env.stream().map(k -> k.getName() + "=" + k.getValue()).collect(Collectors.toList());
        assertTrue(Arrays.stream(envVariablesOut).anyMatch(keysAndValues::contains));


This test fails (and has been failing since bc0717a). We should fix or remove it.

Yes good catch. This test didn't make sense anymore. I removed it.

amisevsk · 2016-12-15T22:02:58Z

...gin-docker-client/src/main/java/org/eclipse/che/plugin/docker/client/OpenShiftConnector.java

-        openShiftClient.delete(pod);
+        if ( pod != null ) {
+            LOG.info("Removing OpenShift Pod " + pod.getMetadata().getName());
+            openShiftClient.resource(pod).delete();


I think this may be causing an issue where OpenShift prevents full deletion of the pod. After I stop a workspace, OpenShift immediately recreates the pod, which is then stuck in ImagePullBackOff

> oc get pods NAME READY STATUS RESTARTS AGE che-host-1-oas8k 1/1 Running 0 1h che-ws-946kcw4tm6djd9k9-1058218227-rkm5w 0/1 ImagePullBackOff 0 5m che-ws-y714ql3djvc4q1dy-1150559083-dyo0h 0/1 ImagePullBackOff 0 6m router-1-wotpb 2/2 Running 10 20d

These pods have replica sets that are not removed, and so they are recreated. Since a pod with the name exists, the corresponding workspace cannot be restarted.

> oc get replicasets NAME DESIRED CURRENT AGE che-ws-946kcw4tm6djd9k9-1058218227 1 1 3m che-ws-y714ql3djvc4q1dy-1150559083 1 1 1h >oc get deployments NAME DESIRED CURRENT UP-TO-DATE AVAILABLE AGE che-ws-946kcw4tm6djd9k9 1 1 1 0 21m che-ws-y714ql3djvc4q1dy 1 1 1 0 1h

I've fixed that too thanks

l0rd · 2016-12-16T03:48:49Z

@ibuziuk I can't reproduce your issue neither now. But I had this behaviour when I first started to test fabric8. That's related to the SSL certificate of minishift but I have no clue how to solve it: the issue has disappeared on my laptop without any particular change in the code or config (maybe try restarting minishift).

ibuziuk · 2016-12-16T12:45:07Z

@l0rd deleting minishift cluster and creating it from scratch helped \o/ Not sure but might be coupled with a fact that I had obsolete openshift/origin-haproxy-router image. +1 for this changes once CQ is approved

ibuziuk · 2016-12-19T17:00:17Z

...gin-docker-client/src/main/java/org/eclipse/che/plugin/docker/client/OpenShiftConnector.java

-        openShiftClient.delete(pod);
+        if ( pod != null ) {
+            LOG.info("Removing OpenShift Pod " + pod.getMetadata().getName());
+            openShiftClient.resource(pod).delete();


@l0rd missing curly brace after rebase

benoitf · 2016-12-19T17:05:03Z

...gin-docker-client/src/main/java/org/eclipse/che/plugin/docker/client/OpenShiftConnector.java

-        IService svc = getCheServiceBySelector("deploymentConfig", deploymentConfig);
-        Map<String, String> annotations = convertKubernetesNamesToLabels(svc.getAnnotations());
+        Pod pod = getChePodByContainerId(info.getId());
+        if (pod == null ) {


spaces for if () are different in the file

expected formatting is
if (var != value)

we have
if ( var != value )
or
if (var != value )
or
if (var!=value)

Signed-off-by: Mario Loriedo <mloriedo@redhat.com>

l0rd · 2016-12-20T10:36:26Z

@ibuziuk @benoitf guys thank you for reviewing this. I did the necessary updates.

The CQ has been updated with the right version (1.4.26). @benoitf everything should be ok to merge this to openshift-connector branch?

benoitf · 2016-12-20T10:44:15Z

@l0rd yes for this PR all is fine for me.
Issue on CQ is only on eclipse-che/che-dependencies#23

benoitf reviewed Dec 14, 2016

View reviewed changes

ibuziuk reviewed Dec 14, 2016

View reviewed changes

l0rd force-pushed the openshift-connector-fabric8 branch from 0200c9d to 47e77c6 Compare December 15, 2016 13:58

amisevsk reviewed Dec 15, 2016

View reviewed changes

l0rd force-pushed the openshift-connector-fabric8 branch from 47e77c6 to d2a667e Compare December 16, 2016 03:21

l0rd force-pushed the openshift-connector-fabric8 branch from d2a667e to 01bae4d Compare December 19, 2016 16:33

ibuziuk reviewed Dec 19, 2016

View reviewed changes

benoitf reviewed Dec 19, 2016

View reviewed changes

ibuziuk mentioned this pull request Dec 19, 2016

CHE-55: Adding io.fabric8 dependencies eclipse-che/che-dependencies#23

Merged

skabashnyuk requested review from skabashnyuk and garagatyi December 19, 2016 20:38

skabashnyuk approved these changes Dec 20, 2016

View reviewed changes

skabashnyuk removed the request for review from garagatyi December 20, 2016 09:37

Replaced openshift-rest-client library with fabric8.kubernetes

3e80ee6

Signed-off-by: Mario Loriedo <mloriedo@redhat.com>

l0rd force-pushed the openshift-connector-fabric8 branch from 01bae4d to 3e80ee6 Compare December 20, 2016 10:26

benoitf approved these changes Dec 20, 2016

View reviewed changes

l0rd merged commit b087db8 into eclipse-che:openshift-connector Dec 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced openshift-rest-client library with fabric8.kubernetes #3381

Replaced openshift-rest-client library with fabric8.kubernetes #3381

l0rd commented Dec 14, 2016

codenvy-ci commented Dec 14, 2016

benoitf Dec 14, 2016

l0rd Dec 14, 2016

benoitf Dec 14, 2016

ibuziuk Dec 14, 2016

ibuziuk commented Dec 14, 2016

ibuziuk commented Dec 15, 2016

ibuziuk commented Dec 15, 2016 •

edited

Loading

amisevsk commented Dec 15, 2016

amisevsk left a comment

amisevsk Dec 15, 2016

l0rd Dec 16, 2016

amisevsk Dec 15, 2016

l0rd Dec 16, 2016

amisevsk Dec 15, 2016

l0rd Dec 16, 2016

l0rd commented Dec 16, 2016

ibuziuk commented Dec 16, 2016

ibuziuk Dec 19, 2016

benoitf Dec 19, 2016

l0rd commented Dec 20, 2016

benoitf commented Dec 20, 2016

Replaced openshift-rest-client library with fabric8.kubernetes #3381

Replaced openshift-rest-client library with fabric8.kubernetes #3381

Conversation

l0rd commented Dec 14, 2016

codenvy-ci commented Dec 14, 2016

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 Dec 14, 2016

ibuziuk commented Dec 15, 2016

ibuziuk commented Dec 15, 2016 • edited Loading

amisevsk commented Dec 15, 2016

amisevsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0rd commented Dec 16, 2016

ibuziuk commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0rd commented Dec 20, 2016

benoitf commented Dec 20, 2016

ibuziuk commented Dec 15, 2016 •

edited

Loading