Merge ~andersson123/autopkgtest-cloud:no_queue_duplicate_requests into autopkgtest-cloud:master

[Tim Andersson]

[Tim Andersson (andersson123)]

Just going to quickly test this in staging then it's good to go.

[Tim Andersson (andersson123)]

Ready for re-review.

[Brian Murray (brian-murray)]

Is there any point in time where either json file will not exist or be empty? If the answer is yes or we aren't sure then the code should be modified to handle this situation as it currently looks like it'll produce a traceback in those situations.

Additionally, https://bugs.launchpad.net/auto-package-testing/+bug/1654761/comments/4 makes reference to adding the check to validate_distro_request() which makes sense to me given the other checks in that function.

Otherwise the content of the functions you've added looks great.

review: Needs Fixing

[Tim Andersson (andersson123)]

I added this too and removed it because I came to the conclusion if those two files don't exist, nothing will be working anyway and the problem will be elsewhere. It could almost be a helpful alarm bell.

[Tim Andersson (andersson123)]

Do you think we also need to add it to validate_git_request as well?

[Tim Andersson (andersson123)]

All amended based on your comments, tested and works as intended. Let me know what you think re validate_git_request. I think it should also go in there

[Brian Murray (brian-murray)]

> I added this too and removed it because I came to the conclusion if those two
> files don't exist, nothing will be working anyway and the problem will be
> elsewhere. It could almost be a helpful alarm bell.

What would the failure scenario look like though? Would the web service completely fall over preventing any requests from being accepted? If that's the case I'd rather accept any request, including a duplicate one, rather then having the service fail.

[Brian Murray (brian-murray)]

Please modify this to return a 403 instead of a 400 and yes I think it should be added to validate_git_request. We don't want duplicate tests queued regardless of the type.

review: Needs Fixing

[Tim Andersson (andersson123)]

Amended and rebased. What do you think about my exit code implementation?

[Brian Murray (brian-murray)]

> Amended and rebased. What do you think about my exit code implementation?

Looking at submit.py there are multiple cases where ValueError is used that seem more like a 400 than a 403, so if we were to look at a simple solution I'd suggest using TypeError for a duplicate request and return a 403 with that.

However, that doesn't feel very clean and I think we should instead look at using a custom exception so that we can more be more granular with our exit / return codes. (I thought I also saw some cases where we'd ideally return a 401 instead of a 400.)

In the interest of just getting this done though let's do the simple solution (or something like it) now and create a card, or bug, for improving the way in which we use http status codes.

[Tim Andersson (andersson123)]

I've added the check in validate_git_request, still not sure how to test though. Although I think the code is somewhat fool-proof.

[Brian Murray (brian-murray)]

I imagine we'll hear pretty quickly if validate_git_request is broken so I'm not too concerned about that. The changes look great and I was able to build the charm locally so the error here is a false positive.

review: Approve

diff --git a/charms/focal/autopkgtest-web/webcontrol/request/app.py b/charms/focal/autopkgtest-web/webcontrol/request/app.py
index c8791a5..beeb22d 100644
--- a/charms/focal/autopkgtest-web/webcontrol/request/app.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/app.py
@@ -9,7 +9,7 @@ from html import escape as _escape
 from flask import Flask, redirect, request, session
 from flask_openid import OpenID
 from helpers.utils import setup_key
-from request.submit import Submit
+from request.submit import DuplicateRequestException, Submit
 from werkzeug.middleware.proxy_fix import ProxyFix
 
 # map multiple GET vars to AMQP JSON request parameter list
@@ -236,6 +236,8 @@ def index_root():
             s.validate_distro_request(**params)
         except (ValueError, TypeError) as e:
             return invalid(e)
+        except DuplicateRequestException as e:
+            return invalid(e, 403)
 
         if params.get("delete"):
             del params["delete"]
diff --git a/charms/focal/autopkgtest-web/webcontrol/request/submit.py b/charms/focal/autopkgtest-web/webcontrol/request/submit.py
index 04d7f61..aeeb9a9 100644
--- a/charms/focal/autopkgtest-web/webcontrol/request/submit.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/submit.py
@@ -18,6 +18,11 @@ from urllib.error import HTTPError
 import amqplib.client_0_8 as amqp
 from distro_info import UbuntuDistroInfo
 
+
+class DuplicateRequestException(Exception):
+    pass
+
+
 # Launchpad REST API base
 LP = "https://api.launchpad.net/1.0/"
 NAME = re.compile("^[a-z0-9][a-z0-9.+-]+$")
