[KVM-AUTOTEST PATCH 1/2] KVM test: optionally convert PPM files to PNG format after test

[KVM-AUTOTEST PATCH 1/2] KVM test: optionally convert PPM files to PNG format after test

[Michael Goldish]

This is intended to save disk space.  Requires ImageMagick (uses mogrify).

To enable:
convert_ppm_files_to_png = yes

To enable only for failed tests:
convert_ppm_files_to_png_on_error = yes

Reminder: by default PPM files are removed after the test (and after the
conversion, if requested).  To keep them specify:
keep_ppm_files = yes

To keep them only for failed tests:
keep_ppm_files_on_error = yes

A reasonable choice would be to keep PNG files for failed tests, and never
keep PPM files.  To do this specify
convert_ppm_files_to_png_on_error = yes
without specifying 'keep_ppm_files = yes' or 'keep_ppm_files_on_error = yes'
(or explicitly set to 'no', if it was set to 'yes' on a higher level in the
config hierarchy).

This patch also makes small cosmetic changes to the 'keep_ppm_files' feature.

Signed-off-by: Michael Goldish <mgoldish@redhat.com>
---
 client/tests/kvm/kvm_preprocessing.py |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/client/tests/kvm/kvm_preprocessing.py b/client/tests/kvm/kvm_preprocessing.py
index 7fac46a..26e5210 100644
--- a/client/tests/kvm/kvm_preprocessing.py
+++ b/client/tests/kvm/kvm_preprocessing.py
@@ -223,11 +223,19 @@ def postprocess(test, params, env):
     """
     process(test, params, env, postprocess_image, postprocess_vm)
 
-    # See if we should get rid of all PPM files
-    if not params.get("keep_ppm_files") == "yes":
-        # Remove them all
+    # Should we convert PPM files to PNG format?
+    if params.get("convert_ppm_files_to_png") == "yes":
+        logging.debug("'convert_ppm_files_to_png' specified; converting PPM"
+                      " files to PNG format...")
+        mogrify_cmd = ("mogrify -format png %s" %
+                       os.path.join(test.debugdir, "*.ppm"))
+        kvm_subprocess.run_fg(mogrify_cmd, logging.debug, "(mogrify) ",
+                              timeout=30.0)
+
+    # Should we keep the PPM files?
+    if params.get("keep_ppm_files") != "yes":
         logging.debug("'keep_ppm_files' not specified; removing all PPM files"
-                      " from results dir...")
+                      " from debug dir...")
         rm_cmd = "rm -vf %s" % os.path.join(test.debugdir, "*.ppm")
         kvm_subprocess.run_fg(rm_cmd, logging.debug, "(rm) ", timeout=5.0)
 
-- 
1.5.4.1

[KVM-AUTOTEST PATCH 2/2] KVM test: kvm_tests.cfg.sample: convert PPM files to PNG by default

[Michael Goldish]

By default, always remove PPM files, and keep PNG files only for failed tests.
This shouldn't do much harm, because while PPMs can be incorporated directly
into step files, PNGs can be converted back to PPMs easily, and take less disk
space.
(PNG is a lossless compression format.)

The 'keep_ppm_files' and 'keep_ppm_files_on_error' settings are commented out
so the user can easily enable them if desired.

Signed-off-by: Michael Goldish <mgoldish@redhat.com>
---
 client/tests/kvm/kvm_tests.cfg.sample |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/client/tests/kvm/kvm_tests.cfg.sample b/client/tests/kvm/kvm_tests.cfg.sample
index 412c4b0..2564e16 100644
--- a/client/tests/kvm/kvm_tests.cfg.sample
+++ b/client/tests/kvm/kvm_tests.cfg.sample
@@ -8,8 +8,9 @@ main_vm = vm1
 
 # Some preprocessor/postprocessor params
 start_vm = yes
-keep_ppm_files = no
-keep_ppm_files_on_error = yes
+convert_ppm_files_to_png_on_error = yes
+#keep_ppm_files = yes
+#keep_ppm_files_on_error = yes
 kill_vm = no
 kill_vm_gracefully = yes
 
-- 
1.5.4.1

Re: [KVM-AUTOTEST PATCH 1/2] KVM test: optionally convert PPM files to PNG format after test

[Lucas Meneghel Rodrigues]

On Tue, 2009-06-16 at 14:00 +0300, Michael Goldish wrote:
> This is intended to save disk space.  Requires ImageMagick (uses mogrify).
> 
> To enable:
> convert_ppm_files_to_png = yes
> 
> To enable only for failed tests:
> convert_ppm_files_to_png_on_error = yes
> 
> Reminder: by default PPM files are removed after the test (and after the
> conversion, if requested).  To keep them specify:
> keep_ppm_files = yes
> 
> To keep them only for failed tests:
> keep_ppm_files_on_error = yes
> 
> A reasonable choice would be to keep PNG files for failed tests, and never
> keep PPM files.  To do this specify
> convert_ppm_files_to_png_on_error = yes
> without specifying 'keep_ppm_files = yes' or 'keep_ppm_files_on_error = yes'
> (or explicitly set to 'no', if it was set to 'yes' on a higher level in the
> config hierarchy).
> 
> This patch also makes small cosmetic changes to the 'keep_ppm_files' feature.
> 
> Signed-off-by: Michael Goldish <mgoldish@redhat.com>
> ---
>  client/tests/kvm/kvm_preprocessing.py |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_preprocessing.py b/client/tests/kvm/kvm_preprocessing.py
> index 7fac46a..26e5210 100644
> --- a/client/tests/kvm/kvm_preprocessing.py
> +++ b/client/tests/kvm/kvm_preprocessing.py
> @@ -223,11 +223,19 @@ def postprocess(test, params, env):
>      """
>      process(test, params, env, postprocess_image, postprocess_vm)
>  
> -    # See if we should get rid of all PPM files
> -    if not params.get("keep_ppm_files") == "yes":
> -        # Remove them all
> +    # Should we convert PPM files to PNG format?
> +    if params.get("convert_ppm_files_to_png") == "yes":
> +        logging.debug("'convert_ppm_files_to_png' specified; converting PPM"
> +                      " files to PNG format...")
> +        mogrify_cmd = ("mogrify -format png %s" %
> +                       os.path.join(test.debugdir, "*.ppm"))
> +        kvm_subprocess.run_fg(mogrify_cmd, logging.debug, "(mogrify) ",
> +                              timeout=30.0)

As the above call can fail if ImageMagick is not installed, we might
perform a check before (the autotest lib os_dep does rudimentary command
and library presence checks), and proceed with conversion only if the
command does exist. It's safer this way.

> +    # Should we keep the PPM files?
> +    if params.get("keep_ppm_files") != "yes":
>          logging.debug("'keep_ppm_files' not specified; removing all PPM files"
> -                      " from results dir...")
> +                      " from debug dir...")
>          rm_cmd = "rm -vf %s" % os.path.join(test.debugdir, "*.ppm")
>          kvm_subprocess.run_fg(rm_cmd, logging.debug, "(rm) ", timeout=5.0)
>  
-- 
Lucas Meneghel Rodrigues
Software Engineer (QE)
Red Hat - Emerging Technologies

Re: [KVM-AUTOTEST PATCH 1/2] KVM test: optionally convert PPM files to PNG format after test

[Lucas Meneghel Rodrigues]

