Skip to content

#10229 Publish all workspace pod events to the machine logs#10228

Merged
ibuziuk merged 1 commit intoeclipse-che:masterfrom
ibuziuk:publishAllPodLogs
Jun 30, 2018
Merged

#10229 Publish all workspace pod events to the machine logs#10228
ibuziuk merged 1 commit intoeclipse-che:masterfrom
ibuziuk:publishAllPodLogs

Conversation

@ibuziuk
Copy link
Copy Markdown
Member

@ibuziuk ibuziuk commented Jun 29, 2018

What does this PR do?

Publish all workspace pod events to the machine logs (not only container events).

image

This would enrich logs automatically with "Scheduled" & "Successful Mount Volume"

What issues does this PR fix or reference?

#10229

N/A

Release Notes

N/A

Docs PR

N/A

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
@ibuziuk ibuziuk requested a review from garagatyi as a code owner June 29, 2018 13:33
@ibuziuk ibuziuk requested review from amisevsk, garagatyi, l0rd and sleshchenko and removed request for garagatyi June 29, 2018 13:33
@codenvy-ci
Copy link
Copy Markdown

Can one of the admins verify this patch?

@ibuziuk ibuziuk requested a review from davidfestal June 29, 2018 13:34
@codenvy-ci
Copy link
Copy Markdown

Can one of the admins verify this patch?

@ibuziuk ibuziuk added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Jun 29, 2018
@ibuziuk ibuziuk changed the title Publish all workspace pod events to the machine logs #10229 Publish all workspace pod events to the machine logs Jun 29, 2018
Copy link
Copy Markdown
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it is very nice to grab other events

@benoitf
Copy link
Copy Markdown
Contributor

benoitf commented Jun 29, 2018

ci-test

Copy link
Copy Markdown
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to have, especially with recent PVC issues.

@codenvy-ci
Copy link
Copy Markdown

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

@ibuziuk ibuziuk merged commit 744196a into eclipse-che:master Jun 30, 2018
@benoitf 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 30, 2018
@benoitf benoitf added this to the 6.8.0 milestone Jun 30, 2018
Copy link
Copy Markdown
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take my inlined comments

machines.getMachines(getContext().getIdentity()).entrySet()) {
final KubernetesMachineImpl machine = entry.getValue();
if (machine.getPodName().equals(podName)
&& machine.getContainerName().equals(containerName)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibuziuk Looks like in the case when there are two containers in one pod, containers related events will be published in both machines, something like the following example in case OpenShift Sql stack:

pulling image `eclipse-che/ubuntu`
pulling image `eclipse-che/ubuntu`
Successfully pulled image `eclipse-che/mysql`
Successfully pulled image `eclipse-che/mysql`
Created container
Created container
Started container
Started container

Is it true or I missed something? If it's true let's filter events by container name if it is not empty.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another fix that I think is needed for multi-container workspace is removing return statement here since two or more machines may be related to the same pod and it's OK to duplicate logs in such cases, otherwise, the only first machine will have logs. WDYT?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sleshchenko yeah, you are right. I missed the part with multi-container workspaces 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement A feature request - must adhere to the feature request template.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants