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

[Łukasz Zemczak]

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

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.

[John Chittum (jchittum)]

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

review: Approve

[Łukasz Zemczak (sil2100)]

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

diff --git a/britney.conf b/britney.conf
index 40fd523..d27e968 100644
--- a/britney.conf
+++ b/britney.conf
@@ -143,6 +143,9 @@ CLOUD_ENABLE            = no
 # A directory to store Cloud test results and logs. Is created at the start of
 # each policy run and deleted after test results are parsed.
 CLOUD_WORK_DIR          = cloud_tests
+# Path to a persistent state file, not to re-run cloud tests for packages that
+# have already been tested.
+CLOUD_STATE_FILE        = cloud_state
 # Who to notify regarding test failures
 CLOUD_FAILURE_EMAILS    = [e-mail]
 # Who to notify regarding test errors
diff --git a/britney2/policies/cloud.py b/britney2/policies/cloud.py
index b19fbae..41da821 100644
--- a/britney2/policies/cloud.py
+++ b/britney2/policies/cloud.py
@@ -50,6 +50,7 @@ Regards, Ubuntu Release Team.
 """
 class CloudPolicy(BasePolicy):
     PACKAGE_SET_FILE = "cloud_package_set"
+    STATE_FILE = "cloud_state"
     DEFAULT_EMAILS = ["[e-mail]"]
     TEST_LOG_FILE = "CTF.log"
 
@@ -68,6 +69,9 @@ class CloudPolicy(BasePolicy):
         self.work_dir = getattr(self.options, "cloud_work_dir", "cloud_tests")
         self.failure_emails = getattr(self.options, "cloud_failure_emails", self.DEFAULT_EMAILS)
         self.error_emails = getattr(self.options, "cloud_error_emails", self.DEFAULT_EMAILS)
+        self.state_filename = getattr(self.options, "cloud_state_file", self.STATE_FILE)
+
+        self.state = {}
 
         adt_ppas = getattr(self.options, "adt_ppas", "")
         if not isinstance(adt_ppas, list):
@@ -83,11 +87,13 @@ class CloudPolicy(BasePolicy):
 
         self.failures = {}
         self.errors = {}
+        self.email_needed = False
 
     def initialise(self, britney):
         super().initialise(britney)
 
         self.package_set = self._retrieve_cloud_package_set_for_series(self.options.series)
+        self._load_state()
 
     def apply_src_policy_impl(self, policy_info, item, source_data_tdist, source_data_srcdist, excuse):
         if item.package not in self.package_set:
@@ -103,10 +109,12 @@ class CloudPolicy(BasePolicy):
         self.failures = {}
         self.errors = {}
 
-        self._run_cloud_tests(item.package, self.options.series, self.sources, self.source_type)
+        self._run_cloud_tests(item.package, source_data_srcdist.version, self.options.series,
+                              self.sources, self.source_type)
 
         if len(self.failures) > 0 or len(self.errors) > 0:
-            self._send_emails_if_needed(item.package, source_data_srcdist.version, self.options.series)
+            if self.email_needed:
+                self._send_emails_if_needed(item.package, source_data_srcdist.version, self.options.series)
 
             self._cleanup_work_directory()
             verdict = PolicyVerdict.REJECTED_PERMANENTLY
@@ -117,6 +125,76 @@ class CloudPolicy(BasePolicy):
             self._cleanup_work_directory()
             return PolicyVerdict.PASS
 
+    def _mark_tests_run(self, package, version, series, source_type, cloud):
+        """Mark the selected package version as already tested.
+        This takes which cloud we're testing into consideration.
+
+        :param package The name of the package to test
+        :param version Version of the package
+        :param series The Ubuntu codename for the series (e.g. jammy)
+        :param source_type Either 'archive' or 'ppa'
+        :param cloud The name of the cloud being tested (e.g. azure)
+        """
+        if cloud not in self.state:
+            self.state[cloud] = {}
+        if source_type not in self.state[cloud]:
+            self.state[cloud][source_type] = {}
+        if series not in self.state[cloud][source_type]:
+            self.state[cloud][source_type][series] = {}
+        self.state[cloud][source_type][series][package] = { 
+            "version": version,
+            "failures": len(self.failures),
+            "errors": len(self.errors)
+        }
+        
+        self.email_needed = True
+
+        self._save_state()
+
+    def _check_if_tests_run(self, package, version, series, source_type, cloud):
+        """Check if tests were already run for the given package version.
+        This takes which cloud we're testing into consideration.
+
+        :param package The name of the package to test
+        :param version Version of the package
+        :param series The Ubuntu codename for the series (e.g. jammy)
+        :param source_type Either 'archive' or 'ppa'
+        :param cloud The name of the cloud being tested (e.g. azure)
+        """
+        try:
+            return self.state[cloud][source_type][series][package]["version"] == version
+        except KeyError:
+            return False
+
+    def _set_previous_failure_and_error(self, package, version, series, source_type, cloud):
+        """Sets the failures and errors from the previous run.
+        This takes which cloud we're testing into consideration.
+
+        :param package The name of the package to test
+        :param version Version of the package
+        :param series The Ubuntu codename for the series (e.g. jammy)
+        :param source_type Either 'archive' or 'ppa'
+        :param cloud The name of the cloud being tested (e.g. azure)
+        """
+        if self.state[cloud][source_type][series][package]["failures"] > 0:
+            self.failures[cloud] = {}
+
+        if self.state[cloud][source_type][series][package]["errors"] > 0:
+            self.errors[cloud] = {}
+
+    def _load_state(self):
+        """Load the save state of which packages have already been tested."""
+        if os.path.exists(self.state_filename):
+            with open(self.state_filename, encoding="utf-8") as data:
+                self.state = json.load(data)
+            self.logger.info("Loaded cloud policy state file %s" % self.state_filename)
+
+    def _save_state(self):
+        """Save which packages have already been tested."""
+        with open(self.state_filename, "w", encoding="utf-8") as data:
+            json.dump(self.state, data)
+        self.logger.info("Saved cloud policy state file %s" % self.state_filename)
+
     def _retrieve_cloud_package_set_for_series(self, series):
         """Retrieves a set of packages for the given series in which cloud
         tests should be run.
