Re: [PATCH v6 08/11] iotests: add testenv.py

[Kevin Wolf]

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.

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.

> +
> +        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.

> +        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)

> +        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?

> +            ('avr', 'mega2560'),
> +            ('rx', 'gdbsim-r5f562n8'),
> +            ('tricore', 'tricore_testboard')
> +        )
> +        for suffix, machine in machine_map:
> +            if self.qemu_prog.endswith(f'qemu-system-{suffix}'):
> +                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)

Kevin