Conditional gatherer of logs of unhealthy pods #509

natiiix · 2021-09-22T07:18:41Z

This PR adds a new conditional gatherer that gathers pod logs based on firing alerts. It looks for KubePodCrashlooping and KubePodNotReady, and collects the appropriate log from their corresponding pods/containers.

Sample Archive

pkg/gatherers/conditional/gathering_rule.schema.json

Documentation

docs/gathered-data.md

Unit Tests

pkg/gatherers/conditional/gather_logs_of_unhealthy_pods_test.go

Privacy

Yes. There are no sensitive data in the newly collected information.

We are already gathering pod logs elsewhere and this gatherer should only be triggered for pods/alerts from openshift-* namespaces.

Changelog

No.

Breaking Changes

No, there are only additions; no modification to existing gathering.

References

Jira Task: https://issues.redhat.com/browse/CCXDEV-5499

openshift-ci · 2021-09-22T07:18:42Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

pkg/gatherers/conditional/gather_logs_of_unhealthy_pods.go

sferich888 · 2021-10-11T15:29:56Z

Is log collection something we really want in 'insights'?

cc: @smarterclayton - I know you and I have talked on this in the past; have we changed our stance/scope on what insights should focus on?

tremes · 2021-10-12T07:03:42Z

@sferich888 I think the answer is yes. We already have some gatherers collecting various container logs and I believe some of them are really important - e.g https://github.com/openshift/insights-operator/blob/master/docs/gathered-data.md#clusteroperatorpodsandevents. Other examples are:

Also note that these logs in this PR are gathered only when the corresponding alert is firing.

docs/insights-archive-sample/insights-operator/conditional-gatherer-rules.json

tremes · 2021-10-12T09:48:37Z

pkg/gatherers/conditional/gather_logs_of_unhealthy_pods.go

+		containerFilter := ""
+		if alertContainer != "" {
+			containerFilter = fmt.Sprintf("^%s$", alertContainer)
+		}


It's just cosmetic detail, but maybe doing:

if alertContainer, ok := alertLabels["container"]; ok { containerFilter = fmt.Sprintf("^%s$", alertContainer) }

would be more straightforward

@tremes You're right. The process that lead to this code had many steps where this was all done very differently, so as I was moving parts of the code around, I didn't realize the alertContainer was no longer used further in the code (because it used to be).

pkg/gatherers/conditional/gather_logs_of_unhealthy_pods.go

tremes · 2021-10-12T09:59:51Z

I didn't try to reproduce the alerts to trigger this new gatherer. I added few minor suggestions, but it looks very good! Thanks. You need to satisfy the linting.

natiiix · 2021-10-12T15:07:12Z

/test lint

natiiix · 2021-10-13T07:01:31Z

/retest

natiiix · 2021-10-13T07:26:19Z

/test unit

natiiix · 2021-10-13T10:38:30Z

/test unit

pkg/gatherers/conditional/gather_logs_of_unhealthy_pods.go

Serhii1011010 · 2021-10-13T12:52:31Z

pkg/gatherers/conditional/gathering_functions.go

@@ -27,6 +27,8 @@ const (
 	GatherAPIRequestCounts GatheringFunctionName = "api_request_counts_of_resource_from_alert"
 )

+const GatherLogsOfUnhealthyPods GatheringFunctionName = "logs_of_unhealthy_pods"


We could probably make it more specific as in api_request_counts_of_resource_from_alert, but dunno.

Well, I would like if this gatherer could be used in general for any case of an alert where we want to gather logs from the corresponding pod, so the only specification I could add would be that it's based on alerts.

pkg/gatherers/conditional/gathering_rule.schema.json

Serhii1011010

looks great

natiiix · 2021-10-25T08:32:29Z

/test e2e-agnostic-upgrade

natiiix · 2021-10-26T13:37:47Z

/test e2e-agnostic-upgrade

xJustin · 2021-11-01T08:48:02Z

/label docs-approved

sferich888 · 2021-11-01T12:12:12Z

/label px-approved

quarckster · 2021-11-01T16:19:55Z

/label qe-approved

rluders

/lgtm

rluders · 2021-11-02T09:35:46Z

/label lgtm

openshift-ci · 2021-11-02T09:35:46Z

@rluders: The label(s) /label lgtm cannot be applied. These labels are supported: platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, downstream-change-needed, backport-risk-assessed, cherry-pick-approved

In response to this:

/label lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-11-02T09:37:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natiiix, rluders

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [natiiix,rluders]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-11-02T11:01:33Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci bot requested review from iNecas and rluders September 22, 2021 07:19

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2021

tremes reviewed Sep 22, 2021

View reviewed changes

pkg/gatherers/conditional/gather_logs_of_unhealthy_pods.go Outdated Show resolved Hide resolved

natiiix force-pushed the conditional-unhealthy-pods-logs branch 2 times, most recently from 507a66d to 1760e64 Compare September 29, 2021 07:16

natiiix force-pushed the conditional-unhealthy-pods-logs branch 2 times, most recently from 4f9d16c to acdb78b Compare October 11, 2021 08:08

natiiix marked this pull request as ready for review October 12, 2021 08:28

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 12, 2021

natiiix changed the title ~~[WIP] Conditional gatherer of logs of unhealthy pods~~ Conditional gatherer of logs of unhealthy pods Oct 12, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 12, 2021

tremes reviewed Oct 12, 2021

View reviewed changes

docs/insights-archive-sample/insights-operator/conditional-gatherer-rules.json Show resolved Hide resolved

tremes reviewed Oct 12, 2021

View reviewed changes

pkg/gatherers/conditional/gather_logs_of_unhealthy_pods.go Outdated Show resolved Hide resolved

natiiix added 7 commits October 13, 2021 13:26

Cond. gather of unhealthy pods' logs

9fbea05

Add Previous switch/prop to log msg filter struct

8dc6ccb

Add FieldSelector to log gathering helper func

83c60fd

Add missing fields to gatherer params

656a2bc

Improve cond. gather of unhealthy pod logs

2a0df37

Add tests for unhealthy pod logs cond. gatherer

e73fd90

Update cond. gatherer rules JSON in sample archive

68f8a86

natiiix added 3 commits October 13, 2021 13:26

Refactor pod log test to satisfy linter

3d759b7

Make linter even happier (pod log cond. gather)

9b4ecbf

Fix linter long lines in pod log cond. gather

345c9e6

natiiix force-pushed the conditional-unhealthy-pods-logs branch from 8509358 to 345c9e6 Compare October 13, 2021 11:26

Serhii1011010 reviewed Oct 13, 2021

View reviewed changes

pkg/gatherers/conditional/gather_logs_of_unhealthy_pods.go Outdated Show resolved Hide resolved

Serhii1011010 reviewed Oct 13, 2021

View reviewed changes

pkg/gatherers/conditional/gather_logs_of_unhealthy_pods.go Outdated Show resolved Hide resolved

Serhii1011010 reviewed Oct 13, 2021

View reviewed changes

pkg/gatherers/conditional/gather_logs_of_unhealthy_pods.go Show resolved Hide resolved

Serhii1011010 reviewed Oct 13, 2021

View reviewed changes

pkg/gatherers/conditional/gathering_rule.schema.json Show resolved Hide resolved

natiiix added 3 commits October 17, 2021 19:03

Add alert name pattern to pod log cond gather conf

bbfe6e9

Couple of tweaks to unhealthy pod log cond. gahter

1206b1c

Update unhealthy pod log cond. gatherer tests

64a3507

Serhii1011010 reviewed Oct 18, 2021

View reviewed changes

Update Gathered Data docs

e2f12ff

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Nov 1, 2021

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Nov 1, 2021

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Nov 1, 2021

rluders approved these changes Nov 2, 2021

View reviewed changes

openshift-ci bot assigned rluders Nov 2, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2021

openshift-merge-robot merged commit 22ea9c9 into openshift:master Nov 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditional gatherer of logs of unhealthy pods #509

Conditional gatherer of logs of unhealthy pods #509

natiiix commented Sep 22, 2021 •

edited

Loading

openshift-ci bot commented Sep 22, 2021

sferich888 commented Oct 11, 2021

tremes commented Oct 12, 2021

tremes Oct 12, 2021

natiiix Oct 12, 2021

tremes commented Oct 12, 2021

natiiix commented Oct 12, 2021

natiiix commented Oct 13, 2021

natiiix commented Oct 13, 2021

natiiix commented Oct 13, 2021

Serhii1011010 Oct 13, 2021

natiiix Oct 13, 2021

Serhii1011010 left a comment

natiiix commented Oct 25, 2021

natiiix commented Oct 26, 2021

xJustin commented Nov 1, 2021

sferich888 commented Nov 1, 2021

quarckster commented Nov 1, 2021

rluders left a comment

rluders commented Nov 2, 2021

openshift-ci bot commented Nov 2, 2021

openshift-ci bot commented Nov 2, 2021

openshift-bot commented Nov 2, 2021

Conditional gatherer of logs of unhealthy pods #509

Conditional gatherer of logs of unhealthy pods #509

Conversation

natiiix commented Sep 22, 2021 • edited Loading

Categories

Sample Archive

Documentation

Unit Tests

Privacy

Changelog

Breaking Changes

References

openshift-ci bot commented Sep 22, 2021

sferich888 commented Oct 11, 2021

tremes commented Oct 12, 2021

tremes Oct 12, 2021

Choose a reason for hiding this comment

natiiix Oct 12, 2021

Choose a reason for hiding this comment

tremes commented Oct 12, 2021

natiiix commented Oct 12, 2021

natiiix commented Oct 13, 2021

natiiix commented Oct 13, 2021

natiiix commented Oct 13, 2021

Serhii1011010 Oct 13, 2021

Choose a reason for hiding this comment

natiiix Oct 13, 2021

Choose a reason for hiding this comment

Serhii1011010 left a comment

Choose a reason for hiding this comment

natiiix commented Oct 25, 2021

natiiix commented Oct 26, 2021

xJustin commented Nov 1, 2021

sferich888 commented Nov 1, 2021

quarckster commented Nov 1, 2021

rluders left a comment

Choose a reason for hiding this comment

rluders commented Nov 2, 2021

openshift-ci bot commented Nov 2, 2021

openshift-ci bot commented Nov 2, 2021

openshift-bot commented Nov 2, 2021

natiiix commented Sep 22, 2021 •

edited

Loading