From fb32accd50dbd2b6cbf7131812b55bc08ff872b8 Mon Sep 17 00:00:00 2001 From: Quan Pham Date: Sat, 5 Apr 2025 16:13:06 -0400 Subject: [PATCH 1/3] Fixed integration with Openshift API It was discovered that Openshift integration did not function because the Openshift allocation would use the same url to make calls to both the account manager and the Openshift API. This resulted in the Openshift API never actually being called. Due to poor error handling by the integration code (more details below), this big went undetected by the functional tests. Appropriate fixes have been added tests, and the Openshift resource type now requires two URLs, one for the account manager, and one for the Openshift API. Regarding the poor error handling, none of the functional Openshift test cases actually checked if the call to `get_federated_user()` returned the expected output. The `get_federated_user()` function itself only emits a log message if the user is not found. This meant that even though `get_federated_user()` never called the Openshift API and would therefore never find the user, the test cases still passed. Additionally, while `_openshift_user_exists()`, the function which was supposed to call the Openshift API, does catch the `kexc.NotFoundError`, this error is not specific enough, as it could be caused by a 404 response made by ANY server (in the case of our tests, the account manager). It was also found that the `RESOURCE_IDENTITY_NAME` attribute, which identifies the idp, was referenced in integration code but never defined, leading to `_openshift_identity_exists` always failing, since `self.id_provider` would always be `None` --- ci/run_functional_tests_openshift.sh | 2 ++ src/coldfront_plugin_cloud/attributes.py | 2 ++ .../commands/add_openshift_resource.py | 16 +++++++++++ src/coldfront_plugin_cloud/openshift.py | 27 +++++++++++++------ src/coldfront_plugin_cloud/tests/base.py | 4 ++- .../functional/openshift/test_allocation.py | 9 ++++--- 6 files changed, 48 insertions(+), 12 deletions(-) diff --git a/ci/run_functional_tests_openshift.sh b/ci/run_functional_tests_openshift.sh index 060ef547..87c37540 100755 --- a/ci/run_functional_tests_openshift.sh +++ b/ci/run_functional_tests_openshift.sh @@ -17,6 +17,8 @@ fi export DJANGO_SETTINGS_MODULE="local_settings" export FUNCTIONAL_TESTS="True" export OS_AUTH_URL="https://onboarding-onboarding.cluster.local" +export OS_API_URL="https://onboarding-onboarding.cluster.local:6443" + coverage run --source="." -m django test coldfront_plugin_cloud.tests.functional.openshift coverage report diff --git a/src/coldfront_plugin_cloud/attributes.py b/src/coldfront_plugin_cloud/attributes.py index a410d150..89f680ff 100644 --- a/src/coldfront_plugin_cloud/attributes.py +++ b/src/coldfront_plugin_cloud/attributes.py @@ -19,6 +19,7 @@ class CloudAllocationAttribute: RESOURCE_AUTH_URL = 'Identity Endpoint URL' +RESOURCE_API_URL = 'OpenShift API Endpoint URL' RESOURCE_IDENTITY_NAME = 'OpenShift Identity Provider Name' RESOURCE_ROLE = 'Role for User in Project' @@ -33,6 +34,7 @@ class CloudAllocationAttribute: RESOURCE_ATTRIBUTES = [ CloudResourceAttribute(name=RESOURCE_AUTH_URL), + CloudResourceAttribute(name=RESOURCE_API_URL), CloudResourceAttribute(name=RESOURCE_IDENTITY_NAME), CloudResourceAttribute(name=RESOURCE_FEDERATION_PROTOCOL), CloudResourceAttribute(name=RESOURCE_IDP), diff --git a/src/coldfront_plugin_cloud/management/commands/add_openshift_resource.py b/src/coldfront_plugin_cloud/management/commands/add_openshift_resource.py index 6e96bcb9..91a2fc22 100644 --- a/src/coldfront_plugin_cloud/management/commands/add_openshift_resource.py +++ b/src/coldfront_plugin_cloud/management/commands/add_openshift_resource.py @@ -17,6 +17,10 @@ def add_arguments(self, parser): help='Name of OpenShift resource') parser.add_argument('--auth-url', type=str, required=True, help='URL of the openshift-acct-mgt endpoint') + parser.add_argument('--api-url', type=str, required=True, + help='API URL of the openshift cluster') + parser.add_argument('--idp', type=str, required=True, + help='Name of Openshift identity provider') parser.add_argument('--role', type=str, default='edit', help='Role for user when added to project (default: edit)') @@ -37,6 +41,18 @@ def handle(self, *args, **options): resource=openshift, value=options['auth_url'] ) + ResourceAttribute.objects.get_or_create( + resource_attribute_type=ResourceAttributeType.objects.get( + name=attributes.RESOURCE_API_URL), + resource=openshift, + value=options['api_url'] + ) + ResourceAttribute.objects.get_or_create( + resource_attribute_type=ResourceAttributeType.objects.get( + name=attributes.RESOURCE_IDENTITY_NAME), + resource=openshift, + value=options['idp'] + ) ResourceAttribute.objects.get_or_create( resource_attribute_type=ResourceAttributeType.objects.get( name=attributes.RESOURCE_ROLE), diff --git a/src/coldfront_plugin_cloud/openshift.py b/src/coldfront_plugin_cloud/openshift.py index 7edb9c43..6760a678 100644 --- a/src/coldfront_plugin_cloud/openshift.py +++ b/src/coldfront_plugin_cloud/openshift.py @@ -77,7 +77,7 @@ def __init__(self, resource, allocation): def k8_client(self): # Load Endpoint URL and Auth token for new k8 client openshift_token = os.getenv(f"OPENSHIFT_{self.safe_resource_name}_TOKEN") - openshift_url = self.resource.get_attribute(attributes.RESOURCE_AUTH_URL) + openshift_url = self.resource.get_attribute(attributes.RESOURCE_API_URL) k8_config = kubernetes.client.Configuration() k8_config.api_key["authorization"] = openshift_token @@ -181,7 +181,7 @@ def reactivate_project(self, project_id): def get_federated_user(self, username): if ( self._openshift_user_exists(username) - and self._openshift_get_identity(username) + and self._openshift_identity_exists(username) and self._openshift_useridentitymapping_exists(username, username) ): return {'username': username} @@ -264,22 +264,33 @@ def _openshift_get_identity(self, id_user): def _openshift_user_exists(self, user_name): try: self._openshift_get_user(user_name) - except kexc.NotFoundError: - return False + except kexc.NotFoundError as e: + # Ensures error raise because resource not found, + # not because of other reasons, like incorrect url + e_info = json.loads(e.body) + if e_info.get("reason") == "NotFound": + return False + raise e return True def _openshift_identity_exists(self, id_user): try: self._openshift_get_identity(id_user) - except kexc.NotFoundError: - return False + except kexc.NotFoundError as e: + e_info = json.loads(e.body) + if e_info.get("reason") == "NotFound": + return False + raise e return True def _openshift_useridentitymapping_exists(self, user_name, id_user): try: user = self._openshift_get_user(user_name) - except kexc.NotFoundError: - return False + except kexc.NotFoundError as e: + e_info = json.loads(e.body) + if e_info.get("reason") == "NotFound": + return False + raise e return any( identity == self.qualified_id_user(id_user) diff --git a/src/coldfront_plugin_cloud/tests/base.py b/src/coldfront_plugin_cloud/tests/base.py index 305410a1..14184c33 100644 --- a/src/coldfront_plugin_cloud/tests/base.py +++ b/src/coldfront_plugin_cloud/tests/base.py @@ -81,13 +81,15 @@ def new_openstack_resource(name=None, auth_url=None) -> Resource: return Resource.objects.get(name=resource_name) @staticmethod - def new_openshift_resource(name=None, auth_url=None) -> Resource: + def new_openshift_resource(name=None, auth_url=None, api_url=None, idp=None) -> Resource: resource_name = name or uuid.uuid4().hex call_command( 'add_openshift_resource', name=resource_name, auth_url=auth_url or 'https://onboarding-onboarding.cluster.local', + api_url=api_url or 'https://onboarding-onboarding.cluster.local:6443', + idp=idp or 'developer', ) return Resource.objects.get(name=resource_name) 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 483d224c..0312516d 100644 --- a/src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py +++ b/src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py @@ -15,7 +15,8 @@ def setUp(self) -> None: super().setUp() self.resource = self.new_openshift_resource( name='Microshift', - auth_url=os.getenv('OS_AUTH_URL') + auth_url=os.getenv('OS_AUTH_URL'), + api_url=os.getenv('OS_API_URL'), ) def test_new_allocation(self): @@ -36,7 +37,8 @@ def test_new_allocation(self): allocator._get_project(project_id) # Check user and roles - allocator.get_federated_user(user.username) + user_info = allocator.get_federated_user(user.username) + self.assertEqual(user_info, {'username': user.username}) allocator._get_role(user.username, project_id) @@ -73,7 +75,8 @@ def test_add_remove_user(self): tasks.add_user_to_allocation(allocation_user2.pk) allocator._get_role(user.username, project_id) - allocator.get_federated_user(user2.username) + user_info = allocator.get_federated_user(user.username) + self.assertEqual(user_info, {'username': user.username}) allocator._get_role(user.username, project_id) allocator._get_role(user2.username, project_id) From b91248ff28b7941921c58d006ebaa2373afa8dfa Mon Sep 17 00:00:00 2001 From: Quan Pham Date: Fri, 4 Apr 2025 14:02:40 -0400 Subject: [PATCH 2/3] Allow direct communication with Openshift Quota API The Openshift allocator will now only make the minimal `resourcequota` object for each namespace, with no support for scopes. Most of the integration code and test cases have been adapted from `openshift-acct-mgt`. Notable exclusions were any code pertaining to the `quota.json`[1] and `limits.json`[2]. [1] https://github.com/CCI-MOC/openshift-acct-mgt/blob/master/k8s/base/quotas.json [2] https://github.com/CCI-MOC/openshift-acct-mgt/blob/master/k8s/base/limits.json --- src/coldfront_plugin_cloud/openshift.py | 117 +++++++++++++++--- .../tests/unit/openshift/test_quota.py | 101 +++++++++++++++ 2 files changed, 204 insertions(+), 14 deletions(-) create mode 100644 src/coldfront_plugin_cloud/tests/unit/openshift/test_quota.py diff --git a/src/coldfront_plugin_cloud/openshift.py b/src/coldfront_plugin_cloud/openshift.py index 6760a678..0881fef4 100644 --- a/src/coldfront_plugin_cloud/openshift.py +++ b/src/coldfront_plugin_cloud/openshift.py @@ -36,12 +36,12 @@ def clean_openshift_metadata(obj): return obj QUOTA_KEY_MAPPING = { - attributes.QUOTA_LIMITS_CPU: lambda x: {":limits.cpu": f"{x * 1000}m"}, - attributes.QUOTA_LIMITS_MEMORY: lambda x: {":limits.memory": f"{x}Mi"}, - attributes.QUOTA_LIMITS_EPHEMERAL_STORAGE_GB: lambda x: {":limits.ephemeral-storage": f"{x}Gi"}, - attributes.QUOTA_REQUESTS_STORAGE: lambda x: {":requests.storage": f"{x}Gi"}, - attributes.QUOTA_REQUESTS_GPU: lambda x: {":requests.nvidia.com/gpu": f"{x}"}, - attributes.QUOTA_PVC: lambda x: {":persistentvolumeclaims": f"{x}"}, + attributes.QUOTA_LIMITS_CPU: lambda x: {"limits.cpu": f"{x * 1000}m"}, + attributes.QUOTA_LIMITS_MEMORY: lambda x: {"limits.memory": f"{x}Mi"}, + attributes.QUOTA_LIMITS_EPHEMERAL_STORAGE_GB: lambda x: {"limits.ephemeral-storage": f"{x}Gi"}, + attributes.QUOTA_REQUESTS_STORAGE: lambda x: {"requests.storage": f"{x}Gi"}, + attributes.QUOTA_REQUESTS_GPU: lambda x: {"requests.nvidia.com/gpu": f"{x}"}, + attributes.QUOTA_PVC: lambda x: {"persistentvolumeclaims": f"{x}"}, } @@ -146,20 +146,64 @@ def create_project(self, suggested_project_name): project_name = project_id self._create_project(project_name, project_id) return self.Project(project_name, project_id) + + def delete_moc_quotas(self, project_id): + """deletes all resourcequotas from an openshift project""" + resourcequotas = self._openshift_get_resourcequotas(project_id) + for resourcequota in resourcequotas: + self._openshift_delete_resourcequota(project_id, resourcequota["metadata"]["name"]) + + logger.info(f"All quotas for {project_id} successfully deleted") def set_quota(self, project_id): - url = f"{self.auth_url}/projects/{project_id}/quota" - payload = dict() + """Sets the quota for a project, creating a minimal resourcequota + object in the project namespace with no extra scopes""" + + quota_spec = {} for key, func in QUOTA_KEY_MAPPING.items(): if (x := self.allocation.get_attribute(key)) is not None: - payload.update(func(x)) - r = self.session.put(url, data=json.dumps({'Quota': payload})) - self.check_response(r) + quota_spec.update(func(x)) + + quota_def = { + "metadata": {"name": f"{project_id}-project"}, + "spec": {"hard": quota_spec}, + } + + self.delete_moc_quotas(project_id) + self._openshift_create_resourcequota(project_id, quota_def) + + logger.info(f"Quota for {project_id} successfully created") + + def _get_moc_quota_from_resourcequotas(self, project_id): + """This returns a dictionary suitable for merging in with the + specification from Adjutant/ColdFront""" + resourcequotas = self._openshift_get_resourcequotas(project_id) + moc_quota = {} + for rq in resourcequotas: + name, spec = rq["metadata"]["name"], rq["spec"] + logger.info(f"processing resourcequota: {project_id}:{name}") + scope_list = spec.get("scopes", [""]) + for quota_name, quota_value in spec.get("hard", {}).items(): + for scope_item in scope_list: + moc_quota_name = f"{scope_item}:{quota_name}" + moc_quota.setdefault(moc_quota_name, quota_value) + return moc_quota def get_quota(self, project_id): - url = f"{self.auth_url}/projects/{project_id}/quota" - r = self.session.get(url) - return self.check_response(r) + quota_from_project = self._get_moc_quota_from_resourcequotas(project_id) + + quota = {} + for quota_name, quota_value in quota_from_project.items(): + if quota_value: + quota[quota_name] = quota_value + + quota_object = { + "Version": "0.9", + "Kind": "MocQuota", + "ProjectName": project_id, + "Quota": quota, + } + return quota_object def create_project_defaults(self, project_id): pass @@ -297,3 +341,48 @@ def _openshift_useridentitymapping_exists(self, user_name, id_user): for identity in user.get("identities", []) ) + def _openshift_get_project(self, project_name): + api = self.get_resource_api(API_PROJECT, "Project") + return clean_openshift_metadata(api.get(name=project_name).to_dict()) + + def _openshift_get_resourcequotas(self, project_id): + """Returns a list of all of the resourcequota objects""" + # Raise a NotFound error if the project doesn't exist + self._openshift_get_project(project_id) + api = self.get_resource_api(API_CORE, "ResourceQuota") + res = clean_openshift_metadata(api.get(namespace=project_id).to_dict()) + + return res["items"] + + def _wait_for_quota_to_settle(self, project_id, resource_quota): + """Wait for quota on resourcequotas to settle. + + When creating a new resourcequota that sets a quota on resourcequota objects, we need to + wait for OpenShift to calculate the quota usage before we attempt to create any new + resourcequota objects. + """ + + if "resourcequotas" in resource_quota["spec"]["hard"]: + logger.info("waiting for resourcequota quota") + + api = self.get_resource_api(API_CORE, "ResourceQuota") + while True: + resp = clean_openshift_metadata( + api.get( + namespace=project_id, name=resource_quota["metadata"]["name"] + ).to_dict() + ) + if "resourcequotas" in resp["status"].get("used", {}): + break + time.sleep(0.1) + + def _openshift_create_resourcequota(self, project_id, quota_def): + api = self.get_resource_api(API_CORE, "ResourceQuota") + res = api.create(namespace=project_id, body=quota_def).to_dict() + self._wait_for_quota_to_settle(project_id, res) + + def _openshift_delete_resourcequota(self, project_id, resourcequota_name): + """In an openshift namespace {project_id) delete a specified resourcequota""" + api = self.get_resource_api(API_CORE, "ResourceQuota") + return api.delete(namespace=project_id, name=resourcequota_name).to_dict() + diff --git a/src/coldfront_plugin_cloud/tests/unit/openshift/test_quota.py b/src/coldfront_plugin_cloud/tests/unit/openshift/test_quota.py new file mode 100644 index 00000000..e62aad7c --- /dev/null +++ b/src/coldfront_plugin_cloud/tests/unit/openshift/test_quota.py @@ -0,0 +1,101 @@ +from unittest import mock + +from coldfront_plugin_cloud.tests import base +from coldfront_plugin_cloud.openshift import OpenShiftResourceAllocator + + +class TestOpenshiftQuota(base.TestBase): + def setUp(self) -> None: + mock_resource = mock.Mock() + mock_allocation = mock.Mock() + self.allocator = OpenShiftResourceAllocator(mock_resource, mock_allocation) + self.allocator.id_provider = "fake_idp" + self.allocator.k8_client = mock.Mock() + + @mock.patch("coldfront_plugin_cloud.openshift.OpenShiftResourceAllocator._openshift_get_project", mock.Mock()) + def test_get_resourcequotas(self): + fake_quota = mock.Mock(spec=["to_dict"]) + fake_quota.to_dict.return_value = {"items": []} + self.allocator.k8_client.resources.get.return_value.get.return_value = fake_quota + res = self.allocator._openshift_get_resourcequotas("fake-project") + self.allocator.k8_client.resources.get.return_value.get.assert_called() + assert res == [] + + + def test_delete_quota(self): + fake_quota = mock.Mock(spec=["to_dict"]) + fake_quota.to_dict.return_value = {} + self.allocator.k8_client.resources.get.return_value.delete.return_value = fake_quota + res = self.allocator._openshift_delete_resourcequota("test-project", "test-quota") + self.allocator.k8_client.resources.get.return_value.delete.assert_called() + assert res == {} + + + @mock.patch("coldfront_plugin_cloud.openshift.OpenShiftResourceAllocator._openshift_get_resourcequotas") + def test_delete_moc_quota(self, fake_get_resourcequotas): + fake_get_resourcequotas.return_value = [{"metadata": {"name": "fake-quota"}}] + self.allocator.delete_moc_quotas("test-project") + self.allocator.k8_client.resources.get.return_value.delete.assert_any_call( + namespace="test-project", name="fake-quota" + ) + + + @mock.patch("coldfront_plugin_cloud.openshift.OpenShiftResourceAllocator._wait_for_quota_to_settle") + def test_create_shift_quotas(self, fake_wait_quota): + quotadefs = { + "metadata": {"name": "fake-project-project"}, + "spec": {"hard": {"configmaps": "1", "cpu": "1", "resourcequotas": "1"}}, + } + + self.allocator.k8_client.resources.get.return_value.create.return_value = mock.Mock() + + self.allocator._openshift_create_resourcequota("fake-project", quotadefs) + + self.allocator.k8_client.resources.get.return_value.create.assert_called_with( + namespace="fake-project", + body={ + "metadata": {"name": "fake-project-project"}, + "spec": {"hard": {"configmaps": "1", "cpu": "1", "resourcequotas": "1"}}, + }, + ) + + fake_wait_quota.assert_called() + + + def test_wait_for_quota_to_settle(self): + fake_quota = mock.Mock(spec=["to_dict"]) + fake_quota.to_dict.return_value = { + "metadata": {"name": "fake-quota"}, + "spec": {"hard": {"resourcequotas": "1"}}, + "status": {"used": {"resourcequotas": "1"}}, + } + self.allocator.k8_client.resources.get.return_value.get.return_value = fake_quota + + self.allocator._wait_for_quota_to_settle("fake-project", fake_quota.to_dict()) + + self.allocator.k8_client.resources.get.return_value.get.assert_called_with( + namespace="fake-project", + name="fake-quota", + ) + + @mock.patch("coldfront_plugin_cloud.openshift.OpenShiftResourceAllocator._get_moc_quota_from_resourcequotas") + def test_get_moc_quota(self, fake_get_quota): + fake_get_quota.return_value = { + ":services": {"value": "2"}, + ":configmaps": {"value": None}, + ":cpu": {"value": "1000"}, + } + res = self.allocator.get_quota("fake-project") + assert res == { + "Version": "0.9", + "Kind": "MocQuota", + "ProjectName": "fake-project", + "Quota": { + ":services": {"value": "2"}, + ":configmaps": {"value": None}, + ":cpu": {"value": "1000"}, + }, + } + + + From fd133741d820ec78edca43e5c9dce9537d193a3a Mon Sep 17 00:00:00 2001 From: Quan Pham Date: Fri, 7 Mar 2025 14:19:13 -0500 Subject: [PATCH 3/3] Allow automatic approval of Openshift allocations if change request (cr) only decreases quotas An edge case is added to prevent approval when quota is requested to be 0. This would normally happen when the user wants to delete their allocation, which is a seperate case from when they only want to decrease quotas. Implementing this feature required several changes: - Several functions in `utils.py` and `tasks.py` to perform automatic approval - New signal handler for when a cr is created. For now, this handler will only perform automatic checks for Openshift allocations - A new function in the Openshift allocator to get resources used (`get_quota_used`) - Slight refactoring on how the Openshift quota is parsed - Unit tests for the functions performing the various checks have been added --- .../commands/validate_allocations.py | 45 +------ src/coldfront_plugin_cloud/openshift.py | 15 +++ src/coldfront_plugin_cloud/signals.py | 23 +++- src/coldfront_plugin_cloud/tasks.py | 34 ++++- .../tests/unit/test_utils.py | 101 ++++++++++++++- src/coldfront_plugin_cloud/utils.py | 119 ++++++++++++++++++ 6 files changed, 290 insertions(+), 47 deletions(-) diff --git a/src/coldfront_plugin_cloud/management/commands/validate_allocations.py b/src/coldfront_plugin_cloud/management/commands/validate_allocations.py index ca9d5379..27dc9789 100644 --- a/src/coldfront_plugin_cloud/management/commands/validate_allocations.py +++ b/src/coldfront_plugin_cloud/management/commands/validate_allocations.py @@ -197,50 +197,7 @@ def handle(self, *args, **options): expected_value = allocation.get_attribute(attr.name) current_value = quota.get(key, None) - PATTERN = r"([0-9]+)(m|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, - "M": 10**6, - "G": 10**9, - "T": 10**12, - "P": 10**15, - "E": 10**18, - } - - if current_value and current_value != "0": - result = re.search(PATTERN, current_value) - - if result is None: - raise CommandError( - f"Unable to parse current_value = '{current_value}' for {attr.name}" - ) - - value = int(result.groups()[0]) - unit = result.groups()[1] - - # Convert to number i.e. without any unit suffix - - if unit is not None: - current_value = value * suffix[unit] - else: - current_value = value - - # Convert some attributes to units that coldfront uses - - if "RAM" in attr.name: - current_value = round(current_value / suffix["Mi"]) - elif "Storage" in attr.name: - current_value = round(current_value / suffix["Gi"]) - elif current_value and current_value == "0": - current_value = 0 + current_value = utils.parse_openshift_quota_value(attr.name, current_value) 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 0881fef4..4c9c2e5d 100644 --- a/src/coldfront_plugin_cloud/openshift.py +++ b/src/coldfront_plugin_cloud/openshift.py @@ -204,6 +204,21 @@ def get_quota(self, project_id): "Quota": quota, } return quota_object + + def _get_moc_quota_used_from_resourcequotas(self, project_id): + resourcequotas = self._openshift_get_resourcequotas(project_id) + moc_quota_used = {} + for rq in resourcequotas: + moc_quota_used.update(rq["status"]["used"]) + return moc_quota_used + + def get_quota_used(self, project_id): + resourcequotas = self._openshift_get_resourcequotas(project_id) + moc_quota_used = {} + # TODO Any concerns about this being a list? Can a project have multiple resourcequotas? + for rq in resourcequotas: + moc_quota_used.update(rq["status"]["used"]) + return moc_quota_used def create_project_defaults(self, project_id): pass diff --git a/src/coldfront_plugin_cloud/signals.py b/src/coldfront_plugin_cloud/signals.py index 9e85d887..e9a99aa3 100644 --- a/src/coldfront_plugin_cloud/signals.py +++ b/src/coldfront_plugin_cloud/signals.py @@ -6,11 +6,15 @@ from coldfront_plugin_cloud.tasks import (activate_allocation, add_user_to_allocation, disable_allocation, - remove_user_from_allocation) + remove_user_from_allocation, + get_allocation_usage, + approve_change_request) +from coldfront_plugin_cloud import utils from coldfront.core.allocation.signals import (allocation_activate, allocation_activate_user, allocation_disable, allocation_remove_user, + allocation_change_created, allocation_change_approved) @@ -52,3 +56,20 @@ def activate_allocation_user_receiver(sender, **kwargs): def allocation_remove_user_receiver(sender, **kwargs): allocation_user_pk = kwargs.get('allocation_user_pk') remove_user_from_allocation(allocation_user_pk) + +# TODO (Quan): How to/should we do the functional test for this? +@receiver(allocation_change_created) +def allocation_change_created_receiver(sender, **kwargs): + allocation_pk = kwargs.get('allocation_pk') + allocation_change_pk = kwargs.get('allocation_change_pk') + + if not utils.check_cr_only_decreases(allocation_change_pk): + return + + if utils.check_cr_set_to_zero(allocation_change_pk): + return + + allocation_quota_usage = get_allocation_usage(allocation_pk) + if allocation_quota_usage and utils.check_usage_is_lower(allocation_change_pk, allocation_quota_usage): + approve_change_request(allocation_change_pk) # Updates attributes on Coldfront side + allocation_change_approved.send(None, allocation_pk=allocation_pk, allocation_change_pk=allocation_change_pk) diff --git a/src/coldfront_plugin_cloud/tasks.py b/src/coldfront_plugin_cloud/tasks.py index 32c8f7b6..7fc5e33a 100644 --- a/src/coldfront_plugin_cloud/tasks.py +++ b/src/coldfront_plugin_cloud/tasks.py @@ -4,7 +4,10 @@ import time from coldfront.core.allocation.models import (Allocation, - AllocationUser) + AllocationUser, + AllocationChangeRequest, + AllocationChangeStatusChoice, + AllocationAttributeChangeRequest) from coldfront_plugin_cloud import (attributes, base, @@ -172,3 +175,32 @@ def remove_user_from_allocation(allocation_user_pk): allocator.remove_role_from_user(username, project_id) else: logger.warning('No project has been created. Nothing to disable.') + + +def get_allocation_usage(allocation_pk): + allocation = Allocation.objects.get(pk=allocation_pk) + # Note(quan): Only supports Openshift for now + if allocator := find_allocator(allocation): + if allocator.resource_type == 'openshift': + if project_id := allocation.get_attribute(attributes.ALLOCATION_PROJECT_ID): + # TODO (Quan) Apply function to convert the fetched quota values into numbers + return allocator.get_quota_used(project_id) + else: + logger.warning('No project has been created. No quota to check.') + return + + +def approve_change_request( + allocation_change_request_pk, +): + allocation_cr = AllocationChangeRequest.objects.get(pk=allocation_change_request_pk) + allocation_change_status_active_obj = AllocationChangeStatusChoice.objects.get( + name='Approved') + allocation_cr.status = allocation_change_status_active_obj + allocation_cr.save() + allocation_attr_cr_list = AllocationAttributeChangeRequest.objects.filter( + allocation_change_request=allocation_cr + ) + for attribute_change in allocation_attr_cr_list: + attribute_change.allocation_attribute.value = attribute_change.new_value + attribute_change.allocation_attribute.save() diff --git a/src/coldfront_plugin_cloud/tests/unit/test_utils.py b/src/coldfront_plugin_cloud/tests/unit/test_utils.py index b93418e7..63c19d9b 100644 --- a/src/coldfront_plugin_cloud/tests/unit/test_utils.py +++ b/src/coldfront_plugin_cloud/tests/unit/test_utils.py @@ -2,8 +2,12 @@ import secrets from random import randrange +from coldfront.core.allocation import models as allocation_models + from coldfront_plugin_cloud.tests import base -from coldfront_plugin_cloud import utils +from coldfront_plugin_cloud import utils, attributes + +MiB_IN_GiB = 1024 class TestGetSanitizedProjectName(base.TestBase): def test_project_name(self): @@ -19,3 +23,98 @@ def test_get_unique_project_name_length(self): self.assertGreater(len(project_name), max_length) new_name = utils.get_unique_project_name(project_name, max_length=max_length) self.assertEqual(len(new_name), max_length) + +class TestCheckChangeRequests(base.TestBase): + def setUp(self): + super().setUp() + # Create test allocation change request and attribute change requests + cpu_quota_attr = allocation_models.AllocationAttributeType.objects.get( + name=attributes.QUOTA_LIMITS_CPU + ) + memory_quota_attr = allocation_models.AllocationAttributeType.objects.get( + name=attributes.QUOTA_LIMITS_MEMORY + ) + test_attr = allocation_models.AllocationAttributeType.objects.get( + name=attributes.ALLOCATION_PROJECT_ID # Not quota attr, should be ignored + ) + + self.allo = self.new_allocation(self.new_project(), self.new_openshift_resource(), 1) + self.allo_cr = allocation_models.AllocationChangeRequest.objects.create( + allocation=self.allo, + status=allocation_models.AllocationChangeStatusChoice.objects.first(), # Doesn't matter which status + ) + self.allo_attr_cr_cpu = allocation_models.AllocationAttributeChangeRequest.objects.create( + allocation_change_request=self.allo_cr, + allocation_attribute=allocation_models.AllocationAttribute.objects.create( + allocation=self.allo, + allocation_attribute_type=cpu_quota_attr, + value=8, + ), + new_value=2, + ) + self.allo_attr_cr_memory = allocation_models.AllocationAttributeChangeRequest.objects.create( + allocation_change_request=self.allo_cr, + allocation_attribute=allocation_models.AllocationAttribute.objects.create( + allocation=self.allo, + allocation_attribute_type=memory_quota_attr, + value=16 * MiB_IN_GiB, + ), + new_value=4 * MiB_IN_GiB, + ) + self.allo_attr_cr_test = allocation_models.AllocationAttributeChangeRequest.objects.create( + allocation_change_request=self.allo_cr, + allocation_attribute=allocation_models.AllocationAttribute.objects.create( + allocation=self.allo, + allocation_attribute_type=test_attr, + value=1, + ), + new_value=10, + ) + + def test_check_cr_only_decreases(self): + # True case, test attr should be ignored + self.assertTrue(utils.check_cr_only_decreases(self.allo_cr.pk)) + + # One attribute increases, should return False + self.allo_attr_cr_cpu.new_value = 100 + self.allo_attr_cr_cpu.save() + self.assertFalse(utils.check_cr_only_decreases(self.allo_cr.pk)) + + # Attribute is not int, current behavior is an error + self.allo_attr_cr_cpu.new_value = "test" + self.allo_attr_cr_cpu.save() + with self.assertRaises(ValueError): + utils.check_cr_only_decreases(self.allo_cr.pk) + + def test_check_cr_set_to_zero(self): + # True case, test attr should be ignored + self.allo_attr_cr_cpu.new_value = 0 + self.allo_attr_cr_cpu.save() + self.assertTrue(utils.check_cr_set_to_zero(self.allo_cr.pk)) + + # One attribute increases, should return False + self.allo_attr_cr_cpu.new_value = 1 + self.allo_attr_cr_cpu.save() + self.assertFalse(utils.check_cr_set_to_zero(self.allo_cr.pk)) + + # Attribute is not int, current behavior is an error + self.allo_attr_cr_cpu.new_value = "test" + self.allo_attr_cr_cpu.save() + with self.assertRaises(ValueError): + utils.check_cr_only_decreases(self.allo_cr.pk) + + def test_check_usage_is_lower(self): + # True case, test attr should be ignored + test_quota_usage = { + "limits.cpu": "1", + "limits.memory": "2Gi", + "limits.ephemeral-storage": "10Gi", # Other quotas should be ignored + "requests.storage": "40Gi", + "requests.nvidia.com/gpu": "0", + "persistentvolumeclaims": "4", + } + self.assertTrue(utils.check_usage_is_lower(self.allo_cr.pk, test_quota_usage)) + + # Requested cpu (2) lower than current used, should return False + test_quota_usage["limits.cpu"] = "16" + self.assertFalse(utils.check_usage_is_lower(self.allo_cr.pk, test_quota_usage)) diff --git a/src/coldfront_plugin_cloud/utils.py b/src/coldfront_plugin_cloud/utils.py index 0edad63f..179cd9e1 100644 --- a/src/coldfront_plugin_cloud/utils.py +++ b/src/coldfront_plugin_cloud/utils.py @@ -12,6 +12,7 @@ AllocationAttributeChangeRequest,) from coldfront_plugin_cloud import attributes +from coldfront_plugin_cloud import openshift def env_safe_name(name): @@ -220,3 +221,121 @@ def get_included_duration(start: datetime.datetime, total_interval_duration -= (e_interval_end - e_interval_start).total_seconds() return math.ceil(total_interval_duration) + + +def parse_openshift_quota_value(attr_name, quota_value): + PATTERN = r"([0-9]+)(m|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, + "M": 10**6, + "G": 10**9, + "T": 10**12, + "P": 10**15, + "E": 10**18, + } + + if quota_value and quota_value != "0": + result = re.search(PATTERN, quota_value) + + if result is None: + raise ValueError( + f"Unable to parse quota_value = '{quota_value}' for {attr_name}" + ) + + value = int(result.groups()[0]) + unit = result.groups()[1] + + # Convert to number i.e. without any unit suffix + + if unit is not None: + quota_value = value * suffix[unit] + else: + quota_value = value + + # Convert some attributes to units that coldfront uses + + if "RAM" in attr_name: + return round(quota_value / suffix["Mi"]) + elif "Storage" in attr_name: + return round(quota_value / suffix["Gi"]) + return quota_value + elif quota_value and quota_value == "0": + return 0 + + +def check_if_quota_attr(attr_name): + for quota_attr in attributes.ALLOCATION_QUOTA_ATTRIBUTES: + if attr_name == quota_attr.name: + return True + return False + + +def check_cr_only_decreases(allocation_change_pk): + """Checks if the change request only decreases the quota. + + :param allocation_change_pk: key of AllocationChangeRequest object. + :return: True if the change request only decreases the quota. + """ + allocation_cr = AllocationChangeRequest.objects.get(pk=allocation_change_pk) + allocation_attr_cr_list = AllocationAttributeChangeRequest.objects.filter( + allocation_change_request=allocation_cr + ) + + for allocation_attr_cr in allocation_attr_cr_list: + attr_name = allocation_attr_cr.allocation_attribute.allocation_attribute_type.name + if check_if_quota_attr(attr_name): + if int(allocation_attr_cr.new_value) > int(allocation_attr_cr.allocation_attribute.value): + return False + return True + + +def check_cr_set_to_zero(allocation_change_pk): + """Checks if the change request only decreases the quota. + + :param allocation_change_pk: key of AllocationChangeRequest object. + :return: True if the change request only decreases the quota. + """ + allocation_cr = AllocationChangeRequest.objects.get(pk=allocation_change_pk) + allocation_attr_cr_list = AllocationAttributeChangeRequest.objects.filter( + allocation_change_request=allocation_cr + ) + + for allocation_attr_cr in allocation_attr_cr_list: + attr_name = allocation_attr_cr.allocation_attribute.allocation_attribute_type.name + if check_if_quota_attr(attr_name): + if int(allocation_attr_cr.new_value) == 0: + return True + return False + + +def check_usage_is_lower(allocation_change_pk, allocation_quota_usage): + """Checks if the usage is lower than the quota. + + :param allocation_change_pk: AllocationChangeRequest object. + :param allocation_quota_usage: Quota usage to compare. + :return: True if the usage is lower than the quota. + """ + allocation_cr = AllocationChangeRequest.objects.get(pk=allocation_change_pk) + allocation_attr_cr_list = AllocationAttributeChangeRequest.objects.filter( + allocation_change_request=allocation_cr + ) + + for allocation_attr_cr in allocation_attr_cr_list: + attr_name = allocation_attr_cr.allocation_attribute.allocation_attribute_type.name + if check_if_quota_attr(attr_name): + # TODO (Quan) Copied from `validate_allocations`, I feel like this is very messy + quota_key_with_lambda = openshift.QUOTA_KEY_MAPPING.get(attr_name, None) + quota_key = list(quota_key_with_lambda(1).keys())[0] + if int(allocation_attr_cr.new_value) < parse_openshift_quota_value(attr_name, allocation_quota_usage[quota_key]): + return False + return True + +