ci: add valgrind stateless test for check memory leak#19800
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
Never mind, above error is for |
|
I have marked several test cases as skipped for memory leak check and the command This patch is ready for review. Should I create several github issue for these memory check skipped test cases for further investigation or just let them rot? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e9cab55a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
b7e0f60 to
d72e49b
Compare
|
Maybe investigating these memory leaks could be my good second issue. Let me dig around. |
1c02fac to
f0faf78
Compare
|
@cathay4t Great work. Maybe it's better to add a new CI action(unit-test-valgrind)? |
|
Sure. |
4072d01 to
f0527e3
Compare
🤖 CI Job Analysis
📊 Summary
❌ NO RETRY NEEDEDAll failures appear to be code/test issues requiring manual fixes. 🔍 Job Details
🤖 AboutAutomated analysis using job annotations to distinguish infrastructure issues (auto-retried) from code/test issues (manual fixes needed). |
d7a3ea7 to
5ce4151
Compare
By using https://github.com/jfrimmel/cargo-valgrind , all unit test will be invoked with valgrind check. Using this command to do memory check on unit test cases: ```bash cargo valgrind \ nextest run -E 'not group(skip-memcheck)' --profile memcheck ``` These are the valgrind arugments for `cargo valgrind`: ``` --show-leak-kinds=definite --errors-for-leak-kinds=definite --max-threads=1024" ``` Some test cases are known to fail this valgrind test, instead of fixing the in this patch, those test cases are marked as `skip-memcheck` in `.config/nextest.toml` and pending for further investigation. The memory leak check is only enabled on Linux x86_64 for now. Added and enabled `test_unit_valgrind` github CI test action. Resolves: databendlabs#5039 Signed-off-by: Gris Ge <cnfourt@gmail.com>
|
@zhang2014 The container in CI is not build from unmerged PR Dockerfile, hence the valgrind test will fail. Do you want me to create a dedicate PR to add |
|
Sorry, I missed the notification for this message. @everpcpc could you please take a look? |
|
I used to add build with sanitizer to check several problems, including memory leak, but seems it will slow down CI workflow speed. So a few months later, we removed it. Is valgrind really better than the sanitizer, or should we consider bringing the sanitizer back? |
|
I just pickup the oldest I only used valgrind in a c binding written in Rust FFI, it correctly identified some memory bugs. Never played with rust address sanitizer, but the document looks promising. If a tool can slow thing down, it will most likely be able to catch some hard-to-find racing problem where devs never be able to found in their fancy setup. So if a certain test is slow thing down, isolate it to run only after all other test groups is better than remove it in the seek of quicker CI results. Just my two cents who holds zero experience on databend. |
|
I can close this PR if you think we should not continue effort on this path. |
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
By using https://github.com/jfrimmel/cargo-valgrind , all unit test will
be invoked with valgrind check.
Using this command to do memory check on unit test cases:
cargo valgrind \ nextest run -E 'not group(skip-memcheck)' --profile memcheckThese are the valgrind arugments for
cargo valgrind:Some test cases are known to fail this valgrind test, instead of fixing
the in this patch, those test cases are marked as
skip-memcheckin.config/nextest.tomland pending for further investigation.The memory leak check is only enabled on Linux x86_64 for now.
Tests
Type of change
This change is