diagnostics: Refactor background diagnostics task

- When running full diagnostics manually, we can use the Operation class. This
allows us to use many of its features.

  - Ensure only one task is running at any time. No need to use running_task
  global variable and a lock for it.

  - Don't run the operation if app install/uninstall or other potentially
  contentious tasks are running.

  - Since Operation object creates a thread, don't create another one with
  glib.schedule(). Don't wait unnecessarily for the operation to finish in the
  glib thread (or glib created thread).

- Since the app will show progress of operations when an operation is running,
it would not be possible to show progress of diagnostics running. So, create a
separate page for diagnostics results.

Tests:

- Run diagnostics and see redirection happens to diagnostics results page.
Results page shows ongoing tests. It refreshes automatically. When tests are
completed, 'Re-run diagnostics' button is shown.

- When visiting /diagnostics/full/ URL is visited without running the test. Only
the re-run button is shown. No results are shown. If tests have been run, re-run
button along with results are shown.

- On the app page, if the tests have been run, a button for viewing results is
shown. Otherwise, the button is not shown.

- In development mode, background diagnostics are run after 3 minutes (change
the time to 150 seconds if database locked errors show up). Results are
available in the results page.

- Make a diagnostic test fail by changing code or disabling a daemon. Run
diagnostics and notice that a notification is shown with the button to go to the
results. Clicking on the button shows the results page. Clicking dismiss removes
the notification.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

plinth/modules/diagnostics/__init__.py

@@ -29,12 +29,8 @@ _description = [
 
 logger = logging.Logger(__name__)
 
-running_task = None
-
 current_results = {}
-
 results_lock = threading.Lock()
-running_task_lock = threading.Lock()
 
 
 class DiagnosticsApp(app_module.App):
@@ -72,7 +68,7 @@ class DiagnosticsApp(app_module.App):
 
         # Run diagnostics once a day
         interval = 180 if cfg.develop else 86400
-        glib.schedule(interval, _start_background_diagnostics)
+        glib.schedule(interval, start_diagnostics, in_thread=False)
 
     def setup(self, old_version):
         """Install and configure the app."""
@@ -90,19 +86,7 @@ class DiagnosticsApp(app_module.App):
         return results
 
 
-def start_task():
+def _run_on_all_enabled_modules():
-    """Start the run task in a separate thread."""
-    global running_task
-    with running_task_lock:
-        if running_task:
-            raise Exception('Task already running')
-
-        running_task = threading.Thread(target=run_on_all_enabled_modules)
-
-    running_task.start()
-
-
-def run_on_all_enabled_modules():
     """Run diagnostics on all the enabled modules and store the result."""
     global current_results
 
@@ -158,10 +142,6 @@ def run_on_all_enabled_modules():
             current_results['progress_percentage'] = \
                 int((current_index + 1) * 100 / len(apps))
 
-    global running_task
-    with running_task_lock:
-        running_task = None
-
 
 def _get_memory_info_from_cgroups():
     """Return information about RAM usage from cgroups."""
@@ -261,44 +241,28 @@ def _warn_about_low_ram_space(request):
                                   actions=actions, data=data, group='admin')
 
 
-def _start_background_diagnostics(request):
+def start_diagnostics(data: None = None):
-    """Start daily diagnostics as a background operation."""
+    """Start full diagnostics as a background operation."""
+    logger.info('Running full diagnostics')
     try:
-        operation = operation_module.manager.new(
+        operation_module.manager.new(op_id='diagnostics-full',
-            op_id='diagnostics-full', app_id='diagnostics',
+                                     app_id='diagnostics',
-            name=gettext_noop('Running background diagnostics'),
+                                     name=gettext_noop('Running diagnostics'),
-            target=_run_background_diagnostics, [], show_message=False,
+                                     target=_run_diagnostics,
-            show_notification=False)
+                                     show_message=False,
+                                     show_notification=False)
     except KeyError:
         logger.warning('Diagnostics are already running')
-        return
-
-    operation.join()
 
 
-def _run_background_diagnostics():
+def _run_diagnostics():
     """Run diagnostics and notify for failures."""
     from plinth.notification import Notification
 
-    # In case diagnostics are already running, skip the background run for
+    _run_on_all_enabled_modules()
-    # today.
-    global running_task
-    with running_task_lock:
-        if running_task:
-            logger.warning('Diagnostics are already running, skip background '
-                           'diagnostics for today.')
-            return
-
-        # Set something in the global so we won't be interrupted.
-        running_task = 'background'
-
-    run_on_all_enabled_modules()
     with results_lock:
         results = current_results['results']
 
-    with running_task_lock:
-        running_task = None
-
     issue_count = 0
     severity = 'warning'
     for _app_id, app_data in results.items():
@@ -331,7 +295,7 @@ def _run_background_diagnostics():
         'type': 'link',
         'class': 'primary',
         'text': gettext_noop('Go to diagnostics results'),
-        'url': 'diagnostics:index'
+        'url': 'diagnostics:full'
     }, {
         'type': 'dismiss'
     }]

plinth/modules/diagnostics/templates/diagnostics.html

@@ -8,46 +8,19 @@
 
 {% block configuration %}
 
-  {% if not is_task_running %}
+  <div class="btn-toolbar">
-    <form class="form form-run-diagnostics" method="post"
+    <form class="form form-diagnostics-full" method="post"
-          action="{% url 'diagnostics:index' %}">
+          action="{% url 'diagnostics:full' %}">
       {% csrf_token %}
-
       <input type="submit" class="btn btn-primary"
              value="{% trans "Run Diagnostics" %}"/>
     </form>
-  {% else %}
-    <p>{% trans "Diagnostics test is currently running" %}</p>
-    <div class="progress">
-      <div class="progress-bar progress-bar-striped active
-                  w-{{ results.progress_percentage }}"
-           role="progressbar" aria-valuemin="0" aria-valuemax="100"
-           aria-valuenow="{{ results.progress_percentage }}">
-        {{ results.progress_percentage }}%
-      </div>
-    </div>
 
-  {% endif %}
+    {% if results %}
-
+      <a class="btn btn-default" role="button"href="{% url 'diagnostics:full' %}">
-  {% if results %}
+        {% trans "View Results" %}
-    <h3>{% trans "Results" %}</h3>
+      </a>
-    {% for app_id, app_data in results.results.items %}
+    {% endif %}
-      <h4>
+  </div>
-        {% blocktrans with app_name=app_data.name %}
-          App: {{app_name}}
-        {% endblocktrans %}
-      </h4>
-
-      {% if app_data.diagnosis %}
-        {% include "diagnostics_results.html" with results=app_data.diagnosis %}
-      {% elif app_data.exception %}
-        <div class="alert alert-danger" role="alert">
-          {{ app_data.exception }}
-        </div>
-      {% else %}
-        <p><span class="fa fa-hourglass-o"></span></p>
-      {% endif %}
-    {% endfor %}
-  {% endif %}
 
 {% endblock %}

plinth/modules/diagnostics/templates/diagnostics_full.html

@@ -0,0 +1,54 @@
+{% extends 'base.html' %}
+{% comment %}
+# SPDX-License-Identifier: AGPL-3.0-or-later
+{% endcomment %}
+
+{% load i18n %}
+
+{% block content %}
+
+  {% if not is_task_running %}
+    <div class="btn-toolbar">
+      <form class="form form-diagnostics-full" method="post"
+            action="{% url 'diagnostics:full' %}">
+        {% csrf_token %}
+
+        <input type="submit" class="btn btn-primary"
+               value="{% trans "Re-run Diagnostics" %}"/>
+      </form>
+    </div>
+  {% else %}
+    <p>{% trans "Diagnostics test is currently running" %}</p>
+    <div class="progress">
+      <div class="progress-bar progress-bar-striped active
+                  w-{{ results.progress_percentage }}"
+           role="progressbar" aria-valuemin="0" aria-valuemax="100"
+           aria-valuenow="{{ results.progress_percentage }}">
+        {{ results.progress_percentage }}%
+      </div>
+    </div>
+
+  {% endif %}
+
+  {% if results %}
+    <h3>{% trans "Results" %}</h3>
+    {% for app_id, app_data in results.results.items %}
+      <h4>
+        {% blocktrans with app_name=app_data.name %}
+          App: {{app_name}}
+        {% endblocktrans %}
+      </h4>
+
+      {% if app_data.diagnosis %}
+        {% include "diagnostics_results.html" with results=app_data.diagnosis %}
+      {% elif app_data.exception %}
+        <div class="alert alert-danger" role="alert">
+          {{ app_data.exception }}
+        </div>
+      {% else %}
+        <p><span class="fa fa-hourglass-o"></span></p>
+      {% endif %}
+    {% endfor %}
+  {% endif %}
+
+{% endblock %}

plinth/modules/diagnostics/urls.py

@@ -10,6 +10,8 @@ from . import views
 urlpatterns = [
     re_path(r'^sys/diagnostics/$', views.DiagnosticsView.as_view(),
             name='index'),
+    re_path(r'^sys/diagnostics/full/$', views.DiagnosticsFullView.as_view(),
+            name='full'),
     re_path(r'^sys/diagnostics/(?P<app_id>[1-9a-z\-_]+)/$', views.diagnose_app,
             name='app'),
 ]

plinth/modules/diagnostics/views.py

@@ -10,7 +10,9 @@ from django.template.response import TemplateResponse
 from django.urls import reverse
 from django.utils.translation import gettext_lazy as _
 from django.views.decorators.http import require_POST
+from django.views.generic import TemplateView
 
+from plinth import operation
 from plinth.app import App
 from plinth.modules import diagnostics
 from plinth.views import AppView
@@ -26,22 +28,41 @@ class DiagnosticsView(AppView):
 
     def post(self, request):
         """Start diagnostics."""
-        with diagnostics.running_task_lock:
+        diagnostics.start_diagnostics()
-            if not diagnostics.running_task:
-                diagnostics.start_task()
-
         return HttpResponseRedirect(reverse('diagnostics:index'))
 
     def get_context_data(self, **kwargs):
         """Return additional context for rendering the template."""
-        with diagnostics.running_task_lock:
-            is_task_running = diagnostics.running_task is not None
-
         with diagnostics.results_lock:
             results = diagnostics.current_results
 
         context = super().get_context_data(**kwargs)
         context['has_diagnostics'] = False
+        context['results'] = results
+        return context
+
+
+class DiagnosticsFullView(TemplateView):
+    """View to run full diagnostics."""
+
+    template_name = 'diagnostics_full.html'
+
+    def post(self, request):
+        """Start diagnostics."""
+        diagnostics.start_diagnostics()
+        return self.get(self, request)
+
+    def get_context_data(self, **kwargs):
+        """Return additional context for rendering the template."""
+        try:
+            is_task_running = bool(operation.manager.get('diagnostics-full'))
+        except KeyError:
+            is_task_running = False
+
+        with diagnostics.results_lock:
+            results = diagnostics.current_results
+
+        context = super().get_context_data(**kwargs)
         context['is_task_running'] = is_task_running
         context['results'] = results
         context['refresh_page_sec'] = 3 if is_task_running else None