Merge ~tsimonq2/autopkgtest-cloud/+git/bug-1654761:disallow-duplicate-tests into autopkgtest-cloud:master

[Simon Quigley]

This is an IN PROGRESS merge request. I would like a review before I proceed further to make sure I'm on the right track.

This change fixes bug 1654761 and supersedes https://code.launchpad.net/~tsimonq2/autopkgtest-cloud/+git/bug-1654761/+merge/348972 - see the most recent review comment for why this code is structured how it is (including the file I used to test this code).

Left to do:
 - Write tests.
 - Implement support for reading the AMQP queue.

From the research that I have done, reading from the AMQP queue will have to be done by:
 - Declaring the queue with no autoacks.
 - Read the contents of the queue.
 - Disconnect without ACKing.

However, I am not entirely sure this is the correct line of action here. From reading through the code, there seems to be a queues.json file I could use. If that is the case, I would need an example file when it has some content to base my code off of. Any advice is appreciated here.

I tested this code by running the following (using the pastebin Iain linked in the last MP):
from inprogress import InProgress
inprogress = InProgress()
inprogress.is_test_running("/tmp/running.json", "linux", "cosmic", "amd64", ["pciutils/1:3.5.2-1ubuntu2"], ["ci-train-ppa-service/stable-phone-overlay", "ci-train-ppa-service/3343"])

[Steve Langasek (vorlon)]

does this take care to exclude requester name from the data being compared?

review: Needs Information

[Simon Quigley (tsimonq2)]

Yes.

In the code comments for is_test_running, the only requester information that is actually iterated on is REQUESTINFO, so the semicolon-separated string with job information in it. It seemed to be much easier to treat that simply as a unique identifier than as a string we actually parse.

When the subdictionaries are being recursed through, the requester key is not even considered.

Let me know if you have any further questions.

[Łukasz Zemczak (sil2100)]

Ok, this still needs some work IMO. Also, I don't think I'd feel safe having this merged without any tests - we need to make sure it all works as expected. Especially when writing code without a good way of testing it manually.

Even though I also don't feel strongly about us parsing the running.json file, I guess that's an acceptable approach. The file is short-lived, yes, but it's quite atomic, so there shouldn't be any corruption while reading it. And I don't see any easier way of getting the running pieces. Well, we might also try to hook up into the AMQP queues like amqp-status-collector does, but that seems more complex and also possibly wasteful? That being said, I'm not an expert in the AMQP API so maybe there's an easier way.

Still, this seems like a good way forward, but it needs more work (and refactoring). Please see my inline comments about things that need fixing.

review: Needs Fixing

ffe754d... by Simon Quigley on 2020-06-08

Stylistic cleanups.

[Simon Quigley (tsimonq2)]

Wow, it's been too long since I've looked at this. I promise, my Python isn't as bad these days as it was a year and a half (or so) go.

The changes sil2100 requested have been made. I'm looking into tests now.

[Brian Murray (brian-murray)]

It looks to me like this only checks the running.json file and not the queues.json file in which case the code doesn't do everything it purports to. I'm currently looking at queues.json (which is quite large for reasons), which has a shocking number of duplicate requests and think that queues.json is the real pain point here not running.json. Is this MP something you want to finish Simon?

review: Needs Fixing

[Brian Murray (brian-murray)]

Also I'm less concerned about unit testing here as functional testing should reveal an issue pretty quickly and we can build a good escape hatch.

[Brian Murray (brian-murray)]

We ended up implementing this, including checking queues.json, a bit ago and have deployed it to production.

Unmerged commits

ffe754d... by Simon Quigley on 2020-06-08

Stylistic cleanups.

72b99f5... by Simon Quigley on 2019-02-25

Initial InProgress functionality.

diff --git a/webcontrol/request/inprogress.py b/webcontrol/request/inprogress.py
 new file mode 100644
index 0000000..6f7c964
--- /dev/null
+++ b/webcontrol/request/inprogress.py
@@ -0,0 +1,62 @@
+"""Check if a specified test is already queued or running
+
+Copyright: Simon Quigley <[e-mail]>
+This file is licensed under the same license as the rest of this program.
+"""
+
+import os
+import json
+
+
+def extract_running_data(rdata):
+    """Try to extract the data about currently-running tests
+
+    If this returns None, that means there is no running data, otherwise
+    the actual running data is returned
+    """
+    try:
+        with open(rdata) as data:
+            return json.load(data)
+    except FileNotFoundError:
+        return None
+
+def test_is_running(runninguri, package, release, arch, triggers, ppas=[]):
+    """Determine if a test is currently running
+
+    This function searches the queue for the specified package. If it is
+    found, True is returned, with False being returned if it is not found.
+
+    The json file passed to this function is organized in this format:
+    {"PACKAGENAME": {"REQUESTINFO": {QUEUE INFO AND LOG TAIL}}}
+
+    PACKAGENAME is defined as the name of the package being tested.
+    REQUESTINFO is identifying information for the request. It uses
+    information like any triggers, PPA names, environment variables, etc.
+    QUEUE INFO AND LOG TAIL is several dictionaries nested within each
+    other containing the series name, architecture, and then build-specific
+    information within that.
+    """
+
+    # Load the JSON file for the currently-running tests
+    running_data = extract_running_data(runninguri)
+    if not running_data or not package in running_data:
+        return False
+
+    # If any bit of the data passed to us in the arguments doesn't match
+    # the data in the JSON file, return False
+    for request in running_data[package]:
+        try:
+            metadata = running_data[package][request][release][arch][0]
+
+            # Convert any of the lists to sets to ensure that any out
+            # of order list elements are not treated as different
+            if set(metadata["triggers"]) != set(triggers):
+                return False
+            elif ppas and "ppas" in metadata and \
+                    set(metadata["ppas"]) != set(ppas):
+                return False
+        except KeyError:
+            return False
+
+    # Everything has passed, return True
+    return True
diff --git a/webcontrol/request/submit.py b/webcontrol/request/submit.py
index 8504127..10b8c32 100644
--- a/webcontrol/request/submit.py
+++ b/webcontrol/request/submit.py
@@ -14,6 +14,7 @@ import urllib.request
 import urllib.parse
 from urllib.error import HTTPError
 from datetime import datetime
+from inprogress import test_is_running
 
 import amqplib.client_0_8 as amqp
 
@@ -30,6 +31,8 @@ ALLOWED_TEAMS = ['canonical-kernel-distro-team']
 # not teams
 ALLOWED_USERS_PERPACKAGE = {'snapcraft': ['snappy-m-o']}
 
+RUNNING = "/tmp/running.json"
+
 
 class Submit:
     def __init__(self):
@@ -138,6 +141,11 @@ class Submit:
                              'Ubuntu, thus you are not allowed to use this '
                              'service.' % (package, trigsrc))
 
+        # Verify that the test is not already in progress
+        if test_is_running(RUNNING, package, release, arch, triggers, ppas):
+            raise ValueError('Test is already in progress, please wait until '
+                             'it is complete before queuing it again.')
+
     def validate_git_request(self, release, arch, package, ppas=[], env=[], **kwargs):
         """Validate parameters for an upstream git test request