Support core dump analysis with pystack and gdb. - #39484
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Assigning reviewers: R: @claudevdm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
62f50c9 to
2a68dfc
Compare
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
| // options experiments (which is automatically set by the Python SDK). This ensures | ||
| // that core dumps are written in /tmp/ with a prefix matching "core.". | ||
| coreDir := "/tmp" | ||
| interval := 5 * time.Second |
There was a problem hiding this comment.
Is the default meant to be 5 seconds? It seems the pipeline option defaults to 60 sec
beam/sdks/python/apache_beam/options/pipeline_options.py
Lines 1724 to 1726 in 58bac32
So the 5 seconds will only survive if someone explicitly sets the pipeline option to 0, otherwise it will default to 600 sec?
Is 600 too long to catch segfault on startup?
There was a problem hiding this comment.
yeah, this 5 second constant is useless, let's remove this altogether.
Re 600... not sure; probably won't eat a ton of CPU to do it more frequently, we can lower to 60sec. the ticker should not run concurrently, perhaps the issue might happen if the core cannot be processed or deleted, and keeps getting reprocessed again and again... hopefully that's not common
There was a problem hiding this comment.
actually i'll keep it 600 sec since the same param is used by other profilers, which might be resource-intensive. Users can set the lower value manually but I suspect this will be good enough.
| return | ||
| } | ||
|
|
||
| prefix := "core." |
There was a problem hiding this comment.
Is it a bit broad to match on any file starting with core.? Maybe we can add a bit more validation/regex?
There was a problem hiding this comment.
good point, fortunately we control the pattern, so we can specify a more unique prefix. beam_py_coredump perhaps.
There was a problem hiding this comment.
changed the prefix.
| logger.Warnf(ctx, "Python (worker %v) exited %v times: %v\nrestarting SDK process", | ||
| workerId, errorCount, err) | ||
| } else { | ||
| logger.Fatalf(ctx, "Python (worker %v) exited %v times: %v\nout of retries, failing container", |
There was a problem hiding this comment.
The last crash's core dump seems to depend on the ticker firing before Fatalf exits the container (default interval is 600s, and fresh cores <2s old are skipped).
Should we do one explicit core-dump sweep + GCS sync right before failing the container, so the crash that killed the worker is always analyzed regardless of timer timing?
There was a problem hiding this comment.
yes, the last crash is not super reliable... dataflow might also tear down the VM before processing finishes.
we could try to save core files into a semi-persist dir, which would survive container restarts, instead of /tmp/ . Let me check if that will work. I wary of increasing complexity of the boot.go.
There was a problem hiding this comment.
Added a callback + explicit postprocessing before crashing the container.
| case <-ticker.C: | ||
| processNewCoredumps(ctx, logger, pcfg, coreDir) | ||
| if isProfilerDisengaged(pcfg) { | ||
| return |
There was a problem hiding this comment.
Is this a race with the 2s file-age guard in processNewCoredumps? With --profiler_stop_after_crash.
Should we do a final synchronous sweep here that waits out the age guard (sleep >2s, re-run processNewCoredumps, sync results to GCS) before returning?
There was a problem hiding this comment.
good catch, yes, it is a race and we should fix this corner case.
There was a problem hiding this comment.
Change the logic to wait until the age is older than 2 seconds.
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| processNewCoredumps(ctx, logger, pcfg, coreDir) |
There was a problem hiding this comment.
Should we syncProfilesToGCS here if the write was success?
There was a problem hiding this comment.
currently profile postprocessing and upload are not coordinated (for this and other profilers). Since we write postprocessed content to a semi-persist directory, it should survive container restarts, so it should not be critical, and the logic is simpler. it is also possible to adjust the uploads to be more frequent.
…ing the container.
b7d95d3 to
b5ce587
Compare
Support analyzing core files with
pystackwhen python SDK processes crash with segfaults.Sample usages:
python my_pipeline.py --runner=DataflowRunner --profiler_agent=coredump --disk_size_gb=100Stack traces from the crash diagnostics are saved into text files that are then synced to GCS. Excerpt of the analysis is published directly into logs: we attempt to find print the stacktrace for the thread that has the GIL and surface it; the complete analysis can be retrieved from GCS (
--profle_locationorgs://<temp_location>/profilesby default).Arguments to pystack can be customized via the
--profiler_extra_argspipeline options.For more information, see: https://bloomberg.github.io/pystack/ and
pystack --help.Currently this will be supported with Dataflow from Beam 2.76.0 but it should be possible to extend support to other runners.
Requires either
pystack, or optionallygdbbe installed in the SDK container image.Pystack might be able to provide most insight, but if that is not sufficient users can install gdb into the container image, then we also attempt to run various gdb analysis commands and save the output into a file.
To control how often core files are postprocessed and saved, use the
--profile_upload_interval_secand--profile_postprocess_interval_sec.Core files are deleted after the analysis to save disk space. We don't dump cores on demand, but only perform a core analysis when when a crash happened.
The
--profiler_stop_after_secand--profiler_stop_after_crashoptions are also supported however it does not stop core files from being saved to disk, but stops the cleanup, which may be undesirable.Sample analysis of a manufactured segfault (retrieved from GCS).
core.python.31-20260728181728.txt
core.python.31-20260728181728.gdb.txt
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.