eventing icon indicating copy to clipboard operation
eventing copied to clipboard

Don't dump K8s events on console log

Open cardil opened this issue 4 years ago • 21 comments

Describe the bug We are dumping K8s events on console log, no matter if the test is successful or not. That makes it really hard to know what actually happened as it makes the log extremely long.

This is the code that does this:

https://github.com/knative/eventing/blob/ede5a2ac8beb888edeae49539f041ea9bb085476/test/lib/test_runner.go#L214-L216

Expected behavior We should dump those directly to a $ARTIFACTS directory instead of console. Maybe we also make if skippable, using environment configuration - for ex.: TEST_RUNNER_DUMP_EVENTS=yes|no|on_error.

To Reproduce Run any eventing test that uses test runner, like upgrade tests.

cardil avatar Nov 25 '20 15:11 cardil

/cc @vaikas

cardil avatar Nov 25 '20 15:11 cardil

/cc @mgencur

cardil avatar Nov 25 '20 15:11 cardil

/assign

cardil avatar Nov 25 '20 15:11 cardil

I think those are k8s events, typically it is not that bad in upgrade test. I guess no one really check the log if the test succeeds. I would vote for keeping the default to always log. Logging to artifact can be difficult to use when multiple tests are run in the same job.

zhongduo avatar Nov 25 '20 15:11 zhongduo

Yeah, those are K8s events. I should have written that explicitly.

We are having hard time reading those logs, especially when they are interwoven with serving logs.

We could create a separate directory for each execution, and print that directory on console log, so it is easy to find those logs if you like to. The same already is happening for XML junit logs, for example.

cardil avatar Nov 25 '20 15:11 cardil

This might be a little tricky for k8s events. For example, in upgrade test there are preupgrade/postupgrade jobs that will emit k8s events too even they are in the same test. Distinguishing them will be tricky unless we tag them as preupgrade and postupgrade logs. I personally use the k8s events a lot to debug a flakiness in our upgrade test. But as long as the default is the same bahavior, there shouldn't be any problem.

Thanks, Jimmy

On Wed, Nov 25, 2020 at 10:17 AM Chris Suszynski [email protected] wrote:

Yeah, those are K8s events.

We are having hard time reading those logs, especially when they are interwoven with serving logs.

We could create a separate directory for each execution, and print that directory on console log, so it is easy to find those logs if you like to. The same already is happening for XML junit logs, for example.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/knative/eventing/issues/4592#issuecomment-733769048, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACE6CNGQAHQ2ET6PYSW7EUDSRUNYPANCNFSM4UCQQ7TQ .

zhongduo avatar Nov 25 '20 15:11 zhongduo

@zhongduo I can add additional environment variable, let's say TEST_RUNNER_DUMP_EVENTS_LOCATION=stdout|logfile. And if logfile will be used I would put those events to a file and write on console:

K8s events for ns test-events-propagation-with-prober-txtxn has been dumped to: artifacts/path/gotest-k8s-events/test-events-propagation-with-prober-txtxn.log

Then it will be super easy to look them up if you like to. Or, you can set that flag to stdout, to keep current behavior.

cardil avatar Nov 26 '20 13:11 cardil

This logging of events at the end of the test is annoying and I agree that dumping them in a file would make the logs more concise and readable. It should be fine as long as the main log insludes references to the files where events are logged for each test.

mgencur avatar Nov 26 '20 13:11 mgencur

Awesome, that is a good idea.

On Thu, Nov 26, 2020 at 8:02 AM Chris Suszynski [email protected] wrote:

@zhongduo https://github.com/zhongduo I can add additional environment variable, let's say TEST_RUNNER_DUMP_EVENTS_LOCATION=stdout|logfile. And if logfile will be used I would put those events to a file and write on console:

K8s events for ns test-events-propagation-with-prober-txtxn has been dumped to: artifacts/path/gotest-k8s-events/test-events-propagation-with-prober-txtxn.log

Then it will be super easy to look them up if you like to. Or, you can set that flag to stdout, to keep current behavior.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/knative/eventing/issues/4592#issuecomment-734285992, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACE6CNCORTNTIEMZF4T5GZDSRZGUZANCNFSM4UCQQ7TQ .

zhongduo avatar Nov 26 '20 14:11 zhongduo

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

github-actions[bot] avatar Mar 01 '21 01:03 github-actions[bot]

@cardil is this issue still relevant? (v0.20 was released several weeks ago)

lberk avatar Mar 08 '21 18:03 lberk

Yep, it is. It's not connected to any version, as it's rather a e2e test setup.

cardil avatar Mar 15 '21 12:03 cardil

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

github-actions[bot] avatar Jun 29 '21 01:06 github-actions[bot]

/remove-lifecycle stale

cardil avatar Jun 29 '21 08:06 cardil

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

github-actions[bot] avatar Sep 28 '21 01:09 github-actions[bot]

/remove-lifecycle stale

cardil avatar Sep 30 '21 21:09 cardil

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

github-actions[bot] avatar Dec 30 '21 01:12 github-actions[bot]

/remove-lifecycle stale

cardil avatar Jan 04 '22 11:01 cardil

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

github-actions[bot] avatar Apr 05 '22 01:04 github-actions[bot]

/remove-lifecycle stale /reopen

cardil avatar Jun 29 '22 16:06 cardil

@cardil: Reopened this issue.

In response to this:

/remove-lifecycle stale /reopen

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.

knative-prow[bot] avatar Jun 29 '22 16:06 knative-prow[bot]

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

github-actions[bot] avatar Sep 28 '22 01:09 github-actions[bot]

/remove-lifecycle stale /triage accepted

cardil avatar Oct 05 '22 09:10 cardil

with rekt-tests this is already better closing this for now

pierDipi avatar Mar 27 '24 18:03 pierDipi