launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #30997
[Merge] ~pelpsi/launchpad:snap-component-integration into launchpad:master
You have been requested to review the proposed merge of ~pelpsi/launchpad:snap-component-integration into launchpad:master.
For more details, see:
https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/461063
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:snap-component-integration into launchpad:master.
diff --git a/lib/lp/snappy/interfaces/snapbuildjob.py b/lib/lp/snappy/interfaces/snapbuildjob.py
index 12e4b8b..90a8d37 100644
--- a/lib/lp/snappy/interfaces/snapbuildjob.py
+++ b/lib/lp/snappy/interfaces/snapbuildjob.py
@@ -13,7 +13,7 @@ __all__ = [
from lazr.restful.fields import Reference
from zope.interface import Attribute, Interface
from zope.interface.interfaces import IObjectEvent
-from zope.schema import Int, TextLine
+from zope.schema import Dict, Int, TextLine
from lp import _
from lp.services.job.interfaces.job import IJob, IJobSource, IRunnableJob
@@ -68,6 +68,17 @@ class ISnapStoreUploadJob(IRunnableJob):
readonly=True,
)
+ components_ids = Dict(
+ title=_(
+ "The IDs returned by the store when uploading snap components."
+ "The key is the component name and the value is the related id."
+ ),
+ key_type=TextLine(),
+ value_type=TextLine(),
+ required=False,
+ readonly=True,
+ )
+
status_url = TextLine(
title=_("The URL on the store to get the status of this build"),
required=False,
diff --git a/lib/lp/snappy/model/snapbuildjob.py b/lib/lp/snappy/model/snapbuildjob.py
index 539fb1c..80260c1 100644
--- a/lib/lp/snappy/model/snapbuildjob.py
+++ b/lib/lp/snappy/model/snapbuildjob.py
@@ -29,6 +29,12 @@ from lp.services.database.stormbase import StormBase
from lp.services.job.model.job import EnumeratedSubclass, Job
from lp.services.job.runner import BaseRunnableJob
from lp.services.propertycache import get_property_cache
+from lp.snappy.interfaces.snap import (
+ CannotFetchSnapcraftYaml,
+ CannotParseSnapcraftYaml,
+ ISnapSet,
+ MissingSnapcraftYaml,
+)
from lp.snappy.interfaces.snapbuildjob import (
ISnapBuildJob,
ISnapBuildStoreUploadStatusChangedEvent,
@@ -244,6 +250,18 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
self.snapbuild.store_upload_metadata["upload_id"] = upload_id
@property
+ def components_ids(self):
+ """See `ISnapStoreUploadJob`."""
+ return self.store_metadata.get("components_ids")
+
+ @components_ids.setter
+ def components_ids(self, components_ids):
+ """See `ISnapStoreUploadJob`."""
+ if self.snapbuild.store_upload_metadata is None:
+ self.snapbuild.store_upload_metadata = {}
+ self.snapbuild.store_upload_metadata["components_ids"] = components_ids
+
+ @property
def status_url(self):
"""See `ISnapStoreUploadJob`."""
return self.store_metadata.get("status_url")
@@ -333,6 +351,24 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
pass
return timedelta(minutes=1)
+ @staticmethod
+ def _extract_component_name(filename, snapcraft_components):
+ """Extract component name from filename
+
+ Use the snapcraft_components keys to extract the component name
+ from the built component filename. Each key correspond to
+ <component_name> while the built filename has the following pattern:
+ <snap_name>+<component_name>_<component_revision>.comp.
+ """
+ for component in snapcraft_components:
+ # Surround component name with `+` and `_` to avoid
+ # superset of the same component name.
+ # ie. testcomp -> testcomponent
+ built_component = "+%s_" % component
+ if built_component in filename:
+ return component
+ return filename
+
def run(self):
"""See `IRunnableJob`."""
client = getUtility(ISnapStoreClient)
@@ -350,16 +386,54 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
# Nothing to do.
self.error_message = None
return
+
+ components_ids = {}
+ components = []
+ try:
+ # Get components from snapcraft.yaml
+ snapcraft_yaml = getUtility(ISnapSet).getSnapcraftYaml(
+ self.snapbuild.snap
+ )
+ if snapcraft_yaml["components"]:
+ for component in snapcraft_yaml["components"]:
+ components_ids[component] = None
+ # Get built components from built files
+ for _, lfa, _ in self.snapbuild.getFiles():
+ if lfa.filename.endswith(".comp"):
+ components.append(lfa)
+ except (
+ MissingSnapcraftYaml,
+ CannotFetchSnapcraftYaml,
+ CannotParseSnapcraftYaml,
+ ):
+ pass
+
+ if "components_ids" not in self.store_metadata:
+ self.components_ids = components_ids
if "upload_id" not in self.store_metadata:
self.upload_id = client.uploadFile(snap_lfa)
# We made progress, so reset attempt_count.
self.attempt_count = 1
+ # Process components
+ for component in components:
+ # if the id is None, we need to upload the component
+ # because it means that that component was never uploaded.
+ # Note that the id is returned directly from SnapStore API.
+ comp_name = self._extract_component_name(
+ component.filename, components_ids
+ )
+ if self.components_ids.get(comp_name) == None:
+ self.components_ids[comp_name] = client.uploadFile(
+ component
+ )
+ self.attempt_count = 1
if "status_url" not in self.store_metadata:
self.status_url = client.push(
- self.snapbuild, self.upload_id
+ self.snapbuild, self.upload_id, self.components_ids
)
# We made progress, so reset attempt_count.
self.attempt_count = 1
+
if self.store_url is None:
# This is no longer strictly necessary as the store is
# handling releases via the release intent, but we
diff --git a/lib/lp/snappy/model/snapstoreclient.py b/lib/lp/snappy/model/snapstoreclient.py
index 536ba17..98d2753 100644
--- a/lib/lp/snappy/model/snapstoreclient.py
+++ b/lib/lp/snappy/model/snapstoreclient.py
@@ -293,7 +293,7 @@ class SnapStoreClient:
lfa.close()
@classmethod
- def _push(cls, snapbuild, upload_id):
+ def _push(cls, snapbuild, upload_id, components=None):
"""Create a new store upload based on the uploaded file."""
snap = snapbuild.snap
assert snap.can_upload_to_store
@@ -305,6 +305,10 @@ class SnapStoreClient:
"series": snap.store_series.name,
"built_at": snapbuild.date_started.isoformat(),
}
+
+ if components:
+ data["components"] = components
+
# The security proxy is useless and breaks JSON serialisation.
channels = removeSecurityProxy(snap.store_channels)
if channels:
@@ -343,10 +347,10 @@ class SnapStoreClient:
raise cls._makeSnapStoreError(UploadFailedResponse, e)
@classmethod
- def push(cls, snapbuild, upload_id):
+ def push(cls, snapbuild, upload_id, components=None):
"""See `ISnapStoreClient`."""
return cls.refreshIfNecessary(
- snapbuild.snap, cls._push, snapbuild, upload_id
+ snapbuild.snap, cls._push, snapbuild, upload_id, components
)
@classmethod
diff --git a/lib/lp/snappy/tests/test_snapbuildjob.py b/lib/lp/snappy/tests/test_snapbuildjob.py
index 7e69f3c..d5239ae 100644
--- a/lib/lp/snappy/tests/test_snapbuildjob.py
+++ b/lib/lp/snappy/tests/test_snapbuildjob.py
@@ -18,6 +18,7 @@ from zope.interface import implementer
from zope.security.proxy import removeSecurityProxy
from lp.buildmaster.enums import BuildStatus
+from lp.code.tests.helpers import BranchHostingFixture
from lp.services.config import config
from lp.services.database.interfaces import IStore
from lp.services.job.interfaces.job import JobStatus
@@ -61,10 +62,30 @@ def run_isolated_jobs(jobs):
transaction.abort()
+class FakeMethodWithMaxCalls(FakeMethod):
+ """Method that throws error after a given number of calls"""
+
+ def __init__(self, max_calls=-1):
+ self.max_calls = max_calls
+ super().__init__(result=1, failure=None)
+
+ def __call__(self, *args, **kwargs):
+ if self.call_count == self.max_calls:
+ self.failure = UploadFailedResponse("Proxy error", can_retry=True)
+ self.result = None
+ else:
+ self.result = 1
+ self.failure = None
+ return super().__call__(*args, **kwargs)
+
+
@implementer(ISnapStoreClient)
class FakeSnapStoreClient:
- def __init__(self):
- self.uploadFile = FakeMethod()
+ def __init__(self, max_calls=-1):
+ if max_calls >= 0:
+ self.uploadFile = FakeMethodWithMaxCalls(max_calls)
+ else:
+ self.uploadFile = FakeMethod()
self.push = FakeMethod()
self.checkStatus = FakeMethod()
@@ -144,6 +165,49 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
)
return snapbuild
+ def makeSnapBuildWithComponents(self, num_components):
+ # Make a build with a builder, components, and a webhook.
+ snapcraft_yaml = """
+ name : test-snap,
+ components:
+ """
+ branch = self.factory.makeBranch()
+ snap = self.factory.makeSnap(store_name="test-snap", branch=branch)
+ snapbuild = self.factory.makeSnapBuild(
+ snap=snap, builder=self.factory.makeBuilder()
+ )
+ snapbuild.updateStatus(BuildStatus.FULLYBUILT)
+ snap_lfa = self.factory.makeLibraryFileAlias(
+ filename="test-snap_0_all.snap", content=b"dummy snap content"
+ )
+ for i in range(num_components):
+ self.factory.makeSnapFile(
+ snapbuild=snapbuild, libraryfile=snap_lfa
+ )
+ component_lfa = self.factory.makeLibraryFileAlias(
+ filename="test-snap+somecomponent%s_0.comp" % i,
+ content=b"component",
+ )
+ self.factory.makeSnapFile(
+ snapbuild=snapbuild, libraryfile=component_lfa
+ )
+ snapcraft_yaml += (
+ """
+ somecomponent%s:
+ description: test
+ """
+ % i
+ )
+ self.useFixture(
+ BranchHostingFixture(
+ blob=bytes(snapcraft_yaml, "utf-8"),
+ )
+ )
+ self.factory.makeWebhook(
+ target=snapbuild.snap, event_types=["snap:build:0.1"]
+ )
+ return snapbuild
+
def assertWebhookDeliveries(
self, snapbuild, expected_store_upload_statuses, logger
):
@@ -207,7 +271,101 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
self.assertThat(
client.uploadFile.calls, FileUploaded("test-snap.snap")
)
- self.assertEqual([((snapbuild, 1), {})], client.push.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)
+ self.assertEqual(1, job.store_revision)
+ self.assertEqual(1, snapbuild.store_upload_revision)
+ self.assertIsNone(job.error_message)
+ self.assertEqual([], pop_notifications())
+ self.assertWebhookDeliveries(
+ snapbuild, ["Pending", "Uploaded"], logger
+ )
+
+ def test_run_with_component(self):
+ # The job uploads the build to the storage with its component
+ # and then pushes it to the store and records the store URL
+ # and revision.
+ logger = self.useFixture(FakeLogger())
+ snapbuild = self.makeSnapBuildWithComponents(1)
+ self.assertContentEqual([], snapbuild.store_upload_jobs)
+ job = SnapStoreUploadJob.create(snapbuild)
+ client = FakeSnapStoreClient()
+ 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])
+
+ # Check if uploadFile is called for snap and its component.
+ self.assertThat(
+ [client.uploadFile.calls[0]], FileUploaded("test-snap_0_all.snap")
+ )
+ self.assertThat(
+ [client.uploadFile.calls[1]],
+ FileUploaded("test-snap+somecomponent0_0.comp"),
+ )
+ self.assertEqual(
+ [((snapbuild, 1, {"somecomponent0": 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)
+ self.assertEqual(1, job.store_revision)
+ self.assertEqual(1, snapbuild.store_upload_revision)
+ self.assertIsNone(job.error_message)
+ self.assertEqual([], pop_notifications())
+ self.assertWebhookDeliveries(
+ snapbuild, ["Pending", "Uploaded"], logger
+ )
+
+ def test_run_with_multiple_components(self):
+ # The job uploads the build to the storage with its components
+ # and then pushes it to the store and records the store URL
+ # and revision.
+ logger = self.useFixture(FakeLogger())
+ snapbuild = self.makeSnapBuildWithComponents(2)
+ self.assertContentEqual([], snapbuild.store_upload_jobs)
+ job = SnapStoreUploadJob.create(snapbuild)
+ client = FakeSnapStoreClient()
+ 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])
+
+ # Check if uploadFile is called for snap and its components.
+ self.assertThat(
+ [client.uploadFile.calls[0]], FileUploaded("test-snap_0_all.snap")
+ )
+ self.assertThat(
+ [client.uploadFile.calls[1]],
+ FileUploaded("test-snap+somecomponent0_0.comp"),
+ )
+ self.assertThat(
+ [client.uploadFile.calls[2]],
+ FileUploaded("test-snap+somecomponent1_0.comp"),
+ )
+ self.assertEqual(
+ [
+ (
+ (
+ snapbuild,
+ 1,
+ {
+ "somecomponent0": 1,
+ "somecomponent1": 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)
@@ -268,7 +426,7 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
self.assertThat(
client.uploadFile.calls, FileUploaded("test-snap.snap")
)
- self.assertEqual([((snapbuild, 1), {})], client.push.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)
@@ -313,6 +471,106 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
snapbuild, ["Pending", "Failed to upload"], logger
)
+ def test_run_502_retries_with_components(self):
+ # A run that gets a 502 error from the store schedules itself to be
+ # retried.
+ logger = self.useFixture(FakeLogger())
+ snapbuild = self.makeSnapBuildWithComponents(2)
+ self.assertContentEqual([], snapbuild.store_upload_jobs)
+ job = SnapStoreUploadJob.create(snapbuild)
+
+ # The error raises on the third uploadFile call.
+ client = FakeSnapStoreClient(2)
+ client.uploadFile.result = 1
+ self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+ with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+ run_isolated_jobs([job])
+ self.assertThat(
+ [client.uploadFile.calls[0]], FileUploaded("test-snap_0_all.snap")
+ )
+ self.assertThat(
+ [client.uploadFile.calls[1]],
+ FileUploaded("test-snap+somecomponent0_0.comp"),
+ )
+ self.assertThat(
+ [client.uploadFile.calls[2]],
+ FileUploaded("test-snap+somecomponent1_0.comp"),
+ )
+ self.assertEqual([], client.push.calls)
+ self.assertEqual([], client.checkStatus.calls)
+ self.assertContentEqual([job], snapbuild.store_upload_jobs)
+
+ # Component upload is incomplete.
+ self.assertEqual(
+ {
+ "somecomponent0": 1,
+ "somecomponent1": None,
+ },
+ job.components_ids,
+ )
+ self.assertIsNone(job.store_url)
+ self.assertIsNone(job.store_revision)
+ self.assertIsNone(snapbuild.store_upload_revision)
+ self.assertIsNone(job.error_message)
+ 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 retried, and this time
+ # it succeeds.
+ job.scheduled_start = None
+ client.uploadFile.max_calls = -1
+ 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])
+
+ # The last call should contains just the component that failed.
+ self.assertEqual(len(client.uploadFile.calls), 4)
+ self.assertThat(
+ [client.uploadFile.calls[3]],
+ FileUploaded("test-snap+somecomponent1_0.comp"),
+ )
+
+ self.assertEqual(
+ {
+ "somecomponent0": 1,
+ "somecomponent1": 1,
+ },
+ job.components_ids,
+ )
+
+ # After that the snap is uploaded to the store.
+ self.assertEqual(
+ [
+ (
+ (
+ snapbuild,
+ 1,
+ {
+ "somecomponent0": 1,
+ "somecomponent1": 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)
+ self.assertEqual(1, job.store_revision)
+ self.assertEqual(1, snapbuild.store_upload_revision)
+ self.assertIsNone(job.error_message)
+ self.assertEqual([], pop_notifications())
+ self.assertEqual(JobStatus.COMPLETED, job.job.status)
+ self.assertWebhookDeliveries(
+ snapbuild, ["Pending", "Uploaded"], logger
+ )
+
def test_run_502_retries(self):
# A run that gets a 502 error from the store schedules itself to be
# retried.
@@ -353,7 +611,7 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
self.assertThat(
client.uploadFile.calls, FileUploaded("test-snap.snap")
)
- self.assertEqual([((snapbuild, 1), {})], client.push.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)
@@ -388,7 +646,7 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
self.assertThat(
client.uploadFile.calls, FileUploaded("test-snap.snap")
)
- self.assertEqual([((snapbuild, 1), {})], client.push.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)
@@ -524,7 +782,7 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
self.assertThat(
client.uploadFile.calls, FileUploaded("test-snap.snap")
)
- self.assertEqual([((snapbuild, 2), {})], client.push.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)
@@ -586,7 +844,7 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
self.assertThat(
client.uploadFile.calls, FileUploaded("test-snap.snap")
)
- self.assertEqual([((snapbuild, 2), {})], client.push.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)
@@ -655,7 +913,7 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
self.assertThat(
client.uploadFile.calls, FileUploaded("test-snap.snap")
)
- self.assertEqual([((snapbuild, 1), {})], client.push.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)
@@ -684,7 +942,7 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
self.assertThat(
client.uploadFile.calls, FileUploaded("test-snap.snap")
)
- self.assertEqual([((snapbuild, 1), {})], client.push.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)
References
