canonical-ubuntu-qa team mailing list archive

[Tim Andersson <mp+454826@xxxxxxxxxxxxxxxxxx>]

Tim Andersson has proposed merging ~andersson123/autopkgtest-cloud:worker-refactor-add-unit-tests into autopkgtest-cloud:master.

Requested reviews:
  Canonical's Ubuntu QA (canonical-ubuntu-qa)

For more details, see:
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/454826
-- 
Your team Canonical's Ubuntu QA is requested to review the proposed merge of ~andersson123/autopkgtest-cloud:worker-refactor-add-unit-tests into autopkgtest-cloud:master.

diff --git a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker
index 980d63b..67b7722 100755
--- a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker
+++ b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker
@@ -53,6 +53,9 @@ try:
 except KeyError:
     influx_client = None
 
+# move these to global variables in uppercase since that's how
+# they referred to in the rest of the script.
+######################################################################
 my_path = os.path.dirname(__file__)
 root_path = os.path.dirname(os.path.abspath(my_path))
 args = None
@@ -70,6 +73,11 @@ hostname = socket.gethostname()
 big_packages = set()
 long_tests = set()
 never_run = set()
+######################################################################
+
+# actual global variables can stay I guess
+# or maybe can be moved to config files?
+# I think that would be nice.
 
 ARCH_RELEASE_ALLOW_MAPPING = {"trusty": ["amd64"], "xenial": ["amd64"]}
 
@@ -322,6 +330,9 @@ def process_output_dir(dir, pkgname, code, triggers):
     # In failure cases where we don't know the version, write 'unknown' out as
     # the version, so that frontends (e.g. autopkgtest-web, or britney) can
     # display the result.
