fix: order resource classes by GPU, CPU, RAM, storage (#420)

* fix: order resource classes by GPU, CPU, RAM, storage

* fix: order resource classes by GPU, CPU, RAM, storage

* fix tests

Author: Panaetius

components/renku_data_services/crc/db.py

@@ -191,7 +191,7 @@ async def filter_resource_pools(
         max_storage: int = 0,
         gpu: int = 0,
     ) -> list[models.ResourcePool]:
-        """Get resource pools from database with indication of which resource class matches the specified crtieria."""
+        """Get resource pools from database with indication of which resource class matches the specified criteria."""
         async with self.session_maker() as session:
             criteria = models.ResourceClass(
                 name="criteria",
@@ -205,18 +205,17 @@ async def filter_resource_pools(
             )
             stmt = (
                 select(schemas.ResourcePoolORM)
-                .join(schemas.ResourcePoolORM.classes)
+                .distinct()
+                .options(selectinload(schemas.ResourcePoolORM.classes))
                 .order_by(
                     schemas.ResourcePoolORM.id,
                     schemas.ResourcePoolORM.name,
-                    schemas.ResourceClassORM.id,
-                    schemas.ResourceClassORM.name,
                 )
             )
             # NOTE: The line below ensures that the right users can access the right resources, do not remove.
             stmt = _resource_pool_access_control(api_user, stmt)
             res = await session.execute(stmt)
-            return [i.dump(self.quotas_repo.get_quota(i.quota), criteria) for i in res.unique().scalars().all()]
+            return [i.dump(self.quotas_repo.get_quota(i.quota), criteria) for i in res.scalars().all()]
 
     @_only_admins
     async def insert_resource_pool(

components/renku_data_services/crc/models.py

@@ -207,10 +207,23 @@ def __post_init__(self) -> None:
         if len(default_classes) != 1:
             raise ValidationError(message="One default class is required in each resource pool.")
 
-        # We need to sort classes to make '__eq__' reliable
-        object.__setattr__(
-            self, "classes", sorted(self.classes, key=lambda x: (x.default, x.cpu, x.memory, x.default_storage, x.name))
-        )
+    def __eq__(self, other: Any) -> bool:
+        """Check two resource pools for equality."""
+        if not isinstance(other, ResourcePool):
+            return False
+
+        if self.id != other.id:
+            return False
+        if self.name != other.name:
+            return False
+        if self.default != other.default or self.public != other.public:
+            return False
+        if self.idle_threshold != other.idle_threshold or self.hibernation_threshold != other.hibernation_threshold:
+            return False
+
+        this_classes = sorted(self.classes, key=lambda x: (x.default, x.cpu, x.memory, x.default_storage, x.name))
+        other_classes = sorted(other.classes, key=lambda x: (x.default, x.cpu, x.memory, x.default_storage, x.name))
+        return this_classes == other_classes
 
     def set_quota(self, val: Quota) -> "ResourcePool":
         """Set the quota for a resource pool."""

components/renku_data_services/crc/orm.py

@@ -144,6 +144,10 @@ class ResourcePoolORM(BaseORM):
         default_factory=list,
         cascade="save-update, merge, delete",
         lazy="selectin",
+        order_by=(
+            "[ResourceClassORM.gpu,ResourceClassORM.cpu,ResourceClassORM.memory,ResourceClassORM.max_storage,"
+            "ResourceClassORM.name,ResourceClassORM.id]"
+        ),
     )
     idle_threshold: Mapped[Optional[int]] = mapped_column(default=None)
     hibernation_threshold: Mapped[Optional[int]] = mapped_column(default=None)

test/bases/renku_data_services/data_api/test_resource_pools.py

@@ -147,6 +147,86 @@ async def test_resource_class_filtering(
     assert len(res.json[0]["classes"]) == len(new_classes)
 
 
+@pytest.mark.asyncio
+async def test_resource_class_ordering(
+    sanic_client: SanicASGITestClient, admin_headers: dict[str, str], valid_resource_pool_payload: dict[str, Any]
+) -> None:
+    new_classes = [
+        {
+            "name": "resource class 5",
+            "cpu": 0.1,
+            "memory": 1,
+            "gpu": 1,
+            "max_storage": 1,
+            "default": False,
+            "default_storage": 1,
+            "node_affinities": [],
+            "tolerations": [],
+        },
+        {
+            "name": "resource class 4",
+            "cpu": 9.0,
+            "memory": 1,
+            "gpu": 0,
+            "max_storage": 1,
+            "default": False,
+            "default_storage": 1,
+            "node_affinities": [],
+            "tolerations": [],
+        },
+        {
+            "name": "resource class 3",
+            "cpu": 0.1,
+            "memory": 100,
+            "gpu": 0,
+            "max_storage": 1,
+            "default": False,
+            "default_storage": 1,
+            "node_affinities": [],
+            "tolerations": [],
+        },
+        {
+            "name": "resource class 2",
+            "cpu": 0.1,
+            "memory": 1,
+            "gpu": 0,
+            "max_storage": 100,
+            "default": False,
+            "default_storage": 1,
+            "node_affinities": [],
+            "tolerations": [],
+        },
+        {
+            "name": "resource class 1",
+            "cpu": 0.1,
+            "memory": 1,
+            "gpu": 0,
+            "max_storage": 10,
+            "default": True,
+            "default_storage": 1,
+            "node_affinities": [],
+            "tolerations": [],
+        },
+    ]
+    payload = valid_resource_pool_payload
+    payload["quota"] = {"cpu": 100, "memory": 100, "gpu": 100}
+    payload["classes"] = new_classes
+    _, res = await create_rp(payload, sanic_client)
+    assert res.status_code == 201
+    _, res = await sanic_client.get(
+        "/api/data/resource_pools",
+        params={"cpu": 1, "gpu": 1},
+        headers=admin_headers,
+    )
+    assert res.status_code == 200
+    assert len(res.json) == 1
+    rp_filtered = res.json[0]
+
+    new_classes_names = [c["name"] for c in new_classes]
+    returned_names = [c["name"] for c in rp_filtered["classes"]]
+    assert new_classes_names[::-1] == returned_names  # classes should show up in reverse order
+
+
 @pytest.mark.asyncio
 async def test_get_single_pool_quota(
     sanic_client: SanicASGITestClient, valid_resource_pool_payload: dict[str, Any], admin_headers: dict[str, str]

test/components/renku_data_services/db/test_sqlalchemy_pool_repo.py

@@ -111,9 +111,7 @@ async def test_resource_pool_update_classes(
         old_classes = [asdict(cls) for cls in list(inserted_rp.classes)]
         new_classes_dicts = [{**cls, **data.draw(rc_update_reqs_dict)} for cls in old_classes]
         new_classes_models = [models.ResourceClass.from_dict(cls) for cls in new_classes_dicts]
-        new_classes_models = sorted(
-            new_classes_models, key=lambda x: (x.default, x.cpu, x.memory, x.default_storage, x.name)
-        )
+        new_classes_models = sorted(new_classes_models, key=lambda x: (x.gpu, x.cpu, x.memory, x.max_storage, x.name))
 
         updated_rp = await pool_repo.update_resource_pool(
             id=inserted_rp.id, classes=new_classes_dicts, api_user=admin_user