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

trio subprocs launching #87

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

trio subprocs launching #87

wants to merge 14 commits into from

Conversation

goodboy
Copy link
Member

@goodboy goodboy commented Dec 18, 2022

Replacement for #55 after going through the slew of merge conflicts.

From that issue verbatim here is the original description:

ping @linuxmaniac


This is in preparation of a python 3.6+ only 1.0.0.alpha release and replaces all the internal processing launching machinery with the new subprocess support in trio.

This not only gets us cross OS support for free but also gives access to async/await based scenario and agent spawning.

A few interesting notes:

  • SIPp turns out to be a good fit with trio cancellation semantics since it has a built in cancellation system via SIGUSR1
  • this now gives us a strictly single threaded implementation
  • trio gives us deterministic teardown allowing for much finer control over per agent failures and subsequent reporting

TODO:

  • adjust to the new trio proc spawning APIs and get CI running clean, also see
  • write some async spawning tests and examples in the readme
  • do we need this right away or can we add it in a follow up?
  • consider playing around with timeout and other cancellation tools such as trio.move_on_after() and trio.fail_after() and figure out how this can interplay with log reporting for interactive use
  • add the mac os env to CI

I look forward to feedback and thoughts!

@goodboy
Copy link
Member Author

goodboy commented Dec 18, 2022

Looks like I've broken something using the new spawn api, not sure what.

I don't have the sipp binary installed locally atm so if someone wants to take a shot at repairing this (hint hint @linuxmaniac) please feel free!

Copy link
Contributor

@linuxmaniac linuxmaniac left a comment

Choose a reason for hiding this comment

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

before the changes I mentioned above:

================================================================================================================================ short test summary info ================================================================================================================================
FAILED tests/test_agent.py::test_failures[ua] - subprocess.CalledProcessError: Command '['/usr/bin/sipp', '-recv_timeout', '5000', '-r', '1', '-l', '1', '-m', '1', '-log_file', '/tmp/None_log_file', '-screen_file', '/tmp/None_screen_file', '-trace_logs', '-trace_screen']' returned non-zero exit status 255.
FAILED tests/test_agent.py::test_failures[uac] - subprocess.CalledProcessError: Command '['/usr/bin/sipp', '-sn', 'uac', '-recv_timeout', '5000', '-r', '1', '-l', '1', '-m', '1', '-log_file', '/tmp/uac_log_file', '-screen_file', '/tmp/uac_screen_file', '-trace_logs', '-trace_screen', '99.99.99.99:5060']' returned non-zero e...
FAILED tests/test_agent.py::test_failures[uas] - subprocess.CalledProcessError: Command '['/usr/bin/sipp', '-sn', 'uas', '-recv_timeout', '5000', '-r', '1', '-l', '1', '-m', '1', '-log_file', '/tmp/uas_log_file', '-screen_file', '/tmp/uas_screen_file', '-trace_logs', '-trace_screen']' died with <Signals.SIGKILL: 9>.
FAILED tests/test_launcher.py::test_agent_fails - subprocess.CalledProcessError: Command '['/usr/bin/sipp', '-i', '99.99.99.99', '-p', '5060', '-sn', 'uas', '-m', '1']' returned non-zero exit status 255.
FAILED tests/test_stack.py::test_async_run - TypeError: arun() got an unexpected keyword argument 'block'
FAILED tests/test_stack.py::test_unreachable_uas[autolocalsocks=True] - subprocess.CalledProcessError: Command '['/usr/bin/sipp', '-i', '127.0.1.1', '-p', '43212', '-rsa', '127.0.1.1:9999', '-mi', '127.0.1.1', '-mp', '33776', '-sn', 'uas', '-recv_timeout', '5000', '-r', '1', '-l', '1', '-m', '1', '-log_file', '/tmp/uas_log_file', '-screen_file', ...
FAILED tests/test_stack.py::test_unreachable_uas[autolocalsocks=False] - subprocess.CalledProcessError: Command '['/usr/bin/sipp', ':9999', '-sn', 'uas', '-recv_timeout', '5000', '-r', '1', '-l', '1', '-m', '1', '-log_file', '/tmp/uas_log_file', '-screen_file', '/tmp/uas_screen_file', '-trace_logs', '-trace_screen']' returned non-zero exit status ...
====================================================================================================================== 7 failed, 52 passed, 18 warnings in 28.35s =======================================================================================================================
ERROR: InvocationError for command /home/vseva/data/projects/pysipp/.tox/py39/bin/pytest tests/ (exited with code 1)
________________________________________________________________________________________________________________________________________ summary ________________________________________________________________________________________________________________________________________
ERROR:   py39: commands failed

