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.

Sign up for GitHub

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

mutest: new output arg for out-of-order cmd execution #39

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

mutest: new output arg for out-of-order cmd execution #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions munet/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,15 @@
raise CalledProcessError(rc, ac, o, e)
return rc, o, e

def _cmd_status(self, cmds, raises=False, warn=True, stdin=None, **kwargs):
def _cmd_status(
self,
cmds,
raises=False,
warn=True,
stdin=None,
output=True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand you're looking for a "fire-and-forget" command, I just wonder if this is the right way to do that.

I think that perhaps instead of output the new param should be no_wait=False.

I'm also wondering if we should explicitly set stdout=subprocess.DEVNULL and stderr=subprocess.DEVNULL.

**kwargs
):
"""Execute a command."""
timeout = None
if "timeout" in kwargs:
Expand All @@ -949,7 +957,11 @@

pinput, stdin = Commander._cmd_status_input(stdin)
p, actual_cmd = self._popen("cmd_status", cmds, stdin=stdin, **kwargs)
o, e = p.communicate(pinput, timeout=timeout)
if output:
o, e = p.communicate(pinput, timeout=timeout)
else:
o = ''
e = ''

Check warning on line 964 in munet/base.py

View check run for this annotation

Codecov / codecov/patch

munet/base.py#L963-L964 

Added lines #L963 - L964 were not covered by tests
return self._cmd_status_finish(p, cmds, actual_cmd, o, e, raises, warn)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This call is expecting to run after the process has completed (one side effect of the p.communicate call above is that it waits until this happens). If we look at _cmd_status_finish you'll see it checks p.returncode as a boolean (expecting 0 to mean success); however, in the not-completed-running case p.returncode will be None so this also looks like success.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured that in the case that an immediate error occurred within the process, then maybe catching it isn't a bad thing. In retrospect though, leaving that up to chance is probably a bad idea and consistency would be preferred so I will probably modify this to skip the _cmd_status_finish entirely.


async def _async_cmd_status(
Expand Down
20 changes: 14 additions & 6 deletions munet/mutest/userapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,7 @@ def _command(
self,
target: str,
cmd: str,
**kwargs,
) -> str:
"""Execute a ``cmd`` and return result.

Expand All @@ -439,7 +440,7 @@ def _command(
cmd: string to execut on the target.
"""
out = self.targets[target].cmd_nostatus(
cmd, stdin=subprocess.DEVNULL, warn=False
cmd, stdin=subprocess.DEVNULL, warn=False, **kwargs
)
self.last = out = out.rstrip()
report = out if out else "<no output>"
Expand Down Expand Up @@ -742,20 +743,26 @@ def section(self, desc: str):
self.oplogf(" section setting __in_section to True")
self.__print_header(self.info.tag, desc, add_nl)

def step(self, target: str, cmd: str) -> str:
def step(
self,
target: str,
cmd: str,
output: bool = True,
) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general creating these fire-and-forget processes is messy b/c there's no guarantee they will complete before the test exists (in which case they will probably be killed by the kernel, or have PIPE closed signals or something). For non-mutest uses one uses popen to start processes that you want to run in the background and you get back a p process object that you can kill or wait on later. I don't know if this is the right pattern to use for mutest though -- so maybe your way here is ok.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder about the actual use case. Might a new API that runs multiple commands simultaneously and waits for them all to complete would be a cleaner solution (step_multi or step_parallel)? It depends on the problem we're trying to solve I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use case I have encountered twice now is the desire to create some sort of traffic generator on a node (or two nodes in the case of setting up a client/server pair situation) and then run a series of tests based on the results (perhaps on separate nodes). While it would probably be feasible to do this with some sort of new waitstep_multi or waitstep_parallel, such would require that API call to be responsible for multiple parallel steps plus multiple waiting steps which seems excessive. I figured that accepting the risk and running a few fire-and-forget cmd through a step (then using match/wait calls as usual but written by the tester to be robust to the state of the background process) was cleanest.

I do agree that letting the kernel kill the processes is a naive solution though, so perhaps the best solution would be to keep track of all fire-and-forget processes in a list and clean it up later when a node is being deleted?

"""See :py:func:`~munet.mutest.userapi.step`.

:meta private:
"""
self.logf(
"#%s.%s:%s:STEP:%s:%s",
"#%s.%s:%s:STEP:%s:%s:%s",
self.tag,
self.steps + 1,
self.info.path,
target,
cmd,
output,
)
return self._command(target, cmd)
return self._command(target, cmd, output=output)

def step_json(self, target: str, cmd: str) -> Union[list, dict]:
"""See :py:func:`~munet.mutest.userapi.step_json`.
Expand Down Expand Up @@ -1007,17 +1014,18 @@ def get_target(name: str) -> Commander:
return TestCase.g_tc.targets[name]


def step(target: str, cmd: str) -> str:
def step(target: str, cmd: str, output: bool = True) -> str:
"""Execute a ``cmd`` on a ``target`` and return the output.

Args:
target: the target to execute the ``cmd`` on.
cmd: string to execute on the target.
output: if False, then DO NOT wait for output. Otherwise waits for ``cmd`` completion.

Returns:
Returns the ``str`` output of the ``cmd``.
"""
return TestCase.g_tc.step(target, cmd)
return TestCase.g_tc.step(target, cmd, output)


def step_json(target: str, cmd: str) -> Union[list, dict]:
Expand Down
9 changes: 9 additions & 0 deletions tests/mutest/mutest_step.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
"""Test that out-of-order cmd execution is supported by step"""
from munet.mutest.userapi import wait_step, section, step

section("Test the no-output arg in step")

step('r1', 'touch test-file1; sleep 1; mv test-file1 test-file2', output=False)

wait_step('r1', 'ls', 'test-file1', 'Saw test-file1', 2, 0.1)
wait_step('r1', 'ls', 'test-file2', 'Saw test-file2', 2, 0.1)