launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:builder-ui-clean-status-duration into launchpad:master.

Commit message:
Show clean status duration on builders

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/388989

Builders sometimes get stuck in the Cleaning status.  The "manage-builders" tool in lp:ubuntu-archive-tools shows how long they've been in that status, but the web UI doesn't, with the result that people sometimes find it hard to tell the difference between cleaning-because-recently-reset and cleaning-because-stuck.  Add an indication to the web UI to make this more obvious.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:builder-ui-clean-status-duration into launchpad:master.

diff --git a/lib/lp/buildmaster/browser/builder.py b/lib/lp/buildmaster/browser/builder.py
index b378ea3..7f8b9b9 100644
--- a/lib/lp/buildmaster/browser/builder.py
+++ b/lib/lp/buildmaster/browser/builder.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser views for builders."""
@@ -16,10 +16,15 @@ __all__ = [
     'BuilderView',
     ]
 
+from datetime import (
+    datetime,
+    timedelta,
+    )
 from itertools import groupby
 import operator
 
 from lazr.restful.utils import smartquote
+import pytz
 import six
 from zope.component import getUtility
 from zope.event import notify
@@ -33,6 +38,7 @@ from lp.app.browser.launchpadform import (
     LaunchpadEditFormView,
     LaunchpadFormView,
     )
+from lp.app.browser.tales import DurationFormatterAPI
 from lp.app.widgets.itemswidgets import LabeledMultiCheckBoxWidget
 from lp.app.widgets.owner import HiddenUserWidget
 from lp.buildmaster.interfaces.builder import (
@@ -141,7 +147,29 @@ class BuilderOverviewMenu(ApplicationMenu):
         return Link('+mode', text, icon='edit')
 
 
-class BuilderSetView(LaunchpadView):
+class CleanInfoMixin:
+    """View support for showing information about a builder's cleaning status.
+
+    This supports the status-summary macro.  We want the current time to be
+    a cached property so that it's consistent across multiple builders.
+    """
+
+    @cachedproperty
+    def _now(self):
+        return datetime.now(pytz.UTC)
+
+    def getCleanInfo(self, builder):
+        duration = self._now - builder.date_clean_status_changed
+        # A builder that has been cleaning for more than ten minutes is a
+        # little suspicious.
+        if duration > timedelta(minutes=10):
+            return "Cleaning for {}".format(
+                DurationFormatterAPI(duration).approximateduration())
+        else:
+            return "Cleaning"
+
+
+class BuilderSetView(CleanInfoMixin, LaunchpadView):
     """Default BuilderSet view class."""
 
     @property
@@ -285,7 +313,7 @@ class BuilderCategory:
             self._builder_groups.append(builder_group)
 
 
-class BuilderView(LaunchpadView):
+class BuilderView(CleanInfoMixin, LaunchpadView):
     """Default Builder view class
 
     Implements useful actions for the page template.
diff --git a/lib/lp/buildmaster/browser/tests/test_builder.py b/lib/lp/buildmaster/browser/tests/test_builder.py
index 8d71b13..018d7f1 100644
--- a/lib/lp/buildmaster/browser/tests/test_builder.py
+++ b/lib/lp/buildmaster/browser/tests/test_builder.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the lp.soyuz.browser.builder module."""
@@ -7,11 +7,21 @@ from __future__ import absolute_import, print_function, unicode_literals
 
 __metaclass__ = type
 
+from datetime import timedelta
+
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
+from lp.app.browser.tales import DurationFormatterAPI
 from lp.buildmaster.browser.tests.test_builder_views import BuildCreationMixin
-from lp.buildmaster.enums import BuildStatus
+from lp.buildmaster.enums import (
+    BuilderCleanStatus,
+    BuildStatus,
+    )
 from lp.buildmaster.interfaces.builder import IBuilderSet
+from lp.buildmaster.model.builder import Builder
+from lp.services.database.interfaces import IStore
+from lp.services.database.sqlbase import get_transaction_timestamp
 from lp.services.job.model.job import Job
 from lp.testing import (
     admin_logged_in,
@@ -20,6 +30,10 @@ from lp.testing import (
     )
 from lp.testing.layers import LaunchpadFunctionalLayer
 from lp.testing.matchers import HasQueryCount
+from lp.testing.pages import (
+    extract_text,
+    find_tags_by_class,
+    )
 from lp.testing.views import create_initialized_view
 
 
@@ -117,3 +131,33 @@ class TestBuildersHomepage(TestCaseWithFactory, BuildCreationMixin):
         content = builders_homepage_render()
         self.assertNotIn("Virtual build status", content)
         self.assertNotIn("Non-virtual build status", content)
+
+    def test_clean_status_duration(self):
+        now = get_transaction_timestamp(IStore(Builder))
+        durations = [
+            timedelta(minutes=5),
+            timedelta(minutes=11), timedelta(hours=1), timedelta(hours=2)]
+        with admin_logged_in():
+            for builder in getUtility(IBuilderSet):
+                builder.active = False
+            builders = [
+                self.factory.makeBuilder() for _ in range(len(durations))]
+            for builder, duration in zip(builders, durations):
+                naked_builder = removeSecurityProxy(builder)
+                naked_builder.clean_status = BuilderCleanStatus.CLEANING
+                naked_builder.date_clean_status_changed = now - duration
+        content = builders_homepage_render()
+        # We don't show a duration for a builder that has only been cleaning
+        # for a short time.
+        expected_text = ["{}\nCleaning".format(builders[0].name)]
+        # We show durations for builders that have been cleaning for more
+        # than ten minutes.
+        expected_text.extend([
+            "{}\nCleaning for {}".format(
+                builder.name,
+                DurationFormatterAPI(duration).approximateduration())
+            for builder, duration in zip(builders[1:], durations[1:])])
+        self.assertEqual(
+            expected_text,
+            [extract_text(row)
+             for row in find_tags_by_class(content, "builder-row")])
diff --git a/lib/lp/buildmaster/templates/builder-index.pt b/lib/lp/buildmaster/templates/builder-index.pt
index d81ff68..2c5172d 100644
--- a/lib/lp/buildmaster/templates/builder-index.pt
+++ b/lib/lp/buildmaster/templates/builder-index.pt
@@ -96,13 +96,14 @@
       <tal:builderok condition="builder/builderok">
         <tal:idle condition="not: builder/manual">
           <tal:cleaning condition="not: builder/clean_status/enumvalue:CLEAN">
-          <span class="sortkey" tal:content="string:0" />
-          <img width="14" height="14" src="/@@/processing" alt="[cleaning]" />
-          Cleaning
+            <span class="sortkey" tal:content="string:0" />
+            <img width="14" height="14" src="/@@/processing"
+                 alt="[cleaning]" />
+            <tal:info content="python: view.getCleanInfo(builder)" />
           </tal:cleaning>
           <tal:clean condition="builder/clean_status/enumvalue:CLEAN">
-          <span class="sortkey" tal:content="string:1" />
-          <span class="sprite yes">Idle</span>
+            <span class="sortkey" tal:content="string:1" />
+            <span class="sprite yes">Idle</span>
           </tal:clean>
         </tal:idle>
         <tal:manual tal:condition="builder/manual">