che #9946: Better handling of unrecoverable events for k8s & openshift infra (processing both 'container' and 'pod' events) by ibuziuk · Pull Request #10155 · eclipse-che/che

ibuziuk · 2018-06-22T13:49:06Z

What does this PR do?

Better handling of unrecoverable events for k8s & openshift infra (processing both 'container' and 'pod' events). Previously only containers (sub-set of pod events) were processed. However, some unrecoverable events happens on pod level and simply not taken into account during processing Unrecoverable events. So, after discussion with @sleshchenko it was decided to process all pod events and make more precise filtering on a handler level e.g. MachineLogsPublisher will still publish only container events and UnrecoverableEventHandler would process both container & pod events

[WIP] status is just because I want to test it a bit more before pushing and probably add some javadocs

What issues does this PR fix or reference?

che #9946

Release Notes

N/A

Docs PR

N/A (yet)

codenvy-ci · 2018-06-22T13:51:02Z

Can one of the admins verify this patch?

codenvy-ci · 2018-06-22T13:54:02Z

Can one of the admins verify this patch?

garagatyi · 2018-06-22T15:14:24Z

@riuvshin Is it possible to include Ilya to the list of trusted github users for the CI?

garagatyi

Looks good! An additional thank you for simplifying the code!
Can you add more details to the PR description since it is pretty hard to understand what was changed, why and why it wasn't done before? I see the reason and I have enough context to understand it fully but it might be a bit complex overall or when the context is lost.
Can you also exaplins why it is in the WIP state?

garagatyi · 2018-06-22T15:26:49Z

ci-test

ibuziuk · 2018-06-22T15:57:56Z

@garagatyi thanks for your comment. WIP is just because I want to test it a bit more. It appeared to be that events itself changed a bit between versions of openshift (e.g. on 3.6 when there is a no pv event is "FailedSheduling" and on 3.9 it is "Failed Mount" with MountVolume.SetUp failed for volume ... message [1] we discussed it a bit with @eivantsov yesterday

[1] #9946 (comment)

garagatyi · 2018-06-22T16:03:40Z

Would be awesome to decide which versions of Openshift/Kubernetes we support and test events on all supported versions not to miss rewording of the error messages in some variants. (Not a requirement for this PR).
@eivantsov do we have somewhere specified supported versions of Openshift/Kubernetes?

ibuziuk · 2018-06-22T16:03:51Z

@riuvshin Is it possible to include Ilya to the list of trusted github users for the CI?

I believe I'm already included and can trigger the ci via ci-test (it does not seem be triggered automatically on PR send though)

codenvy-ci · 2018-06-22T21:03:41Z

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

sleshchenko

Looks good
Please take a look my comments

sleshchenko · 2018-06-23T06:15:10Z

...n/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/event/EventHandler.java

 * @author Sergii Leshchenko
 */
-public interface ContainerEventHandler {
+public interface EventHandler {


Would it more specified if we rename EventHandler to PodEventHandler since it handles only pods events?

sleshchenko · 2018-06-23T06:16:47Z

.../main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/event/PodEvent.java

+import org.eclipse.che.commons.annotation.Nullable;

 /**
 * The event that should be published when container change occurs, e.g. image pulled, container


Please fix java docs. Now it should be published even when no container changes occur. It would be useful to add an example of a case when pod event is published.

yeah, I plan to double-check all the javadocs and than remove WIP from the PR description

sleshchenko · 2018-06-23T06:18:37Z

.../main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/event/PodEvent.java

+public class PodEvent {
  private final String podName;
-  private final String containerName;
+  @Nullable private final String containerName;


It would be even more useful to have @Nullable annotation on #getContainerName. Also please note in javadoc of #getContainerName method that it may be nullable and when

good point, will fix

sleshchenko · 2018-06-23T06:19:12Z

.../src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/util/ContainerEvents.java

@@ -27,17 +27,13 @@ public final class ContainerEvents {
  private ContainerEvents() {}


Does it make sense to rename this class too?

yeah, will rename to the PodEvents

sleshchenko · 2018-06-23T07:47:14Z

P.S. Don't forget to remove WIP and Issue number from PR title since it will be used for changes log

… & openshift infra (processing both 'container' and 'pod' events) Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk · 2018-06-25T11:40:19Z

ci-test

codenvy-ci · 2018-06-25T16:49:40Z

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

ibuziuk requested review from garagatyi, riuvshin and skabashnyuk as code owners June 22, 2018 13:49

ibuziuk changed the title ~~che #9946: Better handling of unrecoverable events for k8s & openshift infra (processing both 'container' and 'pod' events)~~ [WIP]che #9946: Better handling of unrecoverable events for k8s & openshift infra (processing both 'container' and 'pod' events) Jun 22, 2018

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Jun 22, 2018

garagatyi approved these changes Jun 22, 2018

View reviewed changes

ibuziuk requested a review from sleshchenko June 22, 2018 16:03

sleshchenko approved these changes Jun 23, 2018

View reviewed changes

skabashnyuk approved these changes Jun 23, 2018

View reviewed changes

ibuziuk changed the title ~~[WIP]che #9946: Better handling of unrecoverable events for k8s & openshift infra (processing both 'container' and 'pod' events)~~ che #9946: Better handling of unrecoverable events for k8s & openshift infra (processing both 'container' and 'pod' events) Jun 25, 2018

che eclipse-che#9946: Better handling of unrecoverable events for k8s…

3642d38

… & openshift infra (processing both 'container' and 'pod' events) Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk force-pushed the che-9946 branch from f5c7409 to 3642d38 Compare June 25, 2018 11:32

ibuziuk mentioned this pull request Jun 25, 2018

Better failure model when PVC fails during workspace creation #9946

Closed

ibuziuk merged commit f80c186 into eclipse-che:master Jun 25, 2018

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

benoitf added this to the 6.8.0 milestone Jun 25, 2018

		@@ -27,17 +27,13 @@ public final class ContainerEvents {
		private ContainerEvents() {}

Conversation

ibuziuk commented Jun 22, 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!

codenvy-ci commented Jun 22, 2018

Uh oh!

codenvy-ci commented Jun 22, 2018

Uh oh!

garagatyi commented Jun 22, 2018

Uh oh!

garagatyi left a comment

Choose a reason for hiding this comment

Uh oh!

garagatyi commented Jun 22, 2018

Uh oh!

ibuziuk commented Jun 22, 2018

Uh oh!

garagatyi commented Jun 22, 2018

Uh oh!

ibuziuk commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codenvy-ci commented Jun 22, 2018

Uh oh!

sleshchenko 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!

sleshchenko commented Jun 23, 2018

Uh oh!

ibuziuk commented Jun 25, 2018

Uh oh!

codenvy-ci commented Jun 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ibuziuk commented Jun 22, 2018 •

edited

Loading

ibuziuk commented Jun 22, 2018 •

edited

Loading