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

Proposed by Simon Quigley on 2019-02-25
Status: Needs review
Proposed branch: ~tsimonq2/autopkgtest-cloud/+git/bug-1654761:disallow-duplicate-tests
Merge into: autopkgtest-cloud:master
Diff against target: 113 lines (+83/-0)
2 files modified
webcontrol/request/inprogress.py (+74/-0)
webcontrol/request/submit.py (+9/-0)
Reviewer Review Type Date Requested Status
Łukasz Zemczak Needs Fixing on 2019-07-03
Steve Langasek Needs Information on 2019-02-26
Iain Lane 2019-02-25 Pending
Review via email: mp+363643@code.launchpad.net

Description of the change

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"])

To post a comment you must log in.
Steve Langasek (vorlon) wrote :

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

review: Needs Information
Simon Quigley (tsimonq2) wrote :

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) wrote :

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

Unmerged commits

f977d84... by Simon Quigley on 2019-02-25

Initial InProgress functionality.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/webcontrol/request/inprogress.py b/webcontrol/request/inprogress.py
2new file mode 100644
3index 0000000..25d6415
4--- /dev/null
5+++ b/webcontrol/request/inprogress.py
6@@ -0,0 +1,74 @@
7+"""Check if a specified test is already queued or running
8+
9+Copyright: Simon Quigley <tsimonq2@ubuntu.com>
10+This file is licensed under the same license as the rest of this program.
11+"""
12+
13+import os
14+import json
15+
16+
17+class InProgress:
18+ def extract_running_data(self, rdata):
19+ """Try to extract the data about currently-running tests
20+
21+ If this returns None, that means there is no running data, otherwise
22+ the actual running data is returned
23+ """
24+ try:
25+ with open(rdata) as data:
26+ return json.load(data)
27+ except FileNotFoundError:
28+ return None
29+
30+ def is_test_running(self, runninguri, package, release, arch, triggers,
31+ ppas=[]):
32+ """Determine if a test is currently running
33+
34+ This function searches the queue for the specified package. If it is
35+ found, True is returned, with False being returned if it is not found.
36+
37+ The json file passed to this function is organized in this format:
38+ {"PACKAGENAME": {"REQUESTINFO": {QUEUE INFO AND LOG TAIL}}}
39+
40+ PACKAGENAME is defined as the name of the package being tested.
41+ REQUESTINFO is identifying information for the request. It uses
42+ information like any triggers, PPA names, environment variables, etc.
43+ QUEUE INFO AND LOG TAIL is several dictionaries nested within each
44+ other containing the series name, architecture, and then build-specific
45+ information within that.
46+ """
47+
48+ # Load the JSON file for the currently-running tests
49+ running_data = self.extract_running_data(runninguri)
50+ if running_data is None:
51+ return False
52+
53+ if not package in running_data:
54+ return False
55+
56+ # If any bit of the data passed to us in the arguments doesn't match
57+ # the data in the JSON file, return False right away
58+ for request in running_data[package]:
59+ if not release in running_data[package][request]:
60+ return False
61+ if not arch in running_data[package][request][release]:
62+ return False
63+ metadata = running_data[package][request][release][arch][0]
64+ # Convert any of the lists to sets to ensure that any out
65+ # of order list elements are not treated as different
66+ if set(metadata["triggers"]) != set(triggers):
67+ return False
68+ elif ppas != [] and "ppas" in metadata and \
69+ set(metadata["ppas"]) != set(ppas):
70+ return False
71+
72+ # Everything has passed, return True
73+ return True
74+
75+ def test_in_progress(self, runninguri, package, release, arch, triggers,
76+ ppas=[]):
77+ if is_test_running(runninguri, package, release, arch, triggers, ppas):
78+ return True
79+ else:
80+ return False
81diff --git a/webcontrol/request/submit.py b/webcontrol/request/submit.py
82index 31eddd5..6b81e74 100644
83--- a/webcontrol/request/submit.py
84+++ b/webcontrol/request/submit.py
85@@ -13,6 +13,7 @@ import sqlite3
86 import urllib.request
87 import urllib.parse
88 from urllib.error import HTTPError
89+from inprogress import InProgress
90
91 import amqplib.client_0_8 as amqp
92
93@@ -29,6 +30,9 @@ ALLOWED_TEAMS = ['canonical-kernel-distro-team']
94 # not teams
95 ALLOWED_USERS_PERPACKAGE = {'snapcraft': ['snappy-m-o']}
96
97+INPROGRESS = InProgress()
98+RUNNING = "/tmp/running.json"
99+
100
101 class Submit:
102 def __init__(self):
103@@ -125,6 +129,11 @@ class Submit:
104 'Ubuntu, thus you are not allowed to use this '
105 'service.' % (package, trigsrc))
106
107+ # Verify that the test is not already in progress
108+ if test_in_progress(RUNNING, package, release, arch, triggers, ppas):
109+ raise ValueError('Test is already in progress, please wait until '
110+ 'it is complete before queuing it again.')
111+
112 def validate_git_request(self, release, arch, package, ppas=[], env=[], **kwargs):
113 """Validate parameters for an upstream git test request
114

Subscribers

People subscribed via source and target branches