Re: [PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts

Re: [PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts

[Michael Goldish]

----- "Avi Kivity" <avi@redhat.com> wrote:

> Michael Goldish wrote:
> > ----- "Avi Kivity" <avi@redhat.com> wrote:
> >
> >   
> >> David Huff wrote:
> >>     
> >>> This patch will run pre and post scripts
> >>> defined in config file with the parameter pre_command
> >>> and post_command post_command.
> >>>
> >>> Also exports all the prameters in preprocess for passing
> >>> arguments to the script.
> >>>  
> >>> +   #execute any pre_commands
> >>> +    pre_command = params.get("pre_command")
> >>> +    if pre_command:
> >>> +        # export environment vars
> >>> +        for k in params.keys():
> >>> +            kvm_log.info("Adding KVM_TEST_%s to Environment" %
> >>>       
> >> (k))
> >>     
> >>> +            os.putenv("KVM_TEST_%s" % (k), str(params[k]))
> >>> +        # execute command
> >>> +        kvm_log.info("Executing command '%s'..." % pre_command)
> >>> +        timeout = int(params.get("pre_commmand_timeout", "600"))
> >>> +        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" %
> >>>       
> >> (test.bindir, pre_command),
> >>     
> >>> +                                                 None,
> >>>       
> >> kvm_log.debug, "(pre_command) ", timeout=timeout)
> >>     
> >>> +        if status != 0:
> >>> +            kvm_utils.safe_kill(pid, signal.SIGTERM)
> >>> +            raise error.TestError, "Custom processing
> pre_command
> >>>       
> >> failed"
> >>     
> >>>   
> >>>       
> >> kvm_utils.run_bg should throw an exception instead of returning
> >> status.
> >>     
> >
> > - What if we're interested in the status for some reason? Its value
> > may indicate what went wrong with the child process.
> >   
> Put it in the exception string.

But I want its value to be examined programmatically in the code.
It's less comfortable to retrieve it from a string.

> > - If we throw an exception we should add a parameter that controls
> > whether an exception should be thrown (something like
> "ignore_status")
> > because in some cases we don't want to throw an exception.
> >   
> 
> I'll complain every time I see it.  If you don't care if the command 
> succeeds or not, why run it in the first place?

I care, but sometimes I don't want to fail the test when a command fails.
I can put every run_bg() call in a try-except statement, but that misses the
point of raising exceptions in the first place.

> > - A run_bg call has 3 possible outcomes: success (status == 0),
> failure
> > (status != 0) and timeout (process still running). If we throw an
> exception
> > upon failure, what do we do upon timeout? Sometimes it's good (qemu
> should
> > keep running unless something went wrong) and sometimes bad
> (pre_command
> > should probably not time out). So we end up needing an
> ignore_timeout
> > parameter as well.
> >   
> 
> A run_bg() call should return an object with a .join() method, or
> throw 
> an exception.  At some point you must call the join() method, which 
> throws an exception or returns None.

OK, I have queued patches to make run_bg() return such an object anyway,
except for the exceptions part.

> > - What if we want the test to fail with an informative
> test-specific
> > exception such as "something failed after migration"?
> >   
> 
> Chained exceptions can provide detailed information.

Wouldn't it complicate the test code?
How can I provide a detailed message such as "test command failed after migration" --
can you illustrate this in a small example?

> >> But if status != 0, will there actually be a pid to kill?
> >>     
> >
> > If the timeout expires and the process is still running, status is
> None.
> >   
> 
> functions which can return with three possible outcomes are difficult
> to use.

I tend to see it as two possible outcomes: completion or timeout. In the former
case, status is returned. In the latter, None is returned.

> -- 
> Do not meddle in the internals of kernels, for they are subtle and
> quick to panic.

