Skip to content
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

Merged
merged 1 commit into from
Aug 14, 2023
Merged

Conversation

pirat89
Copy link
Member

@pirat89 pirat89 commented Jul 17, 2023

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

Missing:

  • update unit-tests

@pirat89 pirat89 added the bug label Jul 17, 2023
@github-actions
Copy link

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.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

  • review please @oamg/developers to notify leapp developers of the review request
  • /packit copr-build to submit a public copr build using packit

To launch regression testing public members of oamg organization can leave the following comment:

  • /rerun to schedule basic regression tests using this pr build and leapp-repository*master* as artifacts
  • /rerun 42 to schedule basic regression tests using this pr build and leapp-repository*PR42* as artifacts
  • /rerun-sst to schedule sst tests using this pr build and leapp-repository*master* as artifacts
  • /rerun-sst 42 to schedule sst tests using this pr build and leapp-repository*PR42* as artifacts

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.

@fernflower
Copy link
Member

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

leapp/libraries/stdlib/__init__.py Outdated Show resolved Hide resolved
leapp/libraries/stdlib/__init__.py Outdated Show resolved Hide resolved
@pirat89 pirat89 force-pushed the check-executables branch 3 times, most recently from d90a50d to 6ba8cc7 Compare August 1, 2023 14:44
@pirat89
Copy link
Member Author

pirat89 commented Aug 1, 2023

@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.

@pirat89 pirat89 force-pushed the check-executables branch 3 times, most recently from a52c175 to d9f9e9b Compare August 2, 2023 11:28
@pirat89 pirat89 marked this pull request as ready for review August 2, 2023 11:56
@pirat89
Copy link
Member Author

pirat89 commented Aug 2, 2023

/rerun

@pirat89 pirat89 requested a review from fernflower August 2, 2023 11:56
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/6234502

@github-actions
Copy link

github-actions bot commented Aug 2, 2023

Testing Farm request for RHEL-8.6-rhui/6224797;6234502 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

github-actions bot commented Aug 2, 2023

Testing Farm request for RHEL-7.9-rhui/6224797;6234502 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@pirat89
Copy link
Member Author

pirat89 commented Aug 3, 2023

@fernflower btw, just realized the unit-tests is invalid. updating it now

@fernflower
Copy link
Member

/rerun

@github-actions
Copy link

github-actions bot commented Aug 3, 2023

Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/6237236

@github-actions
Copy link

github-actions bot commented Aug 3, 2023

Testing Farm request for RHEL-8.6-rhui/6234661;6237236 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

github-actions bot commented Aug 3, 2023

Testing Farm request for RHEL-7.9-rhui/6234661;6237236 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@pirat89 pirat89 changed the title WIP. stdlib.run: Fix missing executable stdlib.run: Fix missing executable Aug 3, 2023
fernflower
fernflower previously approved these changes Aug 7, 2023
Copy link
Member

@fernflower fernflower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @pirat89

@pirat89
Copy link
Member Author

pirat89 commented Aug 14, 2023

@fernflower rebased to fix the conflict. can you reapprove/rereview it and merge?

UPDATE: give me a minute. tests started to fail. fixed

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.
@Rezney Rezney merged commit 2301dbb into oamg:master Aug 14, 2023
14 checks passed
@pirat89 pirat89 added this to the 7.9/8.9/9.3 milestone Aug 15, 2023
@pirat89 pirat89 added the changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant label Aug 23, 2023
pirat89 added a commit to pirat89/leapp that referenced this pull request Aug 23, 2023
## 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
@pirat89 pirat89 mentioned this pull request Aug 23, 2023
pirat89 added a commit to pirat89/leapp that referenced this pull request Aug 23, 2023
## 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
Rezney pushed a commit that referenced this pull request Aug 23, 2023
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants