Merge ~pappacena/launchpad:continue-oci-push-on-failure into launchpad:master

[Thiago F. Pappacena]

When an OCI recipe has multiple push rules, continue to the next push rule in case one of them fails to upload.

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/oci/interfaces/ociregistryclient.py b/lib/lp/oci/interfaces/ociregistryclient.py
index 260f75b..184a93f 100644
--- a/lib/lp/oci/interfaces/ociregistryclient.py
+++ b/lib/lp/oci/interfaces/ociregistryclient.py
@@ -10,6 +10,7 @@ __all__ = [
     'BlobUploadFailed',
     'IOCIRegistryClient',
     'ManifestUploadFailed',
+    'MultipleOCIRegistryError',
     'OCIRegistryError',
 ]
 
@@ -24,6 +25,19 @@ class OCIRegistryError(Exception):
         self.errors = errors
 
 
+class MultipleOCIRegistryError(OCIRegistryError):
+    def __init__(self, exceptions):
+        self.exceptions = exceptions
+
+    def __str__(self):
+        return " / ".join(str(i) for i in self.exceptions)
+
+    @property
+    def errors(self):
+        return [i.errors for i in self.exceptions
+                if isinstance(i, OCIRegistryError)]
+
+
 class BlobUploadFailed(OCIRegistryError):
     pass
 
diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index 2ba6da1..0176a89 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -42,6 +42,7 @@ from zope.interface import implementer
 from lp.oci.interfaces.ociregistryclient import (
     BlobUploadFailed,
     IOCIRegistryClient,
+    MultipleOCIRegistryError,
     ManifestUploadFailed,
     )
 from lp.services.timeout import urlfetch
@@ -227,6 +228,59 @@ class OCIRegistryClient:
         return "{}".format("edge")
 
     @classmethod
+    def _upload_to_push_rule(
+            cls, push_rule, build, manifest, digests, preloaded_data):
+        http_client = RegistryHTTPClient.getInstance(push_rule)
+
+        for section in manifest:
+            # Work out names and tags
+            tag = cls._calculateTag(build, push_rule)
+            file_data = preloaded_data[section["Config"]]
+            config = file_data["config_file"]
+            #  Upload the layers involved
+            for diff_id in config["rootfs"]["diff_ids"]:
+                cls._upload_layer(
+                    diff_id,
+                    push_rule,
+                    file_data[diff_id],
+                    http_client)
+            # The config file is required in different forms, so we can
+            # calculate the sha, work these out and upload
+            config_json = json.dumps(config).encode("UTF-8")
+            config_sha = hashlib.sha256(config_json).hexdigest()
+            cls._upload(
+                "sha256:{}".format(config_sha),
+                push_rule,
+                BytesIO(config_json),
+                http_client)
+
+            # Build the registry manifest from the image manifest
+            # and associated configs
+            registry_manifest = cls._build_registry_manifest(
+                digests, config, config_json, config_sha,
+                preloaded_data[section["Config"]])
+
+            # Upload the registry manifest
+            try:
+                manifest_response = http_client.requestPath(
+                    "/manifests/{}".format(tag),
+                    json=registry_manifest,
+                    headers={
+                        "Content-Type":
+                            "application/"
+                            "vnd.docker.distribution.manifest.v2+json"
+                    },
+                    method="PUT")
+            except HTTPError as http_error:
+                manifest_response = http_error.response
+            if manifest_response.status_code != 201:
+                raise cls._makeRegistryError(
+                    ManifestUploadFailed,
+                    "Failed to upload manifest for {} ({}) in {}".format(
+                        build.recipe.name, push_rule.registry_url, build.id),
+                    manifest_response)
+
+    @classmethod
     def upload(cls, build):
         """Upload the artifacts from an OCIRecipeBuild to a registry.
 
@@ -244,56 +298,17 @@ class OCIRegistryClient:
         # Preload the requested files
         preloaded_data = cls._preloadFiles(build, manifest, digests)
 
+        exceptions = []
         for push_rule in build.recipe.push_rules:
-            http_client = RegistryHTTPClient.getInstance(push_rule)
-
-            for section in manifest:
-                # Work out names and tags
-                tag = cls._calculateTag(build, push_rule)
-                file_data = preloaded_data[section["Config"]]
-                config = file_data["config_file"]
-                #  Upload the layers involved
-                for diff_id in config["rootfs"]["diff_ids"]:
-                    cls._upload_layer(
-                        diff_id,
-                        push_rule,
-                        file_data[diff_id],
-                        http_client)
-                # The config file is required in different forms, so we can
-                # calculate the sha, work these out and upload
-                config_json = json.dumps(config).encode("UTF-8")
-                config_sha = hashlib.sha256(config_json).hexdigest()
-                cls._upload(
-                    "sha256:{}".format(config_sha),
-                    push_rule,
-                    BytesIO(config_json),
-                    http_client)
-
-                # Build the registry manifest from the image manifest
-                # and associated configs
-                registry_manifest = cls._build_registry_manifest(
-                    digests, config, config_json, config_sha,
-                    preloaded_data[section["Config"]])
-
-                # Upload the registry manifest
-                try:
-                    manifest_response = http_client.requestPath(
-                        "/manifests/{}".format(tag),
-                        json=registry_manifest,
-                        headers={
-                            "Content-Type":
-                                "application/"
-                                "vnd.docker.distribution.manifest.v2+json"
-                            },
-                        method="PUT")
-                except HTTPError as http_error:
-                    manifest_response = http_error.response
-                if manifest_response.status_code != 201:
-                    raise cls._makeRegistryError(
-                        ManifestUploadFailed,
-                        "Failed to upload manifest for {} in {}".format(
-                            build.recipe.name, build.id),
-                        manifest_response)
+            try:
+                cls._upload_to_push_rule(
+                    push_rule, build, manifest, digests, preloaded_data)
+            except Exception as e:
+                exceptions.append(e)
+        if len(exceptions) == 1:
+            raise exceptions[0]
+        elif len(exceptions) > 1:
+            raise MultipleOCIRegistryError(exceptions)
 
 
 class OCIRegistryAuthenticationError(Exception):
diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
index 2edec41..7459a43 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -38,6 +38,7 @@ from zope.security.proxy import removeSecurityProxy
 from lp.oci.interfaces.ociregistryclient import (
     BlobUploadFailed,
     ManifestUploadFailed,
+    MultipleOCIRegistryError,
     )
 from lp.oci.model.ociregistryclient import (
     BearerTokenRegistryClient,
@@ -216,6 +217,59 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
         self.assertEqual(
             http_client.credentials, ('test-username', 'test-password'))
 
+    @responses.activate
+    def test_upload_skip_failed_push_rule(self):
+        self._makeFiles()
+        upload_fixture = self.useFixture(MockPatch(
+            "lp.oci.model.ociregistryclient.OCIRegistryClient._upload"))
+        self.useFixture(MockPatch(
+            "lp.oci.model.ociregistryclient.OCIRegistryClient._upload_layer"))
+
+        push_rules = [
+            self.push_rule,
+            self.factory.makeOCIPushRule(recipe=self.build.recipe),
+            self.factory.makeOCIPushRule(recipe=self.build.recipe)]
+        # Set the first 2 rules to fail with 400 at the PUT operation.
+        for i, push_rule in enumerate(push_rules):
+            push_rule.registry_credentials.setCredentials({
+                "username": "test-username-%s" % i,
+                "password": "test-password-%s" % i
+            })
+            responses.add(
+                "GET", "%s/v2/" % push_rule.registry_url, status=200)
+
+            manifests_url = "{}/v2/{}/manifests/edge".format(
+                push_rule.registry_credentials.url, push_rule.image_name)
+            status = 400 if i < 2 else 201
+            responses.add("PUT", manifests_url, status=status)
+
+        error = self.assertRaises(
+            MultipleOCIRegistryError, self.client.upload, self.build)
+
+        # Check that it tried to call the upload for each one of the push rules
+        self.assertEqual(3, upload_fixture.mock.call_count)
+        used_credentials = {
+            args[0][-1].credentials
+            for args in upload_fixture.mock.call_args_list}
+        self.assertSetEqual(
+            {('test-username-0', 'test-password-0'),
+             ('test-username-1', 'test-password-1'),
+             ('test-username-2', 'test-password-2')},
+            used_credentials)
+
+        # Check that we received back an exception of the correct type.
+        self.assertIsInstance(error, MultipleOCIRegistryError)
+        self.assertEqual(2, len(error.errors))
+        self.assertEqual(2, len(error.exceptions))
+
+        expected_error_msg = (
+            "Failed to upload manifest for {recipe} ({url1}) in {build} / "
+            "Failed to upload manifest for {recipe} ({url2}) in {build}"
+        ).format(
+            recipe=self.build.recipe.name, build=self.build.id,
+            url1=push_rules[0].registry_url, url2=push_rules[1].registry_url)
+        self.assertEqual(expected_error_msg, str(error))
+
     def test_preloadFiles(self):
         self._makeFiles()
         files = self.client._preloadFiles(
@@ -433,6 +487,8 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
         responses.add(
             "PUT", manifests_url, status=400, json={"errors": put_errors})
 
+        expected_msg = "Failed to upload manifest for {} ({}) in {}".format(
+            self.build.recipe.name, self.push_rule.registry_url, self.build.id)
         self.assertThat(
             partial(self.client.upload, self.build),
             Raises(MatchesException(
@@ -440,9 +496,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
                 MatchesAll(
                     AfterPreprocessing(
                         str,
-                        Equals(
-                            "Failed to upload manifest for {} in {}".format(
-                                self.build.recipe.name, self.build.id))),
+                        Equals(expected_msg)),
                     MatchesStructure.byEquality(errors=put_errors)))))
 
     @responses.activate
@@ -462,6 +516,8 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
             push_rule.image_name)
         responses.add("PUT", manifests_url, status=200)
 
+        expected_msg = "Failed to upload manifest for {} ({}) in {}".format(
+            self.build.recipe.name, self.push_rule.registry_url, self.build.id)
         self.assertThat(
             partial(self.client.upload, self.build),
             Raises(MatchesException(
@@ -469,9 +525,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
                 MatchesAll(
                     AfterPreprocessing(
                         str,
-                        Equals(
-                            "Failed to upload manifest for {} in {}".format(
-                                self.build.recipe.name, self.build.id))),
+                        Equals(expected_msg)),
                     MatchesStructure(errors=Is(None))))))