launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #27515
[Merge] ~cjwatson/launchpad:fix-snap-charm-push-failure into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:fix-snap-charm-push-failure into launchpad:master.
Commit message:
Fix handling of snap/charm push failures after upload
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/408582
`SnapBuildJob` and `CharmRecipeBuildJob` already knew how to retry uploads if they failed. However, the "upload" step as previously defined was actually two HTTP requests, one to upload the file and one to push the uploaded file to the relevant snap or charm, and it's possible for the second of those requests to fail; furthermore, the "upload file" step is not idempotent, because the store refuses blobs that it has already seen.
Resolve this by splitting the "upload" step into "upload file" and "push", which are retried separately.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-snap-charm-push-failure into launchpad:master.
diff --git a/lib/lp/charms/interfaces/charmhubclient.py b/lib/lp/charms/interfaces/charmhubclient.py
index 7f6443e..9a7cc0f 100644
--- a/lib/lp/charms/interfaces/charmhubclient.py
+++ b/lib/lp/charms/interfaces/charmhubclient.py
@@ -94,10 +94,20 @@ class ICharmhubClient(Interface):
Candid caveat.
"""
- def upload(build):
- """Upload a charm recipe build to CharmHub.
+ def uploadFile(lfa):
+ """Upload a file to Charmhub.
+
+ :param lfa: The `ILibraryFileAlias` to upload.
+ :return: An upload ID.
+ :raises UploadFailedResponse: if uploading the file to Charmhub
+ failed.
+ """
+
+ def push(build, upload_id):
+ """Push a charm recipe build to CharmHub.
:param build: The `ICharmRecipeBuild` to upload.
+ :param upload_id: An upload ID previously returned by `uploadFile`.
:return: A URL to poll for upload processing status.
:raises UnauthorizedUploadResponse: if the user who authorised this
upload is not themselves authorised to upload the snap in
diff --git a/lib/lp/charms/interfaces/charmrecipebuildjob.py b/lib/lp/charms/interfaces/charmrecipebuildjob.py
index b0d1e20..2ae320d 100644
--- a/lib/lp/charms/interfaces/charmrecipebuildjob.py
+++ b/lib/lp/charms/interfaces/charmrecipebuildjob.py
@@ -55,14 +55,20 @@ class ICharmhubUploadJob(IRunnableJob):
error_detail = TextLine(
title=_("Error message detail"), required=False, readonly=True)
- store_revision = Int(
- title=_("The revision assigned to this build by Charmhub"),
+ upload_id = Int(
+ title=_(
+ "The ID returned by Charmhub when uploading this build's charm "
+ "file."),
required=False, readonly=True)
status_url = TextLine(
title=_("The URL on Charmhub to get the status of this build"),
required=False, readonly=True)
+ store_revision = Int(
+ title=_("The revision assigned to this build by Charmhub"),
+ required=False, readonly=True)
+
class ICharmhubUploadJobSource(IJobSource):
diff --git a/lib/lp/charms/model/charmhubclient.py b/lib/lp/charms/model/charmhubclient.py
index 4b275b0..33ea281 100644
--- a/lib/lp/charms/model/charmhubclient.py
+++ b/lib/lp/charms/model/charmhubclient.py
@@ -147,7 +147,7 @@ class CharmhubClient:
timeline_action.finish()
@classmethod
- def _uploadToStorage(cls, lfa):
+ def uploadFile(cls, lfa):
"""Upload a single file to Charmhub's storage."""
assert config.charms.charmhub_storage_url is not None
unscanned_upload_url = urlappend(
@@ -182,12 +182,10 @@ class CharmhubClient:
lfa.close()
@classmethod
- def _push(cls, build, upload_id):
+ def push(cls, build, upload_id):
"""Push an already-uploaded charm to Charmhub."""
recipe = build.recipe
- assert config.charms.charmhub_url is not None
- assert recipe.store_name is not None
- assert recipe.store_secrets is not None
+ assert recipe.can_upload_to_store
push_url = urlappend(
config.charms.charmhub_url,
"v1/charm/{}/revisions".format(quote(recipe.store_name)))
@@ -218,7 +216,7 @@ class CharmhubClient:
for _, lfa, _ in build.getFiles():
if not lfa.filename.endswith(".charm"):
continue
- upload_id = cls._uploadToStorage(lfa)
+ upload_id = cls.uploadFile(lfa)
return cls._push(build, upload_id)
@classmethod
diff --git a/lib/lp/charms/model/charmrecipebuildjob.py b/lib/lp/charms/model/charmrecipebuildjob.py
index bafe2ba..cfe86a6 100644
--- a/lib/lp/charms/model/charmrecipebuildjob.py
+++ b/lib/lp/charms/model/charmrecipebuildjob.py
@@ -230,16 +230,16 @@ class CharmhubUploadJob(CharmRecipeBuildJobDerived):
self.metadata["error_detail"] = detail
@property
- def store_revision(self):
+ def upload_id(self):
"""See `ICharmhubUploadJob`."""
- return self.store_metadata.get("store_revision")
+ return self.store_metadata.get("upload_id")
- @store_revision.setter
- def store_revision(self, revision):
+ @upload_id.setter
+ def upload_id(self, upload_id):
"""See `ICharmhubUploadJob`."""
if self.build.store_upload_metadata is None:
self.build.store_upload_metadata = {}
- self.build.store_upload_metadata["store_revision"] = revision
+ self.build.store_upload_metadata["upload_id"] = upload_id
@property
def status_url(self):
@@ -252,6 +252,18 @@ class CharmhubUploadJob(CharmRecipeBuildJobDerived):
self.build.store_upload_metadata = {}
self.build.store_upload_metadata["status_url"] = url
+ @property
+ def store_revision(self):
+ """See `ICharmhubUploadJob`."""
+ return self.store_metadata.get("store_revision")
+
+ @store_revision.setter
+ def store_revision(self, revision):
+ """See `ICharmhubUploadJob`."""
+ if self.build.store_upload_metadata is None:
+ self.build.store_upload_metadata = {}
+ self.build.store_upload_metadata["store_revision"] = revision
+
# Ideally we'd just override Job._set_status or similar, but
# lazr.delegates makes that difficult, so we use this to override all
# the individual Job lifecycle methods instead.
@@ -311,8 +323,17 @@ class CharmhubUploadJob(CharmRecipeBuildJobDerived):
client = getUtility(ICharmhubClient)
try:
try:
+ lfa = next((row[1] for row in self.build.getFiles()), None)
+ if lfa is None:
+ # Nothing to do.
+ self.error_message = None
+ return
+ if "upload_id" not in self.store_metadata:
+ self.upload_id = client.uploadFile(lfa)
+ # We made progress, so reset attempt_count.
+ self.attempt_count = 1
if "status_url" not in self.store_metadata:
- self.status_url = client.upload(self.build)
+ self.status_url = client.push(self.build, self.upload_id)
# We made progress, so reset attempt_count.
self.attempt_count = 1
if self.store_revision is None:
diff --git a/lib/lp/charms/tests/test_charmhubclient.py b/lib/lp/charms/tests/test_charmhubclient.py
index 87e9cca..018709d 100644
--- a/lib/lp/charms/tests/test_charmhubclient.py
+++ b/lib/lp/charms/tests/test_charmhubclient.py
@@ -302,18 +302,15 @@ class TestCharmhubClient(TestCaseWithFactory):
return build
@responses.activate
- def test_upload(self):
- self._setUpSecretStorage()
- build = self.makeUploadableCharmRecipeBuild()
+ def test_uploadFile(self):
+ charm_lfa = self.factory.makeLibraryFileAlias(
+ filename="test-charm.charm", content="dummy charm content")
transaction.commit()
self._addUnscannedUploadResponse()
- self._addCharmPushResponse("test-charm")
# XXX cjwatson 2021-08-19: Use
# config.ICharmhubUploadJobSource.dbuser once that job exists.
with dbuser("charm-build-job"):
- self.assertEqual(
- "/v1/charm/test-charm/revisions/review?upload-id=123",
- self.client.upload(build))
+ self.assertEqual(1, self.client.uploadFile(charm_lfa))
requests = [call.request for call in responses.calls]
self.assertThat(requests, MatchesListwise([
RequestMatches(
@@ -326,6 +323,40 @@ class TestCharmhubClient(TestCaseWithFactory):
value="dummy charm content",
content_type="application/octet-stream",
)}),
+ ]))
+
+ @responses.activate
+ def test_uploadFile_error(self):
+ charm_lfa = self.factory.makeLibraryFileAlias(
+ filename="test-charm.charm", content="dummy charm content")
+ transaction.commit()
+ responses.add(
+ "POST", "http://storage.charmhub.example/unscanned-upload/";,
+ status=502, body="The proxy exploded.\n")
+ # XXX cjwatson 2021-08-19: Use
+ # config.ICharmhubUploadJobSource.dbuser once that job exists.
+ with dbuser("charm-build-job"):
+ err = self.assertRaises(
+ UploadFailedResponse, self.client.uploadFile, charm_lfa)
+ self.assertEqual("502 Server Error: Bad Gateway", str(err))
+ self.assertThat(err, MatchesStructure(
+ detail=Equals("The proxy exploded.\n"),
+ can_retry=Is(True)))
+
+ @responses.activate
+ def test_push(self):
+ self._setUpSecretStorage()
+ build = self.makeUploadableCharmRecipeBuild()
+ transaction.commit()
+ self._addCharmPushResponse("test-charm")
+ # XXX cjwatson 2021-08-19: Use
+ # config.ICharmhubUploadJobSource.dbuser once that job exists.
+ with dbuser("charm-build-job"):
+ self.assertEqual(
+ "/v1/charm/test-charm/revisions/review?upload-id=123",
+ self.client.push(build, 1))
+ requests = [call.request for call in responses.calls]
+ self.assertThat(requests, MatchesListwise([
RequestMatches(
url=Equals(
"http://charmhub.example/v1/charm/test-charm/revisions";),
@@ -337,11 +368,10 @@ class TestCharmhubClient(TestCaseWithFactory):
]))
@responses.activate
- def test_upload_unauthorized(self):
+ def test_push_unauthorized(self):
self._setUpSecretStorage()
build = self.makeUploadableCharmRecipeBuild()
transaction.commit()
- self._addUnscannedUploadResponse()
charm_push_error = {
"code": "permission-required",
"message": "Missing required permission: package-manage-revisions",
@@ -356,25 +386,7 @@ class TestCharmhubClient(TestCaseWithFactory):
self.assertRaisesWithContent(
UnauthorizedUploadResponse,
"Missing required permission: package-manage-revisions",
- self.client.upload, build)
-
- @responses.activate
- def test_upload_file_error(self):
- self._setUpSecretStorage()
- build = self.makeUploadableCharmRecipeBuild()
- transaction.commit()
- responses.add(
- "POST", "http://storage.charmhub.example/unscanned-upload/";,
- status=502, body="The proxy exploded.\n")
- # XXX cjwatson 2021-08-19: Use
- # config.ICharmhubUploadJobSource.dbuser once that job exists.
- with dbuser("charm-build-job"):
- err = self.assertRaises(
- UploadFailedResponse, self.client.upload, build)
- self.assertEqual("502 Server Error: Bad Gateway", str(err))
- self.assertThat(err, MatchesStructure(
- detail=Equals("The proxy exploded.\n"),
- can_retry=Is(True)))
+ self.client.push, build, 1)
@responses.activate
def test_checkStatus_pending(self):
diff --git a/lib/lp/charms/tests/test_charmrecipebuildjob.py b/lib/lp/charms/tests/test_charmrecipebuildjob.py
index 9bc020d..d8c1e86 100644
--- a/lib/lp/charms/tests/test_charmrecipebuildjob.py
+++ b/lib/lp/charms/tests/test_charmrecipebuildjob.py
@@ -78,7 +78,8 @@ def run_isolated_jobs(jobs):
class FakeCharmhubClient:
def __init__(self):
- self.upload = FakeMethod()
+ self.uploadFile = FakeMethod()
+ self.push = FakeMethod()
self.checkStatus = FakeMethod()
self.release = FakeMethod()
@@ -129,10 +130,13 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
repr(job))
def makeCharmRecipeBuild(self, **kwargs):
- # Make a build with a builder and a webhook.
+ # Make a build with a builder, a file, and a webhook.
build = self.factory.makeCharmRecipeBuild(
builder=self.factory.makeBuilder(), **kwargs)
build.updateStatus(BuildStatus.FULLYBUILT)
+ charm_lfa = self.factory.makeLibraryFileAlias(
+ filename="test-charm.charm", content="dummy charm content")
+ self.factory.makeCharmFile(build=build, library_file=charm_lfa)
self.factory.makeWebhook(
target=build.recipe, event_types=["charm-recipe:build:0.1"])
return build
@@ -176,15 +180,18 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
# revision.
logger = self.useFixture(FakeLogger())
build = self.makeCharmRecipeBuild()
+ charm_lfa = build.getFiles()[0][1]
self.assertContentEqual([], build.store_upload_jobs)
job = CharmhubUploadJob.create(build)
client = FakeCharmhubClient()
- client.upload.result = self.status_url
+ client.uploadFile.result = 1
+ client.push.result = self.status_url
client.checkStatus.result = 1
self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((build,), {})], client.upload.calls)
+ self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((build, 1), {})], client.push.calls)
self.assertEqual(
[((build, self.status_url), {})], client.checkStatus.calls)
self.assertEqual([], client.release.calls)
@@ -198,14 +205,16 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
# A failed run sets the store upload status to FAILED.
logger = self.useFixture(FakeLogger())
build = self.makeCharmRecipeBuild()
+ charm_lfa = build.getFiles()[0][1]
self.assertContentEqual([], build.store_upload_jobs)
job = CharmhubUploadJob.create(build)
client = FakeCharmhubClient()
- client.upload.failure = ValueError("An upload failure")
+ client.uploadFile.failure = ValueError("An upload failure")
self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((build,), {})], client.upload.calls)
+ self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([], client.push.calls)
self.assertEqual([], client.checkStatus.calls)
self.assertEqual([], client.release.calls)
self.assertContentEqual([job], build.store_upload_jobs)
@@ -225,15 +234,18 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
build = self.makeCharmRecipeBuild(
requester=requester_team, name="test-charm", owner=requester_team,
project=project)
+ charm_lfa = build.getFiles()[0][1]
self.assertContentEqual([], build.store_upload_jobs)
job = CharmhubUploadJob.create(build)
client = FakeCharmhubClient()
- client.upload.failure = UnauthorizedUploadResponse(
+ client.uploadFile.result = 1
+ client.uploadFile.failure = UnauthorizedUploadResponse(
"Authorization failed.")
self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((build,), {})], client.upload.calls)
+ self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([], client.push.calls)
self.assertEqual([], client.checkStatus.calls)
self.assertEqual([], client.release.calls)
self.assertContentEqual([job], build.store_upload_jobs)
@@ -275,15 +287,17 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
# retried.
logger = self.useFixture(FakeLogger())
build = self.makeCharmRecipeBuild()
+ charm_lfa = build.getFiles()[0][1]
self.assertContentEqual([], build.store_upload_jobs)
job = CharmhubUploadJob.create(build)
client = FakeCharmhubClient()
- client.upload.failure = UploadFailedResponse(
+ client.uploadFile.failure = UploadFailedResponse(
"Proxy error", can_retry=True)
self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((build,), {})], client.upload.calls)
+ self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([], client.push.calls)
self.assertEqual([], client.checkStatus.calls)
self.assertEqual([], client.release.calls)
self.assertContentEqual([job], build.store_upload_jobs)
@@ -295,13 +309,15 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
# Try again. The upload part of the job is retried, and this time
# it succeeds.
job.scheduled_start = None
- client.upload.calls = []
- client.upload.failure = None
- client.upload.result = self.status_url
+ client.uploadFile.calls = []
+ client.uploadFile.failure = None
+ client.uploadFile.result = 1
+ client.push.result = self.status_url
client.checkStatus.result = 1
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((build,), {})], client.upload.calls)
+ self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((build, 1), {})], client.push.calls)
self.assertEqual(
[((build, self.status_url), {})], client.checkStatus.calls)
self.assertEqual([], client.release.calls)
@@ -323,15 +339,17 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
build = self.makeCharmRecipeBuild(
requester=requester_team, name="test-charm", owner=requester_team,
project=project)
+ charm_lfa = build.getFiles()[0][1]
self.assertContentEqual([], build.store_upload_jobs)
job = CharmhubUploadJob.create(build)
client = FakeCharmhubClient()
- client.upload.failure = UploadFailedResponse(
+ client.uploadFile.failure = UploadFailedResponse(
"Failed to upload", detail="The proxy exploded.\n")
self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((build,), {})], client.upload.calls)
+ self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([], client.push.calls)
self.assertEqual([], client.checkStatus.calls)
self.assertEqual([], client.release.calls)
self.assertContentEqual([job], build.store_upload_jobs)
@@ -374,15 +392,18 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
# charm schedules itself to be retried.
logger = self.useFixture(FakeLogger())
build = self.makeCharmRecipeBuild()
+ charm_lfa = build.getFiles()[0][1]
self.assertContentEqual([], build.store_upload_jobs)
job = CharmhubUploadJob.create(build)
client = FakeCharmhubClient()
- client.upload.result = self.status_url
+ client.uploadFile.result = 2
+ client.push.result = self.status_url
client.checkStatus.failure = UploadNotReviewedYetResponse()
self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((build,), {})], client.upload.calls)
+ self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((build, 2), {})], client.push.calls)
self.assertEqual(
[((build, self.status_url), {})], client.checkStatus.calls)
self.assertEqual([], client.release.calls)
@@ -392,16 +413,18 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
self.assertEqual([], pop_notifications())
self.assertEqual(JobStatus.WAITING, job.job.status)
self.assertWebhookDeliveries(build, ["Pending"], logger)
- # Try again. The upload part of the job is not retried, and this
- # time the review completes.
+ # Try again. The upload and push parts of the job are not retried,
+ # and this time the review completes.
job.scheduled_start = None
- client.upload.calls = []
+ client.uploadFile.calls = []
+ client.push.calls = []
client.checkStatus.calls = []
client.checkStatus.failure = None
client.checkStatus.result = 1
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([], client.upload.calls)
+ self.assertEqual([], client.uploadFile.calls)
+ self.assertEqual([], client.push.calls)
self.assertEqual(
[((build, self.status_url), {})], client.checkStatus.calls)
self.assertEqual([], client.release.calls)
@@ -422,16 +445,19 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
build = self.makeCharmRecipeBuild(
requester=requester_team, name="test-charm", owner=requester_team,
project=project)
+ charm_lfa = build.getFiles()[0][1]
self.assertContentEqual([], build.store_upload_jobs)
job = CharmhubUploadJob.create(build)
client = FakeCharmhubClient()
- client.upload.result = self.status_url
+ client.uploadFile.result = 2
+ client.push.result = self.status_url
client.checkStatus.failure = ReviewFailedResponse(
"Review failed.\nCharm is terrible.")
self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((build,), {})], client.upload.calls)
+ self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((build, 2), {})], client.push.calls)
self.assertEqual(
[((build, self.status_url), {})], client.checkStatus.calls)
self.assertEqual([], client.release.calls)
@@ -472,15 +498,18 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
# channels does so.
logger = self.useFixture(FakeLogger())
build = self.makeCharmRecipeBuild(store_channels=["stable", "edge"])
+ charm_lfa = build.getFiles()[0][1]
self.assertContentEqual([], build.store_upload_jobs)
job = CharmhubUploadJob.create(build)
client = FakeCharmhubClient()
- client.upload.result = self.status_url
+ client.uploadFile.result = 1
+ client.push.result = self.status_url
client.checkStatus.result = 1
self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((build,), {})], client.upload.calls)
+ self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((build, 1), {})], client.push.calls)
self.assertEqual(
[((build, self.status_url), {})], client.checkStatus.calls)
self.assertEqual([((build, 1), {})], client.release.calls)
@@ -501,16 +530,19 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
build = self.makeCharmRecipeBuild(
requester=requester_team, name="test-charm", owner=requester_team,
project=project, store_channels=["stable", "edge"])
+ charm_lfa = build.getFiles()[0][1]
self.assertContentEqual([], build.store_upload_jobs)
job = CharmhubUploadJob.create(build)
client = FakeCharmhubClient()
- client.upload.result = self.status_url
+ client.uploadFile.result = 1
+ client.push.result = self.status_url
client.checkStatus.result = 1
client.release.failure = ReleaseFailedResponse("Failed to release")
self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
JobRunner([job]).runAll()
- self.assertEqual([((build,), {})], client.upload.calls)
+ self.assertEqual([((charm_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((build, 1), {})], client.push.calls)
self.assertEqual(
[((build, self.status_url), {})], client.checkStatus.calls)
self.assertEqual([((build, 1), {})], client.release.calls)
@@ -552,16 +584,26 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
build = self.makeCharmRecipeBuild()
job = CharmhubUploadJob.create(build)
client = FakeCharmhubClient()
- client.upload.failure = UploadFailedResponse(
+ client.uploadFile.failure = UploadFailedResponse(
"Proxy error", can_retry=True)
self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertNotIn("status_url", job.metadata)
+ self.assertNotIn("upload_id", job.store_metadata)
self.assertEqual(timedelta(seconds=60), job.retry_delay)
job.scheduled_start = None
- client.upload.failure = None
- client.upload.result = self.status_url
+ client.uploadFile.failure = None
+ client.uploadFile.result = 1
+ client.push.failure = UploadFailedResponse(
+ "Proxy error", can_retry=True)
+ with dbuser(config.ICharmhubUploadJobSource.dbuser):
+ run_isolated_jobs([job])
+ self.assertIn("upload_id", job.build.store_upload_metadata)
+ self.assertNotIn("status_url", job.store_metadata)
+ self.assertEqual(timedelta(seconds=60), job.retry_delay)
+ job.scheduled_start = None
+ client.push.failure = None
+ client.push.result = self.status_url
client.checkStatus.failure = UploadNotReviewedYetResponse()
for expected_delay in (15, 15, 30, 30, 60):
with dbuser(config.ICharmhubUploadJobSource.dbuser):
@@ -579,39 +621,80 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
self.assertEqual(JobStatus.COMPLETED, job.job.status)
def test_retry_after_upload_does_not_upload(self):
- # If the job has uploaded, but failed to release, it should
- # not attempt to upload again on the next run.
+ # If the job has uploaded but failed to push, it should not attempt
+ # to upload again on the next run.
self.useFixture(FakeLogger())
build = self.makeCharmRecipeBuild(store_channels=["stable", "edge"])
self.assertContentEqual([], build.store_upload_jobs)
job = CharmhubUploadJob.create(build)
client = FakeCharmhubClient()
- client.upload.result = self.status_url
- client.checkStatus.result = 1
- client.release.failure = ReleaseFailedResponse(
+ client.uploadFile.result = 1
+ client.push.failure = UploadFailedResponse(
"Proxy error", can_retry=True)
self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- previous_upload = client.upload.calls
- previous_checkStatus = client.checkStatus.calls
- len_previous_release = len(client.release.calls)
+ # We performed the upload as expected, but the store failed to release
+ # it.
+ self.assertIsNone(job.store_revision)
+ self.assertEqual(timedelta(seconds=60), job.retry_delay)
+ self.assertEqual(1, len(client.uploadFile.calls))
+ self.assertIsNone(job.error_message)
+
+ # Run the job again.
+ client.uploadFile.calls = []
+ client.push.calls = []
+ client.push.failure = None
+ client.push.result = self.status_url
+ client.checkStatus.result = 1
+ with dbuser(config.ICharmhubUploadJobSource.dbuser):
+ run_isolated_jobs([job])
- # Check we uploaded as expected
+ # The push has now succeeded. Make sure that we didn't try to
+ # upload the file again first.
self.assertEqual(1, job.store_revision)
- self.assertEqual(timedelta(seconds=60), job.retry_delay)
- self.assertEqual(1, len(client.upload.calls))
+ self.assertEqual([], client.uploadFile.calls)
+ self.assertEqual(1, len(client.push.calls))
self.assertIsNone(job.error_message)
- # Run the job again
+ def test_retry_after_push_does_not_upload_or_push(self):
+ # If the job has uploaded and pushed but has not yet been reviewed,
+ # it should not attempt to upload or push again on the next run.
+ self.useFixture(FakeLogger())
+ build = self.makeCharmRecipeBuild(store_channels=["stable", "edge"])
+ self.assertContentEqual([], build.store_upload_jobs)
+ job = CharmhubUploadJob.create(build)
+ client = FakeCharmhubClient()
+ client.uploadFile.result = 1
+ client.push.result = self.status_url
+ client.checkStatus.failure = UploadNotReviewedYetResponse()
+ self.useFixture(ZopeUtilityFixture(client, ICharmhubClient))
with dbuser(config.ICharmhubUploadJobSource.dbuser):
run_isolated_jobs([job])
- # We should not have called `upload`, but moved straight to `release`
- self.assertEqual(previous_upload, client.upload.calls)
- self.assertEqual(previous_checkStatus, client.checkStatus.calls)
- self.assertEqual(len_previous_release + 1, len(client.release.calls))
+ # We performed the upload as expected, but the store is still
+ # reviewing it.
+ self.assertIsNone(job.store_revision)
+ self.assertEqual(timedelta(seconds=15), job.retry_delay)
+ self.assertEqual(1, len(client.uploadFile.calls))
+ self.assertIsNone(job.error_message)
+
+ # Run the job again.
+ client.uploadFile.calls = []
+ client.push.calls = []
+ client.checkStatus.calls = []
+ client.checkStatus.failure = None
+ client.checkStatus.result = 1
+ with dbuser(config.ICharmhubUploadJobSource.dbuser):
+ run_isolated_jobs([job])
+
+ # The store has now reviewed the upload. Make sure that we didn't
+ # try to upload or push it again first.
+ self.assertEqual(1, job.store_revision)
+ self.assertEqual([], client.uploadFile.calls)
+ self.assertEqual([], client.push.calls)
+ self.assertEqual(1, len(client.checkStatus.calls))
self.assertIsNone(job.error_message)
def test_with_build_metadata_as_none(self):
diff --git a/lib/lp/snappy/interfaces/snapbuildjob.py b/lib/lp/snappy/interfaces/snapbuildjob.py
index 3b174f4..826bc07 100644
--- a/lib/lp/snappy/interfaces/snapbuildjob.py
+++ b/lib/lp/snappy/interfaces/snapbuildjob.py
@@ -61,6 +61,16 @@ class ISnapStoreUploadJob(IRunnableJob):
error_detail = TextLine(
title=_("Error message detail"), required=False, readonly=True)
+ upload_id = Int(
+ title=_(
+ "The ID returned by the store when uploading this build's snap "
+ "file."),
+ required=False, readonly=True)
+
+ status_url = TextLine(
+ title=_("The URL on the store to get the status of this build"),
+ required=False, readonly=True)
+
store_url = TextLine(
title=_("The URL on the store corresponding to this build"),
required=False, readonly=True)
@@ -69,10 +79,6 @@ class ISnapStoreUploadJob(IRunnableJob):
title=_("The revision assigned to this build by the store"),
required=False, readonly=True)
- status_url = TextLine(
- title=_("The URL on the store to get the status of this build"),
- required=False, readonly=True)
-
class ISnapStoreUploadJobSource(IJobSource):
diff --git a/lib/lp/snappy/interfaces/snapstoreclient.py b/lib/lp/snappy/interfaces/snapstoreclient.py
index e9ff638..f4db218 100644
--- a/lib/lp/snappy/interfaces/snapstoreclient.py
+++ b/lib/lp/snappy/interfaces/snapstoreclient.py
@@ -88,10 +88,20 @@ class ISnapStoreClient(Interface):
this snap.
"""
- def upload(snapbuild):
- """Upload a snap build to the store.
+ def uploadFile(lfa):
+ """Upload a file to the store.
+
+ :param lfa: The `ILibraryFileAlias` to upload.
+ :return: An upload ID.
+ :raises UploadFailedResponse: if uploading the file to the store
+ failed.
+ """
+
+ def push(snapbuild, upload_id):
+ """Push a snap build to the store.
:param snapbuild: The `ISnapBuild` to upload.
+ :param upload_id: An upload ID previously returned by `uploadFile`.
:return: A URL to poll for upload processing status.
:raises BadRefreshResponse: if the authorising macaroons need to be
refreshed, but attempting to do so fails.
diff --git a/lib/lp/snappy/model/snapbuildjob.py b/lib/lp/snappy/model/snapbuildjob.py
index a8fc49f..8e79973 100644
--- a/lib/lp/snappy/model/snapbuildjob.py
+++ b/lib/lp/snappy/model/snapbuildjob.py
@@ -1,4 +1,4 @@
-# Copyright 2016-2019 Canonical Ltd. This software is licensed under the
+# Copyright 2016-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Snap build jobs."""
@@ -241,6 +241,29 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
self.metadata["error_messages"] = messages
@property
+ def upload_id(self):
+ """See `ISnapStoreUploadJob`."""
+ return self.store_metadata.get("upload_id")
+
+ @upload_id.setter
+ def upload_id(self, upload_id):
+ """See `ISnapStoreUploadJob`."""
+ if self.snapbuild.store_upload_metadata is None:
+ self.snapbuild.store_upload_metadata = {}
+ self.snapbuild.store_upload_metadata["upload_id"] = upload_id
+
+ @property
+ def status_url(self):
+ """See `ISnapStoreUploadJob`."""
+ return self.store_metadata.get("status_url")
+
+ @status_url.setter
+ def status_url(self, url):
+ if self.snapbuild.store_upload_metadata is None:
+ self.snapbuild.store_upload_metadata = {}
+ self.snapbuild.store_upload_metadata["status_url"] = url
+
+ @property
def store_url(self):
"""See `ISnapStoreUploadJob`."""
return self.store_metadata.get("store_url")
@@ -265,17 +288,6 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
self.snapbuild.store_upload_metadata["store_revision"] = revision
self.snapbuild._store_upload_revision = revision
- @property
- def status_url(self):
- """See `ISnapStoreUploadJob`."""
- return self.store_metadata.get('status_url')
-
- @status_url.setter
- def status_url(self, url):
- if self.snapbuild.store_upload_metadata is None:
- self.snapbuild.store_upload_metadata = {}
- self.snapbuild.store_upload_metadata["status_url"] = url
-
# Ideally we'd just override Job._set_status or similar, but
# lazr.delegates makes that difficult, so we use this to override all
# the individual Job lifecycle methods instead.
@@ -333,8 +345,18 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
client = getUtility(ISnapStoreClient)
try:
try:
+ lfa = next((row[1] for row in self.snapbuild.getFiles()), None)
+ if lfa is None:
+ # Nothing to do.
+ self.error_message = None
+ return
+ if "upload_id" not in self.store_metadata:
+ self.upload_id = client.uploadFile(lfa)
+ # We made progress, so reset attempt_count.
+ self.attempt_count = 1
if "status_url" not in self.store_metadata:
- self.status_url = client.upload(self.snapbuild)
+ self.status_url = client.push(
+ self.snapbuild, self.upload_id)
# We made progress, so reset attempt_count.
self.attempt_count = 1
if self.store_url is None:
diff --git a/lib/lp/snappy/model/snapstoreclient.py b/lib/lp/snappy/model/snapstoreclient.py
index 3d1e65d..91b05c5 100644
--- a/lib/lp/snappy/model/snapstoreclient.py
+++ b/lib/lp/snappy/model/snapstoreclient.py
@@ -237,7 +237,7 @@ class SnapStoreClient:
timeline_action.finish()
@classmethod
- def _uploadFile(cls, lfa, lfc):
+ def uploadFile(cls, lfa):
"""Upload a single file."""
assert config.snappy.store_upload_url is not None
unscanned_upload_url = urlappend(
@@ -262,24 +262,22 @@ class SnapStoreClient:
response_data = response.json()
if not response_data.get("successful", False):
raise UploadFailedResponse(response.text)
- return {"upload_id": response_data["upload_id"]}
+ return response_data["upload_id"]
except requests.HTTPError as e:
raise cls._makeSnapStoreError(UploadFailedResponse, e)
finally:
lfa.close()
@classmethod
- def _uploadApp(cls, snapbuild, upload_data):
+ def _push(cls, snapbuild, upload_id):
"""Create a new store upload based on the uploaded file."""
snap = snapbuild.snap
- assert config.snappy.store_url is not None
- assert snap.store_name is not None
- assert snap.store_secrets is not None
+ assert snap.can_upload_to_store
assert snapbuild.date_started is not None
upload_url = urlappend(config.snappy.store_url, "dev/api/snap-push/")
data = {
"name": snap.store_name,
- "updown_id": upload_data["upload_id"],
+ "updown_id": upload_id,
"series": snap.store_series.name,
"built_at": snapbuild.date_started.isoformat(),
}
@@ -312,15 +310,10 @@ class SnapStoreClient:
raise cls._makeSnapStoreError(UploadFailedResponse, e)
@classmethod
- def upload(cls, snapbuild):
+ def push(cls, snapbuild, upload_id):
"""See `ISnapStoreClient`."""
- assert snapbuild.snap.can_upload_to_store
- for _, lfa, lfc in snapbuild.getFiles():
- if not lfa.filename.endswith(".snap"):
- continue
- upload_data = cls._uploadFile(lfa, lfc)
- return cls.refreshIfNecessary(
- snapbuild.snap, cls._uploadApp, snapbuild, upload_data)
+ return cls.refreshIfNecessary(
+ snapbuild.snap, cls._push, snapbuild, upload_id)
@classmethod
def refreshDischargeMacaroon(cls, snap):
diff --git a/lib/lp/snappy/tests/test_snapbuildjob.py b/lib/lp/snappy/tests/test_snapbuildjob.py
index 5b56264..1c1f316 100644
--- a/lib/lp/snappy/tests/test_snapbuildjob.py
+++ b/lib/lp/snappy/tests/test_snapbuildjob.py
@@ -1,4 +1,4 @@
-# Copyright 2016-2019 Canonical Ltd. This software is licensed under the
+# Copyright 2016-2021 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for snap build jobs."""
@@ -73,7 +73,8 @@ def run_isolated_jobs(jobs):
class FakeSnapStoreClient:
def __init__(self):
- self.upload = FakeMethod()
+ self.uploadFile = FakeMethod()
+ self.push = FakeMethod()
self.checkStatus = FakeMethod()
self.listChannels = FakeMethod(result=[])
@@ -120,10 +121,13 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
repr(job))
def makeSnapBuild(self, **kwargs):
- # Make a build with a builder and a webhook.
+ # Make a build with a builder, a file, and a webhook.
snapbuild = self.factory.makeSnapBuild(
builder=self.factory.makeBuilder(), **kwargs)
snapbuild.updateStatus(BuildStatus.FULLYBUILT)
+ snap_lfa = self.factory.makeLibraryFileAlias(
+ filename="test-snap.snap", content=b"dummy snap content")
+ self.factory.makeSnapFile(snapbuild=snapbuild, libraryfile=snap_lfa)
self.factory.makeWebhook(
target=snapbuild.snap, event_types=["snap:build:0.1"])
return snapbuild
@@ -165,15 +169,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
# and revision.
logger = self.useFixture(FakeLogger())
snapbuild = self.makeSnapBuild()
+ snap_lfa = snapbuild.getFiles()[0][1]
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.result = self.status_url
+ client.uploadFile.result = 1
+ client.push.result = self.status_url
client.checkStatus.result = (self.store_url, 1)
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((snapbuild,), {})], client.upload.calls)
+ self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((snapbuild, 1), {})], client.push.calls)
self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertEqual(self.store_url, job.store_url)
@@ -189,14 +196,16 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
# A failed run sets the store upload status to FAILED.
logger = self.useFixture(FakeLogger())
snapbuild = self.makeSnapBuild()
+ snap_lfa = snapbuild.getFiles()[0][1]
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.failure = ValueError("An upload failure")
+ client.uploadFile.failure = ValueError("An upload failure")
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((snapbuild,), {})], client.upload.calls)
+ self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([], client.push.calls)
self.assertEqual([], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertIsNone(job.store_url)
@@ -216,15 +225,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
owner=requester, name="requester-team", members=[requester])
snapbuild = self.makeSnapBuild(
requester=requester_team, name="test-snap", owner=requester_team)
+ snap_lfa = snapbuild.getFiles()[0][1]
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.failure = UnauthorizedUploadResponse(
+ client.uploadFile.result = 1
+ client.push.failure = UnauthorizedUploadResponse(
"Authorization failed.")
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((snapbuild,), {})], client.upload.calls)
+ self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((snapbuild, 1), {})], client.push.calls)
self.assertEqual([], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertIsNone(job.store_url)
@@ -265,15 +277,17 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
# retried.
logger = self.useFixture(FakeLogger())
snapbuild = self.makeSnapBuild()
+ snap_lfa = snapbuild.getFiles()[0][1]
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.failure = UploadFailedResponse(
+ client.uploadFile.failure = UploadFailedResponse(
"Proxy error", can_retry=True)
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((snapbuild,), {})], client.upload.calls)
+ self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([], client.push.calls)
self.assertEqual([], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertIsNone(job.store_url)
@@ -287,13 +301,15 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
# Try again. The upload part of the job is retried, and this time
# it succeeds.
job.scheduled_start = None
- client.upload.calls = []
- client.upload.failure = None
- client.upload.result = self.status_url
+ client.uploadFile.calls = []
+ client.uploadFile.failure = None
+ client.uploadFile.result = 1
+ client.push.result = self.status_url
client.checkStatus.result = (self.store_url, 1)
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((snapbuild,), {})], client.upload.calls)
+ self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((snapbuild, 1), {})], client.push.calls)
self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertEqual(self.store_url, job.store_url)
@@ -315,14 +331,17 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
owner=requester, name="requester-team", members=[requester])
snapbuild = self.makeSnapBuild(
requester=requester_team, name="test-snap", owner=requester_team)
+ snap_lfa = snapbuild.getFiles()[0][1]
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.failure = BadRefreshResponse("SSO melted.")
+ client.uploadFile.result = 1
+ client.push.failure = BadRefreshResponse("SSO melted.")
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((snapbuild,), {})], client.upload.calls)
+ self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((snapbuild, 1), {})], client.push.calls)
self.assertEqual([], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertIsNone(job.store_url)
@@ -368,15 +387,17 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
owner=requester, name="requester-team", members=[requester])
snapbuild = self.makeSnapBuild(
requester=requester_team, name="test-snap", owner=requester_team)
+ snap_lfa = snapbuild.getFiles()[0][1]
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.failure = UploadFailedResponse(
+ client.uploadFile.failure = UploadFailedResponse(
"Failed to upload", detail="The proxy exploded.\n")
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((snapbuild,), {})], client.upload.calls)
+ self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([], client.push.calls)
self.assertEqual([], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertIsNone(job.store_url)
@@ -420,15 +441,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
# package schedules itself to be retried.
logger = self.useFixture(FakeLogger())
snapbuild = self.makeSnapBuild()
+ snap_lfa = snapbuild.getFiles()[0][1]
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.result = self.status_url
+ client.uploadFile.result = 2
+ client.push.result = self.status_url
client.checkStatus.failure = UploadNotScannedYetResponse()
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((snapbuild,), {})], client.upload.calls)
+ self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((snapbuild, 2), {})], client.push.calls)
self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertIsNone(job.store_url)
@@ -439,16 +463,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
self.assertEqual([], pop_notifications())
self.assertEqual(JobStatus.WAITING, job.job.status)
self.assertWebhookDeliveries(snapbuild, ["Pending"], logger)
- # Try again. The upload part of the job is not retried, and this
- # time the scan completes.
+ # Try again. The upload and push parts of the job are not retried,
+ # and this time the scan completes.
job.scheduled_start = None
- client.upload.calls = []
+ client.uploadFile.calls = []
+ client.push.calls = []
client.checkStatus.calls = []
client.checkStatus.failure = None
client.checkStatus.result = (self.store_url, 1)
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([], client.upload.calls)
+ self.assertEqual([], client.uploadFile.calls)
+ self.assertEqual([], client.push.calls)
self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertEqual(self.store_url, job.store_url)
@@ -469,10 +495,12 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
owner=requester, name="requester-team", members=[requester])
snapbuild = self.makeSnapBuild(
requester=requester_team, name="test-snap", owner=requester_team)
+ snap_lfa = snapbuild.getFiles()[0][1]
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.result = self.status_url
+ client.uploadFile.result = 2
+ client.push.result = self.status_url
client.checkStatus.failure = ScanFailedResponse(
"Scan failed.\nConfinement not allowed.",
messages=[
@@ -481,7 +509,8 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((snapbuild,), {})], client.upload.calls)
+ self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((snapbuild, 2), {})], client.push.calls)
self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertIsNone(job.store_url)
@@ -526,15 +555,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
# URL or revision.
logger = self.useFixture(FakeLogger())
snapbuild = self.makeSnapBuild()
+ snap_lfa = snapbuild.getFiles()[0][1]
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.result = self.status_url
+ client.uploadFile.result = 1
+ client.push.result = self.status_url
client.checkStatus.result = (None, None)
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((snapbuild,), {})], client.upload.calls)
+ self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((snapbuild, 1), {})], client.push.calls)
self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertIsNone(job.store_url)
@@ -551,15 +583,18 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
# channels does so.
logger = self.useFixture(FakeLogger())
snapbuild = self.makeSnapBuild(store_channels=["stable", "edge"])
+ snap_lfa = snapbuild.getFiles()[0][1]
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.result = self.status_url
+ client.uploadFile.result = 1
+ client.push.result = self.status_url
client.checkStatus.result = (self.store_url, 1)
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertEqual([((snapbuild,), {})], client.upload.calls)
+ self.assertEqual([((snap_lfa,), {})], client.uploadFile.calls)
+ self.assertEqual([((snapbuild, 1), {})], client.push.calls)
self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
self.assertContentEqual([job], snapbuild.store_upload_jobs)
self.assertEqual(self.store_url, job.store_url)
@@ -579,16 +614,26 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
snapbuild = self.makeSnapBuild()
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.failure = UploadFailedResponse(
+ client.uploadFile.failure = UploadFailedResponse(
"Proxy error", can_retry=True)
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- self.assertNotIn("status_url", job.metadata)
+ self.assertNotIn("upload_id", job.store_metadata)
self.assertEqual(timedelta(seconds=60), job.retry_delay)
job.scheduled_start = None
- client.upload.failure = None
- client.upload.result = self.status_url
+ client.uploadFile.failure = None
+ client.uploadFile.result = 1
+ client.push.failure = UploadFailedResponse(
+ "Proxy error", can_retry=True)
+ with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+ run_isolated_jobs([job])
+ self.assertIn("upload_id", job.snapbuild.store_upload_metadata)
+ self.assertNotIn("status_url", job.store_metadata)
+ self.assertEqual(timedelta(seconds=60), job.retry_delay)
+ job.scheduled_start = None
+ client.push.failure = None
+ client.push.result = self.status_url
client.checkStatus.failure = UploadNotScannedYetResponse()
for expected_delay in (15, 15, 30, 30, 60):
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
@@ -608,39 +653,88 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
self.assertEqual(JobStatus.COMPLETED, job.job.status)
def test_retry_after_upload_does_not_upload(self):
- # If the job has uploaded, but failed to release, it should
- # not attempt to upload again on the next run.
+ # If the job has uploaded but failed to push, it should not attempt
+ # to upload again on the next run.
self.useFixture(FakeLogger())
snapbuild = self.makeSnapBuild(store_channels=["stable", "edge"])
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.result = self.status_url
- client.checkStatus.result = (self.store_url, 1)
+ client.uploadFile.result = 1
+ client.push.failure = UploadFailedResponse(
+ "Proxy error", can_retry=True)
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- previous_upload = client.upload.calls
- previous_checkStatus = client.checkStatus.calls
+ # We performed the upload as expected, but the push failed.
+ self.assertIsNone(job.store_url)
+ self.assertIsNone(job.store_revision)
+ self.assertIsNone(
+ removeSecurityProxy(snapbuild)._store_upload_revision)
+ self.assertEqual(timedelta(seconds=60), job.retry_delay)
+ self.assertEqual(1, len(client.uploadFile.calls))
+ self.assertIsNone(job.error_message)
+
+ # Run the job again.
+ client.uploadFile.calls = []
+ client.push.calls = []
+ client.push.failure = None
+ client.push.result = self.status_url
+ client.checkStatus.result = (self.store_url, 1)
+ with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+ run_isolated_jobs([job])
- # Check we uploaded as expected
+ # The push has now succeeded. Make sure that we didn't try to
+ # upload the file again first.
self.assertEqual(self.store_url, job.store_url)
self.assertEqual(1, job.store_revision)
- self.assertEqual(1,
- removeSecurityProxy(snapbuild)._store_upload_revision)
- self.assertEqual(timedelta(seconds=60), job.retry_delay)
- self.assertEqual(1, len(client.upload.calls))
+ self.assertEqual([], client.uploadFile.calls)
+ self.assertEqual(1, len(client.push.calls))
self.assertIsNone(job.error_message)
- # Run the job again
+ def test_retry_after_push_does_not_upload_or_push(self):
+ # If the job has uploaded and pushed but has not yet been scanned,
+ # it should not attempt to upload or push again on the next run.
+ self.useFixture(FakeLogger())
+ snapbuild = self.makeSnapBuild(store_channels=["stable", "edge"])
+ self.assertContentEqual([], snapbuild.store_upload_jobs)
+ job = SnapStoreUploadJob.create(snapbuild)
+ client = FakeSnapStoreClient()
+ client.uploadFile.result = 1
+ client.push.result = self.status_url
+ client.checkStatus.failure = UploadNotScannedYetResponse()
+ self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+ with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+ run_isolated_jobs([job])
+
+ # We performed the upload and push as expected, but the store is
+ # still scanning it.
+ self.assertIsNone(job.store_url)
+ self.assertIsNone(job.store_revision)
+ self.assertIsNone(
+ removeSecurityProxy(snapbuild)._store_upload_revision)
+ self.assertEqual(timedelta(seconds=15), job.retry_delay)
+ self.assertEqual(1, len(client.uploadFile.calls))
+ self.assertEqual(1, len(client.push.calls))
+ self.assertIsNone(job.error_message)
+
+ # Run the job again.
+ client.uploadFile.calls = []
+ client.push.calls = []
+ client.checkStatus.calls = []
+ client.checkStatus.failure = None
+ client.checkStatus.result = (self.store_url, 1)
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
run_isolated_jobs([job])
- # Release is not called due to release intent in upload
- # but ensure that we have not called upload twice
- self.assertEqual(previous_upload, client.upload.calls)
- self.assertEqual(previous_checkStatus, client.checkStatus.calls)
+ # The store has now scanned the upload. Make sure that we didn't
+ # try to upload or push it again first.
+ self.assertEqual(self.store_url, job.store_url)
+ self.assertEqual(1, job.store_revision)
+ self.assertEqual([], client.uploadFile.calls)
+ self.assertEqual([], client.push.calls)
+ self.assertEqual(1, len(client.checkStatus.calls))
self.assertIsNone(job.error_message)
def test_with_snapbuild_metadata_as_none(self):
diff --git a/lib/lp/snappy/tests/test_snapstoreclient.py b/lib/lp/snappy/tests/test_snapstoreclient.py
index c738171..3cc26e2 100644
--- a/lib/lp/snappy/tests/test_snapstoreclient.py
+++ b/lib/lp/snappy/tests/test_snapstoreclient.py
@@ -361,15 +361,13 @@ class TestSnapStoreClient(TestCaseWithFactory):
return snapbuild
@responses.activate
- def test_upload(self):
- snapbuild = self.makeUploadableSnapBuild()
+ def test_uploadFile(self):
+ snap_lfa = self.factory.makeLibraryFileAlias(
+ filename="test-snap.snap", content=b"dummy snap content")
transaction.commit()
self._addUnscannedUploadResponse()
- self._addSnapPushResponse()
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
- self.assertEqual(
- "http://sca.example/dev/api/snaps/1/builds/1/status";,
- self.client.upload(snapbuild))
+ self.assertEqual(1, self.client.uploadFile(snap_lfa))
requests = [call.request for call in responses.calls]
self.assertThat(requests, MatchesListwise([
RequestMatches(
@@ -381,6 +379,34 @@ class TestSnapStoreClient(TestCaseWithFactory):
value=b"dummy snap content",
type="application/octet-stream",
)}),
+ ]))
+
+ @responses.activate
+ def test_uploadFile_error(self):
+ snap_lfa = self.factory.makeLibraryFileAlias(
+ filename="test-snap.snap", content=b"dummy snap content")
+ transaction.commit()
+ responses.add(
+ "POST", "http://updown.example/unscanned-upload/";, status=502,
+ body="The proxy exploded.\n")
+ with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+ err = self.assertRaises(
+ UploadFailedResponse, self.client.uploadFile, snap_lfa)
+ self.assertEqual("502 Server Error: Bad Gateway", str(err))
+ self.assertEqual("The proxy exploded.\n", err.detail)
+ self.assertTrue(err.can_retry)
+
+ @responses.activate
+ def test_push(self):
+ snapbuild = self.makeUploadableSnapBuild()
+ transaction.commit()
+ self._addSnapPushResponse()
+ with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+ self.assertEqual(
+ "http://sca.example/dev/api/snaps/1/builds/1/status";,
+ self.client.push(snapbuild, 1))
+ requests = [call.request for call in responses.calls]
+ self.assertThat(requests, MatchesListwise([
RequestMatches(
url=Equals("http://sca.example/dev/api/snap-push/";),
method=Equals("POST"),
@@ -394,28 +420,18 @@ class TestSnapStoreClient(TestCaseWithFactory):
]))
@responses.activate
- def test_upload_with_release_intent(self):
+ def test_push_with_release_intent(self):
snapbuild = self.makeUploadableSnapBuild()
snapbuild.snap.store_channels = ['beta', 'edge']
transaction.commit()
- self._addUnscannedUploadResponse()
self._addSnapPushResponse()
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
self.assertEqual(
"http://sca.example/dev/api/snaps/1/builds/1/status";,
- self.client.upload(snapbuild))
+ self.client.push(snapbuild, 1))
requests = [call.request for call in responses.calls]
self.assertThat(requests, MatchesListwise([
RequestMatches(
- url=Equals("http://updown.example/unscanned-upload/";),
- method=Equals("POST"),
- form_data={
- "binary": MatchesStructure.byEquality(
- name="binary", filename="test-snap.snap",
- value=b"dummy snap content",
- type="application/octet-stream",
- )}),
- RequestMatches(
url=Equals("http://sca.example/dev/api/snap-push/";),
method=Equals("POST"),
headers=ContainsDict(
@@ -429,30 +445,20 @@ class TestSnapStoreClient(TestCaseWithFactory):
]))
@responses.activate
- def test_upload_no_discharge(self):
+ def test_push_no_discharge(self):
root_key = hashlib.sha256(self.factory.getUniqueBytes()).hexdigest()
root_macaroon = Macaroon(key=root_key)
snapbuild = self.makeUploadableSnapBuild(
store_secrets={"root": root_macaroon.serialize()})
transaction.commit()
- self._addUnscannedUploadResponse()
self._addSnapPushResponse()
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
self.assertEqual(
"http://sca.example/dev/api/snaps/1/builds/1/status";,
- self.client.upload(snapbuild))
+ self.client.push(snapbuild, 1))
requests = [call.request for call in responses.calls]
self.assertThat(requests, MatchesListwise([
RequestMatches(
- url=Equals("http://updown.example/unscanned-upload/";),
- method=Equals("POST"),
- form_data={
- "binary": MatchesStructure.byEquality(
- name="binary", filename="test-snap.snap",
- value=b"dummy snap content",
- type="application/octet-stream",
- )}),
- RequestMatches(
url=Equals("http://sca.example/dev/api/snap-push/";),
method=Equals("POST"),
headers=ContainsDict(
@@ -465,7 +471,7 @@ class TestSnapStoreClient(TestCaseWithFactory):
]))
@responses.activate
- def test_upload_encrypted_discharge(self):
+ def test_push_encrypted_discharge(self):
private_key = PrivateKey.generate()
self.pushConfig(
"snappy",
@@ -475,24 +481,14 @@ class TestSnapStoreClient(TestCaseWithFactory):
bytes(private_key)).decode("UTF-8"))
snapbuild = self.makeUploadableSnapBuild(encrypted=True)
transaction.commit()
- self._addUnscannedUploadResponse()
self._addSnapPushResponse()
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
self.assertEqual(
"http://sca.example/dev/api/snaps/1/builds/1/status";,
- self.client.upload(snapbuild))
+ self.client.push(snapbuild, 1))
requests = [call.request for call in responses.calls]
self.assertThat(requests, MatchesListwise([
RequestMatches(
- url=Equals("http://updown.example/unscanned-upload/";),
- method=Equals("POST"),
- form_data={
- "binary": MatchesStructure.byEquality(
- name="binary", filename="test-snap.snap",
- value=b"dummy snap content",
- type="application/octet-stream",
- )}),
- RequestMatches(
url=Equals("http://sca.example/dev/api/snap-push/";),
method=Equals("POST"),
headers=ContainsDict(
@@ -505,11 +501,10 @@ class TestSnapStoreClient(TestCaseWithFactory):
]))
@responses.activate
- def test_upload_unauthorized(self):
+ def test_push_unauthorized(self):
store_secrets = self._make_store_secrets()
snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets)
transaction.commit()
- self._addUnscannedUploadResponse()
snap_push_error = {
"code": "macaroon-permission-required",
"message": "Permission is required: package_push",
@@ -522,14 +517,13 @@ class TestSnapStoreClient(TestCaseWithFactory):
self.assertRaisesWithContent(
UnauthorizedUploadResponse,
"Permission is required: package_push",
- self.client.upload, snapbuild)
+ self.client.push, snapbuild, 1)
@responses.activate
- def test_upload_needs_discharge_macaroon_refresh(self):
+ def test_push_needs_discharge_macaroon_refresh(self):
store_secrets = self._make_store_secrets()
snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets)
transaction.commit()
- self._addUnscannedUploadResponse()
responses.add(
"POST", "http://sca.example/dev/api/snap-push/";, status=401,
headers={"WWW-Authenticate": "Macaroon needs_refresh=1"})
@@ -538,10 +532,9 @@ class TestSnapStoreClient(TestCaseWithFactory):
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
self.assertEqual(
"http://sca.example/dev/api/snaps/1/builds/1/status";,
- self.client.upload(snapbuild))
+ self.client.push(snapbuild, 1))
requests = [call.request for call in responses.calls]
self.assertThat(requests, MatchesListwise([
- MatchesStructure.byEquality(path_url="/unscanned-upload/"),
MatchesStructure.byEquality(path_url="/dev/api/snap-push/"),
MatchesStructure.byEquality(path_url="/api/v2/tokens/refresh"),
MatchesStructure.byEquality(path_url="/dev/api/snap-push/"),
@@ -551,7 +544,7 @@ class TestSnapStoreClient(TestCaseWithFactory):
snapbuild.snap.store_secrets["discharge"])
@responses.activate
- def test_upload_needs_encrypted_discharge_macaroon_refresh(self):
+ def test_push_needs_encrypted_discharge_macaroon_refresh(self):
private_key = PrivateKey.generate()
self.pushConfig(
"snappy",
@@ -562,7 +555,6 @@ class TestSnapStoreClient(TestCaseWithFactory):
store_secrets = self._make_store_secrets(encrypted=True)
snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets)
transaction.commit()
- self._addUnscannedUploadResponse()
responses.add(
"POST", "http://sca.example/dev/api/snap-push/";, status=401,
headers={"WWW-Authenticate": "Macaroon needs_refresh=1"})
@@ -571,10 +563,9 @@ class TestSnapStoreClient(TestCaseWithFactory):
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
self.assertEqual(
"http://sca.example/dev/api/snaps/1/builds/1/status";,
- self.client.upload(snapbuild))
+ self.client.push(snapbuild, 1))
requests = [call.request for call in responses.calls]
self.assertThat(requests, MatchesListwise([
- MatchesStructure.byEquality(path_url="/unscanned-upload/"),
MatchesStructure.byEquality(path_url="/dev/api/snap-push/"),
MatchesStructure.byEquality(path_url="/api/v2/tokens/refresh"),
MatchesStructure.byEquality(path_url="/dev/api/snap-push/"),
@@ -584,37 +575,21 @@ class TestSnapStoreClient(TestCaseWithFactory):
snapbuild.snap.store_secrets["discharge_encrypted"])
@responses.activate
- def test_upload_unsigned_agreement(self):
+ def test_push_unsigned_agreement(self):
store_secrets = self._make_store_secrets()
snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets)
transaction.commit()
- self._addUnscannedUploadResponse()
snap_push_error = {"message": "Developer has not signed agreement."}
responses.add(
"POST", "http://sca.example/dev/api/snap-push/";, status=403,
json={"error_list": [snap_push_error]})
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
err = self.assertRaises(
- UploadFailedResponse, self.client.upload, snapbuild)
+ UploadFailedResponse, self.client.push, snapbuild, 1)
self.assertEqual("Developer has not signed agreement.", str(err))
self.assertFalse(err.can_retry)
@responses.activate
- def test_upload_file_error(self):
- store_secrets = self._make_store_secrets()
- snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets)
- transaction.commit()
- responses.add(
- "POST", "http://updown.example/unscanned-upload/";, status=502,
- body="The proxy exploded.\n")
- with dbuser(config.ISnapStoreUploadJobSource.dbuser):
- err = self.assertRaises(
- UploadFailedResponse, self.client.upload, snapbuild)
- self.assertEqual("502 Server Error: Bad Gateway", str(err))
- self.assertEqual("The proxy exploded.\n", err.detail)
- self.assertTrue(err.can_retry)
-
- @responses.activate
def test_refresh_discharge_macaroon(self):
store_secrets = self._make_store_secrets()
snap = self.factory.makeSnap(
