-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(app): use crc service #1483
Conversation
999ea22
to
396d5c5
Compare
396d5c5
to
fc534a5
Compare
fc534a5
to
40a568c
Compare
0560372
to
9fb8044
Compare
9fb8044
to
b29012e
Compare
101928b
to
894420a
Compare
00719ca
to
6f99453
Compare
You can access the deployment of this PR at https://renku-ci-nb-1483.dev.renku.ch |
6f99453
to
6f399d3
Compare
6f399d3
to
a13623b
Compare
9887ed8
to
9277024
Compare
6766199
to
65bbfc7
Compare
4da21c8
to
3808ad8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you! nice to see this taking shape 🙂
renku_notebooks/api/classes/crc.py
Outdated
# Memory and disk space in CRC are assumed to be in gigabytes whereas | ||
# the notebook service assumes that if a plain number is used then it is bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused here. storage
comes directly from LaunchNotebookRequestWithoutS3
/launch_notebook()
, so isn't it in bytes? but above, we compare it directly to max_storage
, which should be in gigabytes? Shouldn't there be a conversion somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the comment is not right. I updated the schema for the request in launch_notebook to be in gigabytes. But I did not update the comment. Now I added the assumption that disk and memory are in gigabytes in the docstring.
renku_notebooks/api/classes/crc.py
Outdated
headers = None | ||
if user.access_token is not None: | ||
headers = {"Authorization": f"bearer {user.access_token}"} | ||
res = requests.get(self.crc_url + "/resource_pools", headers=headers) | ||
if res.status_code != 200: | ||
raise IntermittentError( | ||
message="The compute resource access control service sent " | ||
"an unexpected response, please try again later", | ||
) | ||
resource_pools = res.json() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is duplicated three times (here and above), maybe it should be its own method?
) | ||
else: | ||
resources["limits"]["cpu"] = cpu_request | ||
resources["limits"]["cpu"] = 3 * cpu_request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the 3
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to allow the cpu to spike up momentarily. Because if we set the limit on the cpu it gets throttled and then when you create a new session it takes a really long time for the session to start up. If the limits are set to "lax" (i.e. "relaxed") then the limit is larger than the request. But if the limits are set to strict then the limit is the same as the request and the cpu is throttled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. you should give it a descriptive name, random magic numbers in code aren't nice 🙂
@Panaetius I addressed all the comments. The integration tests are failing because it seems that the oauth application used by the tests has been removed from gitlab. I will fix this later. I ran the integration tests locally and they all passed. |
cd698a3
to
b84aa32
Compare
Uses the compute resource access control service for launching sessions.