feat: ensure notebook endpoints do their job #388

sgaist · 2024-09-06T11:31:48Z

Describe your changes

This PR ensure that the notebook API part correctly answers and acts on the requests sent to make it on par with the renku- notebook implementation.

Issue ticket number and link

Part of #303

olevski

Thank you for adding this Samuel. It looks really good! Just a few changes.

.../renku_data_services/message_queue/avro_models/io/renku/events/v2/reprovisioning_finished.py

...s/renku_data_services/message_queue/avro_models/io/renku/events/v2/reprovisioning_started.py

test/bases/renku_data_services/data_api/utils.py

pyproject.toml

olevski · 2024-09-16T12:01:46Z

components/renku_data_services/notebooks/blueprints.py

+            try:
+                await self.nb_config.k8s_client.delete_server(server_name, safe_username=user.id)
+            except MissingResourceError as err:
+                raise exceptions.NotFound(message=err.message)


suggestion: Just supress a missing resource error for deletion. I.e. we want the server to not be there so if the resource is not there we can just return a 204. This is what we do in most other endpoints.

I was wondering whether there should be a trace left for the admin if there was at some point too many tentative to delete a server.

Sure if you want log a warning instead of fully suppressing.

olevski · 2024-09-16T12:03:42Z

components/renku_data_services/notebooks/blueprints.py

+                )
+                return json(ServerLogs().dump(logs))
+            except MissingResourceError as err:
+                raise exceptions.NotFound(message=err.message)


suggestion: just use MissingResourceError from errors.errors - when we raise errors we should be using these exceptions. Then our centralized error handler properly formats these. Not sure why you have even added this try/except because the k8s client will already raise the proper error and message if the server does not exist. So you dont have to catch and reraise here at all.

I did this because the MissingResourceError exception would trigger a 500 error and not the NotFound from sanic.

If I understand things correctly, k8s_client should use the one from errors.errors rather than notebook.errors.user.

If I understand things correctly, k8s_client should use the one from errors.errors rather than notebook.errors.user.

Yes that is exactly right. We should just fully eliminate the errors from notebook.errors

test/bases/renku_data_services/data_api/test_notebooks.py

chore: filter environments by owner type squashme: address comments feat!: expand environment specification This is a breaking change in the API. chore: add tests and minor fixes chore: test the global environments migration chore: fix tests chore: minor improvements to db session handling squashme: minor fix squashme: fixups for conflict resolutuion after merge squashme: fix failing tests chore: address comments

This includes major edits to the notebooks code to work with the data service.

The gitlab credentials header from the notebooks is really complicated. We used it here just to get the access token expiry. I modified the gateway to now pass in an extra header value to indicate the gitlab token expiry.

The query args retrieval was incorrect and the log retrieval needed to be waited on. Slight refactor to ensure mypy is happy with int conversion

…ntication

The returned values must be tested more thoroughly as for example the hibernation status is not in the returned json.

The manifest being now validated, it looks like the v2 variant does not provide some of the fields. The question is the following: should the manifest definition reflect that or the code adapted to provide the values. Currently the second option is used.

olevski · 2024-09-24T08:48:22Z

Squash and merge please when you merge this.

We should fix the tests before we merge.

This configuration forces the use of ipv4 which is required to be able to start on GitHub's runner which do not provide ipv6.

It's a leftover of the original API and note used in the current implementation.

The message shown during the CI run on error is not fully visible so it's hard to understand what is going wrong.

The cluster creation in the CI fails with kind however k3d was successfully used in renku-notebooks for the same kind of tests.

The exit method will not be called and the cluster will thus not be properly deleted.

This ensures that each of them get its own cluster in case one of them is not properly deleted.

This allows to use the same tool in development and CI.

This work brings the notebook related endpoints to a working state to serve as replacement for the renku-notebooks external service. Short summary: - API tests have been added - Code has been fixed to answer / handle exception - Automated test cluster creation has been added using k3d --------- Co-authored-by: Tasko Olevski <tasko.olevski@sdsc.ethz.ch>

sgaist requested a review from a team as a code owner September 6, 2024 11:31

sgaist force-pushed the feature/notebook-api-working branch 4 times, most recently from bb70ba7 to 0ed563d Compare September 11, 2024 13:28

olevski requested changes Sep 16, 2024

View reviewed changes

sgaist mentioned this pull request Sep 19, 2024

refactor: make APIUser a frozen dataclass #412

Closed

olevski force-pushed the feat-run-notebook-service-simplify-history branch 3 times, most recently from 9e88bad to 1a44f2e Compare September 23, 2024 08:41

