eventing
eventing copied to clipboard
Don't dump K8s events on console log
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.
/cc @vaikas
/cc @mgencur
/assign
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.
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.
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 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.
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.
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 .
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
.
@cardil is this issue still relevant? (v0.20 was released several weeks ago)
Yep, it is. It's not connected to any version, as it's rather a e2e test setup.
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
.
/remove-lifecycle stale
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
.
/remove-lifecycle stale
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
.
/remove-lifecycle stale
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
.
/remove-lifecycle stale /reopen
@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.
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
.
/remove-lifecycle stale /triage accepted
with rekt-tests this is already better closing this for now