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

Expose tracebacks from actor exceptions #802

Merged
merged 3 commits into from
Jan 19, 2023

Conversation

prilr
Copy link
Contributor

@prilr prilr commented Nov 9, 2022

Previously when leapp would be running actors, any exceptions raised by
those actors in child processes would only be output to stderr,
while the main process would only have the exit code to examine.

Consequently, there was no real way to log occurring exceptions -
only providing the aforementioned exit code was possible.

This patch adds a pipe between the main and the actor processes,
through which a formatted exception + traceback string is provided to
the main process.
This string is then packaged into the LeappRuntimeError thrown from the
process, to be utilized downstream in
workflow/command code.

This should serve as a step to closing #692 - although actually logging
the exception traceback likely should be done on the leapp-repository side.

prilr added 2 commits November 9, 2022 07:22
Previously when leapp would be running actors, any exceptions raised by
those actors in child processes would only be output to stderr,
while the main process would only have the exit code to examine.

Consequently, there was no real way to log occuring exceptions -
only providing the aforementioned exit code was possible.

This patch adds a pipe between the main and the actor processes,
through which a formatted exception + traceback string is provided to
the main process.
This string is then packaged into the LeappRuntimeError thrown from the
process, to be utilized downstream in
workflow/command code.
@github-actions
Copy link

github-actions bot commented Nov 9, 2022

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

  • review please to notify leapp developers of 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-all to schedule all tests (including sst) using this pr build and leapp-repository*master* as artifacts
  • /rerun-all 42 to schedule all tests (including sst) 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.

@leapp-bot
Copy link
Collaborator

This PR has been linked in issue tracker (OAMG-7853).

@pirat89
Copy link
Member

pirat89 commented Nov 25, 2022

@prilr Thanks for the contribution! @vinzenz do you have a time?

@pirat89
Copy link
Member

pirat89 commented Nov 25, 2022

@prilr also please take a look at the failed test


repository_dir = local('/tmp/pytest-of-root/pytest-0/repositories2/testing')

    def test_repo(repository_dir):
        setup_repo(repository_dir)
    
        def _run_test(repo_path):
            with repo_path.as_cwd():
                repository = find_and_scan_repositories(repo_path.dirpath().strpath)
                assert repository
                repository.load(resolve=True)
                assert getattr(leapp.tags, 'TestTag')
                assert repository.lookup_actor('TestActor')
                assert repository.lookup_workflow('TestWorkflow')
                assert not repository.lookup_workflow('MissingWorkflow')
                assert not repository.lookup_actor('MissingActor')
                assert repository.repos
                assert len(repository.serialize()) >= 1
                assert repository.actors
                assert not repository.topics
                assert not repository.models
                assert repository.tags
                assert repository.workflows
                assert repository.tools
                assert repository.libraries
                assert repository.files
                with pytest.raises(LeappRuntimeError):
                    repository.lookup_actor('TestActor')().run()
    
        p = Process(target=_run_test, args=(repository_dir,))
        p.start()
        p.join()
>       assert p.exitcode == 0
E       assert 1 == 0
E         -1
E         +0

tests/scripts/test_repository.py:112: AssertionError

---------------------------- Captured stdout setup -----------------------------
New repository testing has been created in /tmp/pytest-of-root/pytest-0/repositories2/testing
----------------------------- Captured stdout call -----------------------------
New tag TestTag has been created in /tmp/pytest-of-root/pytest-0/repositories2/testing/tags/test.py
New tag TestWorkflowTag has been created in /tmp/pytest-of-root/pytest-0/repositories2/testing/tags/testworkflow.py
New workflow TestWorkflow has been created in /tmp/pytest-of-root/pytest-0/repositories2/testing/workflows/test.py
New actor TestActor has been created at /tmp/pytest-of-root/pytest-0/repositories2/testing/actors/testactor/actor.py
WOOT
WOOT
WOOT
WOOT
----------------------------- Captured stderr call -----------------------------
Process Process-18:2:
Traceback (most recent call last):
  File "/usr/lib64/python2.7/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/usr/lib64/python2.7/multiprocessing/process.py", line 114, in run
    self._target(*self._args, **self._kwargs)
  File "/payload/testenv/lib/python2.7/site-packages/leapp/repository/actor_definition.py", line 74, in _do_run
    actor_instance.run(*args, **kwargs)
  File "/payload/testenv/lib/python2.7/site-packages/leapp/actors/__init__.py", line 289, in run
    self.process(*args)
  File "/tmp/pytest-of-root/pytest-0/repositories2/testing/actors/testactor/actor.py", line 19, in process
    do()
  File "/tmp/pytest-of-root/pytest-0/repositories2/testing/actors/testactor/libraries/lib/__init__.py", line 10, in do
    assert call(['woot-tool']) == 0
