From e3f639b227d26fc4de51d0646492c2dbb298c07a Mon Sep 17 00:00:00 2001 From: djperrefort Date: Sun, 11 Aug 2024 14:20:23 -0400 Subject: [PATCH] QA inspections --- keystone_api/apps/allocations/tasks.py | 25 ++++++++++--------- keystone_api/apps/health/views.py | 2 +- keystone_api/apps/logging/admin.py | 6 +++++ .../tests/tasks/test_rotate_log_files.py | 2 +- .../users/management/commands/ldap_update.py | 2 +- 5 files changed, 22 insertions(+), 15 deletions(-) diff --git a/keystone_api/apps/allocations/tasks.py b/keystone_api/apps/allocations/tasks.py index f4d3bad8..06a7a420 100644 --- a/keystone_api/apps/allocations/tasks.py +++ b/keystone_api/apps/allocations/tasks.py @@ -78,7 +78,7 @@ def update_limit_for_account(account: ResearchGroup, cluster: Cluster) -> None: historical_usage = current_limit - active_sus - closing_sus if historical_usage < 0: log.warning(f"Negative Historical usage found for {account.name} on {cluster.name}:\n" - f"historical: {historical_usage} = current limit: {current_limit} - active: {active_sus} - closing: {closing_sus}\n" + f"historical: {historical_usage}, current: {current_limit}, active: {active_sus}, closing: {closing_sus}\n" f"Assuming zero...") historical_usage = 0 @@ -92,32 +92,33 @@ def update_limit_for_account(account: ResearchGroup, cluster: Cluster) -> None: current_usage = historical_usage closing_summary = (f"Summary of closing allocations:\n" - f" Current Usage before closing: {current_usage}\n") + f"> Current Usage before closing: {current_usage}\n") for allocation in closing_query.all(): allocation.final = min(current_usage, allocation.awarded) - closing_summary += f" Allocation {allocation.id}: {current_usage} - {allocation.final} -> {current_usage - allocation.final}\n" + closing_summary += f"> Allocation {allocation.id}: {current_usage} - {allocation.final} -> {current_usage - allocation.final}\n" current_usage -= allocation.final allocation.save() - closing_summary += f" Current Usage after closing: {current_usage}" + closing_summary += f"> Current Usage after closing: {current_usage}" # This shouldn't happen but if it does somehow, create a warning so an admin will notice if current_usage > active_sus: log.warning(f"The current usage is somehow higher than the limit for {account.name}!") # Set the new account usage limit using the updated historical usage after closing any expired allocations - updated_historical_usage = approved_query.filter(request__expire__lte=date.today()).aggregate(Sum("final"))['final__sum'] or 0 + expired_requests = approved_query.filter(request__expire__lte=date.today()) + updated_historical_usage = expired_requests.aggregate(Sum("final"))['final__sum'] or 0 updated_limit = updated_historical_usage + active_sus set_cluster_limit(account.name, cluster.name, updated_limit) # Log summary of changes during limits update for this Slurm account on this cluster log.debug(f"Summary of limits update for {account.name} on {cluster.name}:\n" - f" Approved allocations found: {len(approved_query)}\n" - f" Service units from {len(active_query)} active allocations: {active_sus}\n" - f" Service units from {len(closing_query)} closing allocations: {closing_sus}\n" - f" {closing_summary}" - f" historical usage change: {historical_usage} -> {updated_historical_usage}\n" - f" limit change: {current_limit} -> {updated_limit}") + f"> Approved allocations found: {len(approved_query)}\n" + f"> Service units from {len(active_query)} active allocations: {active_sus}\n" + f"> Service units from {len(closing_query)} closing allocations: {closing_sus}\n" + f"> {closing_summary}" + f"> historical usage change: {historical_usage} -> {updated_historical_usage}\n" + f"> limit change: {current_limit} -> {updated_limit}") def send_expiry_notification_for_request(user: User, request: AllocationRequest) -> None: @@ -145,7 +146,7 @@ def send_expiry_notification_for_request(user: User, request: AllocationRequest) ) # Exit early if we have not hit a threshold yet - log.debug(f'Request #{request.id} expires in {days_until_expire} days with next threshold at {next_threshold} days.') + log.debug(f'Request #{request.id} expires in {days_until_expire} days. Next threshold at {next_threshold} days.') if next_threshold is None: return diff --git a/keystone_api/apps/health/views.py b/keystone_api/apps/health/views.py index 54ea41be..efefbbf0 100644 --- a/keystone_api/apps/health/views.py +++ b/keystone_api/apps/health/views.py @@ -7,7 +7,7 @@ from django.http import HttpResponse, JsonResponse from django.utils.decorators import method_decorator from django.views.decorators.cache import cache_page -from drf_spectacular.utils import extend_schema, inline_serializer, OpenApiExample +from drf_spectacular.utils import extend_schema, inline_serializer from health_check.mixins import CheckMixin from rest_framework import serializers from rest_framework.generics import GenericAPIView diff --git a/keystone_api/apps/logging/admin.py b/keystone_api/apps/logging/admin.py index 5a9bfe10..6aa56753 100644 --- a/keystone_api/apps/logging/admin.py +++ b/keystone_api/apps/logging/admin.py @@ -31,12 +31,18 @@ class ReadOnlyModelAdminMixin: """Mixin class for creating model admins with read only permissions.""" def has_change_permission(self, request, obj=None) -> False: + """Disable permissions for modifying records.""" + return False def has_add_permission(self, request, obj=None) -> False: + """Disable permissions for creating new records.""" + return False def has_delete_permission(self, request, obj=None) -> False: + """Disable permissions for deleting records.""" + return False diff --git a/keystone_api/apps/logging/tests/tasks/test_rotate_log_files.py b/keystone_api/apps/logging/tests/tasks/test_rotate_log_files.py index 51767b70..374dd87f 100644 --- a/keystone_api/apps/logging/tests/tasks/test_rotate_log_files.py +++ b/keystone_api/apps/logging/tests/tasks/test_rotate_log_files.py @@ -4,7 +4,7 @@ from django.test import override_settings, TestCase -from apps.logging.models import * +from apps.logging.models import AppLog, RequestLog from apps.logging.tasks import rotate_log_files diff --git a/keystone_api/apps/users/management/commands/ldap_update.py b/keystone_api/apps/users/management/commands/ldap_update.py index a398dfc8..cb8e4c7c 100644 --- a/keystone_api/apps/users/management/commands/ldap_update.py +++ b/keystone_api/apps/users/management/commands/ldap_update.py @@ -23,7 +23,7 @@ def add_arguments(self, parser: ArgumentParser) -> None: parser: The argument parser instance """ - parser.add_argument('--prune', action='store_true', help='Delete any accounts with usernames not found in LDAP.') + parser.add_argument('--prune', action='store_true', help='Delete accounts with usernames not found in LDAP.') def handle(self, *args, **options) -> None: """Handle the command execution."""