On Tue, 2009-06-16 at 14:00 +0300, Michael Goldish wrote:
> This is intended to save disk space.  Requires ImageMagick (uses mogrify).
> 
> To enable:
> convert_ppm_files_to_png = yes
> 
> To enable only for failed tests:
> convert_ppm_files_to_png_on_error = yes
> 
> Reminder: by default PPM files are removed after the test (and after the
> conversion, if requested).  To keep them specify:
> keep_ppm_files = yes
> 
> To keep them only for failed tests:
> keep_ppm_files_on_error = yes
> 
> A reasonable choice would be to keep PNG files for failed tests, and never
> keep PPM files.  To do this specify
> convert_ppm_files_to_png_on_error = yes
> without specifying 'keep_ppm_files = yes' or 'keep_ppm_files_on_error = yes'
> (or explicitly set to 'no', if it was set to 'yes' on a higher level in the
> config hierarchy).
> 
> This patch also makes small cosmetic changes to the 'keep_ppm_files' feature.

Forgot to say that other than performing a check to see if we actually
do have ImageMagick before trying to convert the images, the patch
series looks good, once you get this worked out I am going to apply it.

-- 
Lucas Meneghel Rodrigues
Software Engineer (QE)
Red Hat - Emerging Technologies

Re: [KVM-AUTOTEST PATCH 1/2] KVM test: optionally convert PPM files to PNG format after test

[Michael Goldish]

----- "Lucas Meneghel Rodrigues" <lmr@redhat.com> wrote:

