Merge ~ubuntu-release/britney/+git/britney2-ubuntu:sil2100/cloud-only-run-once into ~ubuntu-release/britney/+git/britney2-ubuntu:sil2100/security-britney

Proposed by Łukasz Zemczak
Status: Merged
Merged at revision: 85c6716af6355f9f034d6dc0e656606f3eada113
Proposed branch: ~ubuntu-release/britney/+git/britney2-ubuntu:sil2100/cloud-only-run-once
Merge into: ~ubuntu-release/britney/+git/britney2-ubuntu:sil2100/security-britney
Diff against target: 277 lines (+145/-7)
3 files modified
britney.conf (+3/-0)
britney2/policies/cloud.py (+90/-5)
tests/test_cloud.py (+52/-2)
Reviewer Review Type Date Requested Status
John Chittum (community) Approve
Ubuntu Release Team Pending
Review via email: mp+437274@code.launchpad.net

Commit message

Save state file in-between runs to mark which packages have already been tested on the given clouds.

Description of the change

Save state file in-between runs to mark which packages have already been tested on the given clouds. Without this, every britney invocation would re-run the same tests over and over again, even though the packages did not change.

We could use some of the ADT databases for this instead of our own state file, but this way we can function regardless of whether autopkgtest policies are enabled.

I'd like to add more unit-testing, but had no time as I should be AFK right now.

To post a comment you must log in.
Revision history for this message
John Chittum (jchittum) wrote :

from a general code level, I don't see any issues.