Re: [PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts

[Avi Kivity]

Michael Goldish wrote:
>>>>     
>>>>         
>>> - What if we're interested in the status for some reason? Its value
>>> may indicate what went wrong with the child process.
>>>   
>>>       
>> Put it in the exception string.
>>     
>
> But I want its value to be examined programmatically in the code.
> It's less comfortable to retrieve it from a string.
>   

Then create a custom exception object and put it in a field.

>   
>>> - If we throw an exception we should add a parameter that controls
>>> whether an exception should be thrown (something like
>>>       
>> "ignore_status")
>>     
>>> because in some cases we don't want to throw an exception.
>>>   
>>>       
>> I'll complain every time I see it.  If you don't care if the command 
>> succeeds or not, why run it in the first place?
>>     
>
> I care, but sometimes I don't want to fail the test when a command fails.
> I can put every run_bg() call in a try-except statement, but that misses the
> point of raising exceptions in the first place.
>   

I think it's very rare to want to let the test continue even if some 
command fails.

Can you give examples?

>>> exception such as "something failed after migration"?
>>>   
>>>       
>> Chained exceptions can provide detailed information.
>>     
>
> Wouldn't it complicate the test code?
> How can I provide a detailed message such as "test command failed after migration" --
> can you illustrate this in a small example?
>   

  try:
     code
     code
     code
  except e:
     raise ChainedException('exception while running migration test', e)

Instead of checking each code line, you provide a wrapper for the entire 
test.


>>>> But if status != 0, will there actually be a pid to kill?
>>>>     
>>>>         
>>> If the timeout expires and the process is still running, status is
>>>       
>> None.
>>     
>>>   
>>>       
>> functions which can return with three possible outcomes are difficult
>> to use.
>>     
>
> I tend to see it as two possible outcomes: completion or timeout. In the former
> case, status is returned. In the latter, None is returned.
>   

The caller has to test for three possible outcomes.  Success, failure, 
and 'command is running in the background'.  If your callsites don't 
check for all three, something's wrong.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts

[Michael Goldish]

----- "Avi Kivity" <avi@redhat.com> wrote:

> Michael Goldish wrote:
> >> I think it's very rare to want to let the test continue even if
> some 
> >> command fails.
> >>
> >> Can you give examples?
> >>     
> >
> > Some commands are not critical, like one that converts the
> screendumps
> > from PPM to PNG format. We don't want to fail the test if you don't
> have
> > ImageMagick installed.
> >   
> 
> I think you don't want to run the test at all in that case.

I do -- the conversion is just intended to save some disk space after the
test completes.

> But if you do, you shouldn't invoke ImageMagick if you don't have it installed. 
> 
> Otherwise you can't distinguish between failure due to a corrupted 
> source image, out of disk space, or ImageMagick not installed.

You can, if you look at the output and/or status that run_bg() returns (e.g.
"command not found" and 127). But you don't need to because you shouldn't
do anything special if the command fails, except inform the user that it
failed. This command is really not critical (we're currently not using it
anyway -- it's just planned for the near future).

> Ignoring errors is usually a bug.
>
> > A similar function used for remote commands is sometimes used for
> checking
> > if a file exists on the guest and getting its size at the same
> time,
> > using ls. If the file doesn't exist we should SCP it into the guest,
> not
> > fail the test.
> >   
> 
> Again it doesn't distinguish between real errors and 'file does not 
> exist'.  Use something like 'if test -f file; then ls -l file; else
> echo 
> missing; fi'.

I don't see what's wrong with 'ls filename'. It does exactly what the line
you suggested does -- if the file exists it displays some info, and if it
doesn't it says "No such file ...".
In the test we explicitly check for the "No such file" message, so it's
being handled properly.

> You can use rsync to implement 'copy unless exists'.

The guest may not like rsync. We may have to work with FTP in some cases
(Windows).

> >>
> >>   try:
> >>      code
> >>      code
> >>      code
> >>   except e:
> >>      raise ChainedException('exception while running migration
> test',
> >> e)
> >>
> >> Instead of checking each code line, you provide a wrapper for the
> >> entire test.
> >>     
> >
> > It's not informative to add it for the entire test, because we
> already know
> > what test failed -- we don't need the exception string for that.
> >
> > Instead, we'd have to wrap small sections of the test separately and
> provide
> > a different exception string for each. The migration test may be
> split to
> > four sections: setup, pre-migration, the migration itself and
> post-migration.
> > This may not be so bad, but do you still think it's a good
> solution?
> >   
> 
> You can have a 'stage' variable and assign it your current stage, and
> use it when reporting errors.
> 
> Anything is better than a maze of ifs.
> 
> > There's also another issue: the utility functions that should raise
> exceptions
> > don't know what they're being used for, so there can be exception
> string
> > collisions. For example, both run_bg(), used for local processes,
> and
> > kvm_spawn.get_command_status_output(), used for SSH commands, would
> say
> > something like "command %s failed". It would be incorrect to
> hardcode a more
> > specific message into any of them (in my opinion).
> >
> >   
> 
> The message should explain what happened, no more, no less.

"What happened" depends on the point of view.

It doesn't really help the user to print something like
"read_until_output_matches: Timeout elapsed"
instead of
"Could not log into guest after migration".

It would also not be useful to see
"get_command_status_output: command 'tar xfj autotest.tar.bz2' failed with status 1"
without knowing if it's a remote or a local command (the function itself doesn't know).

> >> The caller has to test for three possible outcomes.  Success,
> failure,
> >> and 'command is running in the background'.  If your callsites
> don't 
> >> check for all three, something's wrong.
> >>     
> >
> > When we want the command to succeed on time, we just test for status
> == 0.
> > If it's a nonzero integer or None, something went wrong, and we
> should kill
> > the process just in case it's still running.
> >   
> 
> But if it's nonzero it has exited already!

That's why we use safe_kill(), which kills only if the process is still running.

> However, I feel I'm not making any progress, so I'll stop.  I just
> wish I could use exceptions in the code I write.

We are making progress: I'll try to think of a simple way to start working
with exceptions without breaking too many things. In the worst case, we can
just keep two sets of functions: low-level ones that don't raise anything,
and high level ones that raise an exception whenever anything seems to have
gone wrong.

I'm sorry for being a bit too stubborn. I prefer to understand things
before going ahead and implementing them, and I still feel that some of my
questions remain unanswered.

> 
> -- 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.

Re: [PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts

[Michael Goldish]

----- "Avi Kivity" <avi@redhat.com> wrote:

> Michael Goldish wrote:
> >>>>     
> >>>>         
> >>> - What if we're interested in the status for some reason? Its
> value
> >>> may indicate what went wrong with the child process.
> >>>   
> >>>       
> >> Put it in the exception string.
> >>     
> >
> > But I want its value to be examined programmatically in the code.
> > It's less comfortable to retrieve it from a string.
> >   
> 
> Then create a custom exception object and put it in a field.
> 
> >   
> >>> - If we throw an exception we should add a parameter that
> controls
> >>> whether an exception should be thrown (something like
> >>>       
> >> "ignore_status")
> >>     
> >>> because in some cases we don't want to throw an exception.
> >>>   
> >>>       
> >> I'll complain every time I see it.  If you don't care if the
> command 
> >> succeeds or not, why run it in the first place?
> >>     
> >
> > I care, but sometimes I don't want to fail the test when a command
> fails.
> > I can put every run_bg() call in a try-except statement, but that
> misses the
> > point of raising exceptions in the first place.
> >   
> 
> I think it's very rare to want to let the test continue even if some 
> command fails.
> 
> Can you give examples?

Some commands are not critical, like one that converts the screendumps
from PPM to PNG format. We don't want to fail the test if you don't have
ImageMagick installed.

A similar function used for remote commands is sometimes used for checking
if a file exists on the guest and getting its size at the same time,
using ls. If the file doesn't exist we should SCP it into the guest, not
fail the test.

> >>> exception such as "something failed after migration"?
> >>>   
> >>>       
> >> Chained exceptions can provide detailed information.
> >>     
> >
> > Wouldn't it complicate the test code?
> > How can I provide a detailed message such as "test command failed
> after migration" --
> > can you illustrate this in a small example?
> >   
> 
>   try:
>      code
>      code
>      code
>   except e:
>      raise ChainedException('exception while running migration test',
> e)
> 
> Instead of checking each code line, you provide a wrapper for the
> entire test.

It's not informative to add it for the entire test, because we already know
what test failed -- we don't need the exception string for that.

Instead, we'd have to wrap small sections of the test separately and provide
a different exception string for each. The migration test may be split to
four sections: setup, pre-migration, the migration itself and post-migration.
This may not be so bad, but do you still think it's a good solution?

There's also another issue: the utility functions that should raise exceptions
don't know what they're being used for, so there can be exception string
collisions. For example, both run_bg(), used for local processes, and
kvm_spawn.get_command_status_output(), used for SSH commands, would say
something like "command %s failed". It would be incorrect to hardcode a more
specific message into any of them (in my opinion).

> >>>> But if status != 0, will there actually be a pid to kill?
> >>>>     
> >>>>         
> >>> If the timeout expires and the process is still running, status
> is
> >>>       
> >> None.
> >>     
> >>>   
> >>>       
> >> functions which can return with three possible outcomes are
> difficult
> >> to use.
> >>     
> >
> > I tend to see it as two possible outcomes: completion or timeout. In
> the former
> > case, status is returned. In the latter, None is returned.
> >   
> 
> The caller has to test for three possible outcomes.  Success, failure,
> and 'command is running in the background'.  If your callsites don't 
> check for all three, something's wrong.

When we want the command to succeed on time, we just test for status == 0.
If it's a nonzero integer or None, something went wrong, and we should kill
the process just in case it's still running.

> -- 
> Do not meddle in the internals of kernels, for they are subtle and
> quick to panic.

Re: [PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts

[Avi Kivity]

Michael Goldish wrote:
>> I think it's very rare to want to let the test continue even if some 
>> command fails.
>>
>> Can you give examples?
>>     
>
> Some commands are not critical, like one that converts the screendumps
> from PPM to PNG format. We don't want to fail the test if you don't have
> ImageMagick installed.
>   

I think you don't want to run the test at all in that case.  But if you 
do, you shouldn't invoke ImageMagick if you don't have it installed.  
Otherwise you can't distinguish between failure due to a corrupted 
source image, out of disk space, or ImageMagick not installed.

Ignoring errors is usually a bug.

> A similar function used for remote commands is sometimes used for checking
> if a file exists on the guest and getting its size at the same time,
> using ls. If the file doesn't exist we should SCP it into the guest, not
> fail the test.
>   

Again it doesn't distinguish between real errors and 'file does not 
exist'.  Use something like 'if test -f file; then ls -l file; else echo 
missing; fi'.

You can use rsync to implement 'copy unless exists'.

>>
>>   try:
>>      code
>>      code
>>      code
>>   except e:
>>      raise ChainedException('exception while running migration test',
>> e)
>>
>> Instead of checking each code line, you provide a wrapper for the
>> entire test.
>>     
>
> It's not informative to add it for the entire test, because we already know
> what test failed -- we don't need the exception string for that.
>
> Instead, we'd have to wrap small sections of the test separately and provide
> a different exception string for each. The migration test may be split to
> four sections: setup, pre-migration, the migration itself and post-migration.
> This may not be so bad, but do you still think it's a good solution?
>   

You can have a 'stage' variable and assign it your current stage, and 
use it when reporting errors.

Anything is better than a maze of ifs.

> There's also another issue: the utility functions that should raise exceptions
> don't know what they're being used for, so there can be exception string
> collisions. For example, both run_bg(), used for local processes, and
> kvm_spawn.get_command_status_output(), used for SSH commands, would say
> something like "command %s failed". It would be incorrect to hardcode a more
> specific message into any of them (in my opinion).
>
>   

The message should explain what happened, no more, no less.

>> The caller has to test for three possible outcomes.  Success, failure,
>> and 'command is running in the background'.  If your callsites don't 
>> check for all three, something's wrong.
>>     
>
> When we want the command to succeed on time, we just test for status == 0.
> If it's a nonzero integer or None, something went wrong, and we should kill
> the process just in case it's still running.
>   

But if it's nonzero it has exited already!

However, I feel I'm not making any progress, so I'll stop.  I just wish 
I could use exceptions in the code I write.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts

[Michael Goldish]

----- "Avi Kivity" <avi@redhat.com> wrote:

> David Huff wrote:
> > This patch will run pre and post scripts
> > defined in config file with the parameter pre_command
> > and post_command post_command.
> >
> > Also exports all the prameters in preprocess for passing
> > arguments to the script.
> >  
> > +   #execute any pre_commands
> > +    pre_command = params.get("pre_command")
> > +    if pre_command:
> > +        # export environment vars
> > +        for k in params.keys():
> > +            kvm_log.info("Adding KVM_TEST_%s to Environment" %
> (k))
> > +            os.putenv("KVM_TEST_%s" % (k), str(params[k]))
> > +        # execute command
> > +        kvm_log.info("Executing command '%s'..." % pre_command)
> > +        timeout = int(params.get("pre_commmand_timeout", "600"))
> > +        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" %
> (test.bindir, pre_command),
> > +                                                 None,
> kvm_log.debug, "(pre_command) ", timeout=timeout)
> > +        if status != 0:
> > +            kvm_utils.safe_kill(pid, signal.SIGTERM)
> > +            raise error.TestError, "Custom processing pre_command
> failed"
> >   
> 
> kvm_utils.run_bg should throw an exception instead of returning
> status.

- What if we're interested in the status for some reason? Its value
may indicate what went wrong with the child process.
- If we throw an exception we should add a parameter that controls
whether an exception should be thrown (something like "ignore_status")
because in some cases we don't want to throw an exception.
- A run_bg call has 3 possible outcomes: success (status == 0), failure
(status != 0) and timeout (process still running). If we throw an exception
upon failure, what do we do upon timeout? Sometimes it's good (qemu should
keep running unless something went wrong) and sometimes bad (pre_command
should probably not time out). So we end up needing an ignore_timeout
parameter as well.
- What if we want the test to fail with an informative test-specific
exception such as "something failed after migration"?

> But if status != 0, will there actually be a pid to kill?

If the timeout expires and the process is still running, status is None.

> -- 
> error compiling committee.c: too many arguments to function
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts

[Avi Kivity]

Michael Goldish wrote:
> ----- "Avi Kivity" <avi@redhat.com> wrote:
>
>   
>> David Huff wrote:
>>     
>>> This patch will run pre and post scripts
>>> defined in config file with the parameter pre_command
>>> and post_command post_command.
>>>
>>> Also exports all the prameters in preprocess for passing
>>> arguments to the script.
>>>  
>>> +   #execute any pre_commands
>>> +    pre_command = params.get("pre_command")
>>> +    if pre_command:
>>> +        # export environment vars
>>> +        for k in params.keys():
>>> +            kvm_log.info("Adding KVM_TEST_%s to Environment" %
>>>       
>> (k))
>>     
>>> +            os.putenv("KVM_TEST_%s" % (k), str(params[k]))
>>> +        # execute command
>>> +        kvm_log.info("Executing command '%s'..." % pre_command)
>>> +        timeout = int(params.get("pre_commmand_timeout", "600"))
>>> +        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" %
>>>       
>> (test.bindir, pre_command),
>>     
>>> +                                                 None,
>>>       
>> kvm_log.debug, "(pre_command) ", timeout=timeout)
>>     
>>> +        if status != 0:
>>> +            kvm_utils.safe_kill(pid, signal.SIGTERM)
>>> +            raise error.TestError, "Custom processing pre_command
>>>       
>> failed"
>>     
>>>   
>>>       
>> kvm_utils.run_bg should throw an exception instead of returning
>> status.
>>     
>
> - What if we're interested in the status for some reason? Its value
> may indicate what went wrong with the child process.
>   
Put it in the exception string.

> - If we throw an exception we should add a parameter that controls
> whether an exception should be thrown (something like "ignore_status")
> because in some cases we don't want to throw an exception.
>   

I'll complain every time I see it.  If you don't care if the command 
succeeds or not, why run it in the first place?

> - A run_bg call has 3 possible outcomes: success (status == 0), failure
> (status != 0) and timeout (process still running). If we throw an exception
> upon failure, what do we do upon timeout? Sometimes it's good (qemu should
> keep running unless something went wrong) and sometimes bad (pre_command
> should probably not time out). So we end up needing an ignore_timeout
> parameter as well.
>   

A run_bg() call should return an object with a .join() method, or throw 
an exception.  At some point you must call the join() method, which 
throws an exception or returns None.

> - What if we want the test to fail with an informative test-specific
> exception such as "something failed after migration"?
>   

Chained exceptions can provide detailed information.

>> But if status != 0, will there actually be a pid to kill?
>>     
>
> If the timeout expires and the process is still running, status is None.
>   

functions which can return with three possible outcomes are difficult to 
use.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts

[Lucas Meneghel Rodrigues]

On Wed, 2009-06-03 at 02:14 -0400, Michael Goldish wrote:
> ----- "Avi Kivity" <avi@redhat.com> wrote:
> 
> > David Huff wrote:
> > > This patch will run pre and post scripts
> > > defined in config file with the parameter pre_command
> > > and post_command post_command.
> > >
> > > Also exports all the prameters in preprocess for passing
> > > arguments to the script.
> > >  
> > > +   #execute any pre_commands
> > > +    pre_command = params.get("pre_command")
> > > +    if pre_command:
> > > +        # export environment vars
> > > +        for k in params.keys():
> > > +            kvm_log.info("Adding KVM_TEST_%s to Environment" %
> > (k))
> > > +            os.putenv("KVM_TEST_%s" % (k), str(params[k]))
> > > +        # execute command
> > > +        kvm_log.info("Executing command '%s'..." % pre_command)
> > > +        timeout = int(params.get("pre_commmand_timeout", "600"))
> > > +        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" %
> > (test.bindir, pre_command),
> > > +                                                 None,
> > kvm_log.debug, "(pre_command) ", timeout=timeout)
> > > +        if status != 0:
> > > +            kvm_utils.safe_kill(pid, signal.SIGTERM)
> > > +            raise error.TestError, "Custom processing pre_command
> > failed"
> > >   
> > 
> > kvm_utils.run_bg should throw an exception instead of returning
> > status.

utils.system or utils.run will throw a error.CmdError exception on exit
code !=0. The representation and the printed format of CmdErrors contain
the exit code, the full string of the command and stuff like that.

And yes, I agree in general that we should throw exceptions when command
fails. If the failure is not bad enough to stop the whole test, we can
catch the exception and increment failure counters or something to keep
record of the failures. 

> - What if we're interested in the status for some reason? Its value
> may indicate what went wrong with the child process.
> - If we throw an exception we should add a parameter that controls
> whether an exception should be thrown (something like "ignore_status")
> because in some cases we don't want to throw an exception.
> - A run_bg call has 3 possible outcomes: success (status == 0), failure
> (status != 0) and timeout (process still running). If we throw an exception
> upon failure, what do we do upon timeout? Sometimes it's good (qemu should
> keep running unless something went wrong) and sometimes bad (pre_command
> should probably not time out). So we end up needing an ignore_timeout
> parameter as well.
> - What if we want the test to fail with an informative test-specific
> exception such as "something failed after migration"?
> 
> > But if status != 0, will there actually be a pid to kill?
> 
> If the timeout expires and the process is still running, status is None.
> 
> > -- 
> > error compiling committee.c: too many arguments to function
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Lucas Meneghel Rodrigues
Software Engineer (QE)
Red Hat - Emerging Technologies

Re: [PATCH] [KVM_Autotest] Added functionality to the preprocessor to run scripts

[Michael Goldish]

Looks good to me. See some comments below.

----- "David Huff" <dhuff@redhat.com> wrote:

> This patch will run pre and post scripts
> defined in config file with the parameter pre_command
> and post_command post_command.
> 
> Also exports all the prameters in preprocess for passing
> arguments to the script.

Why not do this for post_command as well?

> ---
>  client/tests/kvm_runtest_2/kvm_preprocessing.py |   31
> +++++++++++++++++++++-
>  1 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/client/tests/kvm_runtest_2/kvm_preprocessing.py
> b/client/tests/kvm_runtest_2/kvm_preprocessing.py
> index c9eb35d..02df615 100644
> --- a/client/tests/kvm_runtest_2/kvm_preprocessing.py
> +++ b/client/tests/kvm_runtest_2/kvm_preprocessing.py
> @@ -135,8 +135,7 @@ def postprocess_vm(test, params, env, name):
>                  "Waiting for VM to kill itself..."):
>              kvm_log.debug("'kill_vm' specified; killing VM...")
>          vm.destroy(gracefully = params.get("kill_vm_gracefully") ==
> "yes")
> -
> -
> +        

I hate to be petty, but we usually keep two blank lines between top
level functions.

Also, you have some trailing whitespace there...

>  def process(test, params, env, image_func, vm_func):
>      """Pre- or post-process VMs and images according to the
> instructions in params.
>  
> @@ -169,6 +168,7 @@ def preprocess(test, params, env):
>      params -- a dict containing all VM and image parameters
>      env -- the environment (a dict-like object)
>  
> +    Also, runs any setup command defined in the parameter
> pre_command
>      Also, collect some host information, such as the KVM version.
>      """
>      # Verify the identities of all living VMs
> @@ -192,6 +192,22 @@ def preprocess(test, params, env):
>              vm.destroy()
>              del env[key]
>  
> +   #execute any pre_commands
> +    pre_command = params.get("pre_command")
> +    if pre_command:
> +        # export environment vars
> +        for k in params.keys():
> +            kvm_log.info("Adding KVM_TEST_%s to Environment" % (k))
> +            os.putenv("KVM_TEST_%s" % (k), str(params[k]))
> +
> +        # execute command
> +        kvm_log.info("Executing command '%s'..." % pre_command)
> +        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" %
> (test.bindir, pre_command),
> +                                                 None, kvm_log.debug,
> "(pre_command) ", timeout=600)

It wouldn't hurt to make this timeout user-configurable with a default
value of 600 or so:

timeout = int(params.get("pre_commmand_timeout", "600"))
(status, pid, output) = kvm_utils.run_bg(..., timeout=timeout)

We can also do that in a separate patch.

> +        if status != 0:
> +            kvm_utils.safe_kill(pid, signal.SIGTERM)
> +            raise error.TestError, "Custom processing pre_command
> failed"
> +
>      # Preprocess all VMs and images
>      process(test, params, env, preprocess_image, preprocess_vm)
>  
> @@ -232,6 +248,8 @@ def postprocess(test, params, env):
>      test -- an Autotest test object
>      params -- a dict containing all VM and image parameters
>      env -- the environment (a dict-like object)
> +    
> +    Also, runs any command defined in the parameter post_command
>      """
>      process(test, params, env, postprocess_image, postprocess_vm)
>  
> @@ -241,6 +259,15 @@ def postprocess(test, params, env):
>          kvm_log.debug("'keep_ppm_files' not specified; removing all
> PPM files from results dir...")
>          kvm_utils.run_bg("rm -vf %s" % os.path.join(test.debugdir,
> "*.ppm"), None, kvm_log.debug, "(rm) ", timeout=5.0)
>  
> +    #execute any post_commands
> +    post_command = params.get("post_command")
> +    if post_command:
> +        kvm_log.info("Executing command '%s'..." % post_command)
> +        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" %
> (test.bindir, post_command),
> +                                                 None, kvm_log.debug,
> "(post_command) ", timeout=600)
> +        if status != 0:
> +            kvm_utils.safe_kill(pid, signal.SIGTERM)
> +            raise error.TestError, "Custom processing command
> failed"

Same for post_command.

>  def postprocess_on_error(test, params, env):
>      """Perform postprocessing operations required only if the test
> failed.


Thanks,
Michael

Re: [PATCH] [KVM_Autotest] Added functionality to the preprocessor to run scripts

[David Huff]

Michael Goldish wrote:
> Looks good to me. See some comments below.
> 
> ----- "David Huff" <dhuff@redhat.com> wrote:
> 
>> This patch will run pre and post scripts
>> defined in config file with the parameter pre_command
>> and post_command post_command.
>>
>> Also exports all the prameters in preprocess for passing
>> arguments to the script.
> 
> Why not do this for post_command as well?

I didn't do post_command b/c I figured that they would already be
exported in the pre_command, however I guess that there can be the case
where there is no pre and only post and I guess exporting twice will not
hurt anything....  I will add this to the patch.

> 
>> ---
>>  client/tests/kvm_runtest_2/kvm_preprocessing.py |   31
>> +++++++++++++++++++++-
>>  1 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/client/tests/kvm_runtest_2/kvm_preprocessing.py
>> b/client/tests/kvm_runtest_2/kvm_preprocessing.py
>> index c9eb35d..02df615 100644
>> --- a/client/tests/kvm_runtest_2/kvm_preprocessing.py
>> +++ b/client/tests/kvm_runtest_2/kvm_preprocessing.py
>> @@ -135,8 +135,7 @@ def postprocess_vm(test, params, env, name):
>>                  "Waiting for VM to kill itself..."):
>>              kvm_log.debug("'kill_vm' specified; killing VM...")
>>          vm.destroy(gracefully = params.get("kill_vm_gracefully") ==
>> "yes")
>> -
>> -
>> +        
> 
> I hate to be petty, but we usually keep two blank lines between top
> level functions.
> 
> Also, you have some trailing whitespace there...

good catch, I will take care of this

> It wouldn't hurt to make this timeout user-configurable with a default
> value of 600 or so:
> 
> timeout = int(params.get("pre_commmand_timeout", "600"))
> (status, pid, output) = kvm_utils.run_bg(..., timeout=timeout)
> 
> We can also do that in a separate patch.
> 

I'll go ahead and add this while I rework the patch...

-D

[PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts

[David Huff]

This patch will run pre and post scripts
defined in config file with the parameter pre_command
and post_command post_command.

Also exports all the prameters in preprocess for passing
arguments to the script.
---
 client/tests/kvm_runtest_2/kvm_preprocessing.py |   34 +++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/client/tests/kvm_runtest_2/kvm_preprocessing.py b/client/tests/kvm_runtest_2/kvm_preprocessing.py
index c9eb35d..07bfdeb 100644
--- a/client/tests/kvm_runtest_2/kvm_preprocessing.py
+++ b/client/tests/kvm_runtest_2/kvm_preprocessing.py
@@ -169,6 +169,7 @@ def preprocess(test, params, env):
     params -- a dict containing all VM and image parameters
     env -- the environment (a dict-like object)
 
+    Also, runs any setup command defined in the parameter pre_command
     Also, collect some host information, such as the KVM version.
     """
     # Verify the identities of all living VMs
@@ -192,6 +193,22 @@ def preprocess(test, params, env):
             vm.destroy()
             del env[key]
 
+   #execute any pre_commands
+    pre_command = params.get("pre_command")
+    if pre_command:
+        # export environment vars
+        for k in params.keys():
+            kvm_log.info("Adding KVM_TEST_%s to Environment" % (k))
+            os.putenv("KVM_TEST_%s" % (k), str(params[k]))
+        # execute command
+        kvm_log.info("Executing command '%s'..." % pre_command)
+        timeout = int(params.get("pre_commmand_timeout", "600"))
+        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" % (test.bindir, pre_command),
+                                                 None, kvm_log.debug, "(pre_command) ", timeout=timeout)
+        if status != 0:
+            kvm_utils.safe_kill(pid, signal.SIGTERM)
+            raise error.TestError, "Custom processing pre_command failed"
+
     # Preprocess all VMs and images
     process(test, params, env, preprocess_image, preprocess_vm)
 
@@ -232,6 +249,8 @@ def postprocess(test, params, env):
     test -- an Autotest test object
     params -- a dict containing all VM and image parameters
     env -- the environment (a dict-like object)
+
+    Also, runs any command defined in the parameter post_command
     """
     process(test, params, env, postprocess_image, postprocess_vm)
 
@@ -241,6 +260,21 @@ def postprocess(test, params, env):
         kvm_log.debug("'keep_ppm_files' not specified; removing all PPM files from results dir...")
         kvm_utils.run_bg("rm -vf %s" % os.path.join(test.debugdir, "*.ppm"), None, kvm_log.debug, "(rm) ", timeout=5.0)
 
+    #execute any post_commands
+    post_command = params.get("post_command")
+    if post_command:
+        # export environment vars
+        for k in params.keys():
+            kvm_log.info("Adding KVM_TEST_%s to Environment" % (k))
+            os.putenv("KVM_TEST_%s" % (k), str(params[k]))
+        #execute command
+        kvm_log.info("Executing command '%s'..." % post_command)
+        timeout = int(params.get("post_commmand_timeout", "600"))
+        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" % (test.bindir, post_command),
+                                                 None, kvm_log.debug, "(pre_command) ", timeout=timeout)
+        if status != 0:
+            kvm_utils.safe_kill(pid, signal.SIGTERM)
+            raise error.TestError, "Custom processing command failed"
 
 def postprocess_on_error(test, params, env):
     """Perform postprocessing operations required only if the test failed.
-- 
1.6.0.6

Re: [PATCH][KVM_AUTOTEST] Added functionality to the preprocessor to run scripts

[Avi Kivity]

David Huff wrote:
> This patch will run pre and post scripts
> defined in config file with the parameter pre_command
> and post_command post_command.
>
> Also exports all the prameters in preprocess for passing
> arguments to the script.
>  
> +   #execute any pre_commands
> +    pre_command = params.get("pre_command")
> +    if pre_command:
> +        # export environment vars
> +        for k in params.keys():
> +            kvm_log.info("Adding KVM_TEST_%s to Environment" % (k))
> +            os.putenv("KVM_TEST_%s" % (k), str(params[k]))
> +        # execute command
> +        kvm_log.info("Executing command '%s'..." % pre_command)
> +        timeout = int(params.get("pre_commmand_timeout", "600"))
> +        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" % (test.bindir, pre_command),
> +                                                 None, kvm_log.debug, "(pre_command) ", timeout=timeout)
> +        if status != 0:
> +            kvm_utils.safe_kill(pid, signal.SIGTERM)
> +            raise error.TestError, "Custom processing pre_command failed"
>   

kvm_utils.run_bg should throw an exception instead of returning status.

But if status != 0, will there actually be a pid to kill?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] [KVM_Autotest] Added functionality to the preprocessor to run scripts

[sudhir kumar]

On Tue, May 26, 2009 at 9:37 PM, David Huff <dhuff@redhat.com> wrote:
> Michael Goldish wrote:
>> Looks good to me. See some comments below.
>>
>> ----- "David Huff" <dhuff@redhat.com> wrote:
>>
>>> This patch will run pre and post scripts
>>> defined in config file with the parameter pre_command
>>> and post_command post_command.
>>>
>>> Also exports all the prameters in preprocess for passing
>>> arguments to the script.
>>
>> Why not do this for post_command as well?
>
> I didn't do post_command b/c I figured that they would already be
> exported in the pre_command, however I guess that there can be the case
> where there is no pre and only post and I guess exporting twice will not
> hurt anything....  I will add this to the patch.
>
>>
>>> ---
>>>  client/tests/kvm_runtest_2/kvm_preprocessing.py |   31
>>> +++++++++++++++++++++-
>>>  1 files changed, 29 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/client/tests/kvm_runtest_2/kvm_preprocessing.py
>>> b/client/tests/kvm_runtest_2/kvm_preprocessing.py
>>> index c9eb35d..02df615 100644
>>> --- a/client/tests/kvm_runtest_2/kvm_preprocessing.py
>>> +++ b/client/tests/kvm_runtest_2/kvm_preprocessing.py
>>> @@ -135,8 +135,7 @@ def postprocess_vm(test, params, env, name):
>>>                  "Waiting for VM to kill itself..."):
>>>              kvm_log.debug("'kill_vm' specified; killing VM...")
>>>          vm.destroy(gracefully = params.get("kill_vm_gracefully") ==
>>> "yes")
>>> -
>>> -
>>> +
>>
>> I hate to be petty, but we usually keep two blank lines between top
>> level functions.
>>
>> Also, you have some trailing whitespace there...
>
> good catch, I will take care of this
>
>> It wouldn't hurt to make this timeout user-configurable with a default
>> value of 600 or so:
>>
>> timeout = int(params.get("pre_commmand_timeout", "600"))
>> (status, pid, output) = kvm_utils.run_bg(..., timeout=timeout)
>>
>> We can also do that in a separate patch.
>>
>
> I'll go ahead and add this while I rework the patch...
Please ensure that we have a number of timeout categories
now.(remote_login timeout, test execution timeout, migration timeout
etc) If we wana make timeout a variable let us do it for all the
timeouts.
>
> -D
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Sudhir Kumar

[PATCH] [KVM_Autotest] Added functionality to the preprocessor to run scripts

[David Huff]

This patch will run pre and post scripts
defined in config file with the parameter pre_command
and post_command post_command.

Also exports all the prameters in preprocess for passing
arguments to the script.
---
 client/tests/kvm_runtest_2/kvm_preprocessing.py |   31 +++++++++++++++++++++-
 1 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/client/tests/kvm_runtest_2/kvm_preprocessing.py b/client/tests/kvm_runtest_2/kvm_preprocessing.py
index c9eb35d..02df615 100644
--- a/client/tests/kvm_runtest_2/kvm_preprocessing.py
+++ b/client/tests/kvm_runtest_2/kvm_preprocessing.py
@@ -135,8 +135,7 @@ def postprocess_vm(test, params, env, name):
                 "Waiting for VM to kill itself..."):
             kvm_log.debug("'kill_vm' specified; killing VM...")
         vm.destroy(gracefully = params.get("kill_vm_gracefully") == "yes")
-
-
+        
 def process(test, params, env, image_func, vm_func):
     """Pre- or post-process VMs and images according to the instructions in params.
 