AssertionError
Process Process-18:
Traceback (most recent call last):
  File "/usr/lib64/python2.7/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/usr/lib64/python2.7/multiprocessing/process.py", line 114, in run
    self._target(*self._args, **self._kwargs)
  File "/payload/tests/scripts/test_repository.py", line 107, in _run_test
    repository.lookup_actor('TestActor')().run()
  File "/payload/testenv/lib/python2.7/site-packages/leapp/repository/actor_definition.py", line 124, in run
    raise LeappRuntimeError(err_message, exception_info)
  File "/payload/testenv/lib/python2.7/site-packages/leapp/exceptions.py", line 113, in __init__
    super().__init__(message)
TypeError: super() takes at least 1 argument (0 given)

Copy link
Member

@vinzenz vinzenz left a comment

Choose a reason for hiding this comment

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

Please fix the LeappRuntimeError constructor

leapp/exceptions.py Outdated Show resolved Hide resolved
@vinzenz
Copy link
Member

vinzenz commented Nov 25, 2022

Other than the constructor the PR LGTM, once it passes the tests I will give it a try myself.

@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions
Copy link

Testing Farm request for RHEL-8.7.0-Nightly/5057264;5067065 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-8.6.0-Nightly/5057264;5067065 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

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

@github-actions
Copy link

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

@prilr
Copy link
Contributor Author

prilr commented Nov 28, 2022

Please fix the LeappRuntimeError constructor

Should've checked with Python 2 before submitting, sorry about that.
Went ahead and changed UnknownCommandError's constructor to a compatible form while at it.

@fernflower
Copy link
Member

/rerun

@fernflower
Copy link
Member

Wow, that's a great addition that has somehow slipped under the radar. I will try to take a look at it this week.

@fernflower fernflower requested a review from pirat89 January 18, 2023 13:24
@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions
Copy link

Testing Farm request for RHEL-8.6.0-Nightly/5201672;5243870 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-8.7.0-Nightly/5201672;5243870 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

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

@github-actions
Copy link

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

@fernflower
Copy link
Member

Integration tests (apart from the flaky initram one I will hopefully solve soon) are fine.
Tested manually on a rhel7 guest, the LeappRuntimeError indeed has the traceback info. This is a solid first step for actually reporting exceptions, thanks for the contribution!

@fernflower fernflower merged commit 2d7ec8f into oamg:master Jan 19, 2023
@pirat89 pirat89 added the changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant label Feb 21, 2023
pirat89 added a commit to pirat89/leapp that referenced this pull request Feb 21, 2023
## Packaging
-  Change permissions for /var/lib/leapp to 0700 (oamg#807)

## Framework
### Fixes
- Prevent unicode errors when printing error messages (oamg#806)
- Make checks for missing answers more strict (oamg#797)

### Enhancements
- Expose tracebacks from actors (oamg#802)

### Known issue
- introduced new known issue in framework...
@pirat89 pirat89 mentioned this pull request Feb 21, 2023
pirat89 added a commit to pirat89/leapp that referenced this pull request Feb 21, 2023
## Packaging
-  Change permissions for /var/lib/leapp to 0700 (oamg#807)

## Framework
### Fixes
- Prevent unicode errors when printing error messages (oamg#806)
- Make checks for missing answers more strict (oamg#797)

### Enhancements
- Expose tracebacks from actors (oamg#802)
pirat89 added a commit to pirat89/leapp that referenced this pull request Feb 21, 2023
## Packaging
-  Change permissions for /var/lib/leapp to 0700 (oamg#807)

## Framework
### Fixes
- Prevent unicode errors when printing error messages (oamg#806)
- Make checks for missing answers more strict (oamg#797)

### Enhancements
- Expose tracebacks from actors (oamg#802)
pirat89 added a commit to pirat89/leapp that referenced this pull request Feb 21, 2023
## Packaging
-  Change permissions for /var/lib/leapp to 0700 (oamg#807)

## Framework
### Fixes
- Make checks for missing answers more strict (oamg#797)
- Prevent unicode errors when printing error messages (oamg#806)

### Enhancements
- Expose tracebacks from actors (oamg#802)
pirat89 added a commit that referenced this pull request Feb 21, 2023
## Packaging
-  Change permissions for /var/lib/leapp to 0700 (#807)

## Framework
### Fixes
- Make checks for missing answers more strict (#797)
- Prevent unicode errors when printing error messages (#806)

### Enhancements
- Expose tracebacks from actors (#802)
@prilr prilr deleted the actor_exception branch May 11, 2023 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants