From a0032856fdc6788cfdaa00eb3d66e7cd5c79045a Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Tue, 3 Oct 2023 18:16:34 -0700 Subject: [PATCH] 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 Reviewed-by: James Valleroy --- plinth/modules/diagnostics/__init__.py | 64 ++++--------------- .../diagnostics/templates/diagnostics.html | 45 +++---------- .../templates/diagnostics_full.html | 54 ++++++++++++++++ plinth/modules/diagnostics/urls.py | 2 + plinth/modules/diagnostics/views.py | 35 ++++++++-- 5 files changed, 107 insertions(+), 93 deletions(-) create mode 100644 plinth/modules/diagnostics/templates/diagnostics_full.html diff --git a/plinth/modules/diagnostics/__init__.py b/plinth/modules/diagnostics/__init__.py index ba9744247..7e589c6a9 100644 --- a/plinth/modules/diagnostics/__init__.py +++ b/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(): - """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(): +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): - """Start daily diagnostics as a background operation.""" +def start_diagnostics(data: None = None): + """Start full diagnostics as a background operation.""" + logger.info('Running full diagnostics') try: - operation = operation_module.manager.new( - op_id='diagnostics-full', app_id='diagnostics', - name=gettext_noop('Running background diagnostics'), - target=_run_background_diagnostics, [], show_message=False, - show_notification=False) + operation_module.manager.new(op_id='diagnostics-full', + app_id='diagnostics', + name=gettext_noop('Running diagnostics'), + target=_run_diagnostics, + 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 - # 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() + _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' }] diff --git a/plinth/modules/diagnostics/templates/diagnostics.html b/plinth/modules/diagnostics/templates/diagnostics.html index a6e4703f1..ef102596f 100644 --- a/plinth/modules/diagnostics/templates/diagnostics.html +++ b/plinth/modules/diagnostics/templates/diagnostics.html @@ -8,46 +8,19 @@ {% block configuration %} - {% if not is_task_running %} -
+
+ {% csrf_token %} - - {% else %} -

{% trans "Diagnostics test is currently running" %}

-
-
- {{ results.progress_percentage }}% -
-
- {% endif %} - - {% if results %} -

{% trans "Results" %}

- {% for app_id, app_data in results.results.items %} -

- {% blocktrans with app_name=app_data.name %} - App: {{app_name}} - {% endblocktrans %} -

- - {% if app_data.diagnosis %} - {% include "diagnostics_results.html" with results=app_data.diagnosis %} - {% elif app_data.exception %} - - {% else %} -

- {% endif %} - {% endfor %} - {% endif %} + {% if results %} + + {% trans "View Results" %} + + {% endif %} +
{% endblock %} diff --git a/plinth/modules/diagnostics/templates/diagnostics_full.html b/plinth/modules/diagnostics/templates/diagnostics_full.html new file mode 100644 index 000000000..a6e59ebef --- /dev/null +++ b/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 %} +
+
+ {% csrf_token %} + + +
+
+ {% else %} +

{% trans "Diagnostics test is currently running" %}

+
+
+ {{ results.progress_percentage }}% +
+
+ + {% endif %} + + {% if results %} +

{% trans "Results" %}

+ {% for app_id, app_data in results.results.items %} +

+ {% blocktrans with app_name=app_data.name %} + App: {{app_name}} + {% endblocktrans %} +

+ + {% if app_data.diagnosis %} + {% include "diagnostics_results.html" with results=app_data.diagnosis %} + {% elif app_data.exception %} + + {% else %} +

+ {% endif %} + {% endfor %} + {% endif %} + +{% endblock %} diff --git a/plinth/modules/diagnostics/urls.py b/plinth/modules/diagnostics/urls.py index 26ddc4df6..215e5d070 100644 --- a/plinth/modules/diagnostics/urls.py +++ b/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[1-9a-z\-_]+)/$', views.diagnose_app, name='app'), ] diff --git a/plinth/modules/diagnostics/views.py b/plinth/modules/diagnostics/views.py index c8dd8327c..7b1c0830a 100644 --- a/plinth/modules/diagnostics/views.py +++ b/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: - if not diagnostics.running_task: - diagnostics.start_task() - + diagnostics.start_diagnostics() 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