Fixes various problems usage of OpenShift client #7629

akorneta · 2017-11-29T14:44:08Z

What does this PR do?

Replaces the usage of one OpenShiftClient per action to one client per che-server. The reason why we do that is the OpenShiftClient holds an instance of OkHttpClient inside and each time when we create new OpenShiftClient we create new OkHttpClient with all the components inside, on close, the OpenShiftClient prevent cancel all calls currently that are executed by OkHttpClient, close all the connections and stop the dispatcher executor service (that undoubtedly is overhead).
Adds closing of watcher on pod removing.
Disables abnormal stop and container events watching.
Launches bootstrapper as a detached process.

What issues does this PR fix or reference?

#7418
This problem is not reproduced in case of using one OpenShift client per che server: #5902

garagatyi · 2017-11-29T16:04:19Z

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

@@ -103,7 +103,8 @@ protected void doBootstrapAsync(String installerWebsocketEndpoint, String output
            + Integer.toString(installerTimeoutSeconds)
            + " -file "
            + BOOTSTRAPPER_DIR
-            + CONFIG_FILE);
+            + CONFIG_FILE
+            + " &>/dev/null &");


Please add a comment why you've added this. It is not obvious that it is a way to make process detached.

garagatyi · 2017-11-29T16:04:37Z

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

@@ -107,8 +107,9 @@ protected void internalStart(Map<String, String> startOptions) throws Infrastruc
      for (Route route : osEnv.getRoutes().values()) {
        createdRoutes.add(project.routes().create(route));
      }
-      project.pods().watch(new AbnormalStopHandler());
-      project.pods().watchContainers(new MachineLogsPublisher());
+      // TODO link with issue


Please add the corresponding issue.

garagatyi · 2017-11-29T16:05:39Z

...src/main/java/org/eclipse/che/workspace/infrastructure/openshift/OpenShiftClientFactory.java

