Fix leaking FDs in the run (_call) function #880
Merged
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
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
To launch regression testing public members of oamg organization can leave the following comment:
Please open ticket in case you experience technical problem with the CI. (RH internal only) Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please consider rerunning the CI by commenting leapp-ci build (might require several comments). If the problem persists, contact leapp-infra. |
|
/packit copr-build |
|
/packit copr-build |
|
tested and it's still broken with the same error for IPU 9 -> 10. Let's see whether we will be able to fix it before the release, otherwise let's keep it for the next release. Currently we have just ~4d for changes in this release and the issue is discovered only for IPU 9 -> 10. |
|
/packit copr-build |
pirat89
approved these changes
Aug 12, 2024
pirat89
added
the
changelog-checked
pirat89
changed the title
pirat89
pushed a commit
to oamg/leapp-repository
that referenced
this pull request
Aug 13, 2024
Reverting commit 60f500e The original commit only workarounded the root cause - leaked file descriptors in the leapp stdlib when using the `run` function. Dropping the change in the actor as it is not needed anymore. relates: oamg/leapp#880
pirat89
added a commit
to pirat89/leapp
that referenced
this pull request
Aug 16, 2024
## Packaging - Start building for EL 9 in the upstream repository on COPR (oamg#855) ## Framework ### Enhancements - Minor update in the summary overview to highlight what is present in the pre-upgrade report (oamg#858) - Store metadata about actors, workflows, and dialogs inside leapp audit db (oamg#847, oamg#867) ## Leapp (tool) ### Enhancements - Implement singleton leapp execution to prevent multiple running leapp instances on the system in the same time (oamg#851) ## stdlib ### Fixes - Close properly all file descriptors when executing shell commands via `run` (oamg#880) ## Modifications - Code is now Python3.12 compatible (oamg#855)
Merged
pirat89
added a commit
that referenced
this pull request
Aug 16, 2024
## Packaging - Start building for EL 9 in the upstream repository on COPR (#855) ## Framework ### Enhancements - Minor update in the summary overview to highlight what is present in the pre-upgrade report (#858) - Store metadata about actors, workflows, and dialogs inside leapp audit db (#847, #867) ## Leapp (tool) ### Enhancements - Implement singleton leapp execution to prevent multiple running leapp instances on the system in the same time (#851) ## stdlib ### Fixes - Close properly all file descriptors when executing shell commands via `run` (#880) ## Modifications - Code is now Python3.12 compatible (#855)
yuravk
pushed a commit
to yuravk/leapp-repository
that referenced
this pull request
Aug 19, 2024
Reverting commit 60f500e The original commit only workarounded the root cause - leaked file descriptors in the leapp stdlib when using the `run` function. Dropping the change in the actor as it is not needed anymore. relates: oamg/leapp#880 (cherry picked from commit 24700ee)
yuravk
pushed a commit
to yuravk/leapp-repository
that referenced
this pull request
Aug 20, 2024
Reverting commit 60f500e The original commit only workarounded the root cause - leaked file descriptors in the leapp stdlib when using the `run` function. Dropping the change in the actor as it is not needed anymore. relates: oamg/leapp#880 (cherry picked from commit 24700ee)
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.
updated by @pirat89
Original implementation of the
_callfunction (used byrunfrom actors) has been leaking file descriptors per each invocation. So in case an actor executed too many of them during an execution it could reach the limit of allowed opened FDs and the execution failed withOSError: Too many opened fileserror.There have been actually 2 issues:
EventLoop(aliasselect.epoll) have been called before the fork so the child process inherit the created FD toostdoutandstderrpipes (to consumes content from child process) have been created before the fork but have not been closed in the parentEventLoopis closed (and death of the child process) resolved this issue.Warning: totally untested (@pirat89 manually tested on rhel7 & rhel9)