launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24815
[Merge] ~cjwatson/launchpad:oci-detailed-registry-errors into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:oci-detailed-registry-errors into launchpad:master with ~cjwatson/launchpad:oci-registry-client-fix-exceptions as a prerequisite.
Commit message:
Improve handling of detailed OCI registry errors
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/384965
The existing error handling for OCI registry uploads was modelled on that for snap store uploads, and the error conventions aren't quite the same. Adjust it in line with the documented registry API.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:oci-detailed-registry-errors into launchpad:master.
diff --git a/lib/lp/oci/interfaces/ocirecipebuild.py b/lib/lp/oci/interfaces/ocirecipebuild.py
index 95539c0..aa6e291 100644
--- a/lib/lp/oci/interfaces/ocirecipebuild.py
+++ b/lib/lp/oci/interfaces/ocirecipebuild.py
@@ -34,7 +34,9 @@ from zope.schema import (
Bool,
Choice,
Datetime,
+ Dict,
Int,
+ List,
TextLine,
)
@@ -186,6 +188,22 @@ class IOCIRecipeBuildView(IPackageBuild):
required=True, readonly=False
))
+ registry_upload_error_summary = exported(TextLine(
+ title=_("Registry upload error summary"),
+ description=_(
+ "The error summary, if any, from the last attempt to upload this "
+ "build to a registry."),
+ required=False, readonly=True))
+
+ registry_upload_errors = exported(List(
+ title=_("Detailed registry upload errors"),
+ description=_(
+ "A list of errors, as described in "
+ "https://docs.docker.com/registry/spec/api/#errors, from the last "
+ "attempt to upload this build to a registry."),
+ value_type=Dict(key_type=TextLine()),
+ required=False, readonly=True))
+
class IOCIRecipeBuildEdit(Interface):
"""`IOCIRecipeBuild` attributes that require launchpad.Edit permission."""
diff --git a/lib/lp/oci/interfaces/ocirecipebuildjob.py b/lib/lp/oci/interfaces/ocirecipebuildjob.py
index 7f2537b..c0fada6 100644
--- a/lib/lp/oci/interfaces/ocirecipebuildjob.py
+++ b/lib/lp/oci/interfaces/ocirecipebuildjob.py
@@ -1,4 +1,4 @@
-# Copyright 2019 Canonical Ltd. This software is licensed under the
+# Copyright 2019-2020 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""OCIRecipe build job interfaces"""
@@ -13,12 +13,15 @@ __all__ = [
]
from lazr.restful.fields import Reference
-from zope.component.interfaces import IObjectEvent
from zope.interface import (
Attribute,
Interface,
)
-from zope.schema import TextLine
+from zope.schema import (
+ Dict,
+ List,
+ TextLine,
+ )
from lp import _
from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuild
@@ -45,8 +48,17 @@ class IOCIRecipeBuildJob(Interface):
class IOCIRegistryUploadJob(IRunnableJob):
"""A Job that uploads an OCI image to a registry."""
- error_message = TextLine(
- title=_("Error message"), required=False, readonly=True)
+ error_summary = TextLine(
+ title=_("Error summary"), required=False, readonly=True)
+
+ errors = List(
+ title=_("Detailed registry upload errors"),
+ description=_(
+ "A list of errors, as described in "
+ "https://docs.docker.com/registry/spec/api/#errors, from the last "
+ "attempt to run this job."),
+ value_type=Dict(key_type=TextLine()),
+ required=False, readonly=True)
class IOCIRegistryUploadJobSource(IJobSource):
diff --git a/lib/lp/oci/interfaces/ociregistryclient.py b/lib/lp/oci/interfaces/ociregistryclient.py
index 8ac2aa6..260f75b 100644
--- a/lib/lp/oci/interfaces/ociregistryclient.py
+++ b/lib/lp/oci/interfaces/ociregistryclient.py
@@ -10,16 +10,25 @@ __all__ = [
'BlobUploadFailed',
'IOCIRegistryClient',
'ManifestUploadFailed',
+ 'OCIRegistryError',
]
from zope.interface import Interface
-class BlobUploadFailed(Exception):
+class OCIRegistryError(Exception):
+ """An error returned by an OCI registry."""
+
+ def __init__(self, summary, errors):
+ super(OCIRegistryError, self).__init__(summary)
+ self.errors = errors
+
+
+class BlobUploadFailed(OCIRegistryError):
pass
-class ManifestUploadFailed(Exception):
+class ManifestUploadFailed(OCIRegistryError):
pass
diff --git a/lib/lp/oci/model/ocirecipebuild.py b/lib/lp/oci/model/ocirecipebuild.py
index 94b7326..9613404 100644
--- a/lib/lp/oci/model/ocirecipebuild.py
+++ b/lib/lp/oci/model/ocirecipebuild.py
@@ -436,6 +436,16 @@ class OCIRecipeBuild(PackageBuildMixin, Storm):
else:
return OCIRecipeBuildRegistryUploadStatus.FAILEDTOUPLOAD
+ @property
+ def registry_upload_error_summary(self):
+ job = self.last_registry_upload_job
+ return job and job.error_summary
+
+ @property
+ def registry_upload_errors(self):
+ job = self.last_registry_upload_job
+ return (job and job.errors) or []
+
@implementer(IOCIRecipeBuildSet)
class OCIRecipeBuildSet(SpecificBuildFarmJobSourceMixin):
diff --git a/lib/lp/oci/model/ocirecipebuildjob.py b/lib/lp/oci/model/ocirecipebuildjob.py
index fc84c17..73fa4d6 100644
--- a/lib/lp/oci/model/ocirecipebuildjob.py
+++ b/lib/lp/oci/model/ocirecipebuildjob.py
@@ -36,7 +36,10 @@ from lp.oci.interfaces.ocirecipebuildjob import (
IOCIRegistryUploadJob,
IOCIRegistryUploadJobSource,
)
-from lp.oci.interfaces.ociregistryclient import IOCIRegistryClient
+from lp.oci.interfaces.ociregistryclient import (
+ IOCIRegistryClient,
+ OCIRegistryError,
+ )
from lp.services.database.enumcol import DBEnum
from lp.services.database.interfaces import IStore
from lp.services.database.stormbase import StormBase
@@ -205,34 +208,24 @@ class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
self._do_lifecycle("resume", *args, **kwargs)
@property
- def error_message(self):
+ def error_summary(self):
"""See `IOCIRegistryUploadJob`."""
- return self.json_data.get("error_message")
+ return self.json_data.get("error_summary")
- @error_message.setter
- def error_message(self, message):
+ @error_summary.setter
+ def error_summary(self, summary):
"""See `IOCIRegistryUploadJob`."""
- self.json_data["error_message"] = message
+ self.json_data["error_summary"] = summary
@property
- def error_detail(self):
- """See `IOCIRegistryUploadJob`."""
- return self.json_data.get("error_detail")
-
- @error_detail.setter
- def error_detail(self, detail):
+ def errors(self):
"""See `IOCIRegistryUploadJob`."""
- self.json_data["error_detail"] = detail
+ return self.json_data.get("errors")
- @property
- def error_messages(self):
+ @errors.setter
+ def errors(self, errors):
"""See `IOCIRegistryUploadJob`."""
- return self.json_data.get("error_messages")
-
- @error_messages.setter
- def error_messages(self, messages):
- """See `IOCIRegistryUploadJob`."""
- self.json_data["error_messages"] = messages
+ self.json_data["errors"] = errors
def run(self):
"""See `IRunnableJob`."""
@@ -242,10 +235,13 @@ class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
try:
try:
client.upload(self.build)
+ except OCIRegistryError as e:
+ self.error_summary = str(e)
+ self.errors = e.errors
+ raise
except Exception as e:
- self.error_message = str(e)
- self.error_messages = getattr(e, "messages", None)
- self.error_detail = getattr(e, "detail", None)
+ self.error_summary = str(e)
+ self.errors = None
raise
except Exception:
transaction.commit()
diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index f65a986..2ba6da1 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -15,6 +15,10 @@ from functools import partial
import hashlib
from io import BytesIO
import json
+try:
+ from json.decoder import JSONDecodeError
+except ImportError:
+ JSONDecodeError = ValueError
import logging
import tarfile
@@ -61,6 +65,18 @@ class OCIRegistryClient:
finally:
reference.close()
+ @classmethod
+ def _makeRegistryError(cls, error_class, summary, response):
+ errors = None
+ if response.content:
+ try:
+ response_data = response.json()
+ except JSONDecodeError:
+ pass
+ else:
+ errors = response_data.get("errors")
+ return error_class(summary, errors)
+
# Retry this on a ConnectionError, 5 times with 3 seconds wait.
# Log each attempt so we can see they are happening.
@classmethod
@@ -106,9 +122,11 @@ class OCIRegistryClient:
except HTTPError as http_error:
put_response = http_error.response
if put_response.status_code != 201:
- msg = "Upload of {} for {} failed".format(
- digest, push_rule.image_name)
- raise BlobUploadFailed(msg)
+ raise cls._makeRegistryError(
+ BlobUploadFailed,
+ "Upload of {} for {} failed".format(
+ digest, push_rule.image_name),
+ put_response)
@classmethod
def _upload_layer(cls, digest, push_rule, lfa, http_client):
@@ -271,9 +289,11 @@ class OCIRegistryClient:
except HTTPError as http_error:
manifest_response = http_error.response
if manifest_response.status_code != 201:
- raise ManifestUploadFailed(
+ raise cls._makeRegistryError(
+ ManifestUploadFailed,
"Failed to upload manifest for {} in {}".format(
- build.recipe.name, build.id))
+ build.recipe.name, build.id),
+ manifest_response)
class OCIRegistryAuthenticationError(Exception):
diff --git a/lib/lp/oci/tests/test_ocirecipebuild.py b/lib/lp/oci/tests/test_ocirecipebuild.py
index 2ebffde..80e94d5 100644
--- a/lib/lp/oci/tests/test_ocirecipebuild.py
+++ b/lib/lp/oci/tests/test_ocirecipebuild.py
@@ -32,11 +32,14 @@ from lp.oci.interfaces.ocirecipebuild import (
IOCIFileSet,
IOCIRecipeBuild,
IOCIRecipeBuildSet,
+ OCIRecipeBuildRegistryUploadStatus,
)
+from lp.oci.interfaces.ocirecipebuildjob import IOCIRegistryUploadJobSource
from lp.oci.model.ocirecipebuild import OCIRecipeBuildSet
from lp.registry.interfaces.series import SeriesStatus
from lp.services.config import config
from lp.services.features.testing import FeatureFixture
+from lp.services.job.interfaces.job import JobStatus
from lp.services.propertycache import clear_property_cache
from lp.services.webapp.publisher import canonical_url
from lp.services.webhooks.testing import LogsScheduledWebhooks
@@ -281,6 +284,71 @@ class TestOCIRecipeBuild(TestCaseWithFactory):
clear_property_cache(self.build)
self.assertFalse(self.build.estimate)
+ def test_registry_upload_status_unscheduled(self):
+ build = self.factory.makeOCIRecipeBuild(status=BuildStatus.FULLYBUILT)
+ self.assertEqual(
+ OCIRecipeBuildRegistryUploadStatus.UNSCHEDULED,
+ build.registry_upload_status)
+
+ def test_registry_upload_status_pending(self):
+ build = self.factory.makeOCIRecipeBuild(status=BuildStatus.FULLYBUILT)
+ getUtility(IOCIRegistryUploadJobSource).create(build)
+ self.assertEqual(
+ OCIRecipeBuildRegistryUploadStatus.PENDING,
+ build.registry_upload_status)
+
+ def test_registry_upload_status_uploaded(self):
+ build = self.factory.makeOCIRecipeBuild(status=BuildStatus.FULLYBUILT)
+ job = getUtility(IOCIRegistryUploadJobSource).create(build)
+ naked_job = removeSecurityProxy(job)
+ naked_job.job._status = JobStatus.COMPLETED
+ self.assertEqual(
+ OCIRecipeBuildRegistryUploadStatus.UPLOADED,
+ build.registry_upload_status)
+
+ def test_registry_upload_status_failed_to_upload(self):
+ build = self.factory.makeOCIRecipeBuild(status=BuildStatus.FULLYBUILT)
+ job = getUtility(IOCIRegistryUploadJobSource).create(build)
+ naked_job = removeSecurityProxy(job)
+ naked_job.job._status = JobStatus.FAILED
+ self.assertEqual(
+ OCIRecipeBuildRegistryUploadStatus.FAILEDTOUPLOAD,
+ build.registry_upload_status)
+
+ def test_registry_upload_error_summary_no_job(self):
+ build = self.factory.makeOCIRecipeBuild(status=BuildStatus.FULLYBUILT)
+ self.assertIsNone(build.registry_upload_error_summary)
+
+ def test_registry_upload_error_summary_job_no_error(self):
+ build = self.factory.makeOCIRecipeBuild(status=BuildStatus.FULLYBUILT)
+ getUtility(IOCIRegistryUploadJobSource).create(build)
+ self.assertIsNone(build.registry_upload_error_summary)
+
+ def test_registry_upload_error_summary_job_error(self):
+ build = self.factory.makeOCIRecipeBuild(status=BuildStatus.FULLYBUILT)
+ job = getUtility(IOCIRegistryUploadJobSource).create(build)
+ removeSecurityProxy(job).error_summary = "Boom"
+ self.assertEqual("Boom", build.registry_upload_error_summary)
+
+ def test_registry_upload_errors_no_job(self):
+ build = self.factory.makeOCIRecipeBuild(status=BuildStatus.FULLYBUILT)
+ self.assertEqual([], build.registry_upload_errors)
+
+ def test_registry_upload_errors_job_no_error(self):
+ build = self.factory.makeOCIRecipeBuild(status=BuildStatus.FULLYBUILT)
+ getUtility(IOCIRegistryUploadJobSource).create(build)
+ self.assertEqual([], build.registry_upload_errors)
+
+ def test_registry_upload_errors_job_error(self):
+ build = self.factory.makeOCIRecipeBuild(status=BuildStatus.FULLYBUILT)
+ job = getUtility(IOCIRegistryUploadJobSource).create(build)
+ removeSecurityProxy(job).errors = [
+ {"code": "BOOM", "message": "Boom", "detail": "It went boom"},
+ ]
+ self.assertEqual(
+ [{"code": "BOOM", "message": "Boom", "detail": "It went boom"}],
+ build.registry_upload_errors)
+
class TestOCIRecipeBuildSet(TestCaseWithFactory):
diff --git a/lib/lp/oci/tests/test_ocirecipebuildjob.py b/lib/lp/oci/tests/test_ocirecipebuildjob.py
index ef3af8a..fd3e0ff 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildjob.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildjob.py
@@ -27,7 +27,10 @@ from lp.oci.interfaces.ocirecipebuildjob import (
IOCIRecipeBuildJob,
IOCIRegistryUploadJob,
)
-from lp.oci.interfaces.ociregistryclient import IOCIRegistryClient
+from lp.oci.interfaces.ociregistryclient import (
+ BlobUploadFailed,
+ IOCIRegistryClient,
+ )
from lp.oci.model.ocirecipebuildjob import (
OCIRecipeBuildJob,
OCIRecipeBuildJobDerived,
@@ -173,12 +176,55 @@ class TestOCIRegistryUploadJob(TestCaseWithFactory):
run_isolated_jobs([job])
self.assertEqual([((ocibuild,), {})], client.upload.calls)
self.assertContentEqual([job], ocibuild.registry_upload_jobs)
- self.assertIsNone(job.error_message)
+ self.assertIsNone(job.error_summary)
+ self.assertIsNone(job.errors)
self.assertEqual([], pop_notifications())
self.assertWebhookDeliveries(ocibuild, ["Pending", "Uploaded"], logger)
- def test_run_failed(self):
- # A failed run sets the registry upload status to FAILED.
+ def test_run_failed_registry_error(self):
+ # A run that fails with a registry error sets the registry upload
+ # status to FAILED, and stores the detailed errors.
+ logger = self.useFixture(FakeLogger())
+ ocibuild = self.makeOCIRecipeBuild()
+ self.assertContentEqual([], ocibuild.registry_upload_jobs)
+ job = OCIRegistryUploadJob.create(ocibuild)
+ client = FakeRegistryClient()
+ error_summary = "Upload of some-digest for some-image failed"
+ errors = [
+ {
+ "code": "UNAUTHORIZED",
+ "message": "authentication required",
+ "detail": [
+ {
+ "Type": "repository",
+ "Class": "",
+ "Name": "some-image",
+ "Action": "pull",
+ },
+ {
+ "Type": "repository",
+ "Class": "",
+ "Name": "some-image",
+ "Action": "push",
+ },
+ ],
+ },
+ ]
+ client.upload.failure = BlobUploadFailed(error_summary, errors)
+ self.useFixture(ZopeUtilityFixture(client, IOCIRegistryClient))
+ with dbuser(config.IOCIRegistryUploadJobSource.dbuser):
+ run_isolated_jobs([job])
+ self.assertEqual([((ocibuild,), {})], client.upload.calls)
+ self.assertContentEqual([job], ocibuild.registry_upload_jobs)
+ self.assertEqual(error_summary, job.error_summary)
+ self.assertEqual(errors, job.errors)
+ self.assertEqual([], pop_notifications())
+ self.assertWebhookDeliveries(
+ ocibuild, ["Pending", "Failed to upload"], logger)
+
+ def test_run_failed_other_error(self):
+ # A run that fails for some reason other than a registry error sets
+ # the registry upload status to FAILED.
logger = self.useFixture(FakeLogger())
ocibuild = self.makeOCIRecipeBuild()
self.assertContentEqual([], ocibuild.registry_upload_jobs)
@@ -190,7 +236,8 @@ class TestOCIRegistryUploadJob(TestCaseWithFactory):
run_isolated_jobs([job])
self.assertEqual([((ocibuild,), {})], client.upload.calls)
self.assertContentEqual([job], ocibuild.registry_upload_jobs)
- self.assertEqual("An upload failure", job.error_message)
+ self.assertEqual("An upload failure", job.error_summary)
+ self.assertIsNone(job.errors)
self.assertEqual([], pop_notifications())
self.assertWebhookDeliveries(
ocibuild, ["Pending", "Failed to upload"], logger)
diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
index 879a562..433d7d3 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -24,9 +24,12 @@ from tenacity import RetryError
from testtools.matchers import (
AfterPreprocessing,
Equals,
+ Is,
+ MatchesAll,
MatchesDict,
MatchesException,
MatchesListwise,
+ MatchesStructure,
Raises,
)
import transaction
@@ -371,11 +374,13 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
"test-digest", push_rule, None, http_client),
Raises(MatchesException(
BlobUploadFailed,
- AfterPreprocessing(
- str,
- Equals(
- "Upload of {} for {} failed".format(
- "test-digest", push_rule.image_name))))))
+ MatchesAll(
+ AfterPreprocessing(
+ str,
+ Equals(
+ "Upload of {} for {} failed".format(
+ "test-digest", push_rule.image_name))),
+ MatchesStructure.byEquality(errors=put_errors)))))
@responses.activate
def test_upload_put_blob_raises_non_201_success(self):
@@ -395,11 +400,13 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
"test-digest", push_rule, None, http_client),
Raises(MatchesException(
BlobUploadFailed,
- AfterPreprocessing(
- str,
- Equals(
- "Upload of {} for {} failed".format(
- "test-digest", push_rule.image_name))))))
+ MatchesAll(
+ AfterPreprocessing(
+ str,
+ Equals(
+ "Upload of {} for {} failed".format(
+ "test-digest", push_rule.image_name))),
+ MatchesStructure(errors=Is(None))))))
@responses.activate
def test_upload_put_manifest_raises_error(self):
@@ -430,11 +437,13 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
partial(self.client.upload, self.build),
Raises(MatchesException(
ManifestUploadFailed,
- AfterPreprocessing(
- str,
- Equals(
- "Failed to upload manifest for {} in {}".format(
- self.build.recipe.name, self.build.id))))))
+ MatchesAll(
+ AfterPreprocessing(
+ str,
+ Equals(
+ "Failed to upload manifest for {} in {}".format(
+ self.build.recipe.name, self.build.id))),
+ MatchesStructure.byEquality(errors=put_errors)))))
@responses.activate
def test_upload_put_manifest_raises_non_201_success(self):
@@ -457,11 +466,13 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
partial(self.client.upload, self.build),
Raises(MatchesException(
ManifestUploadFailed,
- AfterPreprocessing(
- str,
- Equals(
- "Failed to upload manifest for {} in {}".format(
- self.build.recipe.name, self.build.id))))))
+ MatchesAll(
+ AfterPreprocessing(
+ str,
+ Equals(
+ "Failed to upload manifest for {} in {}".format(
+ self.build.recipe.name, self.build.id))),
+ MatchesStructure(errors=Is(None))))))
class TestRegistryHTTPClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
