Re: [Avocado-devel] Subprocess termination

[Lucas Meneghel Rodrigues]

On Mon, Feb 20, 2017 at 3:10 PM Cleber Rosa  wrote:

>
> On 02/20/2017 08:46 AM, Andrei Stepanov wrote:
> > 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.
> >
>
> Andrei,
>
> I've personally thought about this some time ago.  Right now, Avocado
> tries to "isolate" tests from each other by using separate processes.
> This is a rather weak isolation, but still better than what many other
> test runners offer.
>
> I believe that having stronger isolation at a test level would indeed be
> really useful.  Running each test on a separate and disposable
> "execution environment" (container, vm with snapshot, physical machine
> that gets reloaded) would be really powerful.
>
> Still, we don't have that feature at this time...
>

Indeed having a more isolated execution environment for tests would be
powerful. The question is, what should avocado do by default, since the
more isolated the environment gets, the more overhead it adds to test
execution. I remember some time ago someone was complaining that avocado
tests were executed on a separate subprocess, as having everything executed
under the same process is faster, and indeed what unittests and pytest does
by default (there is a multiprocess based pytest plugin).

Which makes me think if we should have configurable levels of isolation:

1 - Fast - everything gets executed under the main avocado process
2 - Default - Tests are executed with multiprocess
3 - Tests are executed in docker containers
4 - Tests are executed in virtual machines with snapshots

One of the problems to make this 'transparent' is the setup and
dependencies necessary to get 3 and 4 working out of the box.

Re: [Avocado-devel] Subprocess termination

[Cleber Rosa]

On 02/20/2017 08:46 AM, Andrei Stepanov wrote:
> 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. 
> 

Andrei,

I've personally thought about this some time ago.  Right now, Avocado
tries to "isolate" tests from each other by using separate processes.
This is a rather weak isolation, but still better than what many other
test runners offer.

I believe that having stronger isolation at a test level would indeed be
really useful.  Running each test on a separate and disposable
"execution environment" (container, vm with snapshot, physical machine
that gets reloaded) would be really powerful.

Still, we don't have that feature at this time...

> 
> 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.
> 

... and it doesn't exclude the careful writing of tests as a good practice.

- Cleber.

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]



signature.asc
Description: OpenPGP digital signature

Re: [Avocado-devel] Subprocess termination

[Lucas Meneghel Rodrigues]

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  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  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
> > > 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  > > wrote:
> >
> > On Fri, Feb 17, 2017 at 9:42 AM, Radek Duda  > > 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
> > > wrote:
> > >>
> > >> On Thu, Feb 16, 2017 at 5:02 PM, Andrei Stepanov
> > >
> > >> wrote:
> > >> > I think
> > >> >
> > >> > nc_process_pid = nc_process.start()
> >

Re: [Avocado-devel] Subprocess termination

[Andrei Stepanov]

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  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
> > > 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  > > wrote:
> >
> > On Fri, Feb 17, 2017 at 9:42 AM, Radek Duda  > > 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
> > > wrote:
> > >>
> > >> On Thu, Feb 16, 2017 at 5:02 PM, Andrei Stepanov
> > >
> > >> 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
> >  

Re: [Avocado-devel] Subprocess termination

[Andrei Stepanov]

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  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  wrote:
>
>> On Fri, Feb 17, 2017 at 9:42 AM, Radek Duda  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 
>> wrote:
>> >>
>> >> On Thu, Feb 16, 2017 at 5:02 PM, Andrei Stepanov 
>> >> 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 
>> wrote:
>> >> >>
>> >> >> On Thu, Feb 16, 2017 at 4:38 PM, Radek Duda 
>> 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
>> >> >>
>> >> >>
>> >> >> >
>> >> >> > 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
>> >> >>
>> >> >
>> >
>> >
>>
>>

Re: [Avocado-devel] Subprocess termination

[Lucas Meneghel Rodrigues]

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  wrote:

> On Fri, Feb 17, 2017 at 9:42 AM, Radek Duda  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  wrote:
> >>
> >> On Thu, Feb 16, 2017 at 5:02 PM, Andrei Stepanov 
> >> 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 
> wrote:
> >> >>
> >> >> On Thu, Feb 16, 2017 at 4:38 PM, Radek Duda 
> 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
> >> >>
> >> >>
> >> >> >
> >> >> > 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
> >> >>
> >> >
> >
> >
>
>

Re: [Avocado-devel] Subprocess termination

[Radek Duda]

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.

Radek

On Thu, Feb 16, 2017 at 5:09 PM, Amador Pahim  wrote:

> On Thu, Feb 16, 2017 at 5:02 PM, Andrei Stepanov 
> 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  wrote:
> >>
> >> On Thu, Feb 16, 2017 at 4:38 PM, Radek Duda  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
> >>
> >>
> >> >
> >> > 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
> >>
> >
>