Skip to content
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

ARROW-7119: [C++][CI] Show automatic backtraces #6202

Closed
wants to merge 29 commits into from

Conversation

kszucs
Copy link
Member

@kszucs kszucs commented Jan 15, 2020

Automatically generate coredumps and backtraces for the failing C++ tests on both Linux (including docker) and macOS.

@kszucs kszucs changed the title Coredump on macos ARROW-7119: [C++][CI] Use scripts/util_coredump.sh to show automatic backtraces Jan 15, 2020
@github-actions
Copy link

@kszucs
Copy link
Member Author

kszucs commented Jan 21, 2020

@kszucs kszucs changed the title ARROW-7119: [C++][CI] Use scripts/util_coredump.sh to show automatic backtraces ARROW-7119: [C++][CI] Use scripts/util_coredump.sh on macOS to show automatic backtraces Jan 21, 2020
@kszucs kszucs requested a review from kou January 21, 2020 00:34
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example coredump https://github.com/apache/arrow/pull/6202/checks?check_run_id=399397059

It seems that the coredump isn't related.
Should we remove ~/Library/Logs/DiagnosticReports/*.core before running our scripts?

ci/scripts/util_coredump.sh Outdated Show resolved Hide resolved
@kszucs
Copy link
Member Author

kszucs commented Jan 21, 2020

That wasn't a segfault, no core file was genereted. Let me force a segfault.

@kou
Copy link
Member

kou commented Jan 21, 2020

https://github.com/apache/arrow/pull/6202/checks?check_run_id=401063744

.crash exists but no core dump.
We may need to run ulimit -c unlimited -S in the Test step.
How about running ulimit -a in the Test step to debug ulimit values?

@kszucs kszucs changed the title ARROW-7119: [C++][CI] Use scripts/util_coredump.sh on macOS to show automatic backtraces ARROW-7119: [C++][CI] Show automatic backtraces Jan 23, 2020
@kszucs
Copy link
Member Author

kszucs commented Jan 24, 2020

Auto backtraces are working on linux within the docker containers, it also works in the GHA jobs.

I've tested it locally on macOS and it also works when the coredump gets generated, but the coredumps are not generated in the GHA job, despite that it is theoretically enabled.

@kou any ideas what am I missing on macOS?

@kszucs
Copy link
Member Author

kszucs commented Jan 25, 2020

The coredumps should be available now for macOS as well https://github.com/apache/arrow/runs/408685826

@kszucs
Copy link
Member Author

kszucs commented Jan 25, 2020

@kou we should reuse the print_coredumps function for the rest of the C++ dependent builds too, but in a follow-up PR.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

I hope that this can work without changing kernel.core_pattern (core is the default) on Linux in a follow-up task!

.github/workflows/python.yml Outdated Show resolved Hide resolved
.github/workflows/ruby.yml Outdated Show resolved Hide resolved
ci/docker/conda-cpp.dockerfile Outdated Show resolved Hide resolved
ci/docker/conda-cpp.dockerfile Outdated Show resolved Hide resolved
cpp/build-support/run-test.sh Outdated Show resolved Hide resolved
cpp/build-support/run-test.sh Outdated Show resolved Hide resolved
cpp/build-support/run-test.sh Outdated Show resolved Hide resolved
@kszucs
Copy link
Member Author

kszucs commented Jan 26, 2020

We need to change the core pattern because the tests are usually run in parallel, so we need to identify the right coredump for the test case.

This PR also print the coredumps for linux, not just macOS.

@kou
Copy link
Member

kou commented Jan 26, 2020

We need to change the core pattern because the tests are usually run in parallel, so we need to identify the right coredump for the test case.

OK.

This PR also print the coredumps for linux, not just macOS.

Oh, sorry. I misunderstood.

Copy link
Contributor

@fsaintjacques fsaintjacques left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that run-test.sh is overly complicated. Most of the things it do is dealing with xml for jenkins, which we don't use. I'd simplify to do the following:

  • setup sanitizers
  • execute tests (and capture pid + return code), that might require some bash magic with jobs and waitpid.
  • link coredump with pid instead of basename pattern.
  • print coredump

Do we use the local stdout/stderr dumped text files @kou @wesm?

@kszucs
Copy link
Member Author

kszucs commented Feb 6, 2020

  • execute tests (and capture pid + return code), that might require some bash magic with jobs and waitpid.
  • link coredump with pid instead of basename pattern.

The chance of the collision vs. required bash magic tradeoff made me choose the base name.

docker-compose.yml Outdated Show resolved Hide resolved
@kou
Copy link
Member

kou commented Feb 7, 2020

Do we use the local stdout/stderr dumped text files

AFAIK, no.
We can simplify run-test.sh but I think that it's out-of-scope of this pull request. We can do it as a separated pull request.

@kszucs
Copy link
Member Author

kszucs commented Feb 10, 2020

@fsaintjacques created a follow-up JIRA as suggested by @kou https://issues.apache.org/jira/browse/ARROW-7814

I think it is ready for review again.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that https://github.com/apache/arrow/pull/6202/checks?check_run_id=436229364 shows backtrace.
I'll merge this after I remove needless sudo.

cpp/build-support/run-test.sh Outdated Show resolved Hide resolved
@kszucs kszucs requested a review from kou February 11, 2020 21:16
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants