systemupdate: refactor code

Much of the code relied on a running code in certain contexts. So far,
that was solved via callbacks, which made reusing some subset of the
functionality or parameterizing tests harder.

Now the flow is broken up into individual steps, with context managers
used whenever cleanup operations are needed.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>

Author: pohly

meta-refkit-core/lib/oeqa/selftest/systemupdate/httpupdate.py

@@ -1,13 +1,101 @@
 from oeqa.selftest.systemupdate.systemupdatebase import SystemUpdateBase
 from oeqa.utils.commands import runqemu, get_bb_vars, bitbake
 
+import contextlib
 import http.server
 import os
 import stat
 import errno
 import tempfile
+import traceback
 import threading
 
+class HTTPServer(object):
+    """
+    Dynamically finds an available port and serves a certain directory there.
+    To be used in a "with HTTPServer(dir) as httpd" construct.
+    """
+    def __init__(self, root, logger):
+        self.root = root
+        self.logger = logger
+        self.server = None
+        self.http_log = []
+        self.stop_at = None
+
+    def __enter__(self):
+        try:
+            class HTTPRequestHandler(http.server.SimpleHTTPRequestHandler):
+                parent = self
+                request_counter = 0
+                def log_message(self, format, *args):
+                    msg = format % args
+                    self.parent.logger.info(msg)
+                    self.parent.http_log.append(msg)
+
+                def translate_path(self, path):
+                    """
+                    Return absolute path based on document root instead of current directory.
+                    """
+
+                    # The original implementation returns an absolute path rooted in the
+                    # current directory. We need to serve a different
+                    # directory without being able to chdir(), because
+                    # doing that would cause commands like bitbake to
+                    # run there, which is undesirable because for
+                    # example bitbake creates a bitbake-cookerdaemon.log
+                    # in the current directory.
+                    path = super().translate_path(path)
+                    relpath = os.path.relpath(path)
+                    path = os.path.join(self.parent.root, relpath)
+                    return path
+
+                def do_GET(self):
+                    """
+                    Inject errors.
+                    """
+                    counter = HTTPRequestHandler.request_counter
+                    HTTPRequestHandler.request_counter += 1
+                    if self.parent.stop_at is not None and counter >= self.parent.stop_at:
+                        self.send_error(500, 'test server is intentionally down')
+                    else:
+                        super().do_GET()
+
+            handler = HTTPRequestHandler
+
+            def create_httpd():
+                for port in range(9999, 10000):
+                    try:
+                        server = http.server.HTTPServer(('localhost', port), handler)
+                        return server
+                    except OSError as ex:
+                        if ex.errno != errno.EADDRINUSE:
+                            raise
+                self.fail('no port available for HTTP server')
+
+            self.server = create_httpd()
+            self.port = self.server.server_port
+            self.logger.info('serving repo %s on port %d' % (self.root, self.port))
+            helper = threading.Thread(name='HTTPD', target=self.server.serve_forever)
+            helper.start()
+
+            # Now let caller do its work while the server runs.
+            return self
+        except:
+            self._stop()
+            raise
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        self._stop()
+
+    def _stop(self):
+        # We have to stop a running server under all circumstances,
+        # otherwise the helper thread will keep running and we end up
+        # with thread locking issues.
+        if self.server:
+            self.server.shutdown()
+            self.server.server_close()
+            self.server = None
+
 class HTTPUpdate(SystemUpdateBase):
     """
     System update tests for image update mechanisms which depend on
@@ -51,7 +139,8 @@ def track_for_cleanup(self, name):
         if 'NO_CLEANUP' not in os.environ:
             super().track_for_cleanup(name)
 
-    def boot_image(self, overrides):
+    @contextlib.contextmanager
+    def boot_image(self, overrides = {}):
         # We don't know the final port yet, so instead we create a placeholder script
         # for qemu to use and rewrite that script once we are ready. The kernel refuses
         # to execute a shell script while we have it open, so here we close it
@@ -60,106 +149,55 @@ def boot_image(self, overrides):
         # The helper script also keeps command line handling a bit simpler (no whitespace
         # in -netdev parameter), which may or may not be relevant.
         self.httpd_netcat = tempfile.NamedTemporaryFile(mode='w', prefix='httpd-netcat-', dir=os.getcwd(), delete=False)
-        self.httpd_netcat.close()
-        os.chmod(self.httpd_netcat.name, stat.S_IRUSR|stat.S_IWUSR|stat.S_IXUSR)
-        self.track_for_cleanup(self.httpd_netcat.name)
-
-        qemuboot_conf = os.path.join(self.image_dir_test,
-                                     '%s-%s.qemuboot.conf' % (self.IMAGE_PN, self.BB_VARS['MACHINE']))
-        with open(qemuboot_conf) as f:
-            conf = f.read()
-        with open(qemuboot_conf, 'w') as f:
-            f.write('\n'.join([x for x in conf.splitlines() if not x.startswith('qb_slirp_opt')]))
-            f.write('\nqb_slirp_opt = -netdev user,id=net0,guestfwd=tcp:%s-cmd:%s\n' % \
-                    (self.HTTPD_SERVER, self.httpd_netcat.name))
-        return runqemu(self.IMAGE_PN,
-                       discard_writes=False, ssh=False,
-                       overrides=overrides,
-                       runqemuparams='ovmf slirp nographic',
-                       image_fstype='wic')
-
-    def update_image(self, qemu):
-        # We need to bring up some simple HTTP server for the
-        # update repo.
-        server = None
-        self.http_log = []
-        http_log = self.http_log
         try:
-            class HTTPRequestHandler(http.server.SimpleHTTPRequestHandler):
-                parent = self
-                request_counter = 0
-                def log_message(self, format, *args):
-                    msg = format % args
-                    self.parent.logger.info(msg)
-                    self.parent.http_log.append(msg)
-
-                def translate_path(self, path):
-                    """
-                    Return absolute path based on document root instead of current directory.
-                    """
-
-                    # The original implementation returns an absolute path rooted in the
-                    # current directory. We need to serve a different
-                    # directory without being able to chdir(), because
-                    # doing that would cause commands like bitbake to
-                    # run there, which is undesirable because for
-                    # example bitbake creates a bitbake-cookerdaemon.log
-                    # in the current directory.
-                    path = super().translate_path(path)
-                    relpath = os.path.relpath(path)
-                    path = os.path.join(self.parent.REPO_DIR, relpath)
-                    return path
-
-                def do_GET(self):
-                    """
-                    Inject errors.
-                    """
-                    counter = HTTPRequestHandler.request_counter
-                    HTTPRequestHandler.request_counter += 1
-                    stop_at = getattr(self.parent, 'stop_serving_http_at', None)
-                    if stop_at is not None and counter >= stop_at:
-                        self.send_error(500, 'test server is intentionally down')
-                    else:
-                        super().do_GET()
+            self.httpd_netcat.close()
+            os.chmod(self.httpd_netcat.name, stat.S_IRUSR|stat.S_IWUSR|stat.S_IXUSR)
+            qemuboot_conf = os.path.join(self.image_dir_test,
+                                         '%s-%s.qemuboot.conf' % (self.IMAGE_PN, self.BB_VARS['MACHINE']))
+            with open(qemuboot_conf) as f:
+                conf = f.read()
+            with open(qemuboot_conf, 'w') as f:
+                f.write('\n'.join([x for x in conf.splitlines() if not x.startswith('qb_slirp_opt')]))
+                f.write('\nqb_slirp_opt = -netdev user,id=net0,guestfwd=tcp:%s-cmd:%s\n' % \
+                        (self.HTTPD_SERVER, self.httpd_netcat.name))
+            with super().boot_image(ssh=False,
+                                    runqemuparams='ovmf slirp nographic',
+                                    image_fstype='wic') as qemu:
+                yield qemu
+        finally:
+            os.unlink(self.httpd_netcat.name)
 
-            handler = HTTPRequestHandler
+    @contextlib.contextmanager
+    def start_httpd(self):
+        """
+        Bring up the HTTP server when entering the context and shut it down when done.
+        """
 
-            def create_httpd():
-                for port in range(9999, 10000):
-                    try:
-                        server = http.server.HTTPServer(('localhost', port), handler)
-                        return server
-                    except OSError as ex:
-                        if ex.errno != errno.EADDRINUSE:
-                            raise
-                self.fail('no port available for HTTP server')
+        # netcat can't be assumed to be present. Build and use socat instead.
+        # It's a bit more complicated but has the advantage that it is in OE-core.
+        socat = os.path.join(self.BB_VARS['RECIPE_SYSROOT_NATIVE'], 'usr', 'bin', 'socat')
+        if not os.path.exists(socat):
+            bitbake('socat-native:do_addto_recipe_sysroot', output_log=self.logger)
+        self.assertExists(socat, 'socat-native was not built as expected')
 
-            server = create_httpd()
-            port = server.server_port
-            self.logger.info('serving repo %s on port %d' % (self.REPO_DIR, port))
-            helper = threading.Thread(name='HTTPD', target=server.serve_forever)
-            helper.start()
-            # netcat can't be assumed to be present. Build and use socat instead.
-            # It's a bit more complicated but has the advantage that it is in OE-core.
-            socat = os.path.join(self.BB_VARS['RECIPE_SYSROOT_NATIVE'], 'usr', 'bin', 'socat')
-            if not os.path.exists(socat):
-                bitbake('socat-native:do_addto_recipe_sysroot', output_log=self.logger)
-            self.assertExists(socat, 'socat-native was not built as expected')
+        with HTTPServer(self.REPO_DIR, self.logger) as httpd:
             with open(self.httpd_netcat.name, 'w') as f:
                 f.write('''#!/bin/sh
 exec %s 2>/tmp/httpd.log -D -v -d -d -d -d STDIO TCP:localhost:%d
-''' % (socat, port))
+''' % (socat, httpd.port))
+            yield httpd
 
+
+    def update_image(self, qemu):
+        # We need to bring up some simple HTTP server for the
+        # update repo.
+        with self.start_httpd() as self.httpd:
             # Now run the real update command inside the virtual machine.
             return self.update_image_via_http(qemu)
 
-        finally:
-            if server:
-                server.shutdown()
-                server.server_close()
-
     def update_image_via_http(self, qemu):
         """
         Called by update_image() with the HTTPD server running.
         """
         return False
+

meta-refkit-core/lib/oeqa/selftest/systemupdate/systemupdatebase.py

@@ -7,6 +7,7 @@
 import oe.path
 
 import base64
+import contextlib
 import fnmatch
 import pathlib
 import pickle
@@ -210,13 +211,20 @@ class SystemUpdateBase(OESelftestTestCase):
     # Expected to be replaced by derived class.
     IMAGE_MODIFY = SystemUpdateModify()
 
-    def boot_image(self, overrides):
+    def boot_image(self, overrides = {}, **kwargs):
         """
         Calls runqemu() such that commands can be started via run_serial().
         Derived classes need to replace with something that adds whatever
         other parameters are needed or useful.
         """
-        return runqemu(self.IMAGE_PN, discard_writes=False, overrides=overrides)
+        # Change DEPLOY_DIR_IMAGE so that we use our copy of the
+        # images from before the update. Further customizations for booting can
+        # be done by rewriting self.image_dir_test/IMAGE_PN-MACHINE.qemuboot.conf
+        # (read, close, write, not just appending as that would also change
+        # the file copy under image_dir).
+        overrides = overrides.copy()
+        overrides['DEPLOY_DIR_IMAGE'] = self.image_dir_test
+        return runqemu(self.IMAGE_PN, discard_writes=False, overrides=overrides, **kwargs)
 
     def update_image(self, qemu):
         """
@@ -241,28 +249,18 @@ def modify_image_build(self, testname, updates, is_update):
             bbappend.append('APPEND_append = " modify_kernel_test=%s"' % ('updated' if is_update else 'original'))
         return '\n'.join(bbappend)
 
-    def do_update(self, testname, updates):
+    def create_image_bbappend(self, testname, updates, is_update):
         """
-        Builds the image, makes a copy of the result, rebuilds to produce
-        an update with configurable changes, boots the original image, updates it,
-        reboots and then checks the updated image.
-
-        'update' is a list of modify_* function names which make the actual changes
-        (adding, removing, modifying files or kernel) that are part of the tests.
+        Creates an IMAGE_BBAPPEND which contains the pickled modification code.
+        A .bbappend is used because it can contain code and is guaranteed to be
+        applied also to image variants.
         """
 
-        def create_image_bbappend(is_update):
-            """
-            Creates an IMAGE_BBAPPEND which contains the pickled modification code.
-            A .bbappend is used because it can contain code and is guaranteed to be
-            applied also to image variants.
-            """
-
-            bbappend = self.IMAGE_BBAPPEND_UPDATE if is_update else self.IMAGE_BBAPPEND
-            self.append_config('BBFILES_append = " %s"' % os.path.abspath(bbappend))
-            self.track_for_cleanup(bbappend)
-            with open(bbappend, 'w') as f:
-                f.write('''
+        bbappend = self.IMAGE_BBAPPEND_UPDATE if is_update else self.IMAGE_BBAPPEND
+        self.append_config('BBFILES_append = " %s"' % os.path.abspath(bbappend))
+        self.track_for_cleanup(bbappend)
+        with open(bbappend, 'w') as f:
+            f.write('''
 python system_update_test_modify () {
     import base64
     import pickle
@@ -283,9 +281,14 @@ def create_image_bbappend(is_update):
        self.IMAGE_CONFIG,
        self.modify_image_build(testname, updates, is_update)))
 
+    def prepare_image(self, testname, updates):
+        """
+        Builds the initial image and prepares it for booting.
+        """
+
         # Creating a .bbappend for the image will trigger a rebuild.
         # To avoid this, use separate image recipes.
-        create_image_bbappend(False)
+        self.create_image_bbappend(testname, updates, False)
         self.logger.info('Building base image')
         result = bitbake(self.IMAGE_PN, output_log=self.logger)
 
@@ -303,28 +306,44 @@ def create_image_bbappend(is_update):
         # when the image recipes are different.
         self.clone_files(self.image_dir, ('ovmf*', vars['IMAGE_LINK_NAME'] + '*'))
 
-        # Change DEPLOY_DIR_IMAGE so that we use our copy of the
-        # images from before the update. Further customizations for booting can
-        # be done by rewriting self.image_dir_test/IMAGE_PN-MACHINE.qemuboot.conf
-        # (read, close, write, not just appending as that would also change
-        # the file copy under image_dir).
-        overrides = { 'DEPLOY_DIR_IMAGE': self.image_dir_test }
-
+    @contextlib.contextmanager
+    def boot_and_verify_image(self, testname, updates):
+        """
+        Boots the initial image and verifies its content.
+        To be used as:
+        with boot_initial_image() as qemu:
+             ... run additional checks in qemu ...
+        """
         # Boot image, verify before and after update.
