Merge pull request #264 from QuanMPhm/257/limit_ranges

knikolla · web-flow · commit 772cd9fcee65 · 2025-11-06T17:04:09.000-05:00
Validate Openshift limit ranges
diff --git 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
@@ -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 = (
diff --git 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
@@ -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
@@ -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
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

Original file line number	Diff line number	Diff line change
`@@ -141,6 +141,9 @@ def object(self, project_id=None, session=None) -> swiftclient.Connection:`
`141`	`141`	`preauthurl=preauth_url,`
`142`	`142`	`)`
`143`	`143`
	`144`	`+ def set_project_configuration(self, project_id, dry_run=False):`
	`145`	`+ pass`
	`146`	`+`
`144`	`147`	`def create_project(self, suggested_project_name) -> base.ResourceAllocator.Project:`
`145`	`148`	`project_name = utils.get_unique_project_name(`
`146`	`149`	`suggested_project_name, max_length=self.project_name_max_length`