[Avocado-devel] Subprocess termination
Lukáš Doktor
ldoktor at redhat.com
Mon Feb 20 15:29:03 UTC 2017
Hello guys,
Dne 20.2.2017 v 14:46 Andrei Stepanov napsal(a):
> It depends.
>
> There could be a scenario where necessary to run ~ 200 tests at once.
> If first bad-by-design test forgets/failed to cleanup, then some
> sequential tests also will fail.
> It is a question about usability.
>
I don't agree the `avocado.utils.process` library should keep track of
processes as it is a generic library on top or `subprocess` module which
is suppose to simplify process execution. On the other hand I understand
the isolation requirement and I remember a discussion we had a long time
before, where I suggested using `cgroups` to distinguish between
processes of each test. The problem was that cgroups were not used
everywhere and they are still not available everywhere. Anyway I don't
see this as an `avocado.utils.process` issue, but rather the generic
concept we mentioned in
https://trello.com/c/XEOCa2tI/909-add-basic-support-for-automated-environment-cleanup-after-each-test
Anyway let me repeat one important thing: "Everybody who creates a
resource is responsible for cleaning it! Always!" So even if we
implement a way to destroy processes on background it should only be a
safety net, not a tool to be used instead of tearDown.
One note regarding the example code, please do not use
`process.SubProcess` directly from tests but use
`process.get_sub_process_klass` which returns the most fitting class
based on the current environment.
Kind regards,
Lukáš
>
> Okay, Radek, it seems that we should carefully write our tests, and kill
> children processes no matter what of error we have in our tests....., at
> least for now.
>
> On Mon, Feb 20, 2017 at 2:02 PM, Lucas Meneghel Rodrigues
> <lookkas at gmail.com <mailto:lookkas at gmail.com>> wrote:
>
> Still, Cleber's point is valid and coincides with my view. If you
> create a subprocess that runs in the background, then you should
> keep track of it and reap it in the cleanup procedures.
>
> On Mon, Feb 20, 2017 at 1:13 PM Andrei Stepanov <astepano at redhat.com
> <mailto:astepano at redhat.com>> wrote:
>
> Hi
>
> Cleber, I think your example is not completely correct. You use
> "jobs".
> Bash/csh/zsh jobs is another topic. Your example is about bash
> jobs.
> Let's take a look a level up:
>
> 1. open xterm/gnome-terminal.
> 2. Run + put it in background: sleep 600 &
> 3. Close the terminal.
> 4. Check for processes. sleep is not disconnected.
>
>
>
>
> On Mon, Feb 20, 2017 at 12:42 PM, Cleber Rosa <crosa at redhat.com
> <mailto:crosa at redhat.com>> wrote:
>
> Hey guys,
>
> I think the following experiment can be didactic:
>
> $ echo $$
> 1000
> $ /bin/bash
> $ echo $$
> 1010
> $ sleep 600 &
> $ exit
> $ echo $$
> 1000
> $ ps -eo ppid,cmd | grep sleep
> 1 sleep 600
>
> We can say that shell with PID 1000 is Avocado, PID 1010 is
> your test
> process, and sleep is the subprocess you're running on your
> test.
>
> What does this mean? When a parent does not wait() for
> their children
> processes, they find a new parent. That's simply how
> UNIX/Linux work.
>
> A process is indeed a resource. Just like a file on a
> filesystem. If
> you don't want it lying around on your system after you
> created it, you
> have to take actions.
>
> My point is: IMHO there's nothing broken with Avocado in
> this regard.
> If you want automatic ("automagic") resources cleanup,
> something like
> running tests on a VM with a snapshot is the way to go. But
> the most
> correct way is indeed trying to keep track of the resources
> you create
> on your test.
>
> - Cleber.
>
> On 02/17/2017 12:48 PM, Andrei Stepanov wrote:
> > Hi.
> >
> > It seems to me, that you are talking about global task: "track of
> > special resources"
> >
> > Radek's case is much simple: kill all children.
> >
> > I think it is correct behavior. This is as it should be.
> >
> > There is nothing to track.
> >
> > On opposite side, if test want to keep running process that it can
> > detach it and make it daemon.
> >
> >
> >
> > On Fri, Feb 17, 2017 at 6:11 PM, Lucas Meneghel Rodrigues
> > <lookkas at gmail.com <mailto:lookkas at gmail.com>
> <mailto:lookkas at gmail.com <mailto:lookkas at gmail.com>>> wrote:
> >
> > I would avoid keeping track of special resources by the test runner
> > itself. It's the sort of thing that the test writer would be
> > expected to implement on cleanup procedures.
> >
> > Implementing said track of subprocesses would be justifiable if we
> > look at the following perspectives:
> >
> > 1) We want to reduce the amount of boilerplate code in tests. This
> > would hold true if Radek's use case was very common, like in many
> > occasions you want to spawn a process in the background and forget
> > about it. It doesn't seem it is.
> > 2) We want avocado to be very good in avoid leaking resources
> > regardless of decisions made by test writers.
> >
> > I can see the point 2) as valid, but again, it's introducing a lot
> > of code to handle certain resources transparently for users that
> > might not need that on a regular basis.
> >
> > On Fri, Feb 17, 2017 at 11:55 AM Amador Pahim <apahim at redhat.com <mailto:apahim at redhat.com>
> > <mailto:apahim at redhat.com <mailto:apahim at redhat.com>>> wrote:
> >
> > On Fri, Feb 17, 2017 at 9:42 AM, Radek Duda <rduda at redhat.com <mailto:rduda at redhat.com>
> > <mailto:rduda at redhat.com <mailto:rduda at redhat.com>>> wrote:
> > > Good morning folks,
> > > Andrei thanks for this line of code I've forgotten to include
> > it. So the
> > > code reads:
> > >
> > > def run(vt_test, test_params, env):
> > > cmd = "nc -l %s" % test_params['some_port']
> > > nc_process = process.SubProcess(cmd)
> > > nc_process_pid = nc_process.start()
> > > return
> > >
> > > Amador thanks for your advice, but it doesn't work for me. I
> > need the
> > > process to run in the background and not block test flow.
> > That's why I used
> > > start() method. I tried run() method as well, but the process
> > does not go to
> > > background and blocked test flow. I think the process should
> > be killed
> > > automatically after test finishes.
> >
> > Oh, now I see what you need.
> > Well, I'd recommend to finish the process in tearDown(). Not sure if
> > we should make the runner to track the sub-processes created by the
> > test... let me open a card to discuss that.
> >
> >
> > >
> > > Radek
> > >
> > > On Thu, Feb 16, 2017 at 5:09 PM, Amador Pahim
> > <apahim at redhat.com <mailto:apahim at redhat.com>
> <mailto:apahim at redhat.com <mailto:apahim at redhat.com>>> wrote:
> > >>
> > >> On Thu, Feb 16, 2017 at 5:02 PM, Andrei Stepanov
> > <astepano at redhat.com <mailto:astepano at redhat.com>
> <mailto:astepano at redhat.com <mailto:astepano at redhat.com>>>
> > >> wrote:
> > >> > I think
> > >> >
> > >> > nc_process_pid = nc_process.start()
> > >>
> > >> In that case, the missing part is the `wait()` (and the possible
> > >> timeout handling) which are both present in `run()`. You can
> > call it
> > >> directly (with `process.run(cmd)`) instead of keeping the
> > SubProcess
> > >> object. Unless you have other reasons to have that object.
> > >>
> > >> >
> > >> > On Thu, Feb 16, 2017 at 4:58 PM, Amador Pahim
> > <apahim at redhat.com <mailto:apahim at redhat.com>
> <mailto:apahim at redhat.com <mailto:apahim at redhat.com>>> wrote:
> > >> >>
> > >> >> On Thu, Feb 16, 2017 at 4:38 PM, Radek Duda
> > <rduda at redhat.com <mailto:rduda at redhat.com>
> <mailto:rduda at redhat.com <mailto:rduda at redhat.com>>> wrote:
> > >> >> > Dear avocado users and developers,
> > >> >> > I have made a testcase in which is executed nc process
> > in run
> > >> >> > function :
> > >> >> > (simplified version):
> > >> >> >
> > >> >> > from avocado.utils import process
> > >> >> >
> > >> >> > def run(vt_test, test_params, env):
> > >> >> > cmd = "nc -l %s" % test_params['some_port']
> > >> >> > nc_process = process.SubProcess(cmd)
> > >> >> > return
> > >> >> >
> > >> >> > after testcase is executed, nc does not terminate and is
> > still
> > >> >> > present.
> > >> >> > To
> > >> >> > avoid this I have to kill the process e.g. by
> > >> >> > process.safe_kill(nc_process_pid, signal.SIGKILL)
> > >> >>
> > >> >> Are you running the process with `nc_process.run()`? It's
> > expected to
> > >> >> wait for the process to finish.
> > >> >>
> > >> >>
> > >> >>
> > >> >>
> > https://github.com/avocado-framework/avocado/blob/master/avocado/utils/process.py#L596-L643
> <https://github.com/avocado-framework/avocado/blob/master/avocado/utils/process.py#L596-L643>
> > <https://github.com/avocado-framework/avocado/blob/master/avocado/utils/process.py#L596-L643
> <https://github.com/avocado-framework/avocado/blob/master/avocado/utils/process.py#L596-L643>>
> > >> >>
> > >> >>
> > >> >> >
> > >> >> > It is pretty awkward to close the process manually
> > particularly in
> > >> >> > case
> > >> >> > of
> > >> >> > complex testcase code
> > >> >> > Shouldn't be the subprocess killed automatically after
> > test exits?
> > >> >> > After all its called SubProcess
> > >> >> >
> > >> >> >
> > >> >> > regards,
> > >> >> >
> > >> >> > Radek Duda
> > >> >>
> > >> >
> > >
> > >
> >
> >
>
> --
> Cleber Rosa
> [ Sr Software Engineer - Virtualization Team - Red Hat ]
> [ Avocado Test Framework - avocado-framework.github.io
> <http://avocado-framework.github.io> ]
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 502 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/avocado-devel/attachments/20170220/a26713a7/attachment.sig>
More information about the Avocado-devel
mailing list