+
+    ################################################################################
+    # make a function
     if code in FAIL_CODES and "testpkg-version" not in files:
         logging.warning(
             'Code %d returned and no testpkg-version - returning "unknown" for %s'
@@ -342,6 +353,7 @@ def process_output_dir(dir, pkgname, code, triggers):
                 d = {"custom_environment": ["ADT_TEST_TRIGGERS=%s" % triggers]}
                 json.dump(d, testinfo, indent=True)
             files.add("testinfo.json")
+    ################################################################################
 
     with open(os.path.join(dir, "testpkg-version"), "r") as tpv:
         testpkg_version = tpv.read().split()[1]
@@ -358,6 +370,8 @@ def process_output_dir(dir, pkgname, code, triggers):
     except FileNotFoundError:
         requester = None
 
+    ################################################################################
+    # make a function
     # these are small and we need only these for gating and indexing
     resultfiles = ["exitcode"]
     # these might not be present in infrastructure failure cases
@@ -372,6 +386,7 @@ def process_output_dir(dir, pkgname, code, triggers):
         if f in files:
             resultfiles.append(f)
     subprocess.check_call(["tar", "cf", "result.tar"] + resultfiles, cwd=dir)
+    ################################################################################
 
     # compress main log file, for direct access
     subprocess.check_call(["gzip", "-9", os.path.join(dir, "log")])
@@ -381,6 +396,8 @@ def process_output_dir(dir, pkgname, code, triggers):
     # to the container as is, as it's used for ACL
     files.discard("readable-by")
 
+    ################################################################################
+    # make a function
     if files:
         # tar up all other artifacts
         subprocess.check_call(
@@ -392,6 +409,7 @@ def process_output_dir(dir, pkgname, code, triggers):
                 shutil.rmtree(path)
             else:
                 os.unlink(path)
+    ################################################################################
 
     return (testpkg_version, duration, requester)
 
@@ -508,12 +526,15 @@ def call_autopkgtest(
     Return exit code.
     """
     # set up status AMQP exchange
+    ################################################################################
+    # make a function ... maybe
     global amqp_con
     status_amqp = amqp_con.channel()
     status_amqp.access_request("/data", active=True, read=False, write=True)
     status_amqp.exchange_declare(
         status_exchange_name, "fanout", durable=False, auto_delete=True
     )
+    ################################################################################
 
     null_fd = open("/dev/null", "w")
     autopkgtest = subprocess.Popen(
@@ -589,6 +610,8 @@ def request(msg):
     dont_run = False
     private = False
 
+    ################################################################################
+    # make a function
     # FIXME: make this more elegant
     fields = msg.delivery_info["routing_key"].split("-")
     if len(fields) == 4:
@@ -599,10 +622,13 @@ def request(msg):
         raise NotImplementedError(
             "cannot parse queue name %s" % msg.delivery_info["routing_key"]
         )
+    ################################################################################
 
     systemd_logging_handler._extra["ADT_RELEASE"] = release
     systemd_logging_handler._extra["ADT_ARCH"] = architecture
 
+    ################################################################################
+    # make a function
     body = msg.body
     if isinstance(body, bytes):
         try:
@@ -629,6 +655,7 @@ def request(msg):
         )
         msg.channel.basic_ack(msg.delivery_tag)
         return
+    ################################################################################
 
     systemd_logging_handler._extra["ADT_PACKAGE"] = pkgname
     systemd_logging_handler._extra["ADT_PARAMS"] = str(params)
@@ -648,6 +675,8 @@ def request(msg):
 
     try:
         out_dir = os.path.join(work_dir, "out")
+        ################################################################################
+        # make a function
         if (
             release.lower() in ARCH_RELEASE_ALLOW_MAPPING
             and architecture not in ARCH_RELEASE_ALLOW_MAPPING[release.lower()]
@@ -671,7 +700,10 @@ def request(msg):
                     f'Package blacklisted we only run {",".join(ARCH_RELEASE_ALLOW_MAPPING[release.lower()])} tests for {release.lower()}'
                 )
             dont_run = True
+        ################################################################################
 
+        ################################################################################
+        # make a function
         if request_matches_per_package(
             pkgname, architecture, release, never_run
         ):
@@ -711,7 +743,10 @@ def request(msg):
                 os.path.join(out_dir, "testpkg-version"), "w"
             ) as testpkg_version:
                 testpkg_version.write("%s blacklisted" % pkgname)
+        ################################################################################
 
+        ################################################################################
+        # make a function
         container = "autopkgtest-" + release
         big_pkg = request_matches_per_package(
             pkgname, architecture, release, big_packages
@@ -755,7 +790,10 @@ def request(msg):
                 pkgname,
             )
             argv += ["--setup-commands", c]
+        ################################################################################
 
+        ################################################################################
+        # make a function
         if "triggers" in params and "qemu-efi-noacpi/0" in params["triggers"]:
             if architecture == "arm64":
                 argv += [
@@ -782,7 +820,10 @@ def request(msg):
                     testpkg_version.write(
                         "invalid trigger: qemu-efi-noacpi/0 is arm64 only"
                     )
+        ################################################################################
 
+        ################################################################################
+        # make a function
         if "ppas" in params and params["ppas"]:
             for ppa in params["ppas"]:
                 try:
@@ -875,12 +916,15 @@ def request(msg):
 
             # put results into separate container, named by the last PPA
             container += "-%s-%s" % (ppauser, ppaname)
+        ################################################################################
 
         # only install the triggering package from -proposed, rest from -release
         # this provides better isolation between -proposed packages; but only do
         # that for Ubuntu itself, not for things from git, PPAs, etc.
         # also skip that for the kernel as the linux vs. linux-meta split always
         # screws up the apt pinning
+        ################################################################################
+        # make a function
         if cfg.get("virt", "args") != "null":
             if (
                 "test-git" not in params
@@ -904,7 +948,10 @@ def request(msg):
                 if pocket_arg:
                     argv.append(pocket_arg)
             argv.append("--apt-upgrade")
+        ################################################################################
 
+        ################################################################################
+        # make a function
         # determine which test to run
         if "test-git" in params:
             testargs = ["--no-built-binaries", params["test-git"]]
@@ -924,7 +971,10 @@ def request(msg):
             testargs = ["--no-built-binaries", checkout_dir]
         else:
             testargs = [pkgname]
+        ################################################################################
 
+        ################################################################################
+        # make a function
         argv += testargs
         if args.debug:
             argv.append("--debug")
@@ -945,7 +995,10 @@ def request(msg):
 
         for e in params.get("env", []):
             argv.append("--env=%s" % e)
+        ################################################################################
 
+        ################################################################################
+        # make a function
         triggers = None
         if "triggers" in params:
             triggers = " ".join(params["triggers"])
@@ -1022,6 +1075,7 @@ def request(msg):
                             % {"f": flavor, "t": totest},
                         ]
                     break
+        ################################################################################
 
         if "testname" in params:
             argv.append("--testname=%s" % params["testname"])
@@ -1057,6 +1111,8 @@ def request(msg):
             running_test = True
             start_time = time.time()
             num_failures = 0
+            ################################################################################
+            # make a function - or have smaller functions within.
             for retry in range(3):
                 retry_start_time = time.time()
                 logging.info("Running %s", " ".join(argv))
@@ -1213,6 +1269,7 @@ def request(msg):
                     sys.exit(99)
 
             duration = int(time.time() - retry_start_time)
+            ################################################################################
 
         logging.info("autopkgtest exited with code %i", code)
         submit_metric(