Copying Michael on the message.
Hi Yolkfull, I have reviewed this patch and I have some comments to
make on it, similar to the ones I made on an earlier version of it:
One of the things that I noticed is that this patch doesn't work very
well out of the box:
[lmr@freedom kvm]$ ./scan_results.py
Test Status Seconds Info
---- ------ ------- ----
(Result file: ../../results/default/status)
smp2.Fedora.11.64.qemu_img.check GOOD 47 completed successfully
smp2.Fedora.11.64.qemu_img.create GOOD 44 completed successfully
smp2.Fedora.11.64.qemu_img.convert.to_qcow2 FAIL 45 Image
converted failed; Command: /usr/bin/qemu-img convert -f qcow2 -O qcow2
/tmp/kvm_autotest_root/images/fc11-64.qcow2
/tmp/kvm_autotest_root/images/fc11-64.qcow2.converted_qcow2;Output is:
qemu-img: Could not open '/tmp/kvm_autotest_root/images/fc11-64.qcow2'
smp2.Fedora.11.64.qemu_img.convert.to_raw FAIL 46 Image
converted failed; Command: /usr/bin/qemu-img convert -f qcow2 -O raw
/tmp/kvm_autotest_root/images/fc11-64.qcow2
/tmp/kvm_autotest_root/images/fc11-64.qcow2.converted_raw;Output is:
qemu-img: Could not open '/tmp/kvm_autotest_root/images/fc11-64.qcow2'
smp2.Fedora.11.64.qemu_img.snapshot FAIL 44 Create
snapshot failed via command: /usr/bin/qemu-img snapshot -c snapshot0
/tmp/kvm_autotest_root/images/fc11-64.qcow2;Output is: qemu-img: Could
not open '/tmp/kvm_autotest_root/images/fc11-64.qcow2'
smp2.Fedora.11.64.qemu_img.commit GOOD 44 completed successfully
smp2.Fedora.11.64.qemu_img.info FAIL 44 Unhandled
str: Unhandled TypeError: argument of type 'NoneType' is not iterable
smp2.Fedora.11.64.qemu_img.rebase TEST_NA 43 Current
kvm user space version does not support 'rebase' subcommand
---- GOOD 412
We need to fix that before upstream inclusion.
Also, one thing that I've noticed is that this test doesn't depend of
any other variants, so we don't need to repeat it to every combination
of guest and qemu command line options. Michael, does it occur to you
a way to get this test out of the variants block, so it gets executed
only once per job and not every combination of guest and other qemu
options?
On Fri, Jan 29, 2010 at 4:00 AM, Yolkfull Chow <yzhou@xxxxxxxxxx> wrote:
> This is designed to test all subcommands of 'qemu-img' however
> so far 'commit' is not implemented.
>
> * For 'check' subcommand test, it will 'dd' to create a file with specified
> size and see whether it's supported to be checked. Then convert it to be
> supported formats ('qcow2' and 'raw' so far) to see whether there's error
> after convertion.
>
> * For 'convert' subcommand test, it will convert both to 'qcow2' and 'raw' from
> the format specified in config file. And only check 'qcow2' after convertion.
>
> * For 'snapshot' subcommand test, it will create two snapshots and list them.
> Finally delete them if no errors found.
>
> * For 'info' subcommand test, it will check image format & size according to
> output of 'info' subcommand at specified image file.
>
> * For 'rebase' subcommand test, it will create first snapshot 'sn1' based on original
> base_img, and create second snapshot based on sn1. And then rebase sn2 to base_img.
> After rebase check the baking_file of sn2.
>
> This supports two rebase mode: unsafe mode and safe mode:
> Unsafe mode:
> With -u an unsafe mode is enabled that doesn't require the backing files to exist.
> It merely changes the backing file reference in the COW image. This is useful for
> renaming or moving the backing file. The user is responsible to make sure that the
> new backing file has no changes compared to the old one, or corruption may occur.
>
> Safe Mode:
> Both the current and the new backing file need to exist, and after the rebase, the
> COW image is guaranteed to have the same guest visible content as before.
> To achieve this, old and new backing file are compared and, if necessary, data is
> copied from the old backing file into the COW image.
>
> Signed-off-by: Yolkfull Chow <yzhou@xxxxxxxxxx>
> ---
> client/tests/kvm/tests/qemu_img.py | 235 ++++++++++++++++++++++++++++++++
> client/tests/kvm/tests_base.cfg.sample | 40 ++++++
> 2 files changed, 275 insertions(+), 0 deletions(-)
> create mode 100644 client/tests/kvm/tests/qemu_img.py
>
> diff --git a/client/tests/kvm/tests/qemu_img.py b/client/tests/kvm/tests/qemu_img.py
> new file mode 100644
> index 0000000..e6352a0
> --- /dev/null
> +++ b/client/tests/kvm/tests/qemu_img.py
> @@ -0,0 +1,235 @@
> +import re, os, logging, commands
> +from autotest_lib.client.common_lib import utils, error
> +import kvm_vm, kvm_utils
> +
> +
> +def run_qemu_img(test, params, env):
> + """
> + `qemu-img' functions test:
> + 1) Judge what subcommand is going to be tested
> + 2) Run subcommand test
> +
> + @param test: kvm test object
> + @param params: Dictionary with the test parameters
> + @param env: Dictionary with test environment.
> + """
> + cmd = kvm_utils.get_path(test.bindir, params.get("qemu_img_binary"))
> + if not os.path.exists(cmd):
> + raise error.TestError("Binary of 'qemu-img' not found")
> + image_format = params.get("image_format")
> + image_size = params.get("image_size", "10G")
> + image_name = kvm_vm.get_image_filename(params, test.bindir)
> +
> + def check(cmd, img):
> + cmd += " check %s" % img
> + logging.info("Checking image '%s'..." % img)
> + o = commands.getoutput(cmd)
> + if "does not support checks" in o or "No errors" in o:
> + return (True, "")
> + return (False, o)
Here you're using the commands module. We already have an API on
autotest, utils,system() that will return the exit code, with command
logging and will raise an exception if the test fails. This would
allow for shorter code and brevity. I see you have used system_output
down on the patch, so why not make it all consistent? In one or two
cases you have to do exception handling, but in the majority of the
executions, a simple utils.system(command) would work.
Also, it would be a good idea to make all the auxiliary functions (ie,
the ones being used by the actual test functions) as private functions
(with an underscore _ at the beginning of its name. Another thing that
occurred to me is that all functions but the top level one lack
docstrings.
> +
> + # Subcommand 'qemu-img check' test
> + # This tests will 'dd' to create a specified size file, and check it.
> + # Then convert it to supported image_format in each loop and check again.
> + def check_test(cmd):
> + test_image = params.get("image_name_dd")
> + create_image_cmd = params.get("create_image_cmd")
> + create_image_cmd = create_image_cmd % test_image
> + s, o = commands.getstatusoutput(create_image_cmd)
> + if s != 0:
> + raise error.TestError("Failed command: %s; Output is: %s" %
> + (create_image_cmd, o))
> + s, o = check(cmd, test_image)
In the above code you are trying to qemu-img check a raw image, which
doesn't support checks. This will fail every single time unless I am
very mistaken.
> + if not s:
> + raise error.TestFail("Check image '%s' failed with error: %s" %
> + (test_image, o))
> + for fmt in params.get("supported_image_formats").split():
> + output_image = test_image + ".%s" % fmt
> + convert(cmd, fmt, test_image, output_image)
> + s, o = check(cmd, output_image)
> + if not s:
> + raise error.TestFail("Check image '%s' got error: %s" %
> + (output_image, o))
> + os.remove(output_image)
> + os.remove(test_image)
> +
> + def create(cmd, img_name, fmt, img_size=None, base_img=None,
> + base_img_fmt=None, encrypted="no"):
> + cmd += " create"
> + if encrypted == "yes":
> + cmd += " -e"
> + if base_img:
> + cmd += " -b %s" % base_img
> + if base_img_fmt:
> + cmd += " -F %s" % base_img_fmt
> + cmd += " -f %s" % fmt
> + cmd += " %s" % img_name
> + if img_size:
> + cmd += " %s" % img_size
> + s, o = commands.getstatusoutput(cmd)
> + if s != 0:
> + raise error.TestFail("Create image '%s' failed: %s;Command:\n%s" %
> + (img_name, o, cmd))
> + logging.info("Created image '%s'" % img_name)
> +
> + # Subcommand 'qemu-img create' test
> + def create_test(cmd):
> + image_large = params.get("image_name_large")
> + img = kvm_utils.get_path(test.bindir, image_large)
> + img += '.' + image_format
> + create(cmd, img_name=img, fmt=image_format,
> + img_size=params.get("image_size_large"))
> + os.remove(img)
> +
> + def convert(cmd, output_fmt, img_name, output_filename,
> + fmt=None, compressed="no", encrypted="no"):
> + cmd += " convert"
> + if compressed == "yes":
> + cmd += " -c"
> + if encrypted == "yes":
> + cmd += " -e"
> + if fmt:
> + cmd += " -f %s" % fmt
> + cmd += " -O %s" % output_fmt
> + cmd += " %s %s" % (img_name, output_filename)
> + logging.info("Converting '%s' from format '%s' to '%s'" %
> + (img_name, fmt, output_fmt))
> + s, o = commands.getstatusoutput(cmd)
> + if s != 0:
> + raise error.TestFail("Image converted failed; Command: %s;"
> + "Output is: %s" % (cmd, o))
> +
> + # Subcommand 'qemu-img convert' test
> + def convert_test(cmd):
> + dest_img_fmt = params.get("dest_image_format")
> + output_filename = "%s.converted_%s" % (image_name, dest_img_fmt)
> +
> + convert(cmd, dest_img_fmt, image_name, output_filename,
> + image_format, params.get("compressed"), params.get("encrypted"))
> +
> + if dest_img_fmt == "qcow2":
> + s, o = check(cmd, output_filename)
> + if s:
> + os.remove(output_filename)
> + else:
> + raise error.TestFail("Check image '%s' failed with error: %s" %
> + (output_filename, o))
> + else:
> + os.remove(output_filename)
> +
> + def info(cmd, img, string=None, fmt=None):
> + cmd += " info"
> + if fmt:
> + cmd += " -f %s" % fmt
> + cmd += " %s" % img
> + s, o = commands.getstatusoutput(cmd)
> + if s != 0:
> + logging.error("Get info of image '%s' failed: %s" % (img, o))
> + return None
> + if not string:
> + return o
> + string += ": (.*)"
> + str = re.findall(string, o)
> + if str:
> + return str[0]
> + return None
> +
> + # Subcommand 'qemu-img info' test
> + def info_test(cmd):
> + img_info = info(cmd, image_name)
> + logging.info("Info of image '%s': \n%s" % (image_name, img_info))
> + if not image_format in img_info:
> + raise error.TestFail("Got unexpected format of image '%s'"
> + " in info test" % image_name)
> + if not image_size in img_info:
> + raise error.TestFail("Got unexpected size of image '%s'"
> + " in info test" % image_name)
> +
> + # Subcommand 'qemu-img snapshot' test
> + def snapshot_test(cmd):
> + cmd += " snapshot"
> + for i in range(2):
> + crtcmd = cmd
> + sn_name = "snapshot%d" % i
> + crtcmd += " -c %s %s" % (sn_name, image_name)
> + s, o = commands.getstatusoutput(crtcmd)
> + if s != 0:
> + raise error.TestFail("Create snapshot failed via command: %s;"
> + "Output is: %s" % (crtcmd, o))
> + logging.info("Created snapshot '%s' in '%s'" % (sn_name,image_name))
> + listcmd = cmd
> + listcmd += " -l %s" % image_name
> + s, o = commands.getstatusoutput(listcmd)
> + if not ("snapshot0" in o and "snapshot1" in o and s == 0):
> + raise error.TestFail("Snapshot created failed or missed;"
> + "snapshot list is: \n%s" % o)
> + for i in range(2):
> + sn_name = "snapshot%d" % i
> + delcmd = cmd
> + delcmd += " -d %s %s" % (sn_name, image_name)
> + s, o = commands.getstatusoutput(delcmd)
> + if s != 0:
> + raise error.TestFail("Delete snapshot '%s' failed: %s" %
> + (sn_name, o))
> +
> + #Subcommand 'qemu-img commit' test
> + def commit_test(cmd):
> + pass
> +
> + def rebase(cmd, img_name, base_img, backing_fmt, mode="unsafe"):
> + cmd += " rebase"
> + if mode == "unsafe":
> + cmd += " -u"
> + cmd += " -b %s -F %s %s" % (base_img, backing_fmt, img_name)
> + logging.info("Trying to rebase '%s' to '%s'..." % (img_name, base_img))
> + s, o = commands.getstatusoutput(cmd)
> + if s != 0:
> + raise error.TestError("Failed to rebase '%s' to '%s': %s" %
> + (img_name, base_img, o))
> +
> + # Subcommand 'qemu-img rebase' test
> + # Change the backing file of a snapshot image in "unsafe mode":
> + # Assume the previous backing file had missed and we just have to change
> + # reference of snapshot to new one. After change the backing file of a
> + # snapshot image in unsafe mode, the snapshot should work still.
> + def rebase_test(cmd):
> + if not 'rebase' in utils.system_output(cmd+' --help',ignore_status=True):
> + raise error.TestNAError("Current kvm user space version does not"
> + " support 'rebase' subcommand")
> + sn_fmt = params.get("snapshot_format", "qcow2")
> + sn1 = params.get("image_name_snapshot1")
> + sn1 = kvm_utils.get_path(test.bindir, sn1) + ".%s" % sn_fmt
> + base_img = kvm_vm.get_image_filename(params, test.bindir)
> + create(cmd, sn1, sn_fmt, base_img=base_img, base_img_fmt=image_format)
> +
> + # Create snapshot2 based on snapshot1
> + sn2 = params.get("image_name_snapshot2")
> + sn2 = kvm_utils.get_path(test.bindir, sn2) + ".%s" % sn_fmt
> + create(cmd, sn2, sn_fmt, base_img=sn1, base_img_fmt=sn_fmt)
> +
> + rebase_mode = params.get("rebase_mode")
> + if rebase_mode == "unsafe":
> + os.remove(sn1)
> +
> + rebase(cmd, sn2, base_img, image_format, mode=rebase_mode)
> +
> + # Check sn2's format and backing_file
> + actual_base_img = info(cmd, sn2, "backing file")
> + base_img_name = os.path.basename(params.get("image_name"))
> + if not base_img_name in actual_base_img:
> + raise error.TestFail("After rebase the backing_file of 'sn2' is "
> + "'%s' which is not expected as '%s'"
> + % (actual_base_img, base_img_name))
> + s, o = check(cmd, sn2)
> + if not s:
> + raise error.TestFail("Check image '%s' failed after rebase;"
> + "got error: %s" % (sn2, o))
> + try:
> + os.remove(sn2)
> + os.remove(sn1)
> + except:
> + pass
> +
> + # Here starts test
> + subcommand = params.get("subcommand")
> + eval("%s_test(cmd)" % subcommand)
> diff --git a/client/tests/kvm/tests_base.cfg.sample b/client/tests/kvm/tests_base.cfg.sample
> index 2d03f1f..e7670c7 100644
> --- a/client/tests/kvm/tests_base.cfg.sample
> +++ b/client/tests/kvm/tests_base.cfg.sample
> @@ -273,6 +273,46 @@ variants:
> type = physical_resources_check
> catch_uuid_cmd = dmidecode | awk -F: '/UUID/ {print $2}'
>
> + - qemu_img:
> + type = qemu_img
> + vms = ''
> + variants:
> + - check:
> + subcommand = check
> + image_name_dd = dd_created_image
> + force_create_image_dd = no
> + remove_image_dd = yes
> + create_image_cmd = "dd if=/dev/zero of=%s bs=1G count=1"
> + # Test the convertion from 'dd_image_name' to specified format
> + supported_image_formats = qcow2 raw
> + - create:
> + subcommand = create
> + images += " large"
> + force_create_image_large = yes
> + image_size_large = 1G
> + image_name_large = create_large_image
> + remove_image_large = yes
> + - convert:
> + subcommand = convert
> + variants:
> + - to_qcow2:
> + dest_image_format = qcow2
> + compressed = no
> + encrypted = no
> + - to_raw:
> + dest_image_format = raw
> + - snapshot:
> + subcommand = snapshot
> + - commit:
> + subcommand = commit
> + - info:
> + subcommand = info
> + - rebase:
> + subcommand = rebase
> + rebase_mode = unsafe
> + image_name_snapshot1 = sn1
> + image_name_snapshot2 = sn2
> +
> # NICs
> variants:
> - @rtl8139:
> --
> 1.5.5.6
>
> _______________________________________________
> Autotest mailing list
> Autotest@xxxxxxxxxxxxxxx
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
>
--
Lucas
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
