feat: move notebook service code #301

olevski · 2024-07-11T12:26:49Z

This PR aims to move (but not enable) the notebooks API from the notebook repo into here.

Enabling the handlers will be done in subsequent PRs.

Please look at: https://drive.google.com/file/d/1w40wZfFeG1H0MBjVp8wdsuw1z0gvWP-n/view?usp=sharing for a good diff of the move.

/deploy

closes #90

* refactor: minor formatting and unnecessary code removal * feat: include notebook username in pod labels * refactor: minor * feat: use k8s API to get user Jupyter servers * refactor: remove ununsed code * fix: minor fix in README.rst * feat: return server data as needed by UI Co-authored-by: rokroskar <[email protected]> * build: make tests pass * fix: apply review comments

…ves (#190) * fix: use shortened commit-sha to check for an autosave match * fix: use commit-sha of remote branch to create autosave branch name * fix: avoid a None dereferencing * fix: use both local and remote commit-sha for autosave

fix: update telepresence script and add vs code debugger

* feat: handle k8s api errors when querying for logs (#216)

feat: return json data from server_logs and expose lines limit fix #224

fix #223

closes #273

- Enable the use of tmpauthenticator for Jupyterhub authentication. This enables notebook sessions for logged-out users. Closes #171 - Enable logged in users without write access to a project to start notebook servers from the project. Closes #275 - Add a helper script which creates a helm values file for a secondary notebooks service deployment from the main Renku values file (some amount of guessing involved).

closes #287

Add cronjob that periodically deletes users image pull secrets along with unit tests

* fix: cannot create user env due to invalid k8s secret labels

* fix: properly determine user server pod status with multiple containers Co-authored-by: Andreas Bleuler <[email protected]>

* chore: simplify tests * chore: make JH start once per session * feat: add schemas and swagger * feat: add swagger descriptions and security schema * chore: add docstrings to schema classes * fix: return server details when pending and spawning to prevent UI content flicker

* fix: add missing fields in renku-notebooks schemas * chore: update tests * fix: simplify definition of UserPodResources schema

Panaetius · 2024-07-12T13:46:49Z

Thank you for doing all this tedious work.
Review:
components/renku_data_services/notebooks/api/amalthea_patches/cloudstorage.py L9: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/general.py L9: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/git_proxy.py L11: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/git_sidecar.pye L10: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/init_containers.py L18: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/init_containers.py L217: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/inject_certificates.py L11: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L16: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L110: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L182: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L238: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/auth.py: not clear from the diff if this file is empty or deleted, should be deleted
components/renku_data_services/notebooks/api/classes/data_service.py L3: we should not use logging directly but import from sanic.log logging
components/renku_data_services/notebooks/api/classes/data_service.py: I guess you'll do that in a followup PR but all the http calls in this file are superfluous and can be replaced with direct calls
components/renku_data_services/notebooks/api/classes/data_service.py L118: Question: It's a bit odd to me that CRCVAlidator has defaults for properties of ServerOptions, shouldn't the defaults be defirned in the latter?
components/renku_data_services/notebooks/api/classes/server.py L3: use sanic.log logging
components/renku_data_services/notebooks/api/classes/server.py L188: remove quotes in middle of string
components/renku_data_services/notebooks/api/classes/user.py L5: use sanic.log
components/renku_data_services/notebooks/api/classes/user.py L38: we should remove this property is we can use isinstance(...,AnonymousUser) instead
components/renku_data_services/notebooks/api/notebooks.py L4: use sanic.log
components/renku_data_services/notebooks/api/notebooks.py L131: shouldn't we model_validate the body?
components/renku_data_services/notebooks/api/notebooks.py L165:rm docstring: don't both methods launch using the old operator, but one launches 1.0 sessions and the other 2.0 sessions?
components/renku_data_services/notebooks/api/notebooks.py L677: the error message could be more descriptive, if I saw this in a log, I wouldn't know what the cause was.
components/renku_data_services/notebooks/api/notebooks.py L683 and 685: isn't this what empty is for?
components/renku_data_services/notebooks/api/schemas/errors.py L91: remove commented code?
components/renku_data_services/notebooks/api/schemas/servers_get.py L145: exit_code can never be int
components/renku_data_services/notebooks/config/init.py: TODO for later, move out of init.py
components/renku_data_services/notebooks/util/retries.py L 19 and others: Is Concatenate needed with a single param?
components/renku_data_services/notebooks/util/authn.py: We need to refactor this code. we create a RegisteredUser no matter what, check if authenticated is True (which it is if all required headers are present, but we don't actually check the content of the headers in the RegisteredUser init) and if not create an AnonymousUser. This seems pretty brittle and is a weird way for this code to be written. and if I read it correctly, I could put whatever I want in those headers and it would consider it a registered user?

does this pass ruff checks? I saw several places that still use Optional[] instead of x | None. It should fail pyupdate checks I think.
for some files like auth.py, health.py it's not clear from the html diff if they're empty or removed. I assume removed? if not, they should be

…ports

olevski · 2024-07-15T14:18:37Z

@Panaetius see my responses:

components/renku_data_services/notebooks/api/amalthea_patches/cloudstorage.py L9: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/general.py L9: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/git_proxy.py L11: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/git_sidecar.pye L10: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/init_containers.py L18: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/init_containers.py L217: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/inject_certificates.py L11: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L16: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L110: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L182: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L238: UserServer does not need to be quoted

Yes it has to be and I have to add the stuff I removed with if TYPE_CHECKING: import ... because without these then we get circular import errors. I will add comments about this.

components/renku_data_services/notebooks/api/classes/data_service.py: I guess you'll do that in a followup PR but all the http calls in this file are superfluous and can be replaced with direct calls

Yes I dont just want to get this merged and do these large changes.

components/renku_data_services/notebooks/api/amalthea_patches/cloudstorage.py L9: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/general.py L9: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/git_proxy.py L11: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/git_sidecar.pye L10: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/init_containers.py L18: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/init_containers.py L217: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/inject_certificates.py L11: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L16: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L110: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L182: UserServer does not need to be quoted
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L238: UserServer does not need to be quoted

Yes it has to be and I have to add the stuff I removed with if TYPE_CHECKING: import ... because without these then we get circular import errors.

components/renku_data_services/notebooks/api/auth.py: not clear from the diff if this file is empty or deleted, should be deleted

It is deleted

components/renku_data_services/notebooks/api/classes/data_service.py L3: we should not use logging directly but import from sanic.log logging

Done

components/renku_data_services/notebooks/api/classes/data_service.py: I guess you'll do that in a followup PR but all the http calls in this file are superfluous and can be replaced with direct calls

Yes I want to do this a followup PR.

components/renku_data_services/notebooks/api/classes/data_service.py L118: Question: It's a bit odd to me that CRCVAlidator has defaults for properties of ServerOptions, shouldn't the defaults be defirned in the latter?

Done

components/renku_data_services/notebooks/api/classes/server.py L3: use sanic.log logging

Done

components/renku_data_services/notebooks/api/classes/server.py L188: remove quotes in middle of string

Done

components/renku_data_services/notebooks/api/classes/user.py L5: use sanic.log

Done

components/renku_data_services/notebooks/api/classes/user.py L38: we should remove this property is we can use isinstance(...,AnonymousUser) instead

Done

components/renku_data_services/notebooks/api/notebooks.py L4: use sanic.log

Done

components/renku_data_services/notebooks/api/notebooks.py L131: shouldn't we model_validate the body?

No need, we already use @validate(json=apispec.LaunchNotebookRequest) on handler which will do that for the request body.

components/renku_data_services/notebooks/api/notebooks.py L165:rm docstring: don't both methods launch using the old operator, but one launches 1.0 sessions and the other 2.0 sessions?

Yes, changed it.

components/renku_data_services/notebooks/api/notebooks.py L677: the error message could be more descriptive, if I saw this in a log, I wouldn't know what the cause was.

Agreed I will leave this as a TODO in a followup PR beacuse that section has the weird change to the code where I indented stuff for a better diff and I want to undo that before merging. I can also probably do this after I undo the changes for indentation for a better diff.

components/renku_data_services/notebooks/api/notebooks.py L683 and 685: isn't this what empty is for?

Is that a function from sanic? Sure I can use that. Same as above I will do it in a follow up.

components/renku_data_services/notebooks/api/schemas/errors.py L91: remove commented code?

Yes. done.

components/renku_data_services/notebooks/api/schemas/servers_get.py L145: exit_code can never be int

That was mean to be exit_code = str(exit_code_raw) if not isinstance(exit_code_raw, int) else exit_code_raw . And in this case exit_code can be int or string. I added that ternary if statement.

components/renku_data_services/notebooks/config/init.py: TODO for later, move out of init.py

Agreed

components/renku_data_services/notebooks/util/retries.py L 19 and others: Is Concatenate needed with a single param?

Mypy was not happy when I removed Concatenate even though it is just a single argument. But it probably wants that because it is not a regular argument but a ParamSpec.

components/renku_data_services/notebooks/util/authn.py: We need to refactor this code. we create a RegisteredUser no matter what, check if authenticated is True (which it is if all required headers are present, but we don't actually check the content of the headers in the RegisteredUser init) and if not create an AnonymousUser. This seems pretty brittle and is a weird way for this code to be written. and if I read it correctly, I could put whatever I want in those headers and it would consider it a registered user?

So the tokens are checked but the signatures are not verified. And we should be verifying the signatures ,just to be safe. Currently from outside of the cluster there is no way to get to the notebook service. And the gateway will always inject verified things in the headers. Either we should clean this up but in the end I think we simply will not use this at all and just fall back to the existing authenticator used everywhere in the data services. So this code will be fully purged. Either way this is a TODO.

does this pass ruff checks? I saw several places that still use Optional[] instead of x | None. It should fail pyupdate checks I think.

It should. I will remove the indentation changes and run the linting.

for some files like auth.py, health.py it's not clear from the html diff if they're empty or removed. I assume removed? if not, they should be

renku_notebooks/api/auth.py and renku_notebooks/api/health.py are both gone - just double checked.

olevski · 2024-07-15T14:24:08Z

Here is an issue with all followup work: #303

RenkuBot · 2024-07-15T14:59:52Z

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

…rom coverage

coveralls · 2024-07-15T19:17:25Z

Pull Request Test Coverage Report for Build 9942973186

Details

0 of 0 changed or added relevant lines in 0 files are covered.
5 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.03%) to 90.359%

Files with Coverage Reduction	New Missed Lines	%
components/renku_data_services/base_api/misc.py	1	97.62%
components/renku_data_services/crc/db.py	2	87.98%
components/renku_data_services/storage/blueprints.py	2	94.65%

Totals
Change from base Build 9937114559:	0.03%
Covered Lines:	8866
Relevant Lines:	9812

💛 - Coveralls

Panaetius · 2024-07-16T06:28:58Z

@Panaetius see my responses:

components/renku_data_services/notebooks/api/amalthea_patches/cloudstorage.py L9: UserServer does not need to be quoted
[...]
components/renku_data_services/notebooks/api/amalthea_patches/jupyter_server.py L238: UserServer does not need to be quoted

Yes it has to be and I have to add the stuff I removed with if TYPE_CHECKING: import ... because without these then we get circular import errors. I will add comments about this.

I didn't know that quoting usage but not the import prevents circular import. In any case, we should tidy this up so we don't have circular imports, like with injecting dependencies instead of importing directly in UserServer. Anyways, todo for later

components/renku_data_services/notebooks/api/notebooks.py L683 and 685: isn't this what empty is for?

Is that a function from sanic? Sure I can use that. Same as above I will do it in a follow up.

Yes it's a sanic function

from sanic import empty
[...]
    return empty(204)

for example

components/renku_data_services/notebooks/util/retries.py L 19 and others: Is Concatenate needed with a single param?

Mypy was not happy when I removed Concatenate even though it is just a single argument. But it probably wants that because it is not a regular argument but a ParamSpec.

#justmypythings I guess 🤷

RenkuBot · 2024-07-16T07:38:10Z

Tearing down the temporary RenkuLab deplyoment for this PR.

rokroskar and others added 30 commits June 3, 2019 17:15

style: fix code style and add tests

1de66f7

refactor: increase encapsulation; minor bug fixes

ebcdb0d

feat: spawn new server for different branch

9faaa28

closes #90

fix: uncaught exception when creating notebooks

77a7790

feat: implement new notebook API

1f53431

feat: can force delete pods (#205)

90d3689

refactor: add more logs (#207)

04897a1

restore telepresence script (#209)

05adf37

fix: update telepresence script and add vs code debugger

feat: use project name in the notebook URL (#211)

6c847b5

feat: handle k8s api errors when querying for logs (#216)

618feed

* feat: handle k8s api errors when querying for logs (#216)

feat: update server_logs api (#225)

a2ad149

feat: return json data from server_logs and expose lines limit fix #224

chore: update werkzeug behavior (#242)

9ab32e1

feat: add requested resources info to servers endpoint (#261)

cb246e1

fix #223

fix: use registry API to find image

5bbbb74

closes #273

fix: use lowercase image repository names

4ff2832

closes #287

feat: use user credentials for pulling images (#327)

22223e7

fix(app): escape username in registry secret name

959cf0e

chore: add sentry environment (#434)

4088eb4

feat: automatically remove user registry secrets (#435)

a5a40e7

Add cronjob that periodically deletes users image pull secrets along with unit tests

bug: fix cull secrets failures with pods forked repos

9564772

feat: specify image in server request

bc9435d

feat(app): move gitlab oauth token to separate proxy container

bb256a1

bug: launching sessions that are part of a subgroup fails (#493)

27c1762

* fix: cannot create user env due to invalid k8s secret labels

fix: properly recognize pod status when terminating (#497)

5ff4a87

* fix: properly determine user server pod status with multiple containers Co-authored-by: Andreas Bleuler <[email protected]>

fix: remove unneccessary log error calls (#512)

b7745c0

fix: pod resources schema (#513)

69a2b71

* fix: add missing fields in renku-notebooks schemas * chore: update tests * fix: simplify definition of UserPodResources schema

olevski added 4 commits July 10, 2024 20:01

chore: unindent code to help with diffs

3b3b5f4

chore: change for better diff

921f94c

chore: other change for better diff

964bb3b

chore: more changes for better diff

700a0d9

olevski requested a review from a team as a code owner July 11, 2024 12:26

olevski added 5 commits July 15, 2024 15:02

Merge branch 'main' into move-notebook-service

740fd43

chore: resolve conflicts in pyproject/poetry.lock

2f7ee22

chore: use sanic.log.logger for logging

54ffa80

squashme: address comments

3c912ae

chore: add back conditional import of UserServer to avoid circular im…

c60d183

…ports

olevski mentioned this pull request Jul 15, 2024

Followup work from moving renku notebooks #303

Open

10 tasks

olevski force-pushed the move-notebook-service branch from 4c1247f to c60d183 Compare July 15, 2024 14:27

olevski added 2 commits July 15, 2024 16:58

chore: revert (manually) indentation changes for diffs

0d88271

chore: fix style checks

129d8b3

olevski force-pushed the move-notebook-service branch from 9ed7b22 to 129d8b3 Compare July 15, 2024 14:58

olevski temporarily deployed to renku-ci-ds-301 July 15, 2024 14:59 — with GitHub Actions Inactive

chore: fix apispec schema conflicts

13e7ba3

olevski temporarily deployed to renku-ci-ds-301 July 15, 2024 15:51 — with GitHub Actions Inactive

chore: ignore apispec with mypy for notebooks and exclude notebooks f…

8a7718e

…rom coverage

olevski deployed to renku-ci-ds-301 July 15, 2024 16:20 — with GitHub Actions View deployment

Panaetius approved these changes Jul 16, 2024

View reviewed changes

olevski merged commit 6f3b3c0 into main Jul 16, 2024
15 checks passed

olevski deleted the move-notebook-service branch July 16, 2024 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: move notebook service code #301

feat: move notebook service code #301

olevski commented Jul 11, 2024 •

edited

Loading

Panaetius commented Jul 12, 2024

olevski commented Jul 15, 2024

olevski commented Jul 15, 2024

RenkuBot commented Jul 15, 2024

coveralls commented Jul 15, 2024

Panaetius commented Jul 16, 2024

RenkuBot commented Jul 16, 2024

feat: move notebook service code #301

feat: move notebook service code #301

Conversation

olevski commented Jul 11, 2024 • edited Loading

Panaetius commented Jul 12, 2024

olevski commented Jul 15, 2024

olevski commented Jul 15, 2024

RenkuBot commented Jul 15, 2024

coveralls commented Jul 15, 2024

Pull Request Test Coverage Report for Build 9942973186

Details

💛 - Coveralls

Panaetius commented Jul 16, 2024

RenkuBot commented Jul 16, 2024

olevski commented Jul 11, 2024 •

edited

Loading