@@ -134,16 +212,17 @@ class CloudPolicy(BasePolicy):
 
         return package_set
 
-    def _run_cloud_tests(self, package, series, sources, source_type):
+    def _run_cloud_tests(self, package, version, series, sources, source_type):
         """Runs any cloud tests for the given package.
         Nothing is returned but test failures and errors are stored in instance variables.
 
         :param package The name of the package to test
+        :param version Version of the package
         :param series The Ubuntu codename for the series (e.g. jammy)
         :param sources List of sources where the package should be installed from (e.g. [proposed] or PPAs)
         :param source_type Either 'archive' or 'ppa'
         """
-        self._run_azure_tests(package, series, sources, source_type)
+        self._run_azure_tests(package, version, series, sources, source_type)
 
     def _send_emails_if_needed(self, package, version, series):
         """Sends email(s) if there are test failures and/or errors
@@ -168,14 +247,19 @@ class CloudPolicy(BasePolicy):
             self.logger.info("Cloud Policy: Sending error email for {}, to {}".format(package, emails))
             self._send_email(emails, message)
 
-    def _run_azure_tests(self, package, series, sources, source_type):
+    def _run_azure_tests(self, package, version, series, sources, source_type):
         """Runs Azure's required package tests.
 
         :param package The name of the package to test
+        :param version Version of the package
         :param series The Ubuntu codename for the series (e.g. jammy)
         :param sources List of sources where the package should be installed from (e.g. [proposed] or PPAs)
         :param source_type Either 'archive' or 'ppa'
         """
+        if self._check_if_tests_run(package, version, series, source_type, "azure"):
+            self._set_previous_failure_and_error(package, version, series, source_type, "azure")
+            return
+
         urn = self._retrieve_urn(series)
 
         self.logger.info("Cloud Policy: Running Azure tests for: {} in {}".format(package, series))
@@ -204,6 +288,7 @@ class CloudPolicy(BasePolicy):
         results_file_paths = self._find_results_files(r"TEST-NetworkTests-[0-9]*.xml")
         self._parse_xunit_test_results("Azure", results_file_paths)
         self._store_extra_test_result_info(self, package)
+        self._mark_tests_run(package, version, series, source_type, "azure")
 
     def _retrieve_urn(self, series):
         """Retrieves an URN from the configuration options based on series.
diff --git a/tests/test_cloud.py b/tests/test_cloud.py
index bbd8595..2e859e7 100644
--- a/tests/test_cloud.py
+++ b/tests/test_cloud.py
@@ -7,6 +7,7 @@
 # (at your option) any later version.
 
 import os
+import json
 import pathlib
 import sys
 from types import SimpleNamespace
@@ -35,7 +36,8 @@ class T(unittest.TestCase):
             verbose = False,
             cloud_source = "zazzy-proposed",
             cloud_source_type = "archive",
-            cloud_azure_zazzy_urn = "fake-urn-value"
+            cloud_azure_zazzy_urn = "fake-urn-value",
+            cloud_state_file = "/tmp/test_state.json"
         )
         self.policy = CloudPolicy(self.fake_options, {})
         self.policy._setup_work_directory()
@@ -43,6 +45,54 @@ class T(unittest.TestCase):
     def tearDown(self):
         self.policy._cleanup_work_directory()
 
+    @patch("britney2.policies.cloud.CloudPolicy._store_extra_test_result_info")
+    @patch("britney2.policies.cloud.CloudPolicy._parse_xunit_test_results")
+    @patch("subprocess.run")
+    def test_run_cloud_tests_state_handling(self, mock_run, mock_xunit, mock_extra):
+        """Cloud tests should save state and not re-run tests for packages
+           already tested."""
+        expected_state = {
+            "azure": {
+                "archive": {
+                    "zazzy": {
+                        "chromium-browser": {
+                            "version": "55.0",
+                            "failures": 0,
+                            "errors": 0,
+                        }
+                    }
+                }
+            }
+        }
+        with open(self.policy.options.cloud_state_file, "w") as file:
+            json.dump(expected_state, file)
+        self.policy._load_state()
+
+        # Package already tested, no tests should run
+        self.policy._run_cloud_tests("chromium-browser", "55.0", "zazzy", ["proposed"], "archive")
+        self.assertDictEqual(expected_state, self.policy.state)
+        mock_run.assert_not_called()
+
+        # A new package appears, tests should run
+        expected_state["azure"]["archive"]["zazzy"]["hello"] = {
+            "version": "2.10",
+            "failures": 0,
+            "errors": 0,
+        }
+        self.policy._run_cloud_tests("hello", "2.10", "zazzy", ["proposed"], "archive")
+        self.assertDictEqual(expected_state, self.policy.state)
+        mock_run.assert_called()
+
+        # A new version of existing package, tests should run
+        expected_state["azure"]["archive"]["zazzy"]["chromium-browser"]["version"] = "55.1"
+        self.policy._run_cloud_tests("chromium-browser", "55.1", "zazzy", ["proposed"], "archive")
+        self.assertDictEqual(expected_state, self.policy.state)
+        self.assertEqual(mock_run.call_count, 2)
+
+        # Make sure the state was saved properly
+        with open(self.policy.options.cloud_state_file, "r") as file:
+            self.assertDictEqual(expected_state, json.load(file))
+
     @patch("britney2.policies.cloud.CloudPolicy._run_cloud_tests")
     def test_run_cloud_tests_called_for_package_in_manifest(self, mock_run):
         """Cloud tests should run for a package in the cloud package set.
@@ -55,7 +105,7 @@ class T(unittest.TestCase):
         )
 
         mock_run.assert_called_once_with(
-            "chromium-browser", "jammy", ["proposed"], "archive"
+            "chromium-browser", "55.0", "jammy", ["proposed"], "archive"
         )
 
     @patch("britney2.policies.cloud.CloudPolicy._run_cloud_tests")