Added propagation of plugin broker failing error by sleshchenko · Pull Request #11344 · eclipse-che/che

sleshchenko · 2018-09-25T09:54:19Z

What does this PR do?

It adds propagation of plugin broker failing error and instead of Plugins installation process timed out.
Changes are separate into different commits:

CHE-11070 Add waiting until Plugin Broker Pod become RUNNING before proceeding to next phase. But actually, it does not solve an issue with propagating original error since Restart policy is Always because of creating pods via Deployments and Pod status is always Running while a particular container may be failed and restarted with some period.
Make BrokerEvent suitable to be used for STARTED status event. It includes renaming BrokerResultEvent to BrokerStatusChangedEvent and removes check that error or plugin list is required.
Extract unrecoverable events listener from KubernetesInternalRuntime. It is needed to be able to reuse it in Plugin Broker.
Added listening to unrecoverable events during Plugin Broker starting.

Example what a user sees when plugin broker fails to start because of non-existing image

What issues does this PR fix or reference?

#11070
#11068

Release Notes

N/A

Docs PR

N/A

garagatyi

Looks good. I have commented on some stuff, please, take a look at inlined comments.
I would LOVE to see unit tests in the PR too! I know that it is me who haven't had them in place firsthand, but the code gets complicated. I started adding unit tests in my recent PRs and would really love some assistance :-)

garagatyi · 2018-09-25T12:01:28Z

...org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/DeployBroker.java

+
+    if (pods.size() > 1) {
+      throw new InternalInfrastructureException(
+          "There are multiply pods in plugin broker environment");


WDYT if we change two checks to just one? Something like:
Plugin brokering supports one pod only. Workspace 'ID' contains 'N' pods

Good idea.
Changed to

format("Plugin broker environment must have only one pod. Workspace %s contains %s pods.", workspaceId, pods.size())

Do you think it is good enough?

garagatyi · 2018-09-25T13:46:20Z

...java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/events/BrokerService.java

    String encodedTooling = event.getTooling();
-    if (event.getStatus() == null
-        || event.getWorkspaceId() == null
-        || (event.getError() == null && event.getTooling() == null)) {


I believe that null tooling may cause an actual NPE here. Please, ensure we don't have an NPE here.
We also can have an NPE because of that. So would be nice to fix this too.

Makes sense. I've added a check before completion of future, and the whole plugin broker process will be failed if DONE event is published but plugins are null 4858738

garagatyi · 2018-09-25T13:48:10Z

...g/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/events/BrokerStatusListener.java

-        finishFuture.completeExceptionally(
-            new InfrastructureException("Broker process failed with error: " + event.getError()));
+        InfrastructureException ex =
+            new InfrastructureException("Broker process failed with error: " + event.getError());


Can you also include the workspace ID in the message? Just in case.

garagatyi · 2018-09-25T14:17:49Z

.../java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/PluginBrokerManager.java

-    DeployBroker deployBroker = getDeployBrokerPhase(kubernetesNamespace, brokerEnvironment);
+    DeployBroker deployBroker =
+        getDeployBrokerPhase(
+            runtimeID.getWorkspaceId(), kubernetesNamespace, brokerEnvironment, startFuture);


I would rather set WS_ID in the BrokerEnvironmentFactory, WDYT?

It was done in the same way for ListenBrokerEvents and PrepareStorage.
Sorry, I'm not sure that I fully understand your idea. Do you propose to store WS_ID in KubernetesEnvvironment that is created by brokerEnvironmentFactory and then fetch this value in phases from there?

Oh I see that it is not in the K8sEnv. Sorry for confusing

sleshchenko · 2018-09-26T09:42:15Z

@garagatyi I've added few more tests classes, please let me know if you think that any other components need to be covered with tests.

sleshchenko · 2018-09-26T12:50:02Z

ci-test

riuvshin · 2018-09-26T13:13:35Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:11344
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ibuziuk

+1 but I have only reviewed bits related to Unrecoverable events

sleshchenko · 2018-09-26T13:43:12Z

ci-test

riuvshin · 2018-09-26T16:43:08Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:11344
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

Ohrimenko1988 · 2018-09-27T07:54:31Z

ci-test

riuvshin · 2018-09-27T10:54:52Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:11344
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

Ohrimenko1988 · 2018-09-27T12:49:18Z

The selenium tests did not reveal any regression in next report #11344 (comment)

sleshchenko added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Sep 25, 2018

sleshchenko self-assigned this Sep 25, 2018

sleshchenko requested review from garagatyi and ibuziuk September 25, 2018 09:54

garagatyi reviewed Sep 25, 2018

View reviewed changes

garagatyi approved these changes Sep 26, 2018

View reviewed changes

ibuziuk approved these changes Sep 26, 2018

View reviewed changes

sleshchenko force-pushed the brokerPodStatus branch from 343c99a to f829da2 Compare September 27, 2018 07:19

garagatyi mentioned this pull request Sep 27, 2018

Revert "Renamed STARTING event to STARTED and added publishing of STARTED event" eclipse-che/che-plugin-broker#10

Closed

vkuznyetsov mentioned this pull request Sep 27, 2018

QE-2018-10-02 (Sprint: 155) #10992

Closed

47 tasks

sleshchenko added 3 commits September 27, 2018 17:49

Make BrokerEvent suitable to be used for STARTED status event

d9c210b

Extract unrecoverable events listener from KubernetesInternalRuntime

21798bf

Listen to unrecoverable events during Plugin Broker starting

adcbe9e

sleshchenko force-pushed the brokerPodStatus branch from f829da2 to adcbe9e Compare September 27, 2018 15:06

sleshchenko merged commit eddff2b into eclipse-che:master Sep 27, 2018

sleshchenko deleted the brokerPodStatus branch September 27, 2018 15:11

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 Sep 27, 2018

benoitf added this to the 6.12.0 milestone Sep 27, 2018

sleshchenko mentioned this pull request Nov 14, 2018

Remove unneeded listening of PluginBrokers Pod events #11940

Merged

Conversation

sleshchenko commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

Uh oh!

garagatyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sleshchenko commented Sep 26, 2018

Uh oh!

sleshchenko commented Sep 26, 2018

Uh oh!

riuvshin commented Sep 26, 2018

Uh oh!

ibuziuk left a comment

Choose a reason for hiding this comment

Uh oh!

sleshchenko commented Sep 26, 2018

Uh oh!

riuvshin commented Sep 26, 2018

Uh oh!

Ohrimenko1988 commented Sep 27, 2018

Uh oh!

riuvshin commented Sep 27, 2018

Uh oh!

Ohrimenko1988 commented Sep 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sleshchenko commented Sep 25, 2018 •

edited

Loading