@@ -52,7 +59,7 @@ public OpenShiftClientFactory(
    if (doTrustCerts != null) {
      configBuilder.withTrustCerts(doTrustCerts);
    }
-    config = configBuilder.build();
+    this.client = new DefaultOpenShiftClient(configBuilder.build());


Consider adding a decorator that would eliminate the risk of calling close on the client.

garagatyi · 2017-11-29T16:06:39Z

.../src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftPods.java

@@ -65,6 +64,9 @@
  private static final Pattern CONTAINER_FIELD_PATH_PATTERN =
      Pattern.compile("spec.containers\\{(?<" + CONTAINER_NAME_GROUP + ">.*)}");

+  // TODO make it configurable
+  public static final int POD_REMOVAL_TIMEOUT_MIN = 5;


Please create an issue for this todo or remove it

garagatyi · 2017-11-29T16:17:36Z

.../src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftPods.java

      } catch (InterruptedException e) {
        Thread.currentThread().interrupt();
        throw new InfrastructureException(
            "Interrupted while waiting for pod removal. " + e.getMessage());
      } catch (ExecutionException e) {
        throw new InfrastructureException(
            "Error occurred while waiting for pod removing. " + e.getMessage());
+      } catch (TimeoutException ex) {
+        throw new InfrastructureException("Pods removal timeout reached " + ex.getMessage());


Can you elaborate on what will happen in this situation with cleanup of other resources and with the possible following start of the same workspace?

It is similar to other exception that is possible on cleanup, I would say that we should revise cleanup in general. Because if any step of cleanup is failed then, other resources won't be cleaned. I'll create an issue for that.

garagatyi · 2017-11-29T16:18:09Z

.../src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftPods.java

+      return deleteFuture
+          .thenAccept((v) -> watch.close())
+          .exceptionally(
+              e -> {


Shouldn't we also log the exception?

added exception logging

garagatyi

Looks good. The investigation is cool!

sleshchenko · 2017-11-30T08:19:34Z

.../src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftPods.java

-      return deleteFuture;
+
+      return deleteFuture
+          .thenAccept((v) -> watch.close())


Looks like it's the same as

return deleteFuture .handle((v, e) -> { watch.close(); return null; });

But it will supress rethrowing of exception from original delete future. Is it OK?

sleshchenko · 2017-11-30T15:21:21Z

...src/main/java/org/eclipse/che/workspace/infrastructure/openshift/OpenShiftClientFactory.java

+  /** Decorates the {@link DefaultOpenShiftClient} so that it can not be closed from the outside. */
+  private static class UnclosableOpenShiftClient extends DefaultOpenShiftClient {
+
+    public UnclosableOpenShiftClient(OpenShiftConfig build) {


Let's rename it to config =)

sleshchenko · 2017-11-30T15:28:16Z

...src/main/java/org/eclipse/che/workspace/infrastructure/openshift/OpenShiftClientFactory.java


 /** @author Sergii Leshchenko */
+@Singleton


Don't be shy to add yourself as an author 😉

garagatyi

Good job!

akorneta added kind/bug Outline of a bug - must adhere to the bug report template. status/in-progress This issue has been taken by an engineer and is under active development. target/che6 labels Nov 29, 2017

akorneta self-assigned this Nov 29, 2017

akorneta requested review from skabashnyuk, garagatyi and sleshchenko November 29, 2017 14:44

akorneta mentioned this pull request Nov 29, 2017

Upgrade OpenShift Kubernetes OkHttp clients versions eclipse-che/che-dependencies#78

Merged

garagatyi reviewed Nov 29, 2017

View reviewed changes

sleshchenko reviewed Nov 30, 2017

View reviewed changes

akorneta force-pushed the CHE-5902 branch from 709f2ba to 312dad1 Compare November 30, 2017 09:48

This was referenced Nov 30, 2017

Alternative implementation of abnormal machine stop and container events propagation #7653

Closed

Make OpenShift pod removal timeout configurable #7656

Closed

akorneta added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. and removed status/in-progress This issue has been taken by an engineer and is under active development. labels Nov 30, 2017

sleshchenko reviewed Nov 30, 2017

View reviewed changes

sleshchenko approved these changes Nov 30, 2017

View reviewed changes

sleshchenko reviewed Nov 30, 2017

View reviewed changes

garagatyi approved these changes Nov 30, 2017

View reviewed changes

akorneta force-pushed the CHE-5902 branch from 6bf3763 to eaa1ca6 Compare November 30, 2017 16:13

akorneta added 4 commits November 30, 2017 18:14

Make OpenShiftClient one per che server

f1af421

Make bootstrapper start command detached

4ee80ba

Add closing of pod removal watcher

ea712b2

Disable AbnormalStopHandler watching and Container events watching

af16ab2

akorneta force-pushed the CHE-5902 branch from eaa1ca6 to af16ab2 Compare November 30, 2017 16:14

akorneta merged commit 09df5d5 into che6 Nov 30, 2017

akorneta deleted the CHE-5902 branch November 30, 2017 16:27

akorneta 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 30, 2017

benoitf added this to the 6.0.0-M3 milestone Nov 30, 2017

ibuziuk mentioned this pull request Apr 9, 2018

Provide a better error message when PV failed to be attached redhat-developer/rh-che#557

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes various problems usage of OpenShift client #7629

Fixes various problems usage of OpenShift client #7629

akorneta commented Nov 29, 2017

garagatyi Nov 29, 2017

garagatyi Nov 29, 2017

garagatyi Nov 29, 2017

garagatyi Nov 29, 2017

garagatyi Nov 29, 2017

akorneta Nov 30, 2017

akorneta Nov 30, 2017

garagatyi Nov 29, 2017

akorneta Nov 30, 2017

garagatyi left a comment

sleshchenko Nov 30, 2017

sleshchenko Nov 30, 2017

sleshchenko Nov 30, 2017

garagatyi left a comment

Fixes various problems usage of OpenShift client #7629

Fixes various problems usage of OpenShift client #7629

Conversation

akorneta commented Nov 29, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

garagatyi left a comment

Choose a reason for hiding this comment