review: Approve
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Thanks! I'll merge it + check Aleksa's changes on top.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/britney.conf b/britney.conf
2index 40fd523..d27e968 100644
3--- a/britney.conf
4+++ b/britney.conf
5@@ -143,6 +143,9 @@ CLOUD_ENABLE = no
6 # A directory to store Cloud test results and logs. Is created at the start of
7 # each policy run and deleted after test results are parsed.
8 CLOUD_WORK_DIR = cloud_tests
9+# Path to a persistent state file, not to re-run cloud tests for packages that
10+# have already been tested.
11+CLOUD_STATE_FILE = cloud_state
12 # Who to notify regarding test failures
13 CLOUD_FAILURE_EMAILS = cpc@canonical.com
14 # Who to notify regarding test errors
15diff --git a/britney2/policies/cloud.py b/britney2/policies/cloud.py
16index b19fbae..41da821 100644
17--- a/britney2/policies/cloud.py
18+++ b/britney2/policies/cloud.py
19@@ -50,6 +50,7 @@ Regards, Ubuntu Release Team.
20 """
21 class CloudPolicy(BasePolicy):
22 PACKAGE_SET_FILE = "cloud_package_set"
23+ STATE_FILE = "cloud_state"
24 DEFAULT_EMAILS = ["cpc@canonical.com"]
25 TEST_LOG_FILE = "CTF.log"
26
27@@ -68,6 +69,9 @@ class CloudPolicy(BasePolicy):
28 self.work_dir = getattr(self.options, "cloud_work_dir", "cloud_tests")
29 self.failure_emails = getattr(self.options, "cloud_failure_emails", self.DEFAULT_EMAILS)
30 self.error_emails = getattr(self.options, "cloud_error_emails", self.DEFAULT_EMAILS)
31+ self.state_filename = getattr(self.options, "cloud_state_file", self.STATE_FILE)
32+
33+ self.state = {}
34
35 adt_ppas = getattr(self.options, "adt_ppas", "")
36 if not isinstance(adt_ppas, list):
37@@ -83,11 +87,13 @@ class CloudPolicy(BasePolicy):
38
39 self.failures = {}
40 self.errors = {}
41+ self.email_needed = False
42
43 def initialise(self, britney):
44 super().initialise(britney)
45
46 self.package_set = self._retrieve_cloud_package_set_for_series(self.options.series)
47+ self._load_state()
48
49 def apply_src_policy_impl(self, policy_info, item, source_data_tdist, source_data_srcdist, excuse):
50 if item.package not in self.package_set:
51@@ -103,10 +109,12 @@ class CloudPolicy(BasePolicy):
52 self.failures = {}
53 self.errors = {}
54
55- self._run_cloud_tests(item.package, self.options.series, self.sources, self.source_type)
56+ self._run_cloud_tests(item.package, source_data_srcdist.version, self.options.series,
57+ self.sources, self.source_type)
58
59 if len(self.failures) > 0 or len(self.errors) > 0:
60- self._send_emails_if_needed(item.package, source_data_srcdist.version, self.options.series)
61+ if self.email_needed:
62+ self._send_emails_if_needed(item.package, source_data_srcdist.version, self.options.series)
63
64 self._cleanup_work_directory()
65 verdict = PolicyVerdict.REJECTED_PERMANENTLY
66@@ -117,6 +125,76 @@ class CloudPolicy(BasePolicy):
67 self._cleanup_work_directory()
68 return PolicyVerdict.PASS
69
70+ def _mark_tests_run(self, package, version, series, source_type, cloud):
71+ """Mark the selected package version as already tested.
72+ This takes which cloud we're testing into consideration.
73+
74+ :param package The name of the package to test
75+ :param version Version of the package
76+ :param series The Ubuntu codename for the series (e.g. jammy)
77+ :param source_type Either 'archive' or 'ppa'
78+ :param cloud The name of the cloud being tested (e.g. azure)
79+ """
80+ if cloud not in self.state:
81+ self.state[cloud] = {}
82+ if source_type not in self.state[cloud]:
83+ self.state[cloud][source_type] = {}
84+ if series not in self.state[cloud][source_type]:
85+ self.state[cloud][source_type][series] = {}
86+ self.state[cloud][source_type][series][package] = {
87+ "version": version,
88+ "failures": len(self.failures),
89+ "errors": len(self.errors)
90+ }
91+
92+ self.email_needed = True
93+
94+ self._save_state()
95+
96+ def _check_if_tests_run(self, package, version, series, source_type, cloud):
97+ """Check if tests were already run for the given package version.
98+ This takes which cloud we're testing into consideration.
99+
100+ :param package The name of the package to test
101+ :param version Version of the package
102+ :param series The Ubuntu codename for the series (e.g. jammy)
103+ :param source_type Either 'archive' or 'ppa'
104+ :param cloud The name of the cloud being tested (e.g. azure)
105+ """
106+ try:
107+ return self.state[cloud][source_type][series][package]["version"] == version
108+ except KeyError:
109+ return False
110+
111+ def _set_previous_failure_and_error(self, package, version, series, source_type, cloud):
112+ """Sets the failures and errors from the previous run.
113+ This takes which cloud we're testing into consideration.
114+
115+ :param package The name of the package to test
116+ :param version Version of the package
117+ :param series The Ubuntu codename for the series (e.g. jammy)
118+ :param source_type Either 'archive' or 'ppa'
119+ :param cloud The name of the cloud being tested (e.g. azure)
120+ """
121+ if self.state[cloud][source_type][series][package]["failures"] > 0:
122+ self.failures[cloud] = {}
123+
124+ if self.state[cloud][source_type][series][package]["errors"] > 0:
125+ self.errors[cloud] = {}
126+
127+ def _load_state(self):
128+ """Load the save state of which packages have already been tested."""
129+ if os.path.exists(self.state_filename):
130+ with open(self.state_filename, encoding="utf-8") as data:
131+ self.state = json.load(data)
132+ self.logger.info("Loaded cloud policy state file %s" % self.state_filename)
133+
134+ def _save_state(self):
135+ """Save which packages have already been tested."""
136+ with open(self.state_filename, "w", encoding="utf-8") as data:
137+ json.dump(self.state, data)
138+ self.logger.info("Saved cloud policy state file %s" % self.state_filename)
139+
140 def _retrieve_cloud_package_set_for_series(self, series):
141 """Retrieves a set of packages for the given series in which cloud
142 tests should be run.
143@@ -134,16 +212,17 @@ class CloudPolicy(BasePolicy):
144
145 return package_set
146
147- def _run_cloud_tests(self, package, series, sources, source_type):
148+ def _run_cloud_tests(self, package, version, series, sources, source_type):
149 """Runs any cloud tests for the given package.
150 Nothing is returned but test failures and errors are stored in instance variables.
151
152 :param package The name of the package to test
153+ :param version Version of the package
154 :param series The Ubuntu codename for the series (e.g. jammy)
155 :param sources List of sources where the package should be installed from (e.g. [proposed] or PPAs)
156 :param source_type Either 'archive' or 'ppa'
157 """
158- self._run_azure_tests(package, series, sources, source_type)
159+ self._run_azure_tests(package, version, series, sources, source_type)
160
161 def _send_emails_if_needed(self, package, version, series):
162 """Sends email(s) if there are test failures and/or errors
163@@ -168,14 +247,19 @@ class CloudPolicy(BasePolicy):
164 self.logger.info("Cloud Policy: Sending error email for {}, to {}".format(package, emails))
165 self._send_email(emails, message)
166
167- def _run_azure_tests(self, package, series, sources, source_type):
168+ def _run_azure_tests(self, package, version, series, sources, source_type):
169 """Runs Azure's required package tests.
170
171 :param package The name of the package to test
172+ :param version Version of the package
173 :param series The Ubuntu codename for the series (e.g. jammy)
174 :param sources List of sources where the package should be installed from (e.g. [proposed] or PPAs)
175 :param source_type Either 'archive' or 'ppa'
176 """
177+ if self._check_if_tests_run(package, version, series, source_type, "azure"):
178+ self._set_previous_failure_and_error(package, version, series, source_type, "azure")
179+ return
180+
181 urn = self._retrieve_urn(series)
182
183 self.logger.info("Cloud Policy: Running Azure tests for: {} in {}".format(package, series))
184@@ -204,6 +288,7 @@ class CloudPolicy(BasePolicy):
185 results_file_paths = self._find_results_files(r"TEST-NetworkTests-[0-9]*.xml")
186 self._parse_xunit_test_results("Azure", results_file_paths)
187 self._store_extra_test_result_info(self, package)
188+ self._mark_tests_run(package, version, series, source_type, "azure")
189
190 def _retrieve_urn(self, series):
191 """Retrieves an URN from the configuration options based on series.
192diff --git a/tests/test_cloud.py b/tests/test_cloud.py
193index bbd8595..2e859e7 100644
194--- a/tests/test_cloud.py
195+++ b/tests/test_cloud.py
196@@ -7,6 +7,7 @@
197 # (at your option) any later version.
198
199 import os
200+import json
201 import pathlib
202 import sys
203 from types import SimpleNamespace
204@@ -35,7 +36,8 @@ class T(unittest.TestCase):
205 verbose = False,
206 cloud_source = "zazzy-proposed",
207 cloud_source_type = "archive",
208- cloud_azure_zazzy_urn = "fake-urn-value"
209+ cloud_azure_zazzy_urn = "fake-urn-value",
210+ cloud_state_file = "/tmp/test_state.json"
211 )
212 self.policy = CloudPolicy(self.fake_options, {})
213 self.policy._setup_work_directory()
214@@ -43,6 +45,54 @@ class T(unittest.TestCase):
215 def tearDown(self):
216 self.policy._cleanup_work_directory()
217
218+ @patch("britney2.policies.cloud.CloudPolicy._store_extra_test_result_info")
219+ @patch("britney2.policies.cloud.CloudPolicy._parse_xunit_test_results")
220+ @patch("subprocess.run")
221+ def test_run_cloud_tests_state_handling(self, mock_run, mock_xunit, mock_extra):
222+ """Cloud tests should save state and not re-run tests for packages
223+ already tested."""
224+ expected_state = {
225+ "azure": {
226+ "archive": {
227+ "zazzy": {
228+ "chromium-browser": {
229+ "version": "55.0",
230+ "failures": 0,
231+ "errors": 0,
232+ }
233+ }
234+ }
235+ }
236+ }
237+ with open(self.policy.options.cloud_state_file, "w") as file:
238+ json.dump(expected_state, file)
239+ self.policy._load_state()
240+
241+ # Package already tested, no tests should run
242+ self.policy._run_cloud_tests("chromium-browser", "55.0", "zazzy", ["proposed"], "archive")
243+ self.assertDictEqual(expected_state, self.policy.state)
244+ mock_run.assert_not_called()
245+
246+ # A new package appears, tests should run
247+ expected_state["azure"]["archive"]["zazzy"]["hello"] = {
248+ "version": "2.10",
249+ "failures": 0,
250+ "errors": 0,
251+ }
252+ self.policy._run_cloud_tests("hello", "2.10", "zazzy", ["proposed"], "archive")
253+ self.assertDictEqual(expected_state, self.policy.state)
254+ mock_run.assert_called()
255+
256+ # A new version of existing package, tests should run
257+ expected_state["azure"]["archive"]["zazzy"]["chromium-browser"]["version"] = "55.1"
258+ self.policy._run_cloud_tests("chromium-browser", "55.1", "zazzy", ["proposed"], "archive")
259+ self.assertDictEqual(expected_state, self.policy.state)
260+ self.assertEqual(mock_run.call_count, 2)
261+
262+ # Make sure the state was saved properly
263+ with open(self.policy.options.cloud_state_file, "r") as file:
264+ self.assertDictEqual(expected_state, json.load(file))
265+
266 @patch("britney2.policies.cloud.CloudPolicy._run_cloud_tests")
267 def test_run_cloud_tests_called_for_package_in_manifest(self, mock_run):
268 """Cloud tests should run for a package in the cloud package set.
269@@ -55,7 +105,7 @@ class T(unittest.TestCase):
270 )
271
272 mock_run.assert_called_once_with(
273- "chromium-browser", "jammy", ["proposed"], "archive"
274+ "chromium-browser", "55.0", "jammy", ["proposed"], "archive"
275 )
276
277 @patch("britney2.policies.cloud.CloudPolicy._run_cloud_tests")

Subscribers

People subscribed via source and target branches