feat: add resource class id in session launcher #249

andre-code · 2024-06-10T19:19:42Z

PR to support resource class in session launcher.

/deploy

RenkuBot · 2024-06-10T19:20:41Z

You can access the deployment of this PR at https://renku-ci-ds-249.dev.renku.ch

Panaetius

Thank you, this is really nice. I have some comments, but that's mostly things that were already wrong before you started, so nothing you did wrong and we need to clean this up during cooldown. But we should still address them in the newly added code at least. Ping me if there's questions of I can help 🙂

Panaetius · 2024-06-21T08:22:41Z

components/renku_data_services/crc/blueprints.py

@@ -318,7 +318,7 @@ def get_no_pool(self) -> BlueprintFactoryResponse:
        @authenticate(self.authenticator)
        @validate_db_ids
        async def _get_no_pool(_: Request, user: base_models.APIUser, class_id: int) -> HTTPResponse:
-            res = await self.repo.get_classes(api_user=user, id=class_id)
+            res = await self.repo.get_classes(api_user=None, id=class_id)


nitpick: if the user is not used in this method at all, you can just remove the @authenticate decorator above

Sure, included in the latest commit.

Panaetius · 2024-06-21T09:26:47Z

components/renku_data_services/session/db.py

+            resource_class_id = new_launcher.resource_class_id
+            if resource_class_id is not None:
+                res = await session.scalars(
+                    select(schemas.ResourceClassORM).where(schemas.ResourceClassORM.id == resource_class_id)
+                )
+                resource_class = res.one_or_none()
+                if resource_class is None:
+                    raise errors.MissingResourceError(
+                        message=f"Resource class with id '{resource_class_id}' does not exist or you do not have access to it."  # noqa: E501
+                    )
+


issue: we should use get_classes with the user here. doing DB queries in the session db on things owned by the crc DB doesn't separate concerns properly and despite the error saying or you do not have access to it, this does not actually check permissions.
So here we should call ResourcePoolRepository.get_classes. For this to work, we need to inject the ResourcePoolRepository into the SessionRepository. similar to line 47 for project_authz.
To be clear, this is not something you did wrong, but something this whole file is guilty of and that we need to address in the future. But at least for new code we should do it properly.

Sure, included in the latest commit.

Panaetius · 2024-06-21T09:27:17Z

components/renku_data_services/session/db.py

+            resource_class_id = kwargs.get("resource_class_id")
+            if resource_class_id is not None:
+                res = await session.scalars(
+                    select(schemas.ResourceClassORM).where(schemas.ResourceClassORM.id == resource_class_id)
+                )
+                resource_class = res.one_or_none()
+                if resource_class is None:
+                    raise errors.MissingResourceError(
+                        message=f"Resource class with id '{resource_class_id}' does not exist or you do not have access to it."  # noqa: E501
+                    )


issue: same here, we need to properly validate that a user actually has access to the class.

Panaetius · 2024-06-21T09:28:36Z

components/renku_data_services/session/orm.py

@@ -139,6 +150,7 @@ def dump(self) -> models.SessionLauncher:
            description=self.description,
            environment_kind=self.environment_kind,
            environment_id=self.environment_id if self.environment_id is not None else None,
+            resource_class_id=self.resource_class_id if self.resource_class_id is not None else None,


nitpick: I don't think this if actually does something, same for the line above. we can just assign directly.

Panaetius · 2024-06-21T09:33:04Z

test/bases/renku_data_services/data_api/test_sessions.py

@@ -277,6 +277,7 @@ async def test_post_session_launcher(sanic_client: SanicASGITestClient, user_hea
    assert res.json.get("environment_kind") == "container_image"
    assert res.json.get("container_image") == "some_image:some_tag"
    assert res.json.get("environment_id") is None
+    assert res.json.get("resource_class_id") is None


suggestion: it would be nice to test that setting this to a value also works, though that's a bit more difficult as we don't have a fixture to have a pool/class available.

It'd be nice to have something like https://github.com/SwissDataScienceCenter/renku-data-services/blob/main/test/bases/renku_data_services/data_api/test_resource_pools.py#L68-L74 as a fixture that you can depend on. Let me know if you need help with that.

sure, I added the fixture to create a resource pool and the test to validate if the user has access to the resource class when creating or updating a launcher.

andre-code · 2024-06-25T06:40:56Z

Thank you, @Panaetius and @leafty, for your review. I have incorporated your suggestions, and the changes are ready for your review again.

coveralls · 2024-06-25T15:21:44Z

Pull Request Test Coverage Report for Build 9659135346

Details

53 of 64 (82.81%) changed or added relevant lines in 9 files are covered.
4 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.002%) to 90.16%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
components/renku_data_services/session/db.py	13	24	54.17%

Files with Coverage Reduction	New Missed Lines	%
components/renku_data_services/crc/blueprints.py	1	90.42%
components/renku_data_services/storage/blueprints.py	1	95.16%
components/renku_data_services/session/db.py	2	77.71%

Totals
Change from base Build 9614271413:	0.002%
Covered Lines:	8613
Relevant Lines:	9553

