launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24585
[Merge] ~cjwatson/launchpad:oci-recipe-build-cancel-rescore into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:oci-recipe-build-cancel-rescore into launchpad:master with ~cjwatson/launchpad:fix-oci-recipe-build-ui as a prerequisite.
Commit message:
Implement cancel and rescore for OCI recipe builds
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1847444 in Launchpad itself: "Support OCI image building"
https://bugs.launchpad.net/launchpad/+bug/1847444
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/382098
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:oci-recipe-build-cancel-rescore into launchpad:master.
diff --git a/lib/lp/oci/browser/configure.zcml b/lib/lp/oci/browser/configure.zcml
index 941cccc..3fbb0f1 100644
--- a/lib/lp/oci/browser/configure.zcml
+++ b/lib/lp/oci/browser/configure.zcml
@@ -69,6 +69,9 @@
for="lp.oci.interfaces.ocirecipebuild.IOCIRecipeBuild"
path_expression="string:+build/${id}"
attribute_to_parent="recipe" />
+ <browser:menus
+ module="lp.oci.browser.ocirecipebuild"
+ classes="OCIRecipeBuildContextMenu" />
<browser:navigation
module="lp.oci.browser.ocirecipebuild"
classes="OCIRecipeBuildNavigation" />
@@ -81,6 +84,18 @@
permission="launchpad.View"
name="+index"
template="../templates/ocirecipebuild-index.pt" />
+ <browser:page
+ for="lp.oci.interfaces.ocirecipebuild.IOCIRecipeBuild"
+ class="lp.oci.browser.ocirecipebuild.OCIRecipeBuildCancelView"
+ permission="launchpad.Edit"
+ name="+cancel"
+ template="../../app/templates/generic-edit.pt" />
+ <browser:page
+ for="lp.oci.interfaces.ocirecipebuild.IOCIRecipeBuild"
+ class="lp.oci.browser.ocirecipebuild.OCIRecipeBuildRescoreView"
+ permission="launchpad.Admin"
+ name="+rescore"
+ template="../../app/templates/generic-edit.pt" />
<adapter
provides="lp.services.webapp.interfaces.IBreadcrumb"
for="lp.oci.interfaces.ocirecipebuild.IOCIRecipeBuild"
diff --git a/lib/lp/oci/browser/ocirecipebuild.py b/lib/lp/oci/browser/ocirecipebuild.py
index 8b3a727..dfed462 100644
--- a/lib/lp/oci/browser/ocirecipebuild.py
+++ b/lib/lp/oci/browser/ocirecipebuild.py
@@ -7,18 +7,28 @@ from __future__ import absolute_import, print_function, unicode_literals
__metaclass__ = type
__all__ = [
+ 'OCIRecipeBuildCancelView',
+ 'OCIRecipeBuildContextMenu',
'OCIRecipeBuildNavigation',
+ 'OCIRecipeBuildRescoreView',
'OCIRecipeBuildView',
]
from zope.interface import Interface
-from lp.app.browser.launchpadform import LaunchpadFormView
+from lp.app.browser.launchpadform import (
+ action,
+ LaunchpadFormView,
+ )
from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuild
from lp.services.webapp import (
canonical_url,
+ ContextMenu,
+ enabled_with_permission,
+ Link,
Navigation,
)
+from lp.soyuz.interfaces.binarypackagebuild import IBuildRescoreForm
class OCIRecipeBuildNavigation(Navigation):
@@ -26,6 +36,28 @@ class OCIRecipeBuildNavigation(Navigation):
usedfor = IOCIRecipeBuild
+class OCIRecipeBuildContextMenu(ContextMenu):
+ """Context menu for OCI recipe builds."""
+
+ usedfor = IOCIRecipeBuild
+
+ facet = "overview"
+
+ links = ("cancel", "rescore")
+
+ @enabled_with_permission("launchpad.Edit")
+ def cancel(self):
+ return Link(
+ "+cancel", "Cancel build", icon="remove",
+ enabled=self.context.can_be_cancelled)
+
+ @enabled_with_permission("launchpad.Admin")
+ def rescore(self):
+ return Link(
+ "+rescore", "Rescore build", icon="edit",
+ enabled=self.context.can_be_rescored)
+
+
class OCIRecipeBuildView(LaunchpadFormView):
"""Default view of an OCIRecipeBuild."""
@@ -41,3 +73,53 @@ class OCIRecipeBuildView(LaunchpadFormView):
@property
def next_url(self):
return canonical_url(self.context)
+
+
+class OCIRecipeBuildCancelView(LaunchpadFormView):
+ """View for cancelling an OCI recipe build."""
+
+ class schema(Interface):
+ """Schema for cancelling a build."""
+
+ page_title = label = "Cancel build"
+
+ @property
+ def cancel_url(self):
+ return canonical_url(self.context)
+ next_url = cancel_url
+
+ @action("Cancel build", name="cancel")
+ def request_action(self, action, data):
+ """Cancel the build."""
+ self.context.cancel()
+
+
+class OCIRecipeBuildRescoreView(LaunchpadFormView):
+ """View for rescoring an OCI recipe build."""
+
+ schema = IBuildRescoreForm
+
+ page_title = label = "Rescore build"
+
+ def __call__(self):
+ if self.context.can_be_rescored:
+ return super(OCIRecipeBuildRescoreView, self).__call__()
+ self.request.response.addWarningNotification(
+ "Cannot rescore this build because it is not queued.")
+ self.request.response.redirect(canonical_url(self.context))
+
+ @property
+ def cancel_url(self):
+ return canonical_url(self.context)
+ next_url = cancel_url
+
+ @action("Rescore build", name="rescore")
+ def request_action(self, action, data):
+ """Rescore the build."""
+ score = data.get("priority")
+ self.context.rescore(score)
+ self.request.response.addNotification("Build rescored to %s." % score)
+
+ @property
+ def initial_values(self):
+ return {"score": str(self.context.buildqueue_record.lastscore)}
diff --git a/lib/lp/oci/browser/tests/test_ocirecipebuild.py b/lib/lp/oci/browser/tests/test_ocirecipebuild.py
index 97afb77..9c6eae3 100644
--- a/lib/lp/oci/browser/tests/test_ocirecipebuild.py
+++ b/lib/lp/oci/browser/tests/test_ocirecipebuild.py
@@ -9,15 +9,25 @@ __metaclass__ = type
import re
+from fixtures import FakeLogger
+import soupmatchers
from storm.locals import Store
from testtools.matchers import StartsWith
+import transaction
+from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
+from zope.testbrowser.browser import LinkNotFoundError
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.buildmaster.enums import BuildStatus
from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE
from lp.services.features.testing import FeatureFixture
from lp.services.webapp import canonical_url
from lp.testing import (
+ ANONYMOUS,
BrowserTestCase,
+ login,
+ person_logged_in,
TestCaseWithFactory,
)
from lp.testing.layers import (
@@ -27,6 +37,7 @@ from lp.testing.layers import (
from lp.testing.pages import (
extract_text,
find_main_content,
+ find_tags_by_class,
)
@@ -86,9 +97,100 @@ class TestOCIRecipeBuildOperations(BrowserTestCase):
def setUp(self):
super(TestOCIRecipeBuildOperations, self).setUp()
+ self.useFixture(FakeLogger())
self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
self.build = self.factory.makeOCIRecipeBuild()
self.build_url = canonical_url(self.build)
+ self.requester = self.build.requester
+ self.buildd_admin = self.factory.makePerson(
+ member_of=[getUtility(ILaunchpadCelebrities).buildd_admin])
+
+ def test_cancel_build(self):
+ # The requester of a build can cancel it.
+ self.build.queueBuild()
+ transaction.commit()
+ browser = self.getViewBrowser(self.build, user=self.requester)
+ browser.getLink("Cancel build").click()
+ self.assertEqual(self.build_url, browser.getLink("Cancel").url)
+ browser.getControl("Cancel build").click()
+ self.assertEqual(self.build_url, browser.url)
+ login(ANONYMOUS)
+ self.assertEqual(BuildStatus.CANCELLED, self.build.status)
+
+ def test_cancel_build_random_user(self):
+ # An unrelated non-admin user cannot cancel a build.
+ self.build.queueBuild()
+ transaction.commit()
+ user = self.factory.makePerson()
+ browser = self.getViewBrowser(self.build, user=user)
+ self.assertRaises(LinkNotFoundError, browser.getLink, "Cancel build")
+ self.assertRaises(
+ Unauthorized, self.getUserBrowser, self.build_url + "/+cancel",
+ user=user)
+
+ def test_cancel_build_wrong_state(self):
+ # If the build isn't queued, you can't cancel it.
+ browser = self.getViewBrowser(self.build, user=self.requester)
+ self.assertRaises(LinkNotFoundError, browser.getLink, "Cancel build")
+
+ def test_rescore_build(self):
+ # A buildd admin can rescore a build.
+ self.build.queueBuild()
+ transaction.commit()
+ browser = self.getViewBrowser(self.build, user=self.buildd_admin)
+ browser.getLink("Rescore build").click()
+ self.assertEqual(self.build_url, browser.getLink("Cancel").url)
+ browser.getControl("Priority").value = "1024"
+ browser.getControl("Rescore build").click()
+ self.assertEqual(self.build_url, browser.url)
+ login(ANONYMOUS)
+ self.assertEqual(1024, self.build.buildqueue_record.lastscore)
+
+ def test_rescore_build_invalid_score(self):
+ # Build scores can only take numbers.
+ self.build.queueBuild()
+ transaction.commit()
+ browser = self.getViewBrowser(self.build, user=self.buildd_admin)
+ browser.getLink("Rescore build").click()
+ self.assertEqual(self.build_url, browser.getLink("Cancel").url)
+ browser.getControl("Priority").value = "tentwentyfour"
+ browser.getControl("Rescore build").click()
+ self.assertEqual(
+ "Invalid integer data",
+ extract_text(find_tags_by_class(browser.contents, "message")[1]))
+
+ def test_rescore_build_not_admin(self):
+ # A non-admin user cannot cancel a build.
+ self.build.queueBuild()
+ transaction.commit()
+ user = self.factory.makePerson()
+ browser = self.getViewBrowser(self.build, user=user)
+ self.assertRaises(LinkNotFoundError, browser.getLink, "Rescore build")
+ self.assertRaises(
+ Unauthorized, self.getUserBrowser, self.build_url + "/+rescore",
+ user=user)
+
+ def test_rescore_build_wrong_state(self):
+ # If the build isn't NEEDSBUILD, you can't rescore it.
+ self.build.queueBuild()
+ with person_logged_in(self.requester):
+ self.build.cancel()
+ browser = self.getViewBrowser(self.build, user=self.buildd_admin)
+ self.assertRaises(LinkNotFoundError, browser.getLink, "Rescore build")
+
+ def test_rescore_build_wrong_state_stale_link(self):
+ # An attempt to rescore a non-queued build from a stale link shows a
+ # sensible error message.
+ self.build.queueBuild()
+ with person_logged_in(self.requester):
+ self.build.cancel()
+ browser = self.getViewBrowser(
+ self.build, "+rescore", user=self.buildd_admin)
+ self.assertEqual(self.build_url, browser.url)
+ self.assertThat(browser.contents, soupmatchers.HTMLContains(
+ soupmatchers.Tag(
+ "notification", "div", attrs={"class": "warning message"},
+ text="Cannot rescore this build because it is not queued.")))
def test_builder_history(self):
Store.of(self.build).flush()
diff --git a/lib/lp/oci/interfaces/ocirecipebuild.py b/lib/lp/oci/interfaces/ocirecipebuild.py
index a41e2c8..752e61d 100644
--- a/lib/lp/oci/interfaces/ocirecipebuild.py
+++ b/lib/lp/oci/interfaces/ocirecipebuild.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).
"""Interfaces for a build record for OCI recipes."""
@@ -17,6 +17,7 @@ from zope.interface import Interface
from zope.schema import (
Bool,
Datetime,
+ Int,
TextLine,
)
@@ -30,20 +31,8 @@ from lp.services.librarian.interfaces import ILibraryFileAlias
from lp.soyuz.interfaces.distroarchseries import IDistroArchSeries
-class IOCIRecipeBuildEdit(Interface):
-
- # XXX twom 2020-02-10 This will probably need cancel() implementing
-
- def addFile(lfa, layer_file_digest):
- """Add an OCI file to this build.
-
- :param lfa: An `ILibraryFileAlias`.
- :param layer_file_digest: Digest for this file, used for image layers.
- :return: An `IOCILayerFile`.
- """
-
-
class IOCIRecipeBuildView(IPackageBuild):
+ """`IOCIRecipeBuild` attributes that require launchpad.View permission."""
requester = PublicPersonChoice(
title=_("Requester"),
@@ -88,10 +77,52 @@ class IOCIRecipeBuildView(IPackageBuild):
title=_("The series and architecture for which to build."),
required=True, readonly=True)
+ score = Int(
+ title=_("Score of the related build farm job (if any)."),
+ required=False, readonly=True)
+
+ can_be_rescored = Bool(
+ title=_("Can be rescored"),
+ required=True, readonly=True,
+ description=_("Whether this build record can be rescored manually."))
+
+ can_be_cancelled = Bool(
+ title=_("Can be cancelled"),
+ required=True, readonly=True,
+ description=_("Whether this build record can be cancelled."))
+
+
+class IOCIRecipeBuildEdit(Interface):
+ """`IOCIRecipeBuild` attributes that require launchpad.Edit permission."""
+
+ def addFile(lfa, layer_file_digest):
+ """Add an OCI file to this build.
+
+ :param lfa: An `ILibraryFileAlias`.
+ :param layer_file_digest: Digest for this file, used for image layers.
+ :return: An `IOCILayerFile`.
+ """
+
+ def cancel():
+ """Cancel the build if it is either pending or in progress.
+
+ Check the can_be_cancelled property prior to calling this method to
+ find out if cancelling the build is possible.
+
+ If the build is in progress, it is marked as CANCELLING until the
+ buildd manager terminates the build and marks it CANCELLED. If the
+ build is not in progress, it is marked CANCELLED immediately and is
+ removed from the build queue.
+
+ If the build is not in a cancellable state, this method is a no-op.
+ """
+
class IOCIRecipeBuildAdmin(Interface):
- # XXX twom 2020-02-10 This will probably need rescore() implementing
- pass
+ """`IOCIRecipeBuild` attributes that require launchpad.Admin permission."""
+
+ def rescore(score):
+ """Change the build's score."""
class IOCIRecipeBuild(IOCIRecipeBuildAdmin, IOCIRecipeBuildEdit,
diff --git a/lib/lp/oci/model/ocirecipebuild.py b/lib/lp/oci/model/ocirecipebuild.py
index d8bd496..21c1cce 100644
--- a/lib/lp/oci/model/ocirecipebuild.py
+++ b/lib/lp/oci/model/ocirecipebuild.py
@@ -163,6 +163,47 @@ class OCIRecipeBuild(PackageBuildMixin, Storm):
self.recipe.oci_project.pillar.name, self.recipe.oci_project.name,
self.recipe.name)
+ @property
+ def score(self):
+ """See `IOCIRecipeBuild`."""
+ if self.buildqueue_record is None:
+ return None
+ else:
+ return self.buildqueue_record.lastscore
+
+ @property
+ def can_be_rescored(self):
+ """See `IOCIRecipeBuild`."""
+ return (
+ self.buildqueue_record is not None and
+ self.status is BuildStatus.NEEDSBUILD)
+
+ @property
+ def can_be_cancelled(self):
+ """See `IOCIRecipeBuild`."""
+ if not self.buildqueue_record:
+ return False
+
+ cancellable_statuses = [
+ BuildStatus.BUILDING,
+ BuildStatus.NEEDSBUILD,
+ ]
+ return self.status in cancellable_statuses
+
+ def rescore(self, score):
+ """See `IOCIRecipeBuild`."""
+ assert self.can_be_rescored, "Build %s cannot be rescored" % self.id
+ self.buildqueue_record.manualScore(score)
+
+ def cancel(self):
+ """See `IOCIRecipeBuild`."""
+ if not self.can_be_cancelled:
+ return
+ # BuildQueue.cancel() will decide whether to go straight to
+ # CANCELLED, or go through CANCELLING to let buildd-manager clean up
+ # the slave.
+ self.buildqueue_record.cancel()
+
def calculateScore(self):
# XXX twom 2020-02-11 - This might need an addition?
return 2510
diff --git a/lib/lp/oci/templates/ocirecipebuild-index.pt b/lib/lp/oci/templates/ocirecipebuild-index.pt
index 04b7fcf..b9ff8c1 100644
--- a/lib/lp/oci/templates/ocirecipebuild-index.pt
+++ b/lib/lp/oci/templates/ocirecipebuild-index.pt
@@ -79,6 +79,9 @@
on <a tal:content="context/builder/title"
tal:attributes="href context/builder/fmt:url"/>
</tal:built>
+ <tal:cancel define="link context/menu:context/cancel"
+ condition="link/enabled"
+ replace="structure link/fmt:link" />
</p>
<ul>
@@ -130,6 +133,14 @@
(<span tal:replace="file/content/filesize/fmt:bytes" />)
</li>
</ul>
+
+ <div
+ style="margin-top: 1.5em"
+ tal:define="link context/menu:context/rescore"
+ tal:condition="link/enabled"
+ >
+ <a tal:replace="structure link/fmt:link"/>
+ </div>
</metal:macro>
<metal:macro define-macro="buildlog">
diff --git a/lib/lp/oci/tests/test_ocirecipebuild.py b/lib/lp/oci/tests/test_ocirecipebuild.py
index e60a362..5ae8b78 100644
--- a/lib/lp/oci/tests/test_ocirecipebuild.py
+++ b/lib/lp/oci/tests/test_ocirecipebuild.py
@@ -100,6 +100,35 @@ class TestOCIRecipeBuild(TestCaseWithFactory):
self.build.getLayerFileByDigest,
'missing')
+ def test_can_be_cancelled(self):
+ # For all states that can be cancelled, can_be_cancelled returns True.
+ ok_cases = [
+ BuildStatus.BUILDING,
+ BuildStatus.NEEDSBUILD,
+ ]
+ for status in BuildStatus:
+ if status in ok_cases:
+ self.assertTrue(self.build.can_be_cancelled)
+ else:
+ self.assertFalse(self.build.can_be_cancelled)
+
+ def test_cancel_not_in_progress(self):
+ # The cancel() method for a pending build leaves it in the CANCELLED
+ # state.
+ self.build.queueBuild()
+ self.build.cancel()
+ self.assertEqual(BuildStatus.CANCELLED, self.build.status)
+ self.assertIsNone(self.build.buildqueue_record)
+
+ def test_cancel_in_progress(self):
+ # The cancel() method for a building build leaves it in the
+ # CANCELLING state.
+ bq = self.build.queueBuild()
+ bq.markAsBuilding(self.factory.makeBuilder())
+ self.build.cancel()
+ self.assertEqual(BuildStatus.CANCELLING, self.build.status)
+ self.assertEqual(bq, self.build.buildqueue_record)
+
def test_estimateDuration(self):
# Without previous builds, the default time estimate is 30m.
self.assertEqual(1800, self.build.estimateDuration().seconds)
diff --git a/lib/lp/security.py b/lib/lp/security.py
index a44393a..683af3f 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.py
@@ -3514,6 +3514,27 @@ class ViewOCIRecipeBuild(AnonymousAuthorization):
usedfor = IOCIRecipeBuild
+class EditOCIRecipeBuild(AdminByBuilddAdmin):
+ permission = 'launchpad.Edit'
+ usedfor = IOCIRecipeBuild
+
+ def checkAuthenticated(self, user):
+ """Check edit access for OCI recipe builds.
+
+ Allow admins, buildd admins, and the owner of the OCI recipe.
+ (Note that the requester of the build is required to be in the team
+ that owns the OCI recipe.)
+ """
+ auth_recipe = EditOCIRecipe(self.obj.recipe)
+ if auth_recipe.checkAuthenticated(user):
+ return True
+ return super(EditOCIRecipeBuild, self).checkAuthenticated(user)
+
+
+class AdminOCIRecipeBuild(AdminByBuilddAdmin):
+ usedfor = IOCIRecipeBuild
+
+
class ViewOCIRegistryCredentials(AuthorizationBase):
permission = 'launchpad.View'
usedfor = IOCIRegistryCredentials
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 8e0f1c1..db46838 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -160,7 +160,9 @@ from lp.hardwaredb.interfaces.hwdb import (
from lp.oci.interfaces.ocipushrule import IOCIPushRuleSet
from lp.oci.interfaces.ocirecipe import IOCIRecipeSet
from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuildSet
-from lp.oci.interfaces.ociregistrycredentials import IOCIRegistryCredentialsSet
+from lp.oci.interfaces.ociregistrycredentials import (
+ IOCIRegistryCredentialsSet,
+ )
from lp.oci.model.ocirecipe import OCIRecipeArch
from lp.oci.model.ocirecipebuild import OCIFile
from lp.registry.enums import (
@@ -4988,7 +4990,7 @@ class BareLaunchpadObjectFactory(ObjectFactory):
processor = self.makeProcessor()
return OCIRecipeArch(recipe, processor)
- def makeOCIRecipeBuild(self, requester=None, recipe=None,
+ def makeOCIRecipeBuild(self, requester=None, registrant=None, recipe=None,
distro_arch_series=None, date_created=DEFAULT,
status=BuildStatus.NEEDSBUILD, builder=None,
duration=None):
@@ -5009,7 +5011,10 @@ class BareLaunchpadObjectFactory(ObjectFactory):
if recipe is None:
oci_project = self.makeOCIProject(
pillar=distro_arch_series.distroseries.distribution)
- recipe = self.makeOCIRecipe(oci_project=oci_project)
+ if registrant is None:
+ registrant = requester
+ recipe = self.makeOCIRecipe(
+ registrant=registrant, oci_project=oci_project)
oci_build = getUtility(IOCIRecipeBuildSet).new(
requester, recipe, distro_arch_series, date_created)
if duration is not None:
