From 06bfd2d2ae49533284094fadfe18437823e59ea4 Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Mon, 30 Jan 2023 17:37:06 -0800 Subject: [PATCH] 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 --- lib/utils.pm | 32 +++++++++-------- tox.ini | 14 +++++--- updvercheck.py | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 19 deletions(-) create mode 100644 updvercheck.py diff --git a/lib/utils.pm b/lib/utils.pm index 18de4a20..aa6833c5 100644 --- a/lib/utils.pm +++ b/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, - # we have a problem. - if (script_run 'comm -23 /tmp/installedupdatepkgs.txt /mnt/updatepkgs.txt > /mnt/installednotupdatedpkgs.txt') { - # occasionally, for some reason, it's unhappy about sorting; - # we shouldn't fail the test in this case, just make a note - # of it so we can look why... - diag "Installed vs. all update package comparison unexpectedly returned non-zero!"; + # download the check script and run it + # FIXME: change this to main when the PR is merged + 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; + my $advisory = get_var("ADVISORY"); + my $cmd = 'python3 ./updvercheck.py /mnt/updatepkgs.txt /tmp/installedupdatepkgs.txt'; + $cmd .= " $advisory" if ($advisory); + 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 it's 0, that's *BAD*: we want to upload the file and fail - unless (script_run 'test -s /mnt/installednotupdatedpkgs.txt') { - upload_logs "/mnt/installednotupdatedpkgs.txt", failok => 1; - my $message = "Package(s) from update not installed when it should have been! See installednotupdatedpkgs.txt"; + if ($ret == 1 || $ret == 3) { + my $message = "Package(s) from update not installed when it should have been! See script output"; + $message = "Script failed unexpectedly!" if ($ret == 1); if ($args{fatal}) { set_var("_ACNMP_DONE", "1"); die $message; diff --git a/tox.ini b/tox.ini index 260ef479..12d62693 100644 --- a/tox.ini +++ b/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 = diff --git a/updvercheck.py b/updvercheck.py new file mode 100644 index 00000000..1dde82ee --- /dev/null +++ b/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)