💛 - Coveralls

Panaetius · 2024-06-26T07:33:39Z

components/renku_data_services/session/db.py

@@ -217,15 +222,22 @@ async def insert_launcher(
                resource_class = res.one_or_none()
                if resource_class is None:
                    raise errors.MissingResourceError(
-                        message=f"Resource class with id '{resource_class_id}' does not exist or you do not have access to it."  # noqa: E501
+                        message=f"Resource class with id '{resource_class_id}' does not exist."  # noqa: E501


nitpick: I think now the noqa: E501 is not necessary anymore, as the line is not too long.
With pre-commit hooks installed, yesqa should catch this

Panaetius · 2024-06-26T07:33:48Z

components/renku_data_services/session/db.py

@@ -269,7 +281,14 @@ async def update_launcher(
                resource_class = res.one_or_none()
                if resource_class is None:
                    raise errors.MissingResourceError(
-                        message=f"Resource class with id '{resource_class_id}' does not exist or you do not have access to it."  # noqa: E501
+                        message=f"Resource class with id '{resource_class_id}' does not exist."  # noqa: E501
+                    )


same as above

…update; include tests for unauthorized resource_class_id

… session launcher

leafty · 2024-06-27T11:10:41Z

...nku_data_services/migrations/versions/57dfd69ea814_add_resource_class_in_session_launcher.py

+
+    # Recreate the foreign key with ON DELETE SET NULL


non-blocking: I think this comment can be removed here.

Suggested change

# Recreate the foreign key with ON DELETE SET NULL

coveralls · 2024-06-27T13:37:18Z

Pull Request Test Coverage Report for Build 9696088375

Details

52 of 64 (81.25%) changed or added relevant lines in 9 files are covered.
8 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.04%) to 90.117%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
components/renku_data_services/crc/blueprints.py	1	2	50.0%
components/renku_data_services/session/db.py	13	24	54.17%

Files with Coverage Reduction	New Missed Lines	%
components/renku_data_services/storage/blueprints.py	1	94.62%
components/renku_data_services/session/db.py	2	77.71%
components/renku_data_services/crc/blueprints.py	5	89.46%

Totals
Change from base Build 9676504188:	-0.04%
Covered Lines:	8635
Relevant Lines:	9582

💛 - Coveralls

RenkuBot · 2024-07-01T07:36:29Z

Tearing down the temporary RenkuLab deplyoment for this PR.

andre-code temporarily deployed to renku-ci-ds-249 June 10, 2024 19:20 — with GitHub Actions Inactive

andre-code force-pushed the andrea/add-launcher-resource-class branch from 5bb41c8 to 60e7448 Compare June 14, 2024 09:34

andre-code temporarily deployed to renku-ci-ds-249 June 14, 2024 09:34 — with GitHub Actions Inactive

andre-code force-pushed the andrea/add-launcher-resource-class branch from 60e7448 to c4eb800 Compare June 14, 2024 12:58

andre-code temporarily deployed to renku-ci-ds-249 June 14, 2024 12:58 — with GitHub Actions Inactive

andre-code temporarily deployed to renku-ci-ds-249 June 14, 2024 14:47 — with GitHub Actions Inactive

andre-code temporarily deployed to renku-ci-ds-249 June 14, 2024 14:58 — with GitHub Actions Inactive

andre-code temporarily deployed to renku-ci-ds-249 June 14, 2024 15:10 — with GitHub Actions Inactive

andre-code temporarily deployed to renku-ci-ds-249 June 14, 2024 18:05 — with GitHub Actions Inactive

andre-code force-pushed the andrea/add-launcher-resource-class branch from c4eb800 to 93d2dfa Compare June 14, 2024 19:06

andre-code temporarily deployed to renku-ci-ds-249 June 14, 2024 19:06 — with GitHub Actions Inactive

andre-code force-pushed the andrea/add-launcher-resource-class branch from 93d2dfa to 19540e0 Compare June 17, 2024 07:30

andre-code had a problem deploying to renku-ci-ds-249 June 17, 2024 07:30 — with GitHub Actions Error

andre-code force-pushed the andrea/add-launcher-resource-class branch from 19540e0 to f15c97f Compare June 17, 2024 07:36

andre-code had a problem deploying to renku-ci-ds-249 June 17, 2024 07:37 — with GitHub Actions Failure

andre-code force-pushed the andrea/add-launcher-resource-class branch from f15c97f to 0971091 Compare June 17, 2024 07:45

andre-code had a problem deploying to renku-ci-ds-249 June 17, 2024 07:45 — with GitHub Actions Error

andre-code force-pushed the andrea/add-launcher-resource-class branch from 0971091 to 79f8d84 Compare June 17, 2024 07:46

andre-code had a problem deploying to renku-ci-ds-249 June 17, 2024 07:46 — with GitHub Actions Failure

andre-code had a problem deploying to renku-ci-ds-249 June 17, 2024 08:11 — with GitHub Actions Error

andre-code force-pushed the andrea/add-launcher-resource-class branch from 79f8d84 to 5c02a77 Compare June 17, 2024 08:12

andre-code had a problem deploying to renku-ci-ds-249 June 17, 2024 08:12 — with GitHub Actions Failure

andre-code had a problem deploying to renku-ci-ds-249 June 17, 2024 08:23 — with GitHub Actions Failure

andre-code had a problem deploying to renku-ci-ds-249 June 17, 2024 08:27 — with GitHub Actions Failure

andre-code had a problem deploying to renku-ci-ds-249 June 17, 2024 08:51 — with GitHub Actions Failure

andre-code had a problem deploying to renku-ci-ds-249 June 17, 2024 09:36 — with GitHub Actions Failure

andre-code force-pushed the andrea/add-launcher-resource-class branch from 5c02a77 to 310b5b1 Compare June 17, 2024 10:36

andre-code had a problem deploying to renku-ci-ds-249 June 17, 2024 10:36 — with GitHub Actions Failure

andre-code force-pushed the andrea/add-launcher-resource-class branch from 310b5b1 to 0445579 Compare June 17, 2024 11:22

Panaetius requested changes Jun 21, 2024

View reviewed changes

andre-code had a problem deploying to renku-ci-ds-249 June 24, 2024 19:44 — with GitHub Actions Failure

andre-code had a problem deploying to renku-ci-ds-249 June 24, 2024 20:21 — with GitHub Actions Failure

andre-code temporarily deployed to renku-ci-ds-249 June 24, 2024 20:33 — with GitHub Actions Inactive

andre-code requested review from Panaetius and leafty June 25, 2024 06:47

leafty temporarily deployed to renku-ci-ds-249 June 25, 2024 07:51 — with GitHub Actions Inactive

andre-code temporarily deployed to renku-ci-ds-249 June 25, 2024 08:32 — with GitHub Actions Inactive

andre-code mentioned this pull request Jun 25, 2024

feat: set resource class for a session launcher SwissDataScienceCenter/renku-ui#3196

Merged

10 tasks

Panaetius requested changes Jun 26, 2024

View reviewed changes

andre-code and others added 4 commits June 27, 2024 12:45

feat: add resource class id in session launcher

ce4d761

feat: add validation for resource_class_id access in launcher insert/…

c4f5808

…update; include tests for unauthorized resource_class_id

fixes

55575c6

update migration file to don't include index for resource_class_id in…

cfe55ec

… session launcher

andre-code force-pushed the andrea/add-launcher-resource-class branch from 826850b to 149d71e Compare June 27, 2024 10:45

andre-code temporarily deployed to renku-ci-ds-249 June 27, 2024 10:46 — with GitHub Actions Inactive

remove unnecessary noqa: E501

b8b8464

andre-code force-pushed the andrea/add-launcher-resource-class branch from 149d71e to b8b8464 Compare June 27, 2024 10:58

andre-code temporarily deployed to renku-ci-ds-249 June 27, 2024 10:59 — with GitHub Actions Inactive

leafty reviewed Jun 27, 2024

View reviewed changes

remove unnecessary comment and fix style_check

bab430e

andre-code deployed to renku-ci-ds-249 June 27, 2024 11:56 — with GitHub Actions View deployment

Panaetius approved these changes Jun 27, 2024

View reviewed changes

leafty approved these changes Jun 27, 2024

View reviewed changes

andre-code merged commit 489086a into main Jul 1, 2024
14 of 15 checks passed

andre-code deleted the andrea/add-launcher-resource-class branch July 1, 2024 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add resource class id in session launcher #249

feat: add resource class id in session launcher #249

andre-code commented Jun 10, 2024 •

edited

Loading

RenkuBot commented Jun 10, 2024

Panaetius left a comment

Panaetius Jun 21, 2024

andre-code Jun 25, 2024 •

edited

Loading

Panaetius Jun 21, 2024

andre-code Jun 25, 2024

Panaetius Jun 21, 2024

Panaetius Jun 21, 2024

Panaetius Jun 21, 2024 •

edited

Loading

andre-code Jun 25, 2024

andre-code commented Jun 25, 2024

coveralls commented Jun 25, 2024 •

edited

Loading

Panaetius Jun 26, 2024

Panaetius Jun 26, 2024

leafty Jun 27, 2024

coveralls commented Jun 27, 2024 •

edited

Loading

RenkuBot commented Jul 1, 2024

feat: add resource class id in session launcher #249

feat: add resource class id in session launcher #249

Conversation

andre-code commented Jun 10, 2024 • edited Loading

RenkuBot commented Jun 10, 2024

Panaetius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andre-code Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Panaetius Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andre-code commented Jun 25, 2024

coveralls commented Jun 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9659135346

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 27, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9696088375

Details

💛 - Coveralls

RenkuBot commented Jul 1, 2024

andre-code commented Jun 10, 2024 •

edited

Loading

andre-code Jun 25, 2024 •

edited

Loading

Panaetius Jun 21, 2024 •

edited

Loading

coveralls commented Jun 25, 2024 •

edited

Loading

coveralls commented Jun 27, 2024 •

edited

Loading