Hello guys, Dne 20.2.2017 v 14:46 Andrei Stepanov napsal(a):
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-testIt 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.
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 <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> 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 > <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> 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 <[email protected] <mailto:[email protected]> > <mailto:[email protected] <mailto:[email protected]>>> wrote: > > On Fri, Feb 17, 2017 at 9:42 AM, Radek Duda <[email protected] <mailto:[email protected]> > <mailto:[email protected] <mailto:[email protected]>>> 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 > <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote: > >> > >> On Thu, Feb 16, 2017 at 5:02 PM, Andrei Stepanov > <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> > >> 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 > <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote: > >> >> > >> >> On Thu, Feb 16, 2017 at 4:38 PM, Radek Duda > <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> 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> ]
signature.asc
Description: OpenPGP digital signature