> On Tue, 2009-06-16 at 14:00 +0300, Michael Goldish wrote:
> > This is intended to save disk space.  Requires ImageMagick (uses
> mogrify).
> > 
> > To enable:
> > convert_ppm_files_to_png = yes
> > 
> > To enable only for failed tests:
> > convert_ppm_files_to_png_on_error = yes
> > 
> > Reminder: by default PPM files are removed after the test (and after
> the
> > conversion, if requested).  To keep them specify:
> > keep_ppm_files = yes
> > 
> > To keep them only for failed tests:
> > keep_ppm_files_on_error = yes
> > 
> > A reasonable choice would be to keep PNG files for failed tests, and
> never
> > keep PPM files.  To do this specify
> > convert_ppm_files_to_png_on_error = yes
> > without specifying 'keep_ppm_files = yes' or
> 'keep_ppm_files_on_error = yes'
> > (or explicitly set to 'no', if it was set to 'yes' on a higher level
> in the
> > config hierarchy).
> > 
> > This patch also makes small cosmetic changes to the 'keep_ppm_files'
> feature.
> > 
> > Signed-off-by: Michael Goldish <mgoldish@redhat.com>
> > ---
> >  client/tests/kvm/kvm_preprocessing.py |   16 ++++++++++++----
> >  1 files changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/client/tests/kvm/kvm_preprocessing.py
> b/client/tests/kvm/kvm_preprocessing.py
> > index 7fac46a..26e5210 100644
> > --- a/client/tests/kvm/kvm_preprocessing.py
> > +++ b/client/tests/kvm/kvm_preprocessing.py
> > @@ -223,11 +223,19 @@ def postprocess(test, params, env):
> >      """
> >      process(test, params, env, postprocess_image, postprocess_vm)
> >  
> > -    # See if we should get rid of all PPM files
> > -    if not params.get("keep_ppm_files") == "yes":
> > -        # Remove them all
> > +    # Should we convert PPM files to PNG format?
> > +    if params.get("convert_ppm_files_to_png") == "yes":
> > +        logging.debug("'convert_ppm_files_to_png' specified;
> converting PPM"
> > +                      " files to PNG format...")
> > +        mogrify_cmd = ("mogrify -format png %s" %
> > +                       os.path.join(test.debugdir, "*.ppm"))
> > +        kvm_subprocess.run_fg(mogrify_cmd, logging.debug,
> "(mogrify) ",
> > +                              timeout=30.0)
> 
> As the above call can fail if ImageMagick is not installed, we might
> perform a check before (the autotest lib os_dep does rudimentary
> command
> and library presence checks), and proceed with conversion only if the
> command does exist. It's safer this way.

The call should never fail because we currently don't raise exceptions.
It's actually easier to skip the check, because in case ImageMagick
isn't installed, you'll just get a nice message like:

'convert_ppm_files_to_png' specified; converting PPM files to PNG format...
(mogrify) bash: mogrify: command not found
(mogrify) (Process terminated with status 127)

Personally I am fond of these messages because they indicate to me that
kvm_subprocess is functioning properly, and I also think they're quite
readable because they contain "bash's own words", but since we might
raise exceptions in run_bg()/run_fg() in the future, I see your point.

I suppose I should do something like:

logging.debug("converting ...")
found = True
try:
  os_dep.command("mogrify")
  found = True
except:
  logging.error("'mogrify' not found; ImageMagick is probably not installed")
  found = False
if found:
  mogrify_cmd = ...
  kvm_subprocess.run_fg(...)

Do you see a shorter/more elegant syntax to do this?
(We can put the conversion code inside the 'try' clause to avoid using
that 'found' variable, but it's generally recommended to make the code
inside a 'try' clause as specific as possible.)
In any case, I don't want to fail the test just because mogrify is missing,
so we do need to catch the exception raised by os_dep.command().

> > +    # Should we keep the PPM files?
> > +    if params.get("keep_ppm_files") != "yes":
> >          logging.debug("'keep_ppm_files' not specified; removing all
> PPM files"
> > -                      " from results dir...")
> > +                      " from debug dir...")
> >          rm_cmd = "rm -vf %s" % os.path.join(test.debugdir,
> "*.ppm")
> >          kvm_subprocess.run_fg(rm_cmd, logging.debug, "(rm) ",
> timeout=5.0)
> >  
> -- 
> Lucas Meneghel Rodrigues
> Software Engineer (QE)
> Red Hat - Emerging Technologies

Re: [KVM-AUTOTEST PATCH 1/2] KVM test: optionally convert PPM files to PNG format after test

[Lucas Meneghel Rodrigues]

On Thu, 2009-06-18 at 06:59 -0400, Michael Goldish wrote:

> The call should never fail because we currently don't raise exceptions.
> It's actually easier to skip the check, because in case ImageMagick
> isn't installed, you'll just get a nice message like:
> 
> 'convert_ppm_files_to_png' specified; converting PPM files to PNG format...
> (mogrify) bash: mogrify: command not found
> (mogrify) (Process terminated with status 127)
> 
> Personally I am fond of these messages because they indicate to me that
> kvm_subprocess is functioning properly, and I also think they're quite
> readable because they contain "bash's own words", but since we might
> raise exceptions in run_bg()/run_fg() in the future, I see your point.

Yes, that was the point. I am not particularly against leaving it as it
is, since we still haven't decided about the behavior of these utility
functions.

> I suppose I should do something like:
> 
> logging.debug("converting ...")
> found = True
> try:
>   os_dep.command("mogrify")
>   found = True
> except:
>   logging.error("'mogrify' not found; ImageMagick is probably not installed")
>   found = False
> if found:
>   mogrify_cmd = ...
>   kvm_subprocess.run_fg(...)

Yes, something along these lines

> Do you see a shorter/more elegant syntax to do this?

Nope, since os_dep() already trhows exceptions by default

> (We can put the conversion code inside the 'try' clause to avoid using
> that 'found' variable, but it's generally recommended to make the code
> inside a 'try' clause as specific as possible.)

Yes, because if something else unexpected happens inside the try clause
we will capture the exception and tell the user generically that the
problem is ImageMagick not installed. So I am for keeping the conversion
out of the try clause.

> In any case, I don't want to fail the test just because mogrify is missing,
> so we do need to catch the exception raised by os_dep.command().

Yes, sounds good to me. I am leaning towards implementing this check
instead of leaving the patch on its original form, so let me know what
is your decision.

-- 
Lucas Meneghel Rodrigues
Software Engineer (QE)
Red Hat - Emerging Technologies