@@ -169,6 +168,7 @@ def preprocess(test, params, env):
     params -- a dict containing all VM and image parameters
     env -- the environment (a dict-like object)
 
+    Also, runs any setup command defined in the parameter pre_command
     Also, collect some host information, such as the KVM version.
     """
     # Verify the identities of all living VMs
@@ -192,6 +192,22 @@ def preprocess(test, params, env):
             vm.destroy()
             del env[key]
 
+   #execute any pre_commands
+    pre_command = params.get("pre_command")
+    if pre_command:
+        # export environment vars
+        for k in params.keys():
+            kvm_log.info("Adding KVM_TEST_%s to Environment" % (k))
+            os.putenv("KVM_TEST_%s" % (k), str(params[k]))
+
+        # execute command
+        kvm_log.info("Executing command '%s'..." % pre_command)
+        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" % (test.bindir, pre_command),
+                                                 None, kvm_log.debug, "(pre_command) ", timeout=600)
+        if status != 0:
+            kvm_utils.safe_kill(pid, signal.SIGTERM)
+            raise error.TestError, "Custom processing pre_command failed"
+
     # Preprocess all VMs and images
     process(test, params, env, preprocess_image, preprocess_vm)
 
@@ -232,6 +248,8 @@ def postprocess(test, params, env):
     test -- an Autotest test object
     params -- a dict containing all VM and image parameters
     env -- the environment (a dict-like object)
+    
+    Also, runs any command defined in the parameter post_command
     """
     process(test, params, env, postprocess_image, postprocess_vm)
 
@@ -241,6 +259,15 @@ def postprocess(test, params, env):
         kvm_log.debug("'keep_ppm_files' not specified; removing all PPM files from results dir...")
         kvm_utils.run_bg("rm -vf %s" % os.path.join(test.debugdir, "*.ppm"), None, kvm_log.debug, "(rm) ", timeout=5.0)
 
+    #execute any post_commands
+    post_command = params.get("post_command")
+    if post_command:
+        kvm_log.info("Executing command '%s'..." % post_command)
+        (status, pid, output) = kvm_utils.run_bg("cd %s; %s" % (test.bindir, post_command),
+                                                 None, kvm_log.debug, "(post_command) ", timeout=600)
+        if status != 0:
+            kvm_utils.safe_kill(pid, signal.SIGTERM)
+            raise error.TestError, "Custom processing command failed"
 
 def postprocess_on_error(test, params, env):
     """Perform postprocessing operations required only if the test failed.
-- 
1.6.0.6