-
Notifications
You must be signed in to change notification settings - Fork 372
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
Refactor start_extension_command to handle subprocess completion or timeout #1623
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1623 +/- ##
==========================================
Coverage ? 66.58%
==========================================
Files ? 78
Lines ? 11200
Branches ? 1572
==========================================
Hits ? 7458
Misses ? 3413
Partials ? 329
Continue to review full report at Codecov.
|
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
99d907c
to
b2f367f
Compare
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.
Minor suggestion else LGTM
Description
Before, it was the responsibility of
ExtHandlerInstance
'slaunch_command
to wait for the subprocess to terminate or timeout, parse its output and raise exceptions if needed.Since the actual invocation of the subprocess is done in the CGroup API layer, if
systemd-run
times out, we wouldn't know until theExtHandlerInstance
layer, at which point we wouldn't retry invoking the subprocess usingPopen
and we would erroneously raise anExtensionError
which would be counted against the extension's SLA.This PR refactors the code so that these responsibilities are done in the
start_extension_command
in the CGroup API layer instead of inlaunch_command
. Helper functions for waiting for process completion are added toprocess_utils.py
and corresponding unit tests are added or updated.PR information
Quality of Code and Contribution Guidelines
This change is