Skip to content
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(chart): add session tolerations, affinity, nodeSelector #806

Merged
merged 3 commits into from
Nov 15, 2021

Conversation

olevski
Copy link
Member

@olevski olevski commented Nov 15, 2021

This adds the ability to specify tolerations, affinity, etc specifically for the user sessions.

I tested with specifying:

  • node selector and tolerations
  • affinity and tolerations

Note that specifying an affinity for one set of nodes and then node selector for another resulted in an unschedulable pod.

Deployed at tasko.dev.renku.ch and set up with affinity and tolerations for the user dedicated nodes.

As implemented the tolerations, affinities and nodeSelector fields that are by default currently empty as they come from Amalthea are fully replaced - not merged. So if at some point Amalthea assigned any of these fields for a user sessions they would be replaced in the notebook service. I think this is a safe assumption though - we really try to avoid getting in the way with Amalthea and letting the users do their thing. So I don't believe we would ever go for imposing any of these things on the session level in Amalthea.

Lastly by default I left the values file blank. I think it is easiest that way - we do not know for sure what all renku deployments have setup. This means that all existing deployments should update this field in their values file accordingly and as required.

closes #799 #800

@aledegano
Copy link
Contributor

maybe I would consider including in the variables names TAINTS_TOLERATIONS and NODE_AFFINITY, it's a bit more verbose but clears any doubt on what it might be referring to.

Copy link
Contributor

@ableuler ableuler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@pameladelgado
Copy link
Contributor

Lastly by default I left the values file blank.

I see that the code is taking an empty value as default, I guess those would be the default values, right?

@pameladelgado
Copy link
Contributor

we could include some commented lines about how to configure notebooks to enable session-dedicated nodes, in the values file here and in renku's values file.

@ableuler
Copy link
Contributor

maybe I would consider including in the variables names TAINTS_TOLERATIONS and NODE_AFFINITY, it's a bit more verbose but clears any doubt on what it might be referring to.

@aledegano here I would actually prefer to stay with the naming of the fields in the pod spec that we're effectively setting. Especially for the affinity there is room for confusion since nodeAffinity is a sub-field of affinity. What we're setting here is the affinity and in principle there's nothing stopping a user from defining eg an anti-affinity to another pod. Therefore I think adding the option to modify the entire affinity block and not just a nodeAffinity is the right way to go.

@olevski olevski merged commit 49a2d54 into master Nov 15, 2021
@olevski olevski deleted the add-session-tolerations-affinity branch November 15, 2021 13:34
@olevski
Copy link
Member Author

olevski commented Nov 15, 2021

Lastly by default I left the values file blank.

I see that the code is taking an empty value as default, I guess those would be the default values, right?

@pameladelgado by default the values file sets empty lists of dictionaries. I said blank in the description but that is what I meant.

@olevski
Copy link
Member Author

olevski commented Nov 15, 2021

we could include some commented lines about how to configure notebooks to enable session-dedicated nodes, in the values file here and in renku's values file.

I am sorry I rushed the merge. I will add a bit more comments in the values file in the PR for the new renku notebooks release.

@olevski olevski restored the add-session-tolerations-affinity branch November 30, 2022 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tolerations and node affinity for sessions should be configurable
4 participants