Make the update non-matching package check smarter

With Rawhide updates, we quite often run into a situation where
a test runs after a *later* version of the package has already
gone stable. This even happens for stable releases too, though
less often. The current shell-based check just always fails on
this case, but it's usually OK, and manually marking every case
like this with an "it's OK!" comment gets tiring. Instead, let's
use a smarter Python script to do the check. We compare the EVR
of all installed update packages with the EVR of the package
from the update. If it's the same, fine. If the installed package
is lower-versioned, that's always an error, and we fail. If the
installed package is higher-versioned, we check whether the
update already went stable. If it did, then we soft fail, because
probably nothing can go wrong at this point (this is the usual
Rawhide case). If the update did not yet go stable, we still
hard fail, because something can go wrong in this case: if the
update *now* goes stable, the older version from the update may
be tagged over the newer version the test got (presumably from
current stable).

If anything goes wrong with the Bodhi check, or the test is
running on a task not an advisory, we treat both cases as fatal.

The script also gives easier-to-understand output than the old
approach, which should be a bonus.

Signed-off-by: Adam Williamson <awilliam@redhat.com>

lib/utils.pm

@@ -649,7 +649,7 @@ sub _repo_setup_updates {
     # and similar files because, on ostree-based installs where we
     # have to use a toolbox container for part of this, it's common
     # to the host system and container
-    assert_script_run 'rpm -qp *.rpm --qf "%{SOURCERPM} %{EPOCH} %{NAME}-%{VERSION}-%{RELEASE}\n" | sort -u > /mnt/updatepkgs.txt';
+    assert_script_run 'rpm -qp *.rpm --qf "%{SOURCERPM} %{NAME} %{EPOCHNUM} %{VERSION} %{RELEASE}\n" | sort -u > /mnt/updatepkgs.txt';
     upload_logs "/mnt/updatepkgs.txt";
     # also log just the binary package names: this is so we can check
     # later whether any package from the update *should* have been
@@ -1107,7 +1107,7 @@ sub advisory_get_installed_packages {
     # bail out if the file doesn't exist: this is in case we get
     # here in the post-fail hook but we failed before creating it
     return if script_run "test -f /mnt/updatepkgs.txt";
-    assert_script_run 'rpm -qa --qf "%{SOURCERPM} %{EPOCH} %{NAME}-%{VERSION}-%{RELEASE}\n" | sort -u > /tmp/allpkgs.txt', timeout => 90;
+    assert_script_run 'rpm -qa --qf "%{SOURCERPM} %{NAME} %{EPOCHNUM} %{VERSION} %{RELEASE}\n" | sort -u > /tmp/allpkgs.txt', timeout => 90;
     # this finds lines which appear in both files
     # http://www.unix.com/unix-for-dummies-questions-and-answers/34549-find-matching-lines-between-2-files.html
     if (script_run 'comm -12 /tmp/allpkgs.txt /mnt/updatepkgs.txt > /mnt/testedpkgs.txt') {
@@ -1152,24 +1152,26 @@ sub advisory_check_nonmatching_packages {
     # (we need four to reach bash, and half of them get eaten by perl or
     # something along the way). Yes, it only works with *single* quotes. Yes,
     # I hate escaping
-    script_run 'for pkg in $(cat /mnt/updatepkgnames.txt); do rpm -q $pkg && rpm -q $pkg --last | head -1 | cut -d" " -f1 | sed -e \'s,\^,\\\\\\\\^,g\' | xargs rpm -q --qf "%{SOURCERPM} %{EPOCH} %{NAME}-%{VERSION}-%{RELEASE}\n" >> /tmp/installedupdatepkgs.txt; done', timeout => 180;
+    script_run 'for pkg in $(cat /mnt/updatepkgnames.txt); do rpm -q $pkg && rpm -q $pkg --last | head -1 | cut -d" " -f1 | sed -e \'s,\^,\\\\\\\\^,g\' | xargs rpm -q --qf "%{SOURCERPM} %{NAME} %{EPOCHNUM} %{VERSION} %{RELEASE}\n" >> /tmp/installedupdatepkgs.txt; done', timeout => 180;
     script_run 'sort -u -o /tmp/installedupdatepkgs.txt /tmp/installedupdatepkgs.txt';
     # for debugging, may as well always upload these, can't hurt anything
     upload_logs "/tmp/installedupdatepkgs.txt", failok => 1;
     upload_logs "/mnt/updatepkgs.txt", failok => 1;
-    # if any line appears in installedupdatepkgs.txt but not updatepkgs.txt,
+    # download the check script and run it
-    # we have a problem.
+    # FIXME: change this to main when the PR is merged
-    if (script_run 'comm -23 /tmp/installedupdatepkgs.txt /mnt/updatepkgs.txt > /mnt/installednotupdatedpkgs.txt') {
+    assert_script_run 'curl -o updvercheck.py https://pagure.io/fedora-qa/os-autoinst-distri-fedora/raw/better-check-nonmatching/f/updvercheck.py', timeout => 120;
-        # occasionally, for some reason, it's unhappy about sorting;
+    my $advisory = get_var("ADVISORY");
-        # we shouldn't fail the test in this case, just make a note
+    my $cmd = 'python3 ./updvercheck.py /mnt/updatepkgs.txt /tmp/installedupdatepkgs.txt';
-        # of it so we can look why...
+    $cmd .= " $advisory" if ($advisory);
-        diag "Installed vs. all update package comparison unexpectedly returned non-zero!";
+    my $ret = script_run $cmd;
+    # 2 is warnings only, 3 is problems, 1 means the script died in
+    # some other way (probably a bug)
+    if ($ret == 2) {
+        record_soft_failure "Some update package(s) not installed, but this is probably OK, see script output";
     }
-    # this exits 1 if the file is zero-length, 0 if it's longer
+    if ($ret == 1 || $ret == 3) {
-    # if it's 0, that's *BAD*: we want to upload the file and fail
+        my $message = "Package(s) from update not installed when it should have been! See script output";
-    unless (script_run 'test -s /mnt/installednotupdatedpkgs.txt') {
+        $message = "Script failed unexpectedly!" if ($ret == 1);
-        upload_logs "/mnt/installednotupdatedpkgs.txt", failok => 1;
-        my $message = "Package(s) from update not installed when it should have been! See installednotupdatedpkgs.txt";
         if ($args{fatal}) {
             set_var("_ACNMP_DONE", "1");
             die $message;

tox.ini

@@ -1,21 +1,27 @@
 [tox]
 skipsdist = True
-envlist = py37,py38,py39,py310
+envlist = py37,py38,py39,py310,py311,coverage-report
 skip_missing_interpreters = true
 [testenv]
 deps =
     pytest
     jsonschema
     coverage
-    diff-cover
-    pylint
     pytest-cov
-
 commands=
     python ./fifloader.py --clean templates.fif.json templates-updates.fif.json
     python ./check-needles.py
     py.test unittests/
     py.test --cov-report term-missing --cov-report xml --cov fifloader unittests/
+setenv =
+    PYTHONPATH = {toxinidir}
+
+[testenv:coverage-report]
+skip_install = true
+deps =
+    diff-cover
+    pylint
+commands =
     diff-cover coverage.xml --fail-under=90 --compare-branch=origin/main
     diff-quality --violations=pylint --fail-under=90 --compare-branch=origin/main
 setenv =

updvercheck.py

@@ -0,0 +1,96 @@
+#!/usr/bin/python3
+
+"""This script does a version comparison between the versions of packages
+from the update under test and the installed versions of the same-named
+packages, for whichever packages from the update are installed. It expects
+input data that's generated in advisory_check_nonmatching_packages and
+advisory_get_installed_packages; you can run it manually against
+updatepkgs.txt and installedupdatepkgs.txt logs from a completed test run
+if you need to test it or something."""
+
+# no, pylint, global scope variable names in a script like this aren't
+# really "constants"
+# pylint:disable=invalid-name
+
+import json
+import sys
+from urllib.request import urlopen
+
+import rpm  # pylint:disable=import-error
+
+def printver(pname, pepoch, pversion, prelease):
+    """Print a NEVR in the typical human-readable format."""
+    return f"{pname}-{pepoch}:{pversion}-{prelease}"
+
+try:
+    updfname = sys.argv[1]
+    instfname = sys.argv[2]
+except IndexError:
+    sys.exit("Must specify two input filenames!")
+try:
+    updalias = sys.argv[3]
+except IndexError:
+    updalias = None
+
+
+updpkgs = {}
+with open(updfname, "r", encoding="utf-8") as ufh:
+    for uline in ufh.readlines():
+        (_, name, epoch, version, release) = uline.strip().split(" ")
+        updpkgs[name] = (epoch, version, release)
+
+problems = []
+warnings = []
+post = set()
+ret = 0
+updstable = None
+with open(instfname, "r", encoding="utf-8") as ifh:
+    for iline in ifh.readlines():
+        (_, name, epoch, version, release) = iline.strip().split(" ")
+        res = rpm.labelCompare((epoch, version, release), (updpkgs[name]))
+        if res == 0:
+            continue
+        instver = printver(name, epoch, version, release)
+        updver = printver(name, *updpkgs[name])
+        if res < 0:
+            problems.append(f"Installed: {instver} is older than update: {updver}.")
+            post.add("Installed older than update usually means there is a dependency problem preventing the update version being installed.")
+            post.add("Check /var/log/dnf.log, and search for the string 'Problem'.")
+        else:
+            msg = f"Installed: {instver} is newer than update: {updver}"
+            if not updalias:
+                problems.append(f"{msg} and this is not an update test, please check if this is a problem")
+                continue
+            # check if the update is stable
+            if updstable is None:
+                try:
+                    url = f"https://bodhi.fedoraproject.org/updates/{updalias}"
+                    resp = json.loads(urlopen(url).read().decode("utf8"))   # pylint:disable=consider-using-with
+                    updstable = (resp.get("update", {}).get("status", "") == "stable")
+                except: # pylint:disable=bare-except
+                    problems.append(f"{msg} and Bodhi is unreachable.")
+                    continue
+            if updstable:
+                warnings.append(f"{msg} and update is stable, this is probably OK.")
+            else:
+                problems.append(f"{msg} and update is not stable.")
+                post.add("Installed newer than update and update not stable means older version could be pushed over newer if update goes stable.")
+
+if warnings:
+    print("WARNINGS")
+    for warning in warnings:
+        print(warning)
+    ret = 2
+
+if problems:
+    print("PROBLEMS")
+    for problem in problems:
+        print(problem)
+    ret = 3
+
+if post:
+    print("INVESTIGATION TIPS")
+    for postmsg in post:
+        print(postmsg)
+
+sys.exit(ret)