When removing the dependency on sudo in our CI Makefile, I realized
that the list of images that we pull contains some old images that we
no longer support in lcitool, but can be still pulled from the
registry. This is a simple checker which runs in context of the refresh
script which informs the developer/maintainer that there are some old
redundant images that should be purged from the registry, so that
people don't try to run local builds in those (which will most likely
fail anyway).

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 ci/containers/check-registry.py | 96 +++++++++++++++++++++++++++++++++
 ci/containers/refresh           |  5 ++
 2 files changed, 101 insertions(+)
 create mode 100644 ci/containers/check-registry.py

diff --git a/ci/containers/check-registry.py b/ci/containers/check-registry.py
new file mode 100644
index 0000000000..f0159da54d
--- /dev/null
+++ b/ci/containers/check-registry.py
@@ -0,0 +1,96 @@
+#!/usr/bin/env python3
+
+import argparse
+import subprocess
+import shutil
+import sys
+
+import util
+
+
+def get_image_distro(image_name: str):
+    name_prefix = "ci-"
+    name_suffix = "-cross-"
+
+    distro = image_name[len(name_prefix):]
+
+    index = distro.find(name_suffix)
+    if index > 0:
+        distro = distro[:index]
+
+    return distro
+
+
+def get_undesirables(registry_distros_d, hosts_l):
+    """ Returns a dictionary of 'id':'name' pairs of images that can be purged"""
+
+    undesirables_d = {}
+    for distro in registry_distros_d:
+        if distro not in hosts_l:
+            for image in registry_distros_d[distro]:
+                undesirables_d[str(image["id"])] = image["path"]
+
+    return undesirables_d if undesirables_d else None
+
+
+def get_lcitool_hosts(lcitool_path):
+    """ Returns a list of supported hosts by lcitool
+        @param lcitool_path: absolute path to local copy of lcitool, if omitted it is
+        assumed lcitool is installed in the current environment"""
+
+    if not lcitool_path:
+        lcitool_path = "lcitool"
+        if not shutil.which(lcitool_path):
+            sys.exit("error: 'lcitool' not installed")
+
+    lcitool_out = subprocess.check_output([lcitool_path, "hosts"])
+    return lcitool_out.decode("utf-8").splitlines()
+
+
+def main():
+    parser = argparse.ArgumentParser()
+    parser.add_argument("project_id",
+                        help="GitLab project ID")
+    parser.add_argument("--lcitool_path",
+                        dest="lcitool",
+                        metavar="PATH",
+                        help="absolute path to lcitool, CWD is used if omitted")
+
+    args = parser.parse_args()
+
+    uri = util.get_registry_uri(util.PROJECT_ID)
+    images_json = util.list_images(uri + "?per_page=100")
+
+    registry_distros_d = {}
+    for image in images_json:
+        distro_name = get_image_distro(image["name"])
+
+        try:
+            registry_distros_d[distro_name].append(image)
+        except KeyError:
+            registry_distros_d[distro_name] = [image]
+
+    hosts_l = get_lcitool_hosts(args.lcitool)
+
+    # print the list of images that we can safely purge from the registry
+    undesirables_d = get_undesirables(registry_distros_d, hosts_l)
+    if undesirables_d:
+        undesirable_image_names = "\t" + "\n\t".join(undesirables_d.values())
+        undesirable_image_ids = " ".join(undesirables_d.keys())
+
+        sys.exit(f"""
+The following images can be purged from the registry:
+
+{undesirable_image_names}
+
+You can remove the above images over the API with the following code snippet:
+
+\t$ for image_id in {undesirable_image_ids} \\
+\t;do \\
+\t\tcurl --request DELETE --header "PRIVATE-TOKEN: <access_token>" \\
+\t\t{util.get_registry_uri(util.PROJECT_ID)}/$image_id \\
+\t;done""")
+
+
+if __name__ == "__main__":
+    main()
diff --git a/ci/containers/refresh b/ci/containers/refresh
index a39d0b0996..9dfaf75300 100755
--- a/ci/containers/refresh
+++ b/ci/containers/refresh
@@ -40,3 +40,8 @@ do
 
     $LCITOOL dockerfile $host libvirt > ci-$host.Dockerfile
 done
+
+# Check whether there are some old images in the container registry that could
+# be purged - mainly because the list-images script would list them as
+# available
+/usr/bin/env python3 check-registry.py --lcitool_path "$LCITOOL" 192693
-- 
2.29.2

On Wed, 2021-02-10 at 18:24 +0100, Erik Skultety wrote:
> +def get_undesirables(registry_distros_d, hosts_l):
> +    """ Returns a dictionary of 'id':'name' pairs of images that can be purged"""

Is the var_d and var_l a Python convention that I'm not aware of? I
don't think we're using it anywhere in either libvirt or libvirt-ci,
so I'd rather avoid it here as well.

I see you're using some type hints for the get_image_distro()
function, though. Can we perhaps have more of that?

Suggestion: "stale" is both shorter and harder to misspell than
"undesirable" :)

> +def main():
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument("project_id",
> +                        help="GitLab project ID")

Is passing the project ID necessary? We're hardcoding the one for
libvirt/libvirt in util.py and we don't provide a way to override it
for ci-list-images, so I think this can be skipped.

Having a way for the developer to point to their own fork rather than
libvirt/libvirt makes sense, but in that case we probably also want
to make it so the user passes "myusername/libvirt" and we convert
that to a project ID using the GitLab API under the hood.

> +    parser.add_argument("--lcitool_path",
> +                        dest="lcitool",
> +                        metavar="PATH",
> +                        help="absolute path to lcitool, CWD is used if omitted")

Underscores in command line arguments are inconvenient and ugly, so
please don't use them. You can just call the option "--lcitool".

Regarding the documentation for the option, I'm not convinced it's
accurate: AFAICT shutil.which() will use $PATH, not $CWD, to look for
the named binary if an absolute path to it has not been provided.

> +    uri = util.get_registry_uri(util.PROJECT_ID)
> +    images_json = util.list_images(uri + "?per_page=100")

We have "?per_page=100" hardcoded in two places now, so it looks like
we can probably just fold its usage into list_images()?

> +        sys.exit(f"""
> +The following images can be purged from the registry:
> +
> +{undesirable_image_names}

I would like to see the image ID printed along with the image name,
so that I can tweak the shell snippet below to selectively remove
only *some* of the images.

> +You can remove the above images over the API with the following code snippet:
> +
> +\t$ for image_id in {undesirable_image_ids} \\
> +\t;do \\

Having the "do" on a separate line is weird.

Also please indent using either two or four spaces rather than tabs.


Aside from the (mostly minor) issues that I've pointed out, I think
these patches go in the right direction. However, reviewing them made
me wonder what the long-term status of these scripts should be.

Right now we have the following entry points:

  * a Makefile, which can be used to kick off local builds;

  * a couple of 'refresh' shell script that are used to regenerate
    contents derived from the libvirt-ci source of truth;

  * some Python bits that are used by both.

We also know that, at some point in the hopefully not too distant
future, we'll have the results of

  https://gitlab.com/libvirt/libvirt-ci/-/merge_requests/59

available, and that will allow us to turn the refresh scripts into
trivial wrappers.

I think it would make sense to converge towards having a single
Python based entry point, which serves as the counterpart of lcitool,
and which can be used to do all of the above.

We can even get there gradually: the first version of the Python
script could just call out to the Makefile to start builds, and then
we could reimplement the logic in Python at a later point in time.

What do you think?

-- 
Andrea Bolognani / Red Hat / Virtualization

On Tue, 2021-02-16 at 16:28 +0100, Andrea Bolognani wrote:
> I think it would make sense to converge towards having a single
> Python based entry point, which serves as the counterpart of lcitool,
> and which can be used to do all of the above.
> 
> We can even get there gradually: the first version of the Python
> script could just call out to the Makefile to start builds, and then
> we could reimplement the logic in Python at a later point in time.

Initial, pretty rough implementation of this idea:

  https://listman.redhat.com/archives/libvir-list/2021-February/msg00900.html

-- 
Andrea Bolognani / Red Hat / Virtualization