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

[Kevin Wolf]

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