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

[REVIEW] Add tolerations and node/pod affinities, add field verifications, add some default values #205

Closed
wants to merge 37 commits into from

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Jul 4, 2018

Review of ... would be great!

Your time is valuable, so I tried to summarize some things that could be considered during a review. I'd love to have this merged and verified to work properly before JupyterCon 2018 and the workshop we will have.

  • Logging... I want to log a warning, how should I do this? I doubt my implementation very suitable. In-page search for logger in the Files changed section of the PR and you will see what I refer to. From spawner.py the opjects.py function make_pod(... logger=self.log ...) is called which in turns passes the logger onwards to a function defined in utils.py with the signature update_k8s_model(target, source, logger=None, origin=None): where origin is giving some details relevant to log.
  • Version bump... I bumped the requirement to JupyterHub to >= 0.9 instead of => 0.8 and declared in setup.py that KubeSpawner to have version 0.9.1 instead of 0.8.1 when published as a package. Is this okay?
  • Independent test... It would be great to verify that kubespawner works by overriding the previous kubespawner version, it can be done in a hub's Dockerfile like this:
    RUN pip3 install --upgrade https://github.com/consideratio/kubespawner/archive/tolerations-pr.zip
    
  • Documentation... Is it clear how to use the fields and what the do from the help="""... documentation in the traitlets found in spawner.py? Should I improve some methods docstring?
  • Tests... Come to think of anything else that would be testable within the scope of this PR? I've added the following tests
    • Testing that some utils does what they should
    • Testing that all configurable fields render good pod specs individually
    • Testing a basic initialization of a KubeSpawner object and calling spawner.get_pod_manifest() once also rather than just directly calling the standalone make_pod() function defined in objects.py

Thank you for your time, I appreciate it a lot! ❤️

PR Status

PR Summary

Added configuration

Added support for various configuration fields related to pod scheduling. As @gweis pointed out, tolerations and similar fields could previously be set using extra_pod_config. This method has some limitations though and should be considered an escape hatch until we have dedicated options available, and this is why this PR is useful. With dedicated fields will be easier for the user, so much that this was essential to make the fields configurable in the z2jh repo. With dedicated fields like this we can also make the user fail faster on invalid configurations thanks to better verification. We can also extend fields like node_affinity_required easier than extra_pod_config.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms.

The added traitlets

  • scheduler_name
  • tolerations
  • node_affinity_preferred
  • node_affinity_required
  • pod_affinity_preferred
  • pod_affinity_required
  • pod_anti_affinity_preferred
  • pod_anti_affinity_required
  • priority_class_name

Verification

Since the k8s resources being set has an schema the provided values can be verified. Such verification allows us to fail fast, and is in practice done by using kubernetes.client.models and objects like V1Toleration within it. If these are initialized with faulty fields, we fail fast.

    # exert from code where verification occurs...
    # get_k8s_model is attempting to create a V1Toleration object by using the obj dictionary, but will also
    # allow for the toleration object called obj to be a V1Toleration object already.
    if tolerations:
        pod.spec.tolerations = [get_k8s_model(V1Toleration, obj) for obj in tolerations]

Feedback from extra_pod_config merge

KubeSpawner now logs a warning if extra_pod_config overrides a value set in another way.

@consideRatio consideRatio force-pushed the tolerations-pr branch 2 times, most recently from 1a31974 to 8a1c134 Compare July 4, 2018 11:34
@consideRatio consideRatio changed the title Add tolerations to the pod object Support tolerations and node/pod affinities Jul 4, 2018
@consideRatio consideRatio changed the title Support tolerations and node/pod affinities [WIP] Support tolerations and node/pod affinities Jul 4, 2018
@consideRatio consideRatio force-pushed the tolerations-pr branch 3 times, most recently from 7e9d33c to 4b736ea Compare July 5, 2018 08:01
@consideRatio consideRatio changed the title [WIP] Support tolerations and node/pod affinities Support tolerations and node/pod affinities Jul 5, 2018
@consideRatio consideRatio changed the title Support tolerations and node/pod affinities [WIP] Support tolerations and node/pod affinities Jul 5, 2018
@consideRatio consideRatio force-pushed the tolerations-pr branch 4 times, most recently from f6686f6 to daddbcc Compare July 5, 2018 11:26
@consideRatio consideRatio changed the title [WIP] Support tolerations and node/pod affinities Support tolerations and node/pod affinities Jul 5, 2018
@consideRatio consideRatio force-pushed the tolerations-pr branch 6 times, most recently from 4af12f7 to ea73626 Compare July 5, 2018 16:33
@consideRatio consideRatio changed the title Support tolerations and node/pod affinities Add tolerations and node/pod affinities, add field verifications, add some default values Jul 5, 2018
@consideRatio consideRatio force-pushed the tolerations-pr branch 4 times, most recently from 4f8c742 to 8133cd8 Compare July 6, 2018 17:55
@consideRatio consideRatio changed the title Add tolerations and node/pod affinities, add field verifications, add some default values [WIP] Add tolerations and node/pod affinities, add field verifications, add some default values Jul 7, 2018
consideRatio added a commit to consideRatio/zero-to-jupyterhub-k8s that referenced this pull request Jul 10, 2018
consideRatio added a commit to consideRatio/zero-to-jupyterhub-k8s that referenced this pull request Jul 10, 2018
@consideRatio consideRatio force-pushed the tolerations-pr branch 2 times, most recently from 2a81d01 to c1c39dd Compare July 10, 2018 15:39
consideRatio added a commit to consideRatio/zero-to-jupyterhub-k8s that referenced this pull request Aug 2, 2018
@consideRatio consideRatio changed the title Add tolerations and node/pod affinities, add field verifications, add some default values [WIP] Add tolerations and node/pod affinities, add field verifications, add some default values Aug 2, 2018
@consideRatio consideRatio changed the title [WIP] Add tolerations and node/pod affinities, add field verifications, add some default values [REVIEW] Add tolerations and node/pod affinities, add field verifications, add some default values Aug 2, 2018
@consideRatio
Copy link
Member Author

Pinging @gsemet @clkao @gweis as you have made recent commits, if you would like to test / review this PR I'd be very happy ❤️

consideRatio added a commit to consideRatio/zero-to-jupyterhub-k8s that referenced this pull request Aug 2, 2018
betatim added a commit that referenced this pull request Aug 9, 2018
[Part 2 of #205] Added docs on how to run tests
minrk added a commit that referenced this pull request Aug 10, 2018
[Part 1 of #205] Refreshed development instructions
consideRatio added a commit that referenced this pull request Aug 10, 2018
…ation

[Part 3 of #205] Added utils for k8s model verification
consideRatio added a commit to consideRatio/zero-to-jupyterhub-k8s that referenced this pull request Aug 12, 2018
@consideRatio
Copy link
Member Author

Closing in favor of
#230 #231 #232 #236 #237 #238 #239

consideRatio added a commit to consideRatio/zero-to-jupyterhub-k8s that referenced this pull request Aug 14, 2018
minrk added a commit that referenced this pull request Aug 17, 2018
[Part 4 of #205] Bugfixes, input verification and testing - Review gamified!
minrk added a commit that referenced this pull request Aug 17, 2018
[Part 5 of #205] Tolerations for node taints
minrk added a commit that referenced this pull request Aug 18, 2018
minrk added a commit that referenced this pull request Aug 18, 2018
[Part 7 of #205] Affinities (node, pod, anti_pod - required and preferred)
consideRatio added a commit to consideRatio/zero-to-jupyterhub-k8s that referenced this pull request Aug 19, 2018
consideRatio added a commit to consideRatio/zero-to-jupyterhub-k8s that referenced this pull request Aug 19, 2018
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.

3 participants