munet: add Commander.async_spawn()

[liambrady]

spawn(), which conducts a send/expect process is
blocking due to the abscence of async_=True to
pexpect.expect() within the method's main
send/expect loop. Some async methods call
spawn(), however, which leads to such methods
being blocked.

This is particularly problematic in cases where
multiple VMs requiring custom console expect/send
prompts are required (e.g. routers). Since each
QEMU VM sets up both console logging and completes
an expect/send loop without yielding control to
the coroutine of the other QEMU VMs, only one VM
can ever have logging configured (and thus advance in
an expect/send loop) at any given time. Not
only is this inefficient given that all QEMU VMs
are running and waiting for input, but this can
be fatal if console logging is not set up on each
VM in time. If some mandatory console output is
missed, then there is no way to recover and the
setup of the QEMU VM will time out.

However, setting async_=True in pexpect.expect()
leads to errors when mixed with PopenSpawn.
To bypass this issue, we do not use pexpect.expect()
with async_=True, but repeatedly call
pexpect.expect() with a timeout set at 0.1 (Until
a timeout that we track expires).

The end result of this change is that both logging is set
up on all QEMU nodes immediately and that
independent progress can be made in each VM's
expect/send loop simultaneously.

[codecov]

Codecov Report

Attention: Patch coverage is 90.14085% with 7 lines in your changes missing coverage. Please review.

Project coverage is 59.28%. Comparing base (4ec7fe7) to head (fc1a560).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
munet/base.py	89.70%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
+ Coverage   58.84%   59.28%   +0.44%     
==========================================
  Files          19       19              
  Lines        5729     5802      +73     
==========================================
+ Hits         3371     3440      +69     
- Misses       2358     2362       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[choppsv1]

As discussed elsewhere, we need to use a new async_spawn() variant for this to preserve the existing non-asyncio API. The use case you're interested in is coming from an async function so should be fine to just switch to the new variant inside console/shell_spawn.

Also, since async/await only fails with non-pty pexpect.popen_spawn (i.e., pipes) you should restrict the busy loop fix to only that case. For use_pty == True use the supported async from pexpect.

[liambrady]

As discussed elsewhere, we need to use a new async_spawn() variant for this to preserve the existing non-asyncio API. The use case you're interested in is coming from an async function so should be fine to just switch to the new variant inside console/shell_spawn.

I modified both console/shell_spawn and launch/monitor to use the new async variant (both methods are already async and so should use the variant), and now spawn() appears to have fallen out of the code coverage. It appears that commander.spawn() is not called anywhere else within the codebase.

Does this affect the decision to continue to maintain commander.spawn()? Do we need it for future additions to the munet API? It doesn't feel correct to leave dead code within the project (or write new tests to maintain a section of code that isn't being meaningfully used.)

[choppsv1]

In order to create this new function you did a large copy and paste. That's a good indication we need to refactor the function so that more code is shared. :)

That is hard to do sometimes with async/await. I feel like there should be some way to share code better between asyncio and non-asyncio, but I haven't figured it out yet. This is why you see other places in the code where I run the async function inside an asyncio.run() call for the non-asyncio variant. I'm not sure how safe this is though b/c sometimes people aren't using asyncio b/c it's broken for them in which case running it with asyncio.run() will be broken too.

A good place to test this would be in an FRR topotest which uses spawn.

[liambrady]

I don't quite like the asyncio.run() solution for two reasons:

1. The synchronous version of spawn() cannot be used from within an asynchronous method since asyncio does not support nested event loops to the best of my knowledge. I am not confident enough to claim that there will never be a use case where spawn() inside an async method is appropriate (especially since spawn() is located so deep within the API). As such, an asyncio.run() solution does not feel safe.
2. Even if we followed the asyncio.run() solution, then we have to work around the async and use_pty=False hack (i.e. to avoid the exception with p.expect(..., async_=True) and pipes). Specifically, we only want to use the hack when absolutely necessary. However, using asyncio.run() as the solution would imply that we end up using the hack during the synchronous variant of spawn(), which is not desired.

The only other real option then in refactoring is to try and split off as much synchronous code as possible and place it into helper methods. I don't find such a solution satisfying, since that would leave a large portion of the methods untouched, but don't see any other real alternatives.

[liambrady]

See the latest commit for a minor refactor that collects code that is synchronous in either version (sync/async) into a single method to reduce duplicate code.

I am struggling to determine if there are any other changes that I could make to reduce code duplication. The presence of the await statement within the try/except block + while block make it difficult to reduce the logic any further imo.

[choppsv1]

Need to test both async and non-async variants, and the vty and non-vty case.

[liambrady]

The majority of synchronous spawn() is tested, but not all of the duplicate code. I was unable to determine a solution that allowed the send/expect logic to be tested while ensuring that we also test functionality across all the different shells.