On Mon, Nov 7, 2022 at 1:41 PM Hanna Reitz <hre...@redhat.com> wrote:
> On 30.10.22 18:38, Nir Soffer wrote: > > On Wed, Oct 26, 2022 at 4:31 PM Hanna Reitz <hre...@redhat.com> wrote: > > > > On 01.09.22 16:32, Nir Soffer wrote: > > > Add simple tests creating an image with all kinds of extents, > > different > > > formats, different backing chain, different protocol, and different > > > image options. Since all images have the same guest visible > > content they > > > must have the same checksum. > > > > > > To help debugging in case of failures, the output includes a > > json map of > > > every test image. > > > > > > Signed-off-by: Nir Soffer <nsof...@redhat.com> > > > --- > > > tests/qemu-iotests/tests/qemu-img-checksum | 149 > > ++++++++++++++++++ > > > .../qemu-iotests/tests/qemu-img-checksum.out | 74 +++++++++ > > > 2 files changed, 223 insertions(+) > > > create mode 100755 tests/qemu-iotests/tests/qemu-img-checksum > > > create mode 100644 tests/qemu-iotests/tests/qemu-img-checksum.out > > > > > > diff --git a/tests/qemu-iotests/tests/qemu-img-checksum > > b/tests/qemu-iotests/tests/qemu-img-checksum > > > new file mode 100755 > > > index 0000000000..3a85ba33f2 > > > --- /dev/null > > > +++ b/tests/qemu-iotests/tests/qemu-img-checksum > > > @@ -0,0 +1,149 @@ > > > +#!/usr/bin/env python3 > > > +# group: rw auto quick > > > +# > > > +# Test cases for qemu-img checksum. > > > +# > > > +# Copyright (C) 2022 Red Hat, Inc. > > > +# > > > +# This program is free software; you can redistribute it and/or > > modify > > > +# it under the terms of the GNU General Public License as > > published by > > > +# the Free Software Foundation; either version 2 of the License, > or > > > +# (at your option) any later version. > > > +# > > > +# This program is distributed in the hope that it will be useful, > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > +# GNU General Public License for more details. > > > +# > > > +# You should have received a copy of the GNU General Public > License > > > +# along with this program. If not, see > > <http://www.gnu.org/licenses/>. > > > + > > > +import re > > > + > > > +import iotests > > > + > > > +from iotests import ( > > > + filter_testfiles, > > > + qemu_img, > > > + qemu_img_log, > > > + qemu_io, > > > + qemu_nbd_popen, > > > +) > > > + > > > + > > > +def checksum_available(): > > > + out = qemu_img("--help").stdout > > > + return re.search(r"\bchecksum .+ filename\b", out) is not None > > > + > > > + > > > +if not checksum_available(): > > > + iotests.notrun("checksum command not available") > > > + > > > +iotests.script_initialize( > > > + supported_fmts=["raw", "qcow2"], > > > + supported_cache_modes=["none", "writeback"], > > > > It doesn’t work with writeback, though, because it uses -T none > below. > > > > > > Good point > > > > > > Which by the way is a heavy cost, because I usually run tests in > > tmpfs, > > where this won’t work. Is there any way of not doing the -T none > > below? > > > > > > Testing using tempfs is problematic since you cannot test -T none.In > > oVirt > > we alway use /var/tmp which usually uses something that supports > > direct I/O. > > > > Do we have a way to specify cache mode in the tests, so we can use -T > none > > only when the option is set? > > `./check` has a `-c` option (e.g. `./check -c none`), which lands in > `iotests.cachemode`. That isn’t automatically passed to qemu-img calls, > but you can do it manually (i.e. `qemu_img_log("checksum", "-T", > iotests.cachemode, disk_top)` instead of `"-T", "none"`). > Ok, I will change to use the current cache setting. > > > > > + supported_protocols=["file", "nbd"], > > > + required_fmts=["raw", "qcow2"], > > > +) > > > + > > > +print() > > > +print("=== Test images ===") > > > +print() > > > + > > > +disk_raw = iotests.file_path('raw') > > > +qemu_img("create", "-f", "raw", disk_raw, "10m") > > > +qemu_io("-f", "raw", > > > + "-c", "write -P 0x1 0 2m", # data > > > + "-c", "write -P 0x0 2m 2m", # data with zeroes > > > + "-c", "write -z 4m 2m", # zero allocated > > > + "-c", "write -z -u 6m 2m", # zero hole > > > + # unallocated > > > + disk_raw) > > > +print(filter_testfiles(disk_raw)) > > > +qemu_img_log("map", "--output", "json", disk_raw) > > > + > > > +disk_qcow2 = iotests.file_path('qcow2') > > > +qemu_img("create", "-f", "qcow2", disk_qcow2, "10m") > > > +qemu_io("-f", "qcow2", > > > + "-c", "write -P 0x1 0 2m", # data > > > + "-c", "write -P 0x0 2m 2m", # data with zeroes > > > + "-c", "write -z 4m 2m", # zero allocated > > > + "-c", "write -z -u 6m 2m", # zero hole > > > + # unallocated > > > + disk_qcow2) > > > +print(filter_testfiles(disk_qcow2)) > > > +qemu_img_log("map", "--output", "json", disk_qcow2) > > > > This isn’t how iotests work, generally. When run with -qcow2 > > -file, it > > should only test qcow2 on file, not raw on file, not raw on nbd. > > Perhaps > > this way this test could even support other formats than qcow2 and > > raw. > > > > > > For this type of tests, running the same code with raw, qcow2 (and > > other formats) > > and using file or nbd is the best way to test this feature - one test > > verifies all the > > use cases. > > Yes, I see, but that’s a general thing in the iotests. The design is > such that tests don’t cycle through their test matrix themselves, but > that they always only test a single combination of format+protocol on > each run, and the user is responsible for cycling through the desired > test matrix. > > I’m not saying that was definitely the best design decision, but the > problem now that if a test cycles through its test matrix by itself, it > must also ensure that it is run only once when the user cycles through > the same test matrix. For example, a reasonable run of the iotests > consists of `./check -raw`, `./check -qcow2`, and `./check -nbd`. This > test here would then run in all three configurations, but do the same > thing every time (specifically, test all of those configurations every > time). > > So there’s a conflict. Either the test follows the existing design and > only tests a single configuration, as iotests are expected to do, or we > have the question of how to deal with the fact that users will run the > test suite in multiple configurations, but one run of this test would > already cover them all. > > I’m not sternly against cycling through the possible combinations right > here in the test, but I want to lay out the problems with that approach. > > > I can change this to use the current format (raw, qcow2, ...), > > protocol (file, nbd, ...) > > and cache value (none, writeback), but I'm not sure how this can work > > with the > > out files. The output from nbd is different from file. Maybe we need > > different out > > files for file and nbd (qemu-img-checksum.file.out, > > qemu-img-checksum.nbd.out)? > > We already have that for the format (e.g. 178.out.{qcow2,raw}). If you > decide to do that, it shouldn’t be too difficult to implement > (testrunner.py’s `TestRunner.find_reference()`). Alternatively, it’s > also possible to basically ignore the reference output and verify the > expected output right in the test code. > In this kind of test checking the output is the right thing since this is the actual result of the command. I'll change to use the current format and cache mode - this should work fine with file protocol, and will simplify the test a lot. I'm not sure if this will work for NBD but it is less important and can be added later. There is nothing related to NBD in the code. Nir