Base automatically changed from feat-run-notebook-service-simplify-history to release-amaltheas-migration September 23, 2024 11:52

olevski force-pushed the release-amaltheas-migration branch 2 times, most recently from 8b164c0 to 8878712 Compare September 23, 2024 12:03

olevski and others added 17 commits September 23, 2024 22:55

feat: add command and args to environments

8e0cfb1

squashme: notebooks changes

5bef056

This includes major edits to the notebooks code to work with the data service.

chore: resolve changes from conflict resolution

ed3b7c5

chore: do not use the complicated notebooks gitlab header

4790016

The gitlab credentials header from the notebooks is really complicated. We used it here just to get the access token expiry. I modified the gateway to now pass in an extra header value to indicate the gitlab token expiry.

fix(pre-commit): fix applied by pre-commit

60dae48

fix(notebooks): fix url args handling for /notebook/images

5c2b105

fix(notebooks): correct logs retrieval

08242e3

The query args retrieval was incorrect and the log retrieval needed to be waited on. Slight refactor to ensure mypy is happy with int conversion

feat(notebook): add test for server option endpoint

87a60b1

fix(notebook): fix log retrieval

74b3a31

fix(notebooks): add fake renku token header to fixture for user authe…

013bade

…ntication

fix(notebook): make server stop endpoint work as expected

aea155e

fix(notebooks): make patch endpoint work

7de5cd3

The returned values must be tested more thoroughly as for example the hibernation status is not in the returned json.

refactor(notebooks): rename i in loops to something more meaningful

0b07d54

fix(notebooks): correct logic for old start project endpoint

b05b25d

fix(notebook): cleanup tests

0d1336c

olevski self-requested a review September 24, 2024 08:50

sgaist added 4 commits September 24, 2024 11:08

fix(test_notebooks): use specific kind configuration for test cluster

ff88481

This configuration forces the use of ipv4 which is required to be able to start on GitHub's runner which do not provide ipv6.

fix(notebooks): use ErrorResponse and remove ErrorResponseNested

32cb3e6

It's a leftover of the original API and note used in the current implementation.

refactor(utils): explicitly print subprocess output on error

b99befe

The message shown during the CI run on error is not fully visible so it's hard to understand what is going wrong.

refactor(test): use k3d in place of kind for cluster creation

e6a20b8

The cluster creation in the CI fails with kind however k3d was successfully used in renku-notebooks for the same kind of tests.

sgaist force-pushed the feature/notebook-api-working branch from 16eeb93 to e6a20b8 Compare September 24, 2024 14:38

sgaist added 3 commits September 24, 2024 17:15

fix(test_notebooks): delete cluster if image upload fails

9dfb613

The exit method will not be called and the cluster will thus not be properly deleted.

fix(tests): use different cluster names for notebooks and sessions

f9c4f05

This ensures that each of them get its own cluster in case one of them is not properly deleted.

refactor: move from kind to k3d

7a0f8e3

This allows to use the same tool in development and CI.

sgaist force-pushed the feature/notebook-api-working branch from a9ee8de to 7a0f8e3 Compare September 24, 2024 15:56

olevski approved these changes Sep 25, 2024

View reviewed changes

sgaist merged commit 30923b5 into release-amaltheas-migration Sep 25, 2024
15 checks passed

sgaist deleted the feature/notebook-api-working branch September 25, 2024 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ensure notebook endpoints do their job #388

feat: ensure notebook endpoints do their job #388

sgaist commented Sep 6, 2024

olevski left a comment

olevski Sep 16, 2024

sgaist Sep 17, 2024

olevski Sep 19, 2024

olevski Sep 16, 2024

sgaist Sep 17, 2024

olevski Sep 19, 2024

olevski commented Sep 24, 2024

feat: ensure notebook endpoints do their job #388

feat: ensure notebook endpoints do their job #388

Conversation

sgaist commented Sep 6, 2024

Describe your changes

Issue ticket number and link

olevski left a comment

Choose a reason for hiding this comment

olevski Sep 16, 2024

Choose a reason for hiding this comment

sgaist Sep 17, 2024

Choose a reason for hiding this comment

olevski Sep 19, 2024

Choose a reason for hiding this comment

olevski Sep 16, 2024

Choose a reason for hiding this comment

sgaist Sep 17, 2024

Choose a reason for hiding this comment

olevski Sep 19, 2024

Choose a reason for hiding this comment

olevski commented Sep 24, 2024