Use official Kubernetes python client #41

danielfrg · 2017-04-14T20:50:35Z

This PR changes the objects.pyto use the official python kubernetes client. Hopefully this makes k8s specs creation a little easier by adding a pure python dependency.

This also solves #6 by adding a simple nodeSelector to the Pod spec.

I also tried to solve #1 but using /user/{userid} as a path for liveliness return a 404 because of Hub redirects or something (not 100% sure) any other ideas on how to check status of the Notebook server?

…tions

yuvipanda · 2017-04-14T20:59:11Z

Thank you for working on this! This looks great, am reviewing it now.

yuvipanda

It looks ok to me, but I'd like the following changes:

Make using kubernetes client be a separate commit from adding more features. Ideally the new features would be a different PR, but separate commits are also ok. This makes it easier to merge, review and test.
Fix the tests.

This is great work - thanks for taking it on!

yuvipanda · 2017-04-14T21:00:13Z

kubespawner/objects.py

+    pod.metadata = V1ObjectMeta()
+    pod.metadata.name = name
+    pod.metadata.labels = {}
+    pod.metadata.labels.update({"name": name})


why not just have this be one line?

pod.metadata.labels = labels.copy() pod.metadata.labels['name'] = name

seems clearer and possibly faster?

Yes, thats better. Done

yuvipanda · 2017-04-14T21:00:15Z

kubespawner/objects.py

@@ -11,13 +33,15 @@ def make_pod_spec(
    run_as_uid,
    fs_gid,
    env,
+    node_selector,


Can you split out the additional features into separate commits (or even a separate PR) so it's easier to review? I'll leave commnets about them here too tho

I have removed the node_selector things I will make another PR for that.

yuvipanda · 2017-04-14T21:03:43Z

kubespawner/objects.py

+    notebook_container.resources = V1ResourceRequirements()
+    notebook_container.resources.requests = {"cpu": cpu_guarantee, "memory": mem_guarantee}
+    notebook_container.resources.limits = {"cpu": cpu_limit, "memory": mem_limit}
+    # notebook_container.liveness_probe = V1Probe()


Let's get the commented code out? Also would be nice if it were a separate commit :)

Yes, i wanted to have it there temporally in case you wanted it on this PR. It's gone.

yuvipanda · 2017-04-14T21:05:02Z

kubespawner/spawner.py

+    )
+
+    working_dir = Unicode(
+        "/home",


I think the default should be None, which should make it use whatever is specified in the Docker image (which is the current default). Should probably be called singleuser_working_dir as well (I've been trying to do that for all new properties)

Ah yes, I agree that its a better default. Also renamed the property to singleuser_working_dir.

yuvipanda · 2017-04-14T21:05:48Z

kubespawner/spawner.py

@@ -109,6 +108,26 @@ def _namespace_default(self):
        """
    ).tag(config=True)

+
+    node_selector = Dict(


Can this be a separate commit / PR? We need to be careful to support both simple property based selectors and more complex ones...

danielfrg · 2017-04-14T21:59:53Z

I have fixed the tests and removed the node_selector logic and I will make a separate PR for that Issue.

You can probably Squash and Merge this one if you want to keep history clean about changes.

yuvipanda · 2017-04-15T00:49:13Z

kubespawner/spawner.py

@@ -109,6 +108,13 @@ def _namespace_default(self):
        """
    ).tag(config=True)

+    singleuser_working_dir = Unicode(
+        help="""


set the default explicitly to None and also allow_none?

yuvipanda · 2017-04-15T05:08:39Z

(I'll take a more detailed look + test on Monday!)

yuvipanda

More minor changes that I missed on first round - sorry about that! Otherwise it looks good, and I want us to test this on a couple of different k8s clusters before merging. What cluster have you been testing this against? GKE or minikube?

yuvipanda · 2017-04-17T20:45:45Z

kubespawner/objects.py

+    notebook_container.ports.append(port_)
+    notebook_container.env = []
+    for key, value in env.items():
+        env_ = V1EnvVar()


This could just be:

notebook_container.env = [V1EnvVar(k, v) for k, v in env.items()]

no? https://github.com/kubernetes-incubator/client-python/blob/84bf6cc9dc206cb3314a5b4d2fa1a9c2fa2c5137/kubernetes/client/models/v1_env_var.py#L24 says it takes the properties as params to init

Yes, thats good. Done

yuvipanda · 2017-04-17T20:46:51Z

kubespawner/spawner.py

@@ -433,6 +442,15 @@ def _hub_connect_port_default(self):
        """
    )

+    user_storage_match_labels = Dict(
+        help="""


What's the default for this? I'm trying to understand what exactly this feature does - does it allow binding to an existing PVC that isn't created by this? Under what circumstances would I use this?

So by default its nothing so the default behavior should be the same.

No, it still creates a PVC spec as it was doing before. This new property basically it just passes the matchLabels field from the PVC spec so its possible to filter between existing PV. Lets say you have multiple PV you can select which one you want.

From: https://kubernetes.io/docs/concepts/storage/persistent-volumes/ - PersistentVolumeClaims section.

Ah, so is it for the case where the PV is not dynamically created by using a StorageClass?

ok! I think we should set the default explicitly, and explain why you should use this and when in the help. The current help text isn't sufficient, IMO.

Also in the future, it'd be nice to have these as a separate PR, rather than as part of one :) Makes it easier to review.

I'll do some testing and try to get this merged tomorrow!

The more I think about it, the more I think we should take it out now. I think way to include this would be:

Write a 'LabelSelector' trait type, that can be used to express both matchLabels and matchExpressions

Use that for this and for nodeSelectors

I think that's the cleanest and most future compatible way of doing this. what do you think?

It should probably also support expanding {username} and friends in the label values, to provide more targeted matching. Let's definitely get it out of this PR?

Ok, its gone now. Yes, it was for that case where you already had existing PVs. This is the way I have always used PV + PVC, more manual than the automatic creation.

yuvipanda · 2017-04-17T20:48:33Z

kubespawner/spawner.py

-        # This makes sure that we don't accidentally give access to the whole
-        # kubernetes API to the users in the spawned pods.
-        # See https://github.com/kubernetes/kubernetes/issues/16779#issuecomment-157460294
-        hack_volumes = [{


I think this belongs here, and not in the make_pod_spec. We could add an option to allow service account access in here when it's here, and this also keeps the make_pod_spec agnostic about what we do. Someone has actually forked this to make this disabling optional, and that's easier to do when this is here.

Yes I agree with keeping make_pod_spec independent of the hack i just wanted to keep the kubernetes_client code in one place. I put it back now.

yuvipanda · 2017-04-17T20:48:59Z

kubespawner/spawner.py

+            cmd=real_cmd,
+            run_as_uid=singleuser_uid,
+            fs_gid=singleuser_fs_gid,
+            env=self.get_env(),


Passing named args is much cleaner, thank you for moving it!

yuvipanda · 2017-04-17T21:17:25Z

kubespawner/objects.py

+
+    pod.spec.containers = []
+
+    notebook_container = V1Container()


We could also construct notebook_container this way (passing params to init) since that supports it too (https://github.com/kubernetes-incubator/client-python/blob/84bf6cc9dc206cb3314a5b4d2fa1a9c2fa2c5137/kubernetes/client/models/v1_container.py). I'm not super attached to it here, though - because there's a lot more params.

Yes I agree, personally I don't like to pass 3+ params to a function.

danielfrg · 2017-04-17T22:30:48Z

This changes I have only tested on minikube but I still yet to see something in k8s that doesn't work from minukube to GCP.

yuvipanda · 2017-04-18T05:15:30Z

kubespawner/spawner.py

@@ -433,6 +442,15 @@ def _hub_connect_port_default(self):
        """
    )

+    user_storage_match_labels = Dict(
+        help="""


The more I think about it, the more I think we should take it out now. I think way to include this would be:

Write a 'LabelSelector' trait type, that can be used to express both matchLabels and matchExpressions

Use that for this and for nodeSelectors

I think that's the cleanest and most future compatible way of doing this. what do you think?

yuvipanda

Tested locally, is ok! \o/

yuvipanda · 2017-04-21T19:55:50Z

Thank you for the patch!

yuvipanda · 2017-04-21T19:56:15Z

I'm going to test this now with https://github.com/jupyterhub/helm-chart

yuvipanda · 2017-04-21T20:36:33Z

I filed #43 with further options on how to move forward getting rid of our custom code in favor of the official client.

danielfrg added 2 commits April 14, 2017 15:35

Fix URL link

57668f8

Use kubernetes python client and add working_dir and node_selector op…

e388ae8

…tions

yuvipanda requested changes Apr 14, 2017

View reviewed changes

danielfrg added 4 commits April 14, 2017 16:39

Fix tests and remove node_selector

07cdbd3

Rename working_dir to singleuser_working_dir

cbe4cc6

Pin kubernetes version

bf513b9

Pin kubernetes version

56b8bd1

Fix singleuser_working_dir default

e97f7fd

yuvipanda reviewed Apr 15, 2017

View reviewed changes

allow_none for singleuser_working_dir

37dfe93

yuvipanda requested changes Apr 17, 2017

View reviewed changes

yuvipanda reviewed Apr 17, 2017

View reviewed changes

List comprenhesion for env variables

7cabf1e

danielfrg added 2 commits April 17, 2017 18:12

Move hack volume back to spawner.py

c76b5f5

Tests dont use hack volumes

931f45c

yuvipanda requested changes Apr 18, 2017

View reviewed changes

Remove matchLabels logic

fadddbe

yuvipanda approved these changes Apr 21, 2017

View reviewed changes

yuvipanda merged commit 228799a into jupyterhub:master Apr 21, 2017

danielfrg deleted the kube-client branch April 21, 2017 22:04

danielfrg mentioned this pull request Apr 24, 2017

Add option to specify nodeSelector of the Pods #44

Merged

Use official Kubernetes python client #41

Use official Kubernetes python client #41

Conversation

danielfrg commented Apr 14, 2017 • edited Loading

yuvipanda commented Apr 14, 2017

yuvipanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielfrg commented Apr 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvipanda commented Apr 15, 2017

yuvipanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvipanda Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielfrg commented Apr 17, 2017

yuvipanda Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

yuvipanda left a comment

Choose a reason for hiding this comment

yuvipanda commented Apr 21, 2017

yuvipanda commented Apr 21, 2017

yuvipanda commented Apr 21, 2017

danielfrg commented Apr 14, 2017 •

edited

Loading

yuvipanda Apr 18, 2017 •

edited

Loading

yuvipanda Apr 18, 2017 •

edited

Loading