Added an ability to interrupt the start of a Kubernetes/OpenShift runtime #9816

sleshchenko · 2018-05-25T11:23:44Z

What does this PR do?

Adds an ability to interrupt the start of a Kubernetes/OpenShift runtime.

It is done by adding Utility class StartSynchronizer that is designed to synchronize start/stop operations. Synchronization is performed by registering all start/stop operations (like start is launched, an error occurs during start, start is interrupted, etc.)
So, KubernetesInternalRuntime registers when the runtime is starting and then checks failure on each phase of start and interrupts start is a failure is registered.
In turn, KubernetesInternalRuntime checks that start is in progress on runtime stopping, if yes - then start failure will be registered. So, Thread which is performing start will receive the registered exception and interrupt start.

It also works in cluster mode, now there can be two Che Servers during Rolling Update. Synchronization between Che Servers is done by using JGroups Replicated Cache listeners. Each Che Server receives the corresponding event when a runtime stop is launched/completed.

There are 3 more changes:

Rework UnrecoverableEventHandler to fail workspace start instead interruption (earlier ) since there is such ability.
Improve executing of commands in KubernetesPods class to throw if occurs while a WebSocket connection establishing.
KubernetesBootstrapper now checks start interruption before execution each of commands. Without that there is much more probability that commands fails with the following error:

2018-06-01 15:33:21,204[20.200:8443/...]  [ERROR] [.k.c.d.i.ExecWebSocketListener 213]  - Exec Failure: HTTP:403. Message:pods "non-existing" is forbidden: pods "non-existing" not found
java.net.ProtocolException: Expected HTTP 101 response but was '403 Forbidden'
	at okhttp3.internal.ws.RealWebSocket.checkResponse(RealWebSocket.java:216)
	at okhttp3.internal.ws.RealWebSocket$2.onResponse(RealWebSocket.java:183)
	at okhttp3.RealCall$AsyncCall.execute(RealCall.java:141)
	at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

And it leads to connection was leaked warning

Jun 01, 2018 3:01:42 PM okhttp3.internal.platform.Platform log
WARNING: A connection to https://172.19.20.200:8443/ was leaked. Did you forget to close a response body? To see where this was allocated, set the OkHttpClient logger level to FINE: Logger.getLogger(OkHttpClient.class.getName()).setLevel(Level.FINE);

What issues does this PR fix or reference?

#5918

It also fixes #9905

Release Notes

Added an ability to interrupt the start of a Kubernetes/OpenShift runtime.

Docs PR

sleshchenko · 2018-05-31T15:10:45Z

ci-test

codenvy-ci · 2018-05-31T19:04:56Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:9816
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

garagatyi · 2018-06-04T14:29:41Z

...main/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesInternalRuntime.java

