[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 08/11] iotests: add testenv.py
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v6 08/11] iotests: add testenv.py |
Date: |
Fri, 15 Jan 2021 13:45:55 +0100 |
Am 15.01.2021 um 13:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 15.01.2021 14:18, Kevin Wolf wrote:
> > Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > Add TestEnv class, which will handle test environment in a new python
> > > iotests running framework.
> > >
> > > Difference with current ./check interface:
> > > - -v (verbose) option dropped, as it is unused
> > >
> > > - -xdiff option is dropped, until somebody complains that it is needed
> > > - same for -n option
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > > tests/qemu-iotests/testenv.py | 328 ++++++++++++++++++++++++++++++++++
> > > 1 file changed, 328 insertions(+)
> > > create mode 100755 tests/qemu-iotests/testenv.py
> > >
> > > diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> > > new file mode 100755
> > > index 0000000000..ecaf76fb85
> > > --- /dev/null
> > > +++ b/tests/qemu-iotests/testenv.py
> > > @@ -0,0 +1,328 @@
> > > +#!/usr/bin/env python3
> > > +#
> > > +# Parse command line options to manage test environment variables.
> > > +#
> > > +# Copyright (c) 2020 Virtuozzo International GmbH
> > > +#
> > > +# 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 os
> > > +import sys
> > > +import tempfile
> > > +from pathlib import Path
> > > +import shutil
> > > +import collections
> > > +import subprocess
> > > +import argparse
> > > +from typing import List, Dict
> > > +
> > > +
> > > +def get_default_machine(qemu_prog: str) -> str:
> > > + outp = subprocess.run([qemu_prog, '-machine', 'help'], check=True,
> > > + text=True, stdout=subprocess.PIPE).stdout
> > > +
> > > + machines = outp.split('\n')
> > > + default_machine = next(m for m in machines if m.endswith('
> > > (default)'))
> > > + default_machine = default_machine.split(' ', 1)[0]
> > > +
> > > + alias_suf = ' (alias of {})'.format(default_machine)
> > > + alias = next((m for m in machines if m.endswith(alias_suf)), None)
> > > + if alias is not None:
> > > + default_machine = alias.split(' ', 1)[0]
> > > +
> > > + return default_machine
> > > +
> > > +
> > > +class TestEnv:
> > > + """
> > > + Manage system environment for running tests
> > > +
> > > + The following variables are supported/provided. They are represented
> > > by
> > > + lower-cased TestEnv attributes.
> > > + """
> > > + env_variables = ['PYTHONPATH', 'TEST_DIR', 'SOCK_DIR',
> > > 'SAMPLE_IMG_DIR',
> > > + 'OUTPUT_DIR', 'PYTHON', 'QEMU_PROG',
> > > 'QEMU_IMG_PROG',
> > > + 'QEMU_IO_PROG', 'QEMU_NBD_PROG', 'QSD_PROG',
> > > + 'SOCKET_SCM_HELPER', 'QEMU_OPTIONS',
> > > 'QEMU_IMG_OPTIONS',
> > > + 'QEMU_IO_OPTIONS', 'QEMU_NBD_OPTIONS', 'IMGOPTS',
> > > + 'IMGFMT', 'IMGPROTO', 'AIOMODE', 'CACHEMODE',
> > > + 'VALGRIND_QEMU', 'CACHEMODE_IS_DEFAULT',
> > > 'IMGFMT_GENERIC',
> > > + 'IMGOPTSSYNTAX', 'IMGKEYSECRET',
> > > 'QEMU_DEFAULT_MACHINE']
> > > +
> > > + def get_env(self) -> Dict[str, str]:
> > > + env = {}
> > > + for v in self.env_variables:
> > > + val = getattr(self, v.lower(), None)
> > > + if val is not None:
> > > + env[v] = val
> > > +
> > > + return env
> > > +
> > > + _argparser = None
> > > + @classmethod
> > > + def get_argparser(cls) -> argparse.ArgumentParser:
> > > + if cls._argparser is not None:
> > > + return cls._argparser
> > > +
> > > + p = argparse.ArgumentParser(description="= test environment
> > > options =",
> > > + add_help=False,
> > > usage=argparse.SUPPRESS)
> > > +
> > > + p.add_argument('-d', dest='debug', action='store_true',
> > > help='debug')
> > > + p.add_argument('-misalign', action='store_true',
> > > + help='misalign memory allocations')
> > > +
> > > + p.set_defaults(imgfmt='raw', imgproto='file')
> > > +
> > > + format_list = ['raw', 'bochs', 'cloop', 'parallels', 'qcow',
> > > 'qcow2',
> > > + 'qed', 'vdi', 'vpc', 'vhdx', 'vmdk', 'luks',
> > > 'dmg']
> > > + g = p.add_argument_group(
> > > + 'image format options',
> > > + 'The following options sets IMGFMT environment variable. '
> >
> > s/sets/set the/
> >
> > > + 'At most one chose is allowed, default is "raw"')
> >
> > s/chose/choice/
> >
> > > + g = g.add_mutually_exclusive_group()
> > > + for fmt in format_list:
> > > + g.add_argument('-' + fmt, dest='imgfmt',
> > > action='store_const',
> > > + const=fmt)
> > > +
> > > + protocol_list = ['file', 'rbd', 'sheepdoc', 'nbd', 'ssh', 'nfs',
> > > + 'fuse']
> > > + g = p.add_argument_group(
> > > + 'image protocol options',
> > > + 'The following options sets IMGPROTO environment variably. '
> > > + 'At most one chose is allowed, default is "file"')
> >
> > Same as above, but also s/variably/variable/.
> >
> > Do we consider these environment variables user interfaces? So far I
> > thought of them as implementation details, but I guess if we want to
> > allow users to execute test scripts manually, they are some kind of user
> > interface.
> >
> > However, shouldn't the variables themselves then be documented
> > somewhere? As it is, this feels like documenting thing X to be the same
> > as thing Y, without ever saying what Y is.
>
> Yes, I do think that we lack some specification on the test interface,
> including
> environment variables.. And then, probably some unification of the interface,
> to
> be more clear and straightforward. But don't want to improve/refactor these
> things
> in these series.
Yeah, that's fair. If the new help provides at least the same
information as the old one, that is good enough for me.
> >
> > That said...
> >
> > > + g = g.add_mutually_exclusive_group()
> > > + for prt in protocol_list:
> > > + g.add_argument('-' + prt, dest='imgproto',
> > > action='store_const',
> > > + const=prt)
> >
> > ...maybe we should just have help=f"test {fmt/prt}" here to match the
> > old help text. Then this documents the option and the help above
> > actually documents the environment variable.
>
> OK
>
> >
> > > +
> > > + g = p.add_mutually_exclusive_group()
> > > + # We don't set default for cachemode, as we need to distinguish
> > > dafult
> > > + # from user input later.
> > > + g.add_argument('-nocache', dest='cachemode',
> > > action='store_const',
> > > + const='none', help='set cache mode "none"
> > > (O_DIRECT), '
> > > + 'sets CACHEMODE environment variable')
> > > + g.add_argument('-c', dest='cachemode',
> > > + help='sets CACHEMODE environment variable')
> > > +
> > > + p.add_argument('-i', dest='aiomode', default='threads',
> > > + help='sets AIOMODE environment variable')
> > > +
> > > + g = p.add_argument_group('bash tests options',
> > > + 'The following options are ignored by '
> > > + 'python tests. TODO: support them in '
> > > + 'iotests.py')
> >
> > Let's not print TODO comments to the user, but just make it a comment in
> > the code. That makes it stand out better with syntax highlighting
> > anyway.
> >
> > > + g.add_argument('-o', dest='imgopts',
> > > + help='options to pass to qemu-img create/convert,
> > > sets '
> > > + 'IMGOPTS environment variable')
> > > + p.add_argument('-valgrind', dest='VALGRIND_QEMU',
> > > action='store_const',
> > > + const='y', help='use valgrind, sets VALGRIND_QEMU
> > > '
> > > + 'environment variable')
> > > +
> > > + cls._argparser = p
> > > + return p
> > > +
> > > + def init_handle_argv(self, argv: List[str]) -> None:
> > > +
> > > + # Hints for mypy, about arguments which will be set by argparse
> >
> > I don't understand what this comment wants to tell me.
>
> I mean, I could automatically set self. attributes from args, but do
> it explictly to sutisfy mypy. But..
>
> Actually, I'll move argv interface out of the file for v7.
>
> I recently learned from another project, that merging cmdline
> interface (like it done her) into logic(model) class is a bad idea. (I
> just had to refactor it and split ligic from the interface, to reuse
> logic classes with another interface) [1]
>
> Also, trying to fix pylint and mypy complains, I had to inherit
> classes from AbstractContextManager (for mypy).. And then, how to fix
> "testenv.py:1:0: R0801: Similar lines in 2 files" ? You wandered,
> should we disable it.. I think not. It's a good warning, showing bad
> design. True way of fixing is creating a base class with common
> functionality.. But than.. Multiple inheritance to sutisfy linters?
> Haha. This brings us to [1] again.
>
> So, I decided to keep the classes with normal python interface
> (__init__ with normal arguments), and move the whole cmdline interface
> into check script itself. It looks a lot better.. Also, TestEnv is
> complicated enough even without argument parsing.
Ok, then I'll wait for your new version to see what it looks like.
> >
> > > + args, self.remaining_argv =
> > > self.get_argparser().parse_known_args(argv)
> > > + self.imgfmt = args.imgfmt
> > > + self.imgproto = args.imgproto
> > > + self.aiomode = args.aiomode
> > > + self.imgopts = args.imgopts
> > > + self.misalign = args.misalign
> > > + self.debug = args.debug
> > > +
> > > + if args.cachemode is None:
> > > + self.cachemode_is_default = 'true'
> > > + self.cachemode = 'writeback'
> > > + else:
> > > + self.cachemode_is_default = 'false'
> > > + self.cachemode = args.cachemode
> > > +
> > > + def init_directories(self):
> > > + """Init directory variables:
> > > + PYTHONPATH
> > > + TEST_DIR
> > > + SOCK_DIR
> > > + SAMPLE_IMG_DIR
> > > + OUTPUT_DIR
> > > + """
> > > + self.pythonpath = os.getenv('PYTHONPATH')
> > > + if self.pythonpath:
> > > + self.pythonpath = self.source_iotests + os.pathsep + \
> > > + self.pythonpath
> > > + else:
> > > + self.pythonpath = self.source_iotests
> > > +
> > > + self.test_dir = os.getenv('TEST_DIR',
> > > + os.path.join(os.getcwd(), 'scratch'))
> > > + Path(self.test_dir).mkdir(parents=True, exist_ok=True)
> > > +
> > > + self.sock_dir = os.getenv('SOCK_DIR')
> > > + self.tmp_sock_dir = False
> > > + if self.sock_dir:
> > > + Path(self.test_dir).mkdir(parents=True, exist_ok=True)
> > > + else:
> > > + self.sock_dir = tempfile.mkdtemp()
> > > + self.tmp_sock_dir = True
> > > +
> > > + self.sample_img_dir = os.getenv('SAMPLE_IMG_DIR',
> > > + os.path.join(self.source_iotests,
> > > + 'sample_images'))
> > > +
> > > + self.output_dir = os.getcwd() # OUTPUT_DIR
> > > +
> > > + def init_binaries(self):
> > > + """Init binary path variables:
> > > + PYTHON (for bash tests)
> > > + QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, QEMU_NBD_PROG,
> > > QSD_PROG
> > > + SOCKET_SCM_HELPER
> > > + """
> > > + self.python = '/usr/bin/python3 -B'
> >
> > This doesn't look right, we need to respect the Python binary set in
> > configure (which I think we get from common.env)
>
> Oh, I missed the change. Then I should just drop this self.python.
Do we still get the value from elsewhere or do we need to manually parse
common.env?
> >
> > > + def root(*names):
> > > + return os.path.join(self.build_root, *names)
> > > +
> > > + arch = os.uname().machine
> > > + if 'ppc64' in arch:
> > > + arch = 'ppc64'
> > > +
> > > + self.qemu_prog = os.getenv('QEMU_PROG',
> > > root(f'qemu-system-{arch}'))
> > > + self.qemu_img_prog = os.getenv('QEMU_IMG_PROG', root('qemu-img'))
> > > + self.qemu_io_prog = os.getenv('QEMU_IO_PROG', root('qemu-io'))
> > > + self.qemu_nbd_prog = os.getenv('QEMU_NBD_PROG', root('qemu-nbd'))
> > > + self.qsd_prog = os.getenv('QSD_PROG', root('storage-daemon',
> > > +
> > > 'qemu-storage-daemon'))
> > > +
> > > + for b in [self.qemu_img_prog, self.qemu_io_prog,
> > > self.qemu_nbd_prog,
> > > + self.qemu_prog, self.qsd_prog]:
> > > + if not os.path.exists(b):
> > > + exit('Not such file: ' + b)
> > > + if not os.access(b, os.X_OK):
> > > + exit('Not executable: ' + b)
> > > +
> > > + helper_path = os.path.join(self.build_iotests,
> > > 'socket_scm_helper')
> > > + if os.access(helper_path, os.X_OK):
> > > + self.socket_scm_helper = helper_path # SOCKET_SCM_HELPER
> > > +
> > > + def __init__(self, argv: List[str]) -> None:
> > > + """Parse args and environment"""
> > > +
> > > + # Initialize generic paths: build_root, build_iotests,
> > > source_iotests,
> > > + # which are needed to initialize some environment variables.
> > > They are
> > > + # used by init_*() functions as well.
> > > +
> > > +
> > > + if os.path.islink(sys.argv[0]):
> > > + # called from the build tree
> > > + self.source_iotests =
> > > os.path.dirname(os.readlink(sys.argv[0]))
> > > + self.build_iotests =
> > > os.path.dirname(os.path.abspath(sys.argv[0]))
> > > + else:
> > > + # called from the source tree
> > > + self.source_iotests = os.getcwd()
> > > + self.build_iotests = self.source_iotests
> > > +
> > > + self.build_root = os.path.join(self.build_iotests, '..', '..')
> > > +
> > > + self.init_handle_argv(argv)
> > > + self.init_directories()
> > > + self.init_binaries()
> > > +
> > > + # QEMU_OPTIONS
> > > + self.qemu_options = '-nodefaults -display none -accel qtest'
> > > + machine_map = (
> > > + (('arm', 'aarch64'), 'virt'),
> >
> > How does this work? Won't we check for "qemu-system-('arm', 'aarch64')"
> > below, which we'll never find?
>
> Hmm. I just took existing logic from check:
>
> [..]
> case "$QEMU_PROG" in
> *qemu-system-arm|*qemu-system-aarch64)
> export QEMU_OPTIONS="$QEMU_OPTIONS -machine virt"
> ;;
> [..]
What I mean is that the code below doesn't look like it's prepared to
interpret a tuple like ('arm', 'aarch64'), it expects a single string:
> >
> > > + ('avr', 'mega2560'),
> > > + ('rx', 'gdbsim-r5f562n8'),
> > > + ('tricore', 'tricore_testboard')
> > > + )
> > > + for suffix, machine in machine_map:
> > > + if self.qemu_prog.endswith(f'qemu-system-{suffix}'):
Here we get effectively:
suffix: Tuple[str, str] = ('arm', 'aarch64')
The formatted string uses str(suffix), which makes the result
"qemu-system-('arm', 'aarch64')".
Or am I misunderstanding something here?
> > > + self.qemu_options += f' -machine {machine}'
> > > +
> > > + # QEMU_DEFAULT_MACHINE
> > > + self.qemu_default_machine = get_default_machine(self.qemu_prog)
> > > +
> > > + self.qemu_img_options = os.getenv('QEMU_IMG_OPTIONS')
> > > + self.qemu_nbd_options = os.getenv('QEMU_NBD_OPTIONS')
> > > +
> > > + is_generic = self.imgfmt not in ['bochs', 'cloop', 'dmg']
> > > + self.imgfmt_generic = 'true' if is_generic else 'false'
> > > +
> > > + self.qemu_io_options = f'--cache {self.cachemode} --aio
> > > {self.aiomode}'
> > > + if self.misalign:
> > > + self.qemu_io_options += ' --misalign'
> > > +
> > > + self.qemu_io_options_no_fmt = self.qemu_io_options
> > > +
> > > + if self.imgfmt == 'luks':
> > > + self.imgoptssyntax = 'true'
> > > + self.imgkeysecret = '123456'
> > > + if not self.imgopts:
> > > + self.imgopts = 'iter-time=10'
> > > + elif 'iter-time=' not in self.imgopts:
> > > + self.imgopts += ',iter-time=10'
> > > + else:
> > > + self.imgoptssyntax = 'false'
> > > + self.qemu_io_options += ' -f ' + self.imgfmt
> > > +
> > > + if self.imgfmt == 'vmkd':
> > > + if not self.imgopts:
> > > + self.imgopts = 'zeroed_grain=on'
> > > + elif 'zeroed_grain=' not in self.imgopts:
> > > + self.imgopts += ',zeroed_grain=on'
> > > +
> > > + def close(self) -> None:
> > > + if self.tmp_sock_dir:
> > > + shutil.rmtree(self.sock_dir)
> > > +
> > > + def __enter__(self) -> 'TestEnv':
> > > + return self
> > > +
> > > + def __exit__(self, *args) -> None:
> > > + self.close()
> > > +
> > > + def print_env(self) -> None:
> > > + template = """\
> > > +QEMU -- "{QEMU_PROG}" {QEMU_OPTIONS}
> > > +QEMU_IMG -- "{QEMU_IMG_PROG}" {QEMU_IMG_OPTIONS}
> > > +QEMU_IO -- "{QEMU_IO_PROG}" {QEMU_IO_OPTIONS}
> > > +QEMU_NBD -- "{QEMU_NBD_PROG}" {QEMU_NBD_OPTIONS}
> > > +IMGFMT -- {IMGFMT}{imgopts}
> > > +IMGPROTO -- {IMGPROTO}
> > > +PLATFORM -- {platform}
> > > +TEST_DIR -- {TEST_DIR}
> > > +SOCK_DIR -- {SOCK_DIR}
> > > +SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}"""
> > > +
> > > + args = collections.defaultdict(str, self.get_env())
> > > +
> > > + if 'IMGOPTS' in args:
> > > + args['imgopts'] = f" ({args['IMGOPTS']})"
> > > +
> > > + u = os.uname()
> > > + args['platform'] = f'{u.sysname}/{u.machine} {u.nodename}
> > > {u.release}'
> > > +
> > > + print(template.format_map(args))
> > > +
> > > +
> > > +if __name__ == '__main__':
> > > + if len(sys.argv) == 2 and sys.argv[1] in ['-h', '--help']:
> > > + TestEnv.get_argparser().print_help()
> > > + exit()
> > > +
> > > + with TestEnv(sys.argv) as te:
> > > + te.print_env()
> > > + print('\nUnhandled options: ', te.remaining_argv)
>
> Thanks for reviewing! I hope to post v7 today, with cmdline interface
> in 'check' script.
Sounds good. Then I'll continue the review when v7 is on the list.
Kevin
- Re: [PATCH v6 07/11] iotests: add findtests.py, (continued)
- Re: [PATCH v6 08/11] iotests: add testenv.py, Kevin Wolf, 2021/01/15
- Re: [PATCH v6 08/11] iotests: add testenv.py, Vladimir Sementsov-Ogievskiy, 2021/01/15
- Re: [PATCH v6 08/11] iotests: add testenv.py,
Kevin Wolf <=
- Re: [PATCH v6 08/11] iotests: add testenv.py, Vladimir Sementsov-Ogievskiy, 2021/01/15
- Re: [PATCH v6 08/11] iotests: add testenv.py, Kevin Wolf, 2021/01/15
- Re: [PATCH v6 08/11] iotests: add testenv.py, Vladimir Sementsov-Ogievskiy, 2021/01/15
- Re: [PATCH v6 08/11] iotests: add testenv.py, Vladimir Sementsov-Ogievskiy, 2021/01/16
- Re: [PATCH v6 08/11] iotests: add testenv.py, Vladimir Sementsov-Ogievskiy, 2021/01/16
- Re: [PATCH v6 08/11] iotests: add testenv.py, Kevin Wolf, 2021/01/18
- Re: [PATCH v6 08/11] iotests: add testenv.py, Vladimir Sementsov-Ogievskiy, 2021/01/18
[PATCH v6 06/11] iotests: define group in each iotest, Vladimir Sementsov-Ogievskiy, 2021/01/09