Validate Openshift limit ranges

A function `set_project_configuration()` has been added to `openshift.py` to validate limit ranges.
This allows changes to the default limit ranges (`LIMITRANGE_DEFAULTS`) to
be applied to pre-existing Openshift projects.

The plan will be for this function to eventually contain all validation logic
for Openshift project configurations, allowing `validation_allocations`
to be resource-agnostic.

New functional test case added
`parse_quota_value` has been moved to `utils.py` as it is now used in two places

Author: QuanMPhm

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

src/coldfront_plugin_cloud/management/commands/validate_allocations.py

@@ -1,13 +1,12 @@
 import logging
-import re
 
 from coldfront_plugin_cloud import attributes
 from coldfront_plugin_cloud import openstack
 from coldfront_plugin_cloud import openshift
 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 = (

src/coldfront_plugin_cloud/openshift.py

@@ -3,6 +3,8 @@
 import logging
 import os
 import time
+import re
+import copy
 
 import kubernetes
 import kubernetes.dynamic.exceptions as kexc
@@ -54,6 +56,94 @@ 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
+
+
+def limit_ranges_diff(
+    expected_lr_list: list[dict], actual_lr_list: list[dict], project_id: str = ""
+) -> list[str]:
+    expected_lr = copy.deepcopy(expected_lr_list[0])
+    actual_lr = copy.deepcopy(actual_lr_list[0])
+    differences = []
+    for key, item in expected_lr.items():
+        if key == "type":
+            if actual_lr.get("type") != item:
+                differences.append(
+                    f"LimitRange for {project_id} differs [{key}]: "
+                    f"expected {item} but found {actual_lr.get('type')}"
+                )
+            continue
+        for resource, expected_value in item.items():
+            try:
+                # Reassignment to later allow simple check for non-default fields
+                expected_lr[key][resource] = expected_value = parse_quota_value(
+                    expected_value, resource
+                )
+                actual_lr[key][resource] = actual_value = parse_quota_value(
+                    actual_lr[key][resource], resource
+                )
+                if expected_value != actual_value:
+                    differences.append(
+                        f"LimitRange for {project_id} differs [{key}][{resource}]: "
+                        f"expected {expected_value} but found {actual_value}"
+                    )
+            except KeyError:
+                differences.append(
+                    f"LimitRange for {project_id} missing [{key}][{resource}]"
+                )
+
+    if not differences and expected_lr != actual_lr:
+        differences.append(
+            f"Actual LimitRange for {project_id} contains non-default fields"
+        )
+    return differences
+
+
 class ApiException(Exception):
     def __init__(self, message):
         self.message = message
@@ -130,6 +220,37 @@ def get_resource_api(self, api_version: str, kind: str):
         )
         return api
 
+    def set_project_configuration(self, project_id, dry_run=False):
+        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}."
+            )
+            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}.")
+
+        if len(limits) == 1:
+            actual_limits = limits[0]["spec"]["limits"]
+            differences = limit_ranges_diff(
+                LIMITRANGE_DEFAULTS, actual_limits, project_id
+            )
+
+            if differences:
+                for message in differences:
+                    logger.info(message)
+                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}.")
+
     def create_project(self, suggested_project_name):
         sanitized_project_name = utils.get_sanitized_project_name(
             suggested_project_name
@@ -446,6 +567,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())

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

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
+            ),
+            [],
+        )

src/coldfront_plugin_cloud/tests/unit/test_openshift_utils.py

@@ -0,0 +1,54 @@
+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, project_id="foo")
+        self.assertEqual(diffs, [])
+
+        # type mismatch, [default][cpu] value mismatch (1 vs 2), and missing [min][memory]
+        actual[0]["type"] = "Pod"
+        actual[0]["default"]["cpu"] = "2"
+        del actual[0]["min"]["memory"]
+        diffs = openshift.limit_ranges_diff(expected, actual, project_id="foo")
+        self.assertTrue(any("expected Container but found Pod" in d for d in diffs))
+        self.assertTrue(
+            any("differs [default][cpu]: expected 1 but found 2" in d for d in diffs)
+        )
+        self.assertTrue(any("missing [min][memory]" in d for d in diffs))
+
+        # Contains extra fields
+        actual = [copy.deepcopy(expected[0])]
+        actual[0]["default"]["ephemeral-storage"] = "10Gi"
+        diffs = openshift.limit_ranges_diff(expected, actual, project_id="foo")
+        self.assertTrue(any("contains non-default fields" in d for d in diffs))