@@ -139,22 +140,30 @@ public KubernetesInternalRuntime(
    this.executor = sharedPool.getExecutor();
    this.runtimeStates = runtimeStates;
    this.machines = machines;
+    this.startSynchronizer = new StartSynchronizer(context.getIdentity(), eventService);


Side note: looks like event service is just a dependency of StartSynchronizer and KubernetesInternalRuntime doesn't have to have this dependency too. So it might be cleaner to create some factory for this class and hide its dependencies.

I thought about it, and while there is only one injected field creating one more class looks a bit overkill. I would prefer to leave it as it is for now.

garagatyi

Overall looks good.
I have concerns regarding the complexity of KubernetesinternalRuntime and WorkspaceRuntimes classes but it is not concern about this PR but general concern about their design. I encourage you and will try to do that eventually too, to simplify them in future PRs.
I also have some question/comments about this PR which I inlined. Please, elaborate.

garagatyi · 2018-06-04T14:31:16Z

...oups/src/main/java/org/eclipse/che/multiuser/api/distributed/cache/StatusChangeListener.java

+import org.eclipse.che.api.core.model.workspace.WorkspaceStatus;
+
+/**
+ * Listener interface for being notified about workspace status changes.


Can you specify in javadocs why it might be used instead of subscribing to events from EventsService?

Added note about clustering.

garagatyi · 2018-06-05T06:26:52Z

.../main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesPods.java

+      } catch (ExecutionException e) {
+        throw new InfrastructureException(
+            "Error occured while executing command in pod: " + e.getMessage(), e);
+      } catch (TimeoutException ignored) {


This exception is not ignored, so can you fix the variable name?

Not ignored by not used =) Renamed to e )

garagatyi · 2018-06-05T07:00:49Z

...tes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/StartSynchronizer.java

+   *
+   * @throws InternalInfrastructureException when {@link #startThread} is already set.
+   */
+  public synchronized void setStartThread() throws InternalInfrastructureException {


You can include this operation into start method.

I afraid I can't. Because start method is invoked in two places: #internalStart and #markStopping. While #setStartThread is invoked only by #internalStart method

garagatyi · 2018-06-05T07:05:31Z

...tes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/StartSynchronizer.java

+
+  /** Registers a runtime start. */
+  public synchronized void start() {
+    if (!isStarting) {


Can you elaborate what will happen if 2 servers will try to simultaneously start a runtime? Is there a protection from that on this or some another level?

WorkspaceRuntimes class should synchronize invocation of InternalRuntime#start.
So 2 server must not simultaneously start a runtime.

And it's possible that two threads will invoke StartSynchronizer#start method on start interruption because start method is invoked in two places: #internalStart and #markStopping.
Then the first call will set state to STARTING with performing the corresponding actions and the second call will do nothing.

garagatyi · 2018-06-05T07:06:56Z

...tes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/StartSynchronizer.java

+   * @throws RuntimeStartInterruptedException when start was interrupted
+   * @throws InfrastructureException when any other exception occurs during start
+   */
+  public synchronized void complete() throws InfrastructureException {


I see that methods are synchronized but fields might not be synced between threads caches. WDYT?

As far as I understand using synchronized method ensures synchronization shared data between threads, am I wrong?

But those fields are not shared between threads. They can be cached by each thread separately.

garagatyi · 2018-06-05T07:30:29Z

...main/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesInternalRuntime.java

@@ -106,6 +105,7 @@
  private final Executor executor;
  private final KubernetesRuntimeStateCache runtimeStates;
  private final KubernetesMachineCache machines;
+  private final StartSynchronizer startSynchronizer;

  @Inject
  public KubernetesInternalRuntime(


Seems OK, but, to be honest, I no longer can validate the logic of this class as it gets too complicated. I encourage you, and myself, to refactor this class as soon as possible to simplify it drastically.

garagatyi · 2018-06-05T07:32:33Z

...src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/StartSynchronizerTest.java

+ * @author Sergii Leshchenko
+ */
+@Listeners(MockitoTestNGListener.class)
+public class StartSynchronizerTest {


As far as I understand these tests don't check multi-threaded use cases that are covered by the StartSynchronizer design.

You're right

garagatyi

LGTM

sleshchenko · 2018-06-06T12:02:28Z

ci-test

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

It is needed to avoid '403 Pod doesn't exists' errors. It happens when start is interrupted when any of machines is on bootstrapping phase. As result connection leak happens TODO Create an issue for fabric8-client

…blishing a WebSocket connection

…curs

codenvy-ci · 2018-06-06T17:18:38Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:9816
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2018-06-07T05:34:32Z

@eclipse/eclipse-che-qa Please check test report.

Ohrimenko1988 · 2018-06-07T11:23:48Z

In the functionality which is covered by selenium tests - new regression was not found

sleshchenko added kind/enhancement A feature request - must adhere to the feature request template. status/in-progress This issue has been taken by an engineer and is under active development. labels May 25, 2018

sleshchenko self-assigned this May 25, 2018

sleshchenko requested a review from garagatyi as a code owner May 25, 2018 11:23

sleshchenko force-pushed the k8sStartInterruption branch from ac53757 to 0ba805f Compare May 29, 2018 08:08

sleshchenko mentioned this pull request May 30, 2018

Dashboard shows two the same notifications when a workspace start is interrupted #9872

Closed

sleshchenko requested review from riuvshin, skabashnyuk and vparfonov as code owners May 30, 2018 14:24

sleshchenko force-pushed the k8sStartInterruption branch 4 times, most recently from a7f0170 to c00881b Compare May 31, 2018 15:03

sleshchenko force-pushed the k8sStartInterruption branch 2 times, most recently from afbca11 to ebfda55 Compare May 31, 2018 15:41

skabashnyuk approved these changes Jun 1, 2018

View reviewed changes

sleshchenko 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 Jun 1, 2018

sleshchenko changed the title ~~[WIP] Added an ability to interrupt the start of a Kubernetes/OpenShift runtime~~ Added an ability to interrupt the start of a Kubernetes/OpenShift runtime Jun 1, 2018

mshaposhnik approved these changes Jun 1, 2018

View reviewed changes

sleshchenko mentioned this pull request Jun 1, 2018

Workspace can not be deleted after using not accessible image #9905

Closed

sleshchenko force-pushed the k8sStartInterruption branch from ccb8c11 to 0d30c1f Compare June 1, 2018 12:32

garagatyi reviewed Jun 4, 2018

View reviewed changes

garagatyi reviewed Jun 5, 2018

View reviewed changes

garagatyi approved these changes Jun 6, 2018

View reviewed changes

sleshchenko force-pushed the k8sStartInterruption branch 3 times, most recently from 5956e1f to a1bbeb4 Compare June 6, 2018 09:32

sleshchenko force-pushed the k8sStartInterruption branch from f0a800b to 7887c24 Compare June 6, 2018 12:15

sleshchenko added 4 commits June 6, 2018 15:17

CHE-5918 Add an ability to interrupt Kubernetes/OpenShift runtime start

e3fb7e3

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

CHE-5918 Add checking of start interruption by KubernetesBootstrapper

87638e2

It is needed to avoid '403 Pod doesn't exists' errors. It happens when start is interrupted when any of machines is on bootstrapping phase. As result connection leak happens TODO Create an issue for fabric8-client

Improve ExecWatchdog to rethrow exception when it occurred while esta…

5939392

…blishing a WebSocket connection

CHE-5918 Fix K8s/OS runtime start failing when unrecoverable event oc…

0b30ca1

…curs

sleshchenko force-pushed the k8sStartInterruption branch from 7887c24 to 0b30ca1 Compare June 6, 2018 12:17

vkuznyetsov mentioned this pull request Jun 7, 2018

QE-2018-06-19 (Sprint: 150) #9822

Closed

33 tasks

sleshchenko merged commit 8549d95 into eclipse-che:master Jun 7, 2018

sleshchenko deleted the k8sStartInterruption branch June 7, 2018 12:03

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 Jun 7, 2018

benoitf added this to the 6.7.0 milestone Jun 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added an ability to interrupt the start of a Kubernetes/OpenShift runtime #9816

Added an ability to interrupt the start of a Kubernetes/OpenShift runtime #9816

sleshchenko commented May 25, 2018 •

edited

Loading

sleshchenko commented May 31, 2018

codenvy-ci commented May 31, 2018

garagatyi Jun 4, 2018

sleshchenko Jun 4, 2018

garagatyi left a comment

garagatyi Jun 4, 2018

sleshchenko Jun 6, 2018

garagatyi Jun 5, 2018

sleshchenko Jun 6, 2018

garagatyi Jun 5, 2018

sleshchenko Jun 6, 2018

garagatyi Jun 5, 2018

sleshchenko Jun 6, 2018

garagatyi Jun 5, 2018

sleshchenko Jun 6, 2018

garagatyi Jun 6, 2018

garagatyi Jun 5, 2018

garagatyi Jun 5, 2018

sleshchenko Jun 6, 2018

garagatyi left a comment

sleshchenko commented Jun 6, 2018

codenvy-ci commented Jun 6, 2018

sleshchenko commented Jun 7, 2018 •

edited

Loading

Ohrimenko1988 commented Jun 7, 2018

Added an ability to interrupt the start of a Kubernetes/OpenShift runtime #9816

Added an ability to interrupt the start of a Kubernetes/OpenShift runtime #9816

Conversation

sleshchenko commented May 25, 2018 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

sleshchenko commented May 31, 2018

codenvy-ci commented May 31, 2018

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

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

garagatyi left a comment

Choose a reason for hiding this comment

sleshchenko commented Jun 6, 2018

codenvy-ci commented Jun 6, 2018

sleshchenko commented Jun 7, 2018 • edited Loading

Ohrimenko1988 commented Jun 7, 2018

sleshchenko commented May 25, 2018 •

edited

Loading

sleshchenko commented Jun 7, 2018 •

edited

Loading