-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix flaky operator debug
test
#12501
Conversation
The `t.Logf` calls already are picked out from the rest of the test output by being prefixed with the filename.
The goroutine we're blocking for here already tells us its done by sending a value, so block on that instead of an extraneous channel
* `pprof-duration` should be no more than `duration` * `pprof-interval` should be no more than `pprof-duration` * clamp the values rather than throwing errors, which could change the commands that existing users might already have in debugging scripts
If we assert there are no unexpected error outputs, it's easier for the developer to debug when something is going wrong with the tests because the error output will be presented as a failing test, rather than just a failing exit code check. Or worse, no failing exit code check! This also forces us to be explicit about which tests will return 0 exit codes but still emit (presumably ignorable) error outputs.
The `operator debug` tests that stand up servers can't be run in parallel, because we don't have a way of canceling the API calls for pprof. The agent will still be running the last pprof when we exit, and that breaks the next test that talks to that same agent. We could split off each subtest into its own server, but this test suite is already very slow. In future work we should fix this "for real" by making the API call cancelable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #12488
We introduced a
pprof-interval
argument tooperator debug
in #11938, and unfortunately this has resulted in a lot of test flakes. The actual command in use is mostly fine (although I've fixed some quirks here), so what's really happened is that the change has revealed some existing issues in the tests. Best reviewed commit-by-commit but a summary of the changes is below. (No changelog entry because this has only shipped for 1.3-beta.1)Make first pprof collection synchronous to preserve the existing
behavior for the common case where the pprof interval matches the
duration.
Clamp
operator debug
pprof timing to that of the command. Thepprof-duration
should be no more thanduration
and thepprof-interval
should be no more thanpprof-duration
. Clamp thevalues rather than throwing errors, which could change the commands
that existing users might already have in debugging scripts
Testing: remove test parallelism
The
operator debug
tests that stand up servers can't be run inparallel, because we don't have a way of canceling the API calls for
pprof. The agent will still be running the last pprof when we exit,
and that breaks the next test that talks to that same agent.
(Because you can only run one pprof at a time on any process!)
We could split off each subtest into its own server, but this test
suite is already very slow. In future work we should fix this "for
real" by making the API call cancelable.
Testing: assert against unexpected errors in
operator debug
tests.If we assert there are no unexpected error outputs, it's easier for
the developer to debug when something is going wrong with the tests
because the error output will be presented as a failing test, rather
than just a failing exit code check. Or worse, no failing exit code
check!
This also forces us to be explicit about which tests will return 0
exit codes but still emit (presumably ignorable) error outputs.
Additional minor bug fixes (mostly in tests) and test refactorings:
Fix text alignment on pprof Duration in
operator debug
outputRemove "done" channel from
operator debug
event stream test. Thegoroutine we're blocking for here already tells us it's done by
sending a value, so block on that instead of an extraneous channel
Event stream test timer should start at current time, not zero
Remove noise from
operator debug
test log output. Thet.Logf
calls already are picked out from the rest of the test output by
being prefixed with the filename.
Remove explicit pprof args so we use the defaults clamped from
duration/interval