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
Attach gdb in stateless tests #50487
Attach gdb in stateless tests #50487
Conversation
This is an automated comment for commit 42393b5 with description of existing statuses. It's updated for the latest CI running
|
Why we don't want it in CI? We do not restart server in stateless tests, so we just need to attach once. It is not too slow compared to integration tests with different server runs |
@tavplubix what do you think? Will it be ok to add it in CI? |
I think it's okay, it works fine in Stress tests |
docker/test/stateless/run.sh
Outdated
# FIXME Hung check may work incorrectly because of attached gdb | ||
# 1. False positives are possible | ||
# 2. We cannot attach another gdb to get stacktraces if some queries hung |
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.
p. 1 is outdated
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.
Ok, I will update comment. And I will move it to some library to avoid code duplication
|
||
# FIXME Hung check may work incorrectly because of attached gdb | ||
# We cannot attach another gdb to get stacktraces if some queries hung | ||
gdb -batch -command script.gdb -p "$(cat /var/run/clickhouse-server/clickhouse-server.pid)" | ts '%Y-%m-%d %H:%M:%S' >> /test_output/gdb.log & |
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.
We need to kill this gdb before calling get_stacktraces_from_gdb
in clickhouse-test
if some queries hung (of course it would be much better to avoid starting another instance of gdb
and just send some signal from clickhouse-test
to make this instance stop waiting on continue
and print the stacktraces, but I failed to do it). See also:
ClickHouse/tests/ci/stress_tests.lib
Line 154 in 48b23dd
kill -TERM "$(pidof gdb)" ||: |
Lines 122 to 126 in 0598bfd
# We attach gdb to clickhouse-server before running tests | |
# to print stacktraces of all crashes even if clickhouse cannot print it for some reason. | |
# However, it obstruct checking for hung queries. | |
logging.info("Will terminate gdb (if any)") | |
call_with_retry("kill -TERM $(pidof gdb)") |
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.
Will it be ok to just kill gdb in clickhouse-test
inside get_stacktraces_from_gdb
function before starting another gdb?
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.
Yep
Is it a way to reimplement it in a way to not break other older PRs? #50441 The image should assume there is no such file in the repo. |
I guess we can just copypaste the same code in 2 places, but I don't like it. Or maybe copy this file to the image. But I don't see a problem to just update old PRs with fresh master |
Any commit untested before the image is updated won't be green. I've faced the same problems with style check a couple of times too. What about changing:
? |
Changelog category (leave one):
Bug with fibers and tsan stopped reproducing in stress tests, but started in stateless tests where we don't have gdb log with segfault stacktrace. Let's try to attach gdb in stateless tests in this PR and rerun CI until bug reproduces (not sure we want attached gdb in stateless tests in CI, maybe I will add it only for tsan for some time if I won't reproduce it in this PR).