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

[Vladimir Sementsov-Ogievskiy]

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.

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

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

> > +        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"
          ;;
[..]

> > +            ('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

Thanks for reviewing! I hope to post v7 today, with cmdline interface in 
'check' script.

--
Best regards,
Vladimir