launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:oci-upload-manifest-upsert into launchpad:master.

Commit message:
Updating existing manifest (if any) when uploading OCI manifest

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/394265
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:oci-upload-manifest-upsert into launchpad:master.

diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index be370b4..2b920cd 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -235,6 +235,18 @@ class OCIRegistryClient:
         return "{}".format("edge")
 
     @classmethod
+    def _getCurrentRegistryManifest(cls, http_client, push_rule):
+        """Get the current manifest for the given push rule. If manifest
+        doesn't exist, raises HTTPError.
+        """
+        tag = cls._calculateTag(None, push_rule)
+        url = "/manifests/{}".format(tag)
+        accept = "application/vnd.docker.distribution.manifest.list.v2+json"
+        response = http_client.requestPath(
+            url, method="GET", headers={"Accept": accept})
+        return response.json()
+
+    @classmethod
     def _uploadRegistryManifest(cls, http_client, registry_manifest,
                                 push_rule, build=None):
         """Uploads the build manifest, returning its content information.
@@ -353,11 +365,35 @@ class OCIRegistryClient:
             raise MultipleOCIRegistryError(exceptions)
 
     @classmethod
-    def makeMultiArchManifest(cls, build_request, uploaded_builds):
+    def makeMultiArchManifest(cls, http_client, push_rule, build_request,
+                              uploaded_builds):
         """Returns the multi-arch manifest content including all uploaded
         builds of the given build_request.
         """
-        manifests = []
+        def get_manifest_for_architecture(manifests, arch):
+            """Find, in the manifests list, the manifest for the given arch."""
+            expected_platform = {"architecture": arch, "os": "linux"}
+            for m in manifests:
+                if m["platform"] == expected_platform:
+                    return m
+            return None
+
+        try:
+            current_manifest = cls._getCurrentRegistryManifest(
+                http_client, push_rule)
+            # Check if the current manifest is not an incompatible version.
+            version = current_manifest.get("schemaVersion", 1)
+            if version < 2 or "manifests" not in current_manifest:
+                current_manifest = None
+        except HTTPError:
+            current_manifest = None
+        if current_manifest is None:
+            current_manifest = {
+                "schemaVersion": 2,
+                "mediaType": ("application/"
+                              "vnd.docker.distribution.manifest.list.v2+json"),
+                "manifests": []}
+        manifests = current_manifest["manifests"]
         for build in uploaded_builds:
             build_manifest = build_request.uploaded_manifests.get(build.id)
             if not build_manifest:
@@ -365,28 +401,33 @@ class OCIRegistryClient:
             digest = build_manifest["digest"]
             size = build_manifest["size"]
             arch = build.processor.name
-            manifests.append({
-                "mediaType": ("application/"
-                              "vnd.docker.distribution.manifest.v2+json"),
-                "size": size,
-                "digest": digest,
-                "platform": {"architecture": arch, "os": "linux"}
-            })
-        return {
-          "schemaVersion": 2,
-          "mediaType": ("application/"
-                        "vnd.docker.distribution.manifest.list.v2+json"),
-          "manifests": manifests}
+
+            manifest = get_manifest_for_architecture(manifests, arch)
+            if manifest is None:
+                manifest = {
+                    "mediaType": ("application/"
+                                  "vnd.docker.distribution.manifest.v2+json"),
+                    "size": size,
+                    "digest": digest,
+                    "platform": {"architecture": arch, "os": "linux"}
+                }
+                manifests.append(manifest)
+            else:
+                manifest["digest"] = digest
+                manifest["size"] = size
+                manifest["platform"]["architecture"] = arch
+
+        return current_manifest
 
     @classmethod
     def uploadManifestList(cls, build_request, uploaded_builds):
         """Uploads to all build_request.recipe.push_rules the manifest list
         for the builds in the given build_request.
         """
-        multi_manifest_content = cls.makeMultiArchManifest(
-            build_request, uploaded_builds)
         for push_rule in build_request.recipe.push_rules:
             http_client = RegistryHTTPClient.getInstance(push_rule)
+            multi_manifest_content = cls.makeMultiArchManifest(
+                http_client, push_rule, build_request, uploaded_builds)
             cls._uploadRegistryManifest(
                 http_client, multi_manifest_content, push_rule, build=None)
 
diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
index 162de72..05160fd 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -542,7 +542,7 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
                     MatchesStructure(errors=Is(None))))))
 
     @responses.activate
-    def test_multi_arch_manifest_upload(self):
+    def test_multi_arch_manifest_upload_new_manifest(self):
         """Ensure that multi-arch manifest upload works and tags correctly
         the uploaded image."""
         # Creates a build request with 2 builds.
@@ -576,15 +576,21 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
 
         push_rule = self.build.recipe.push_rules[0]
         responses.add(
+            "GET", "{}/v2/{}/manifests/edge".format(
+                push_rule.registry_url, push_rule.image_name),
+            status=404)
+        self.addManifestResponses(push_rule, status_code=201)
+
+        responses.add(
             "GET", "{}/v2/".format(push_rule.registry_url), status=200)
         self.addManifestResponses(push_rule, status_code=201)
 
         # Let's try to generate the manifest for just 2 of the 3 builds:
         self.client.uploadManifestList(build_request, [build1, build2])
-        self.assertEqual(2, len(responses.calls))
-        auth_call, manifest_call = responses.calls
+        self.assertEqual(3, len(responses.calls))
+        auth_call, get_manifest_call, send_manifest_call = responses.calls
         self.assertEndsWith(
-            manifest_call.request.url,
+            send_manifest_call.request.url,
             "/v2/%s/manifests/edge" % push_rule.image_name)
         self.assertEqual({
             "schemaVersion": 2,
@@ -603,7 +609,154 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
                 "digest": "build2digest",
                 "size": 321
             }]
-        }, json.loads(manifest_call.request.body))
+        }, json.loads(send_manifest_call.request.body))
+
+    @responses.activate
+    def test_multi_arch_manifest_upload_update_manifest(self):
+        """Makes sure we update only new architectures if there is already
+        a manifest file in registry.
+        """
+        current_manifest = {
+            "schemaVersion": 2,
+            "mediaType": "application/"
+                         "vnd.docker.distribution.manifest.list.v2+json",
+            "manifests": [{
+                "platform": {"os": "linux", "architecture": "386"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "initial-386-digest",
+                "size": 110
+            }, {
+                "platform": {"os": "linux", "architecture": "amd64"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "initial-amd64-digest",
+                "size": 220
+            }]
+        }
+
+        # Creates a build request with 2 builds: amd64 (which is already in
+        # the manifest) and hppa (that should be added)
+        recipe = self.build.recipe
+        build1 = self.build
+        build2 = self.factory.makeOCIRecipeBuild(
+            recipe=recipe)
+        naked_build1 = removeSecurityProxy(build1)
+        naked_build2 = removeSecurityProxy(build2)
+        naked_build1.processor = getUtility(IProcessorSet).getByName('amd64')
+        naked_build2.processor = getUtility(IProcessorSet).getByName('hppa')
+
+        # Creates a mock IOCIRecipeRequestBuildsJobSource, as it was created
+        # by the celery job and triggered the 3 registry uploads already.
+        job = mock.Mock()
+        job.builds = [build1, build2]
+        job.uploaded_manifests = {
+            build1.id: {"digest": "new-build1-digest", "size": 1111},
+            build2.id: {"digest": "new-build2-digest", "size": 2222},
+        }
+        job_source = mock.Mock()
+        job_source.getByOCIRecipeAndID.return_value = job
+        self.useFixture(
+            ZopeUtilityFixture(job_source, IOCIRecipeRequestBuildsJobSource))
+        build_request = OCIRecipeBuildRequest(recipe, -1)
+
+        push_rule = self.build.recipe.push_rules[0]
+        responses.add(
+            "GET", "{}/v2/{}/manifests/edge".format(
+                push_rule.registry_url, push_rule.image_name),
+            json=current_manifest,
+            status=200)
+        self.addManifestResponses(push_rule, status_code=201)
+
+        responses.add(
+            "GET", "{}/v2/".format(push_rule.registry_url), status=200)
+        self.addManifestResponses(push_rule, status_code=201)
+
+        self.client.uploadManifestList(build_request, [build1, build2])
+        self.assertEqual(3, len(responses.calls))
+        auth_call, get_manifest_call, send_manifest_call = responses.calls
+        self.assertEndsWith(
+            send_manifest_call.request.url,
+            "/v2/%s/manifests/edge" % push_rule.image_name)
+        self.assertEqual({
+            "schemaVersion": 2,
+            "mediaType": "application/"
+                         "vnd.docker.distribution.manifest.list.v2+json",
+            "manifests": [{
+                "platform": {"os": "linux", "architecture": "386"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "initial-386-digest",
+                "size": 110
+            }, {
+                "platform": {"os": "linux", "architecture": "amd64"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "new-build1-digest",
+                "size": 1111
+            }, {
+                "platform": {"os": "linux", "architecture": "hppa"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "new-build2-digest",
+                "size": 2222
+            }]
+        }, json.loads(send_manifest_call.request.body))
+
+    @responses.activate
+    def test_multi_arch_manifest_upload_invalid_current_manifest(self):
+        """Makes sure we update only new architectures if there is already
+        a manifest file in registry.
+        """
+        current_manifest = {'schemaVersion': 1, 'layers': []}
+
+        # Creates a build request with 1 build for amd64.
+        recipe = self.build.recipe
+        build1 = self.build
+        naked_build1 = removeSecurityProxy(build1)
+        naked_build1.processor = getUtility(IProcessorSet).getByName('amd64')
+
+        job = mock.Mock()
+        job.builds = [build1]
+        job.uploaded_manifests = {
+            build1.id: {"digest": "new-build1-digest", "size": 1111},
+        }
+        job_source = mock.Mock()
+        job_source.getByOCIRecipeAndID.return_value = job
+        self.useFixture(
+            ZopeUtilityFixture(job_source, IOCIRecipeRequestBuildsJobSource))
+        build_request = OCIRecipeBuildRequest(recipe, -1)
+
+        push_rule = self.build.recipe.push_rules[0]
+        responses.add(
+            "GET", "{}/v2/{}/manifests/edge".format(
+                push_rule.registry_url, push_rule.image_name),
+            json=current_manifest,
+            status=200)
+        self.addManifestResponses(push_rule, status_code=201)
+
+        responses.add(
+            "GET", "{}/v2/".format(push_rule.registry_url), status=200)
+        self.addManifestResponses(push_rule, status_code=201)
+
+        self.client.uploadManifestList(build_request, [build1])
+        self.assertEqual(3, len(responses.calls))
+        auth_call, get_manifest_call, send_manifest_call = responses.calls
+        self.assertEndsWith(
+            send_manifest_call.request.url,
+            "/v2/%s/manifests/edge" % push_rule.image_name)
+        self.assertEqual({
+            "schemaVersion": 2,
+            "mediaType": "application/"
+                         "vnd.docker.distribution.manifest.list.v2+json",
+            "manifests": [{
+                "platform": {"os": "linux", "architecture": "amd64"},
+                "mediaType": "application/"
+                             "vnd.docker.distribution.manifest.v2+json",
+                "digest": "new-build1-digest",
+                "size": 1111
+            }]
+        }, json.loads(send_manifest_call.request.body))
 
 
 class TestRegistryHTTPClient(OCIConfigHelperMixin, SpyProxyCallsMixin,