rework container.execute to use multiprocessing as watchdog #50
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Workaround `docker.APIClient.exec_start` sometimes blocking indefinitely by running in a sub-process and throwing an exception if the sub-process does not complete within a given timeout. Remove the existing post-exec code which polled the value of `docker.APIClient.exec_inspect` for 15 seconds to determine if the command had completed. This is effectively performed by the new sub-process waiting. I've set the timeout to 30 seconds, up from 15, which (from experimentation) seems to be necessary to account for the extra time it takes to invoke `exec_start` within the timeout period. A future change should make this timeout configurable. This general pattern (of watchdogging the docker library code) might be useful elsewhere, in particular for any future efforts to support parallel test execution. Signed-off-by: Jonathan Dowland <[email protected]>
jmtd
added a commit
to jmtd/redhat-openjdk-container-images
that referenced
this pull request
Nov 30, 2023
The jmtd fork of behave-test-steps has its v1 branch matching this PR: cekit/behave-test-steps#50 This adds a multiprocess watchdog around invoking `docker.APIClient.exec_inspect`, which will abort the current step if that call has not returned within 30 seconds. This means a lock-up during a step will cause that test to fail and not the whole test run. Signed-off-by: Jonathan Dowland <[email protected]>
|
IIRC there is a environment variable that you can set to increase the timeout, BEHAVE_TIMEOUT I guess. |
jmtd
added a commit
to jmtd/redhat-openjdk-container-images
that referenced
this pull request
Jan 8, 2024
The jmtd fork of behave-test-steps has its v1 branch matching this PR: cekit/behave-test-steps#50 This adds a multiprocess watchdog around invoking `docker.APIClient.exec_inspect`, which will abort the current step if that call has not returned within 30 seconds. This means a lock-up during a step will cause that test to fail and not the whole test run. Signed-off-by: Jonathan Dowland <[email protected]>
jmtd
added a commit
to jmtd/redhat-openjdk-container-images
that referenced
this pull request
Jan 8, 2024
The jmtd fork of behave-test-steps has its v1 branch matching this PR: cekit/behave-test-steps#50 This adds a multiprocess watchdog around invoking `docker.APIClient.exec_inspect`, which will abort the current step if that call has not returned within 30 seconds. This means a lock-up during a step will cause that test to fail and not the whole test run. Signed-off-by: Jonathan Dowland <[email protected]>
|
@jmtd Is this ready to merge? |
|
yes. sorry for the delay |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm going to ask for some of the other behave test users I know of to try this out since it's quite a significant change. But it seems to be critical in getting GitHub Actions CI working again for our images: an individual test that blocks will now fail without causing the whole test run to be aborted.
I've pushed this to my fork's
v1branch too, to make it easier to try out.(commit message follows)
Workaround
docker.APIClient.exec_startsometimes blocking indefinitely by running in a sub-process and throwing an exception if the sub-process does not complete within a given timeout.Remove the existing post-exec code which polled the value of
docker.APIClient.exec_inspectfor 15 seconds to determine if the command had completed. This is effectively performed by the new sub-process waiting. I've set the timeout to 30 seconds, up from 15, which (from experimentation) seems to be necessary to account for the extra time it takes to invokeexec_startwithin the timeout period.A future change should make this timeout configurable.
This general pattern (of watchdogging the docker library code) might be useful elsewhere, in particular for any future efforts to support parallel test execution.