-        with self.boot_image(overrides) as qemu:
+        with self.boot_image() as qemu:
             self.verify_image(testname, False, qemu, updates)
+            yield qemu
 
-            # Now we change our .bbappend so that the updated state is generated
-            # during the next rebuild.
-            create_image_bbappend(True)
-            self.logger.info('Building updated image')
-            bitbake(self.IMAGE_PN_UPDATE, output_log=self.logger)
+    def prepare_update(self, testname, updates):
+        """
+        Build the update image.
+        """
+        self.create_image_bbappend(testname, updates, True)
+        self.logger.info('Building updated image')
+        bitbake(self.IMAGE_PN_UPDATE, output_log=self.logger)
 
+    def do_update(self, testname, updates):
+        """
+        Builds the image, makes a copy of the result, rebuilds to produce
+        an update with configurable changes, boots the original image, updates it,
+        reboots and then checks the updated image.
+
+        'update' is a list of modify_* function names which make the actual changes
+        (adding, removing, modifying files or kernel) that are part of the tests.
+        """
+        self.prepare_image(testname, updates)
+        with self.boot_and_verify_image(testname, updates) as qemu:
+            self.prepare_update(testname, updates)
             reboot = self.update_image(qemu)
             if not reboot:
                 self.verify_image(testname, True, qemu, updates)
         if reboot:
-            with self.boot_image(overrides) as qemu:
+            with self.boot_image() as qemu:
                 self.verify_image(testname, True, qemu, updates)
 
     def clone_files(self, dirname, file_patterns):