after:

================================================================================================================================ short test summary info ================================================================================================================================
FAILED tests/test_stack.py::test_async_run - AttributeError: 'Scenario' object has no attribute 'finalize'
====================================================================================================================== 1 failed, 58 passed, 32 warnings in 37.54s =======================================================================================================================
ERROR: InvocationError for command /home/vseva/data/projects/pysipp/.tox/py39/bin/pytest tests/ (exited with code 1)
________________________________________________________________________________________________________________________________________ summary ________________________________________________________________________________________________________________________________________
ERROR:   py39: commands failed

shlex.split(cmd),
stdout=subprocess.DEVNULL,
stderr=subprocess.PIPE
)
Copy link
Contributor

Choose a reason for hiding this comment

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

check=False,

I think you need to disable check in order to avoid trio.run_process() to raise subprocess.CalledProcessError

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah so this will resolve most of the failures then yah?

If you've already patched this please do make a PR onto this one, ow no worries but not sure when i'll get to this next.

)

def __call__(self, *args, **kwargs):
# TODO: deprecation warning here
Copy link
Contributor

Choose a reason for hiding this comment

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

kwargs.pop("block", None)

I had to add this to avoid getting an error

pysipp/launch.py Outdated
msg = report.err_summary(agents2procs)
if msg:
# report logs and stderr
await report.emit_logfiles(agents2procs)
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to remove await since report.emit_logfiles() doesn't return anything

@@ -124,9 +124,16 @@ def test_server():
)
def test_failures(ua, retcode, kwargs, exc):
"""Test failure cases for all types of agents"""
runner = launch.TrioRunner()
Copy link
Contributor

Choose a reason for hiding this comment

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

in line 121 you need to change 0 for -9, the new return value in timeout

@@ -1,15 +1,22 @@
"""
Basic agent/scenario launching

Copy link
Contributor

Choose a reason for hiding this comment

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

In line 92... scenarios doesn't have finalize()

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't really know what to do in this case

@linuxmaniac
Copy link
Contributor

BTW, some of the commits don't pass the pre-commit checks.

when rebasing, you should do git rebase -x 'pre-commit run --from HEAD^ --to HEAD' origin/master in order to force the execution of pre-commit checks per commit

@goodboy
Copy link
Member Author

goodboy commented Dec 19, 2022

@linuxmaniac RE all the little changes you suggest, can you make a PR onto this branch to fix them if you've already gone through it?

@linuxmaniac
Copy link
Contributor

@linuxmaniac RE all the little changes you suggest, can you make a PR onto this branch to fix them if you've already gone through it?

There you are #88

goodboy added a commit that referenced this pull request Dec 20, 2022
trio subprocs: small fixes for #87
@goodboy
Copy link
Member Author

goodboy commented Dec 20, 2022

@linuxmaniac landed it 🏄🏼

goodboy and others added 14 commits December 20, 2022 14:17
After attempting to find an OS portable way to spawn subprocesses using
the stdlib and coming out unsatisfied, I've decided use the new
subprocess launching support in `trio`! This will of course require that
the project moves to python 3.6+ giving us access to a lot of neat
features of modern python including async/await support and adherence to
the structured concurrency principles prominent in `trio`. It turns out
this is a good fit since SIPp already has a built in cancellation
mechanism via the SIGUSR1 signal.

There's a lot of "core" changes to go over in this commit:
- drop the "run protocol" and "runner creation" related hooks since they
  really shouldn't be overridden until there's some need for it and it's
  likely smarter to keep those "machinery" details strictly internal for now
- the run "protocol" has now been relegated to an async function:
  `pysipp.launch.run_all_agents()`
- many routines have been converted to async functions particularly at the
  runner (`pysipp.TrioRunner.run()`, `.get()`) and scenario
  (`pysipp.Scenario.arun()`) levels allowing us to expose both a sync and
  async interface for running subprocesses / agents
- drop all the epoll/select loop stuff as this is entirely delegated to
  `trio.open_process()` and it's underlying machinery and APIs

Resolves #53
@goodboy
Copy link
Member Author

goodboy commented Dec 20, 2022

There rebased clean to master.

@goodboy goodboy mentioned this pull request Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants