diff --git a/appsettings/migrations/0018_worker_settings.py b/appsettings/migrations/0018_worker_settings.py new file mode 100644 index 00000000..9cea00d8 --- /dev/null +++ b/appsettings/migrations/0018_worker_settings.py @@ -0,0 +1,33 @@ +from django.core.management.color import no_style +from django.db import migrations + + +SETTINGS = [ + ("Worker Concurrency", "WORKER_CONCURRENCY", "4", "Jobs the worker runs at the same time (1-16)"), + ("Job Retention Days", "JOB_RETENTION_DAYS", "30", "Days finished jobs stay in the job history (1-3650)"), +] + + +def add_settings(apps, schema_editor): + setting = apps.get_model("appsettings", "AppSettings") + db_alias = schema_editor.connection.alias + # see 0014: move a PostgreSQL id sequence past the fixed ids first + with schema_editor.connection.cursor() as cursor: + for sql in schema_editor.connection.ops.sequence_reset_sql(no_style(), [setting]): + cursor.execute(sql) + for name, key, value, description in SETTINGS: + setting.objects.using(db_alias).get_or_create( + key=key, defaults={"name": name, "value": value, "choices": "", "description": description} + ) + + +def del_settings(apps, schema_editor): + setting = apps.get_model("appsettings", "AppSettings") + db_alias = schema_editor.connection.alias + setting.objects.using(db_alias).filter(key__in=[key for _, key, _, _ in SETTINGS]).delete() + + +class Migration(migrations.Migration): + dependencies = [("appsettings", "0017_snapshot_free_space_settings")] + + operations = [migrations.RunPython(add_settings, del_settings)] diff --git a/appsettings/tests/test_appsettings.py b/appsettings/tests/test_appsettings.py index e8cac7aa..ca739d6c 100644 --- a/appsettings/tests/test_appsettings.py +++ b/appsettings/tests/test_appsettings.py @@ -1,3 +1,4 @@ +from django.contrib.messages import get_messages from django.test import RequestFactory, TestCase from django.urls import reverse @@ -48,3 +49,8 @@ class AppSettingsTestCase(TestCase): # a clone stays on the host of the VM it was made from; the setting # that sent it to a random host is removed by migration 0015 self.assertFalse(AppSettings.objects.filter(key="CLONE_INSTANCE_AUTO_MIGRATE").exists()) + + def test_worker_settings_refuse_values_out_of_bounds(self): + response = self.client.post(reverse("appsettings"), {"WORKER_CONCURRENCY": "0"}) + self.assertEqual(AppSettings.objects.get(key="WORKER_CONCURRENCY").value, "4") + self.assertTrue(any("between 1 and 16" in str(m) for m in get_messages(response.wsgi_request))) diff --git a/appsettings/tests/test_migrations.py b/appsettings/tests/test_migrations.py index 428bd027..a85be710 100644 --- a/appsettings/tests/test_migrations.py +++ b/appsettings/tests/test_migrations.py @@ -51,3 +51,13 @@ class SnapshotFreeSpaceSettingsTestCase(TestCase): limit = AppSettings._meta.get_field(field).max_length if limit: self.assertLessEqual(len(getattr(setting, field)), limit, f"{key} {field}") + + +class WorkerSettingsTestCase(TestCase): + def test_the_rows_fit_their_columns(self): + for key in ("WORKER_CONCURRENCY", "JOB_RETENTION_DAYS"): + setting = AppSettings.objects.get(key=key) + for field in ("name", "key", "value", "description"): + limit = AppSettings._meta.get_field(field).max_length + if limit: + self.assertLessEqual(len(getattr(setting, field)), limit, f"{key} {field}") diff --git a/appsettings/views.py b/appsettings/views.py index 59e10306..9b1d406a 100644 --- a/appsettings/views.py +++ b/appsettings/views.py @@ -9,6 +9,7 @@ from django.http import HttpResponseRedirect from django.shortcuts import render from django.utils.translation import gettext_noop as _ from logs.views import addlogmsg +from operations.utils import BOUNDS from appsettings.models import AppSettings @@ -112,6 +113,13 @@ def appsettings(request): for setting in appsettings: if setting.key in request.POST: + if setting.key in BOUNDS: + low, high, _default = BOUNDS[setting.key] + raw = request.POST.get(setting.key, "") + if not (raw.isascii() and raw.isdigit() and len(raw) <= 4 and low <= int(raw) <= high): + messages.error(request, _("%(setting)s must be a whole number between %(low)s and %(high)s") + % {"setting": setting.name, "low": low, "high": high}) + return HttpResponseRedirect(request.get_full_path()) try: setting.value = request.POST.get(setting.key, "") setting.save() diff --git a/operations/tests/test_conf.py b/operations/tests/test_conf.py new file mode 100644 index 00000000..8a9fbe43 --- /dev/null +++ b/operations/tests/test_conf.py @@ -0,0 +1,13 @@ +from django.test import TestCase + +from appsettings.models import AppSettings +from operations import utils + + +class ConfTestCase(TestCase): + def test_defaults_and_bounds(self): + self.assertEqual((utils.concurrency(), utils.retention_days()), (4, 30)) + AppSettings.objects.filter(key="WORKER_CONCURRENCY").update(value="99") + self.assertEqual(utils.concurrency(), 4) + AppSettings.objects.filter(key="WORKER_CONCURRENCY").update(value="8") + self.assertEqual(utils.concurrency(), 8) diff --git a/operations/utils.py b/operations/utils.py index e781ec64..fbdfadf3 100644 --- a/operations/utils.py +++ b/operations/utils.py @@ -1,6 +1,8 @@ """Secrets never reach a job record, a response or a log line. What a refused request says about the job holding the resource: details for its owner and -superusers, a minimal summary for everyone else.""" +superusers, a minimal summary for everyone else. Worker settings from the +Settings page. The worker reads them itself (the settings middleware only runs +in gunicorn).""" import re @@ -8,6 +10,8 @@ from django.utils import timezone from django.utils.timesince import timesince from django.utils.translation import gettext as _ +from appsettings.models import AppSettings + from . import jobs _NAMES = r"(?:pass|secret|token|key)\w*" @@ -21,6 +25,8 @@ _PATTERNS = [ ] _SECRET_KEY = re.compile(r"pass|secret|token|private|key$", re.I) +BOUNDS = {"WORKER_CONCURRENCY": (1, 16, 4), "JOB_RETENTION_DAYS": (1, 3650, 30)} + def safe_text(text): text = str(text) @@ -60,3 +66,20 @@ def busy_message(busy, user): return _("%(label)s is %(state)s on this resource (started by %(user)s, %(since)s ago); " "see the Jobs page.") % {**values, "user": op.username} return _("%(label)s is %(state)s on this resource (since %(since)s).") % values + + +def _value(key): + low, high, default = BOUNDS[key] + try: + value = int(AppSettings.objects.get(key=key).value) + except (AppSettings.DoesNotExist, ValueError): + return default + return value if low <= value <= high else default + + +def concurrency(): + return _value("WORKER_CONCURRENCY") + + +def retention_days(): + return _value("JOB_RETENTION_DAYS")