diff --git a/src/coldfront_plugin_cloud/base.py b/src/coldfront_plugin_cloud/base.py index 00a4807d..e5960d81 100644 --- a/src/coldfront_plugin_cloud/base.py +++ b/src/coldfront_plugin_cloud/base.py @@ -38,6 +38,10 @@ def auth_url(self): def member_role_name(self): return self.resource.get_attribute(attributes.RESOURCE_ROLE) or "member" + @abc.abstractmethod + def set_project_configuration(self, project_id, dry_run=False): + pass + @abc.abstractmethod def create_project(self, suggested_project_name) -> Project: pass diff --git a/src/coldfront_plugin_cloud/management/commands/validate_allocations.py b/src/coldfront_plugin_cloud/management/commands/validate_allocations.py index ef5ea1ff..dfe9ef14 100644 --- a/src/coldfront_plugin_cloud/management/commands/validate_allocations.py +++ b/src/coldfront_plugin_cloud/management/commands/validate_allocations.py @@ -1,5 +1,4 @@ import logging -import re from coldfront_plugin_cloud import attributes from coldfront_plugin_cloud import openstack @@ -7,7 +6,7 @@ from coldfront_plugin_cloud import utils from coldfront_plugin_cloud import tasks -from django.core.management.base import BaseCommand, CommandError +from django.core.management.base import BaseCommand from coldfront.core.resource.models import Resource from coldfront.core.allocation.models import ( Allocation, @@ -92,56 +91,6 @@ def set_default_quota_on_allocation(allocation, allocator, coldfront_attr): utils.set_attribute_on_allocation(allocation, coldfront_attr, value) return value - @staticmethod - def parse_quota_value(quota_str: str | None, attr: str) -> int | None: - PATTERN = r"([0-9]+)(m|k|Ki|Mi|Gi|Ti|Pi|Ei|K|M|G|T|P|E)?" - - suffix = { - "Ki": 2**10, - "Mi": 2**20, - "Gi": 2**30, - "Ti": 2**40, - "Pi": 2**50, - "Ei": 2**60, - "m": 10**-3, - "k": 10**3, - "K": 10**3, - "M": 10**6, - "G": 10**9, - "T": 10**12, - "P": 10**15, - "E": 10**18, - } - - if quota_str and quota_str != "0": - result = re.search(PATTERN, quota_str) - - if result is None: - raise CommandError( - f"Unable to parse quota_str = '{quota_str}' for {attr}" - ) - - value = int(result.groups()[0]) - unit = result.groups()[1] - - # Convert to number i.e. without any unit suffix - - if unit is not None: - quota_str = value * suffix[unit] - else: - quota_str = value - - # Convert some attributes to units that coldfront uses - - if "RAM" in attr: - quota_str = round(quota_str / suffix["Mi"]) - elif "Storage" in attr: - quota_str = round(quota_str / suffix["Gi"]) - elif quota_str and quota_str == "0": - quota_str = 0 - - return quota_str - def check_institution_specific_code(self, allocation, apply): attr = attributes.ALLOCATION_INSTITUTION_SPECIFIC_CODE isc = allocation.get_attribute(attr) @@ -289,6 +238,10 @@ def handle(self, *args, **options): ) continue + allocator.set_project_configuration( + project_id, dry_run=not options["apply"] + ) + quota = allocator.get_quota(project_id) failed_validation = Command.sync_users( @@ -306,7 +259,7 @@ def handle(self, *args, **options): expected_value = allocation.get_attribute(attr) current_value = quota.get(key, None) - current_value = self.parse_quota_value(current_value, attr) + current_value = openshift.parse_quota_value(current_value, attr) if expected_value is None and current_value is not None: msg = ( diff --git a/src/coldfront_plugin_cloud/openshift.py b/src/coldfront_plugin_cloud/openshift.py index 91b39cdf..315846d9 100644 --- a/src/coldfront_plugin_cloud/openshift.py +++ b/src/coldfront_plugin_cloud/openshift.py @@ -3,6 +3,9 @@ import logging import os import time +import re +import copy +from collections import namedtuple import kubernetes import kubernetes.dynamic.exceptions as kexc @@ -54,6 +57,96 @@ def clean_openshift_metadata(obj): return obj +def parse_quota_value(quota_str: str | None, attr: str) -> int | None: + PATTERN = r"([0-9]+)(m|k|Ki|Mi|Gi|Ti|Pi|Ei|K|M|G|T|P|E)?" + + suffix = { + "Ki": 2**10, + "Mi": 2**20, + "Gi": 2**30, + "Ti": 2**40, + "Pi": 2**50, + "Ei": 2**60, + "m": 10**-3, + "k": 10**3, + "K": 10**3, + "M": 10**6, + "G": 10**9, + "T": 10**12, + "P": 10**15, + "E": 10**18, + } + + if quota_str and quota_str != "0": + result = re.search(PATTERN, quota_str) + + if result is None: + raise ValueError(f"Unable to parse quota_str = '{quota_str}' for {attr}") + + value = int(result.groups()[0]) + unit = result.groups()[1] + + # Convert to number i.e. without any unit suffix + + if unit is not None: + quota_str = value * suffix[unit] + else: + quota_str = value + + # Convert some attributes to units that coldfront uses + + if "RAM" in attr: + quota_str = round(quota_str / suffix["Mi"]) + elif "Storage" in attr: + quota_str = round(quota_str / suffix["Gi"]) + elif quota_str and quota_str == "0": + quota_str = 0 + + return quota_str + + +LimitRangeDifference = namedtuple("LimitRangeDifference", ["key", "expected", "actual"]) + + +def limit_ranges_diff( + expected_lr_list: list[dict], actual_lr_list: list[dict] +) -> list[LimitRangeDifference]: + expected_lr = copy.deepcopy(expected_lr_list[0]) + actual_lr = copy.deepcopy(actual_lr_list[0]) + differences = [] + + for key in expected_lr | actual_lr: + if key == "type": + if actual_lr.get(key) != expected_lr.get(key): + differences.append( + LimitRangeDifference( + key, expected_lr.get(key), actual_lr.get("type") + ) + ) + break + continue + + # Extra fields in actual limit range, so else statement should only be expected fields + if key not in expected_lr: + differences.append(LimitRangeDifference(key, None, actual_lr[key])) + else: + for resource in expected_lr.setdefault(key, {}) | actual_lr.setdefault( + key, {} + ): + expected_value = parse_quota_value( + expected_lr[key].get(resource), resource + ) + actual_value = parse_quota_value(actual_lr[key].get(resource), resource) + if expected_value != actual_value: + differences.append( + LimitRangeDifference( + f"{key},{resource}", expected_value, actual_value + ) + ) + + return differences + + class ApiException(Exception): def __init__(self, message): self.message = message @@ -130,6 +223,40 @@ def get_resource_api(self, api_version: str, kind: str): ) return api + def set_project_configuration(self, project_id, dry_run=False): + def _recreate_limitrange(): + if not dry_run: + self._openshift_delete_limits(project_id) + self._openshift_create_limits(project_id) + logger.info(f"Recreated LimitRanges for namespace {project_id}.") + + limits = self._openshift_get_limits(project_id).get("items", []) + + if not limits: + if not dry_run: + self._openshift_create_limits(project_id) + logger.info(f"Created default LimitRange for namespace {project_id}.") + + elif len(limits) > 1: + logger.warning( + f"More than one LimitRange found for namespace {project_id}." + ) + _recreate_limitrange() + + if len(limits) == 1: + actual_limits = limits[0]["spec"]["limits"] + if len(actual_limits) != 1: + logger.info( + f"LimitRange for more than one object type found for namespace {project_id}." + ) + _recreate_limitrange() + elif differences := limit_ranges_diff(LIMITRANGE_DEFAULTS, actual_limits): + for difference in differences: + logger.info( + f"LimitRange for {project_id} differs {difference.key}: expected {difference.expected} but found {difference.actual}" + ) + _recreate_limitrange() + def create_project(self, suggested_project_name): sanitized_project_name = utils.get_sanitized_project_name( suggested_project_name @@ -446,6 +573,13 @@ def _openshift_create_limits(self, project_name, limits=None): } return api.create(body=payload, namespace=project_name).to_dict() + def _openshift_delete_limits(self, project_name): + api = self.get_resource_api(API_CORE, "LimitRange") + + limit_ranges = self._openshift_get_limits(project_name) + for lr in limit_ranges["items"]: + api.delete(namespace=project_name, name=lr["metadata"]["name"]) + def _openshift_get_namespace(self, namespace_name): api = self.get_resource_api(API_CORE, "Namespace") return clean_openshift_metadata(api.get(name=namespace_name).to_dict()) diff --git a/src/coldfront_plugin_cloud/openstack.py b/src/coldfront_plugin_cloud/openstack.py index b47ec84b..65f03205 100644 --- a/src/coldfront_plugin_cloud/openstack.py +++ b/src/coldfront_plugin_cloud/openstack.py @@ -141,6 +141,9 @@ def object(self, project_id=None, session=None) -> swiftclient.Connection: preauthurl=preauth_url, ) + def set_project_configuration(self, project_id, dry_run=False): + pass + def create_project(self, suggested_project_name) -> base.ResourceAllocator.Project: project_name = utils.get_unique_project_name( suggested_project_name, max_length=self.project_name_max_length diff --git a/src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py b/src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py index a4177792..1cd39881 100644 --- a/src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py +++ b/src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py @@ -503,3 +503,66 @@ def test_needs_renewal_allocation(self): assert user2.username not in allocator.get_users(project_id) call_command("validate_allocations", apply=True) assert user2.username in allocator.get_users(project_id) + + def test_limitrange_defaults_update(self): + """Test validation if default LimitRange changes, or actual LimitRange is deleted.""" + user = self.new_user() + project = self.new_project(pi=user) + allocation = self.new_allocation(project, self.resource, 1) + allocator = openshift.OpenShiftResourceAllocator(self.resource, allocation) + + tasks.activate_allocation(allocation.pk) + allocation.refresh_from_db() + + project_id = allocation.get_attribute(attributes.ALLOCATION_PROJECT_ID) + + # Check initial limit ranges + limit_ranges = allocator._openshift_get_limits(project_id) + self.assertEqual(len(limit_ranges["items"]), 1) + self.assertEqual( + openshift.limit_ranges_diff( + limit_ranges["items"][0]["spec"]["limits"], + openshift.LIMITRANGE_DEFAULTS, + ), + [], + ) + + # Change LimitRange defaults + new_defaults = [ + { + "type": "Container", + "default": {"cpu": "2", "memory": "8192Mi", "nvidia.com/gpu": "1"}, + "defaultRequest": { + "cpu": "1", + "memory": "4096Mi", + "nvidia.com/gpu": "1", + }, + "min": {"cpu": "100m", "memory": "64Mi"}, + } + ] + openshift.LIMITRANGE_DEFAULTS = new_defaults + + call_command("validate_allocations", apply=True) + + limit_ranges = allocator._openshift_get_limits(project_id) + self.assertEqual(len(limit_ranges["items"]), 1) + self.assertEqual( + openshift.limit_ranges_diff( + limit_ranges["items"][0]["spec"]["limits"], new_defaults + ), + [], + ) + + # Delete and re-create limit range using validate_allocations + allocator._openshift_delete_limits(project_id) + limit_ranges = allocator._openshift_get_limits(project_id) + self.assertEqual(len(limit_ranges["items"]), 0) + call_command("validate_allocations", apply=True) + limit_ranges = allocator._openshift_get_limits(project_id) + self.assertEqual(len(limit_ranges["items"]), 1) + self.assertEqual( + openshift.limit_ranges_diff( + limit_ranges["items"][0]["spec"]["limits"], new_defaults + ), + [], + ) diff --git a/src/coldfront_plugin_cloud/tests/unit/test_openshift_utils.py b/src/coldfront_plugin_cloud/tests/unit/test_openshift_utils.py new file mode 100644 index 00000000..1bc81c8d --- /dev/null +++ b/src/coldfront_plugin_cloud/tests/unit/test_openshift_utils.py @@ -0,0 +1,65 @@ +import copy + +from coldfront_plugin_cloud import openshift +from coldfront_plugin_cloud.tests import base + + +class TestOpenShiftUtils(base.TestBase): + def test_parse_quota_unit(self): + parse_quota_unit = openshift.parse_quota_value + answer_dict = [ + (("5m", "cpu"), 5 * 10**-3), + (("10", "cpu"), 10), + (("10k", "cpu"), 10 * 10**3), + (("55M", "cpu"), 55 * 10**6), + (("2G", "cpu"), 2 * 10**9), + (("3T", "cpu"), 3 * 10**12), + (("4P", "cpu"), 4 * 10**15), + (("5E", "cpu"), 5 * 10**18), + (("10", "memory"), 10), + (("125Ki", "memory"), 125 * 2**10), + (("55Mi", "memory"), 55 * 2**20), + (("2Gi", "memory"), 2 * 2**30), + (("3Ti", "memory"), 3 * 2**40), + ] + for (input_value, resource_type), expected in answer_dict: + self.assertEqual(parse_quota_unit(input_value, resource_type), expected) + + with self.assertRaises(ValueError): + parse_quota_unit("abc", "foo") # Non-numeric input + + def test_limit_ranges_diff(self): + # identical limit ranges, different units for memory -> no differences + expected = openshift.LIMITRANGE_DEFAULTS + actual = [copy.deepcopy(expected[0])] + actual[0]["default"]["memory"] = "4Gi" + diffs = openshift.limit_ranges_diff(expected, actual) + self.assertEqual(diffs, []) + + # type mismatch + actual[0]["type"] = "Pod" + actual[0]["default"]["cpu"] = "2" + del actual[0]["min"]["memory"] + diffs = openshift.limit_ranges_diff(expected, actual) + self.assertTrue( + openshift.LimitRangeDifference("type", "Container", "Pod") in diffs + ) + + # Contains extra fields, [default][cpu] value mismatch (1 vs 2), and missing [min][memory] + actual = [copy.deepcopy(expected[0])] + actual[0]["foo"] = "bar" + actual[0]["default"]["ephemeral-storage"] = "10Gi" + actual[0]["default"]["cpu"] = "2" + del actual[0]["min"]["memory"] + diffs = openshift.limit_ranges_diff(expected, actual) + self.assertTrue( + openshift.LimitRangeDifference( + "default,ephemeral-storage", None, 10 * 2**30 + ) + in diffs + ) + self.assertTrue(openshift.LimitRangeDifference("foo", None, "bar") in diffs) + self.assertTrue(openshift.LimitRangeDifference("default,cpu", 1, 2) in diffs) + self.assertTrue( + openshift.LimitRangeDifference("min,memory", 32 * 2**20, None) in diffs + ) diff --git a/src/coldfront_plugin_cloud/tests/unit/test_parse_quota_unit.py b/src/coldfront_plugin_cloud/tests/unit/test_parse_quota_unit.py deleted file mode 100644 index d1127798..00000000 --- a/src/coldfront_plugin_cloud/tests/unit/test_parse_quota_unit.py +++ /dev/null @@ -1,29 +0,0 @@ -from django.core.management.base import CommandError - -from coldfront_plugin_cloud.management.commands.validate_allocations import Command -from coldfront_plugin_cloud.tests import base - - -class TestParseQuotaUnit(base.TestBase): - def test_parse_quota_unit(self): - parse_quota_unit = Command().parse_quota_value - answer_dict = [ - (("5m", "cpu"), 5 * 10**-3), - (("10", "cpu"), 10), - (("10k", "cpu"), 10 * 10**3), - (("55M", "cpu"), 55 * 10**6), - (("2G", "cpu"), 2 * 10**9), - (("3T", "cpu"), 3 * 10**12), - (("4P", "cpu"), 4 * 10**15), - (("5E", "cpu"), 5 * 10**18), - (("10", "memory"), 10), - (("125Ki", "memory"), 125 * 2**10), - (("55Mi", "memory"), 55 * 2**20), - (("2Gi", "memory"), 2 * 2**30), - (("3Ti", "memory"), 3 * 2**40), - ] - for (input_value, resource_type), expected in answer_dict: - self.assertEqual(parse_quota_unit(input_value, resource_type), expected) - - with self.assertRaises(CommandError): - parse_quota_unit("abc", "foo") # Non-numeric input