@@ -31,6 +36,11 @@ ALLOWED_TEAMS = ["canonical-kernel-distro-team", "autopkgtest-requestors"]
 # not teams
 ALLOWED_USERS_PERPACKAGE = {"snapcraft": ["snappy-m-o"]}
 
+# Path to json file detailing the queue
+QUEUE_FP = "/var/lib/cache-amqp/queued.json"
+# Path to json file detailing the running tests
+RUNNING_FP = "/run/amqp-status-collector/running.json"
+
 
 class Submit:
     def __init__(self):
@@ -77,6 +87,12 @@ class Submit:
         Raise ValueError with error messsage if the request is invalid,
         otherwise return.
         """
+        not_needed, msg = self.is_request_queued_or_running(
+            release, arch, package, triggers
+        )
+        if not_needed:
+            raise DuplicateRequestException(msg)
+
         can_upload_any_trigger = False
 
         try:
@@ -213,6 +229,21 @@ class Submit:
         Raise ValueError with error messsage if the request is invalid,
         otherwise return.
         """
+        triggers = []
+        for env_var in env:
+            if "trigger" in env_var:
+                if "," in env_var:
+                    for trig in env_var.split("=")[1].split(","):
+                        triggers.append(trig)
+                else:
+                    triggers.append(env_var.split("=")[1])
+
+        not_needed, msg = self.is_request_queued_or_running(
+            release, arch, package, triggers
+        )
+        if not_needed:
+            raise DuplicateRequestException(msg)
+
         if release not in self.releases:
             raise ValueError("Unknown release " + release)
         if arch not in self.architectures:
@@ -509,3 +540,87 @@ class Submit:
             return (500, None)
         logging.debug("lp_request %s succeeded: %s", url, response)
         return (code, response)
+
+    def is_test_running(self, req_series, req_arch, req_package, req_triggers):
+        if not os.path.isfile(RUNNING_FP):
+            return False
+        data = {}
+        with open(RUNNING_FP, "r") as f:
+            data = json.load(f)
+        for pkg in data:
+            for submitted in data[pkg]:
+                releases = data[pkg][submitted].keys()
+                for release in data[pkg][submitted]:
+                    architectures = data[pkg][submitted][release].keys()
+                    triggers = submitted[submitted.find(";triggers_") + 1 :]
+                    triggers = triggers[: triggers.find(";")]
+                    triggers = triggers[
+                        triggers.find("[") + 1 : triggers.find("]")
+                    ]
+                    triggers = triggers.replace("'", "").split(", ")
+                    if (
+                        req_arch in architectures
+                        and req_series in releases
+                        and req_package == pkg
+                        and sorted(triggers) == sorted(req_triggers)
+                    ):
+                        return True
+        return False
+
+    def is_test_in_queue(
+        self, req_series, req_arch, req_package, req_triggers
+    ):
+        if not os.path.isfile(QUEUE_FP):
+            return False
+        data = {}
+        with open(QUEUE_FP, "r") as f:
+            data = json.load(f)
+        data = data["queues"]
+        this_test = {
+            "release": req_series,
+            "arch": req_arch,
+            "package": req_package,
+            "triggers": sorted(req_triggers),
+        }
+        for test_type in data:
+            for release in data[test_type]:
+                for arch in data[test_type][release]:
+                    packages = data[test_type][release][arch]
+                    if packages["size"] != 0:
+                        for req in packages["requests"]:
+                            pkg = req[: req.find("{")].rstrip()
+                            details = json.loads(req[req.find("{") :])
+                            triggers = details["triggers"]
+                            test = {
+                                "release": release,
+                                "arch": arch,
+                                "package": pkg,
+                                "triggers": sorted(triggers),
+                            }
+                            if test == this_test:
+                                return True
+        return False
+
+    def is_request_queued_or_running(
+        self, req_series, req_arch, req_package, req_triggers=[]
+    ):
+        if self.is_test_running(
+            req_series, req_arch, req_package, req_triggers
+        ):
+            o_msg = "Requested test is already running."
+            o_msg += "\nRelease: " + req_series
+            o_msg += "\nArchitecture: " + req_arch
+            o_msg += "\nPackage: " + req_package
+            o_msg += "\nTriggers: " + ",".join(req_triggers)
+            return True, o_msg
+
+        if self.is_test_in_queue(
+            req_series, req_arch, req_package, req_triggers
+        ):
+            o_msg = "Requested test is already queued."
+            o_msg += "\nRelease: " + req_series
+            o_msg += "\nArchitecture: " + req_arch
+            o_msg += "\nPackage: " + req_package
+            o_msg += "\nTriggers: " + ",".join(req_triggers)
+            return True, o_msg
+        return False, ""