-
Notifications
You must be signed in to change notification settings - Fork 70
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
stdlib.run: Fix missing executable #836
Conversation
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. |
Can you add a unit test please? Currently the description reads pretty scary and not that easy to grasp, a repro can help explain what's it actually about imho |
d90a50d
to
6ba8cc7
Compare
@fernflower seems the current impementatino of the fix is wrong as bunch of checks (some expected by unit tests..) are executed inside the _call function. I will think whether to duplicate some checks or whether to move some functionality deeper. |
a52c175
to
d9f9e9b
Compare
/rerun |
Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/6234502 |
Testing Farm request for RHEL-8.6-rhui/6224797;6234502 regression testing has been created. |
Testing Farm request for RHEL-7.9-rhui/6224797;6234502 regression testing has been created. |
3e44c13
to
ce98b43
Compare
@fernflower btw, just realized the unit-tests is invalid. updating it now |
ce98b43
to
09dcfb6
Compare
/rerun |
Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/6237236 |
Testing Farm request for RHEL-8.6-rhui/6234661;6237236 regression testing has been created. |
Testing Farm request for RHEL-7.9-rhui/6234661;6237236 regression testing has been created. |
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, thanks @pirat89
09dcfb6
to
b5255a5
Compare
@fernflower rebased to fix the conflict. can you reapprove/rereview it and merge? UPDATE: |
Currently, if the executable required in `run` does not exist (or is not executable), the child process's code is not replaced by `os.execvpe` function and it raises the OSError instead. However, the parent process does not get this OSError. It consumes exit code, stderr, ... of the child process. So in case the code does something like this: ``` try: result = run(['non-executable']) except OSError: pass except CalledProcessError: # do something.. ``` the child process passes and do whatever.. In case it ends with zero exit code, the obtained result is usually something totally different than expected in actors. Also there could be problems with non-idempotent code, when some actions could be done twice (once executed by the child, send time executed by the parent [current] process). We have realized that number of existing leapp actors for in-place upgrades already count with the raise of OSError when executable cannot be used. So we choosed for now to check whether executables are present and raise OSError if not, so we are sure that only one process leave the function really. Also applied another seatbelt into the child process - if the OSError is raised anyway despite our checks (e.g. SELinux prevents the execution) let's just kill the process instead giving it a possibility to continue. In such a case, always print a msg to stderr of the child process and exit with ecode 1. Note: To check an executable we use `distutils.spawn.find_executable` which is deprecated in Python 3 and will be dropped in Python 3.12. However it exists now for Python 2 & 3, so we use this one for now and will replace it in future by `shutils.which` when the time comes. Additional changes: * Make pylint happy (set noqa for E721 for the check the Fields is not created directly). The "isinstance" function has a different behaviour in this case than we want. * Fix imports to make pylint happy * Set `result` to empty dict instead of None: in case a ValueError or TypeError is raised inside the _call function, it's expected to copy the `result` content inside the final block, however in case the results is None, it fails and raise additional exception that covers the original one. * Update unit tests to cover issues with missing executable.
b5255a5
to
84ab1e4
Compare
## Packaging - Provide leapp-framework 5.0 (oamg#818, oamg#840) ## Framework ### Fixes - Improve processing of reports with UTF-8 characters (oamg#821, oamg#828) - Fix info reporting with only one path to log (oamg#834) ### Enhancements - Include tracebacks of actors into the leapp.db (oamg#832) ## Leapp (tool) ### Fixes - Dialog creation fails not more for a component without choices (oamg#826) - Empty report is generated correctly (oamg#828) - Fix processing data in remediation instructions with non-ascii characters () ### Enhancements - Improve report summary output to make it more visible (oamg#818, oamg#840) ## stdlib ### Fixes - Fixed the call when the execute cannot be performed (oamg#836) ### Enhancements - changes related just to stdlib - under leapp/libraries/stdlib
## Packaging - Provide leapp-framework 5.0 (oamg#818, oamg#840) ## Framework ### Fixes - Improve processing of reports with UTF-8 characters (oamg#821, oamg#828) - Fix info reporting with only one path to log (oamg#834) ### Enhancements - Include tracebacks of actors into the leapp.db (oamg#832) ## Leapp (tool) ### Fixes - Dialog creation fails not more for a component without choices (oamg#826) - Empty report is generated correctly (oamg#828) - Fix processing data in remediation instructions with non-ascii characters () ### Enhancements - Improve report summary output to make it more visible (oamg#818, oamg#840) ## stdlib ### Fixes - Fixed the call when the execute cannot be performed (oamg#836) ### Enhancements - changes related just to stdlib - under leapp/libraries/stdlib
## Packaging - Provide leapp-framework 5.0 (#818, #840) ## Framework ### Fixes - Improve processing of reports with UTF-8 characters (#821, #828) - Fix info reporting with only one path to log (#834) ### Enhancements - Include tracebacks of actors into the leapp.db (#832) ## Leapp (tool) ### Fixes - Dialog creation fails not more for a component without choices (#826) - Empty report is generated correctly (#828) - Fix processing data in remediation instructions with non-ascii characters () ### Enhancements - Improve report summary output to make it more visible (#818, #840) ## stdlib ### Fixes - Fixed the call when the execute cannot be performed (#836) ### Enhancements - changes related just to stdlib - under leapp/libraries/stdlib
Currently, if the executable required in
run
does not exist (or is not executable), the child process's code is not replaced byos.execvpe
function and it raises the OSError instead. However, the parent process does not get this OSError. It consumes exit code, stderr, ... of the child process. So in case the code does something like this:the child process passes and do whatever.. In case it ends with zero exit code, the obtained result is usually something totally different than expected in actors. Also there could be problems with non-idempotent code, when some actions could be done twice (once executed by the child, send time executed by the parent [current] process).
We have realized that number of existing leapp actors for in-place upgrades already count with the raise of OSError when executable cannot be used. So we choosed for now to check whether executables are present and raise OSError if not, so we are sure that only one process leave the function really.
Also applied another seatbelt into the child process - if the OSError is raised anyway despite our checks (e.g. SELinux prevents the execution) let's just kill the process instead giving it a possibility to continue. In such a case, always print a msg to stderr of the child process and exit with ecode 1.
Note: To check an executable we use
distutils.spawn.find_executable
which is deprecated in Python 3 and will be dropped in Python 3.12. However it exists now for Python 2 & 3, so we use this one for now and will replace it in future byshutils.which
when the time comes.Additional changes:
Make pylint happy (set noqa for E721 for the check the Fields is not
created directly). The "isinstance" function has a different behaviour
in this case than we want.
Fix imports to make pylint happy
Set
result
to empty dict instead of None: in case a ValueErroror TypeError is raised inside the _call function, it's expected
to copy the
result
content inside the final block, however incase the results is None, it fails and raise additional exception
that covers the original one.
Update unit tests to cover issues with missing executable
Missing: