-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Changing Jupyter service to ClusterIP by default. #139
Conversation
Hi @elsonrodriguez. Thanks for your PR. I'm waiting for a kubernetes or google member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
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.
LGTM
LGTM but the code needs to be sync'd. |
217a28a
to
abde4aa
Compare
Rebased |
@@ -186,7 +186,7 @@ c.KubeSpawner.pvc_name_template = 'claim-{username}{servername}' | |||
} | |||
}, | |||
|
|||
jupyterHubLoadBalancer: { | |||
jupyterHubLoadBalancer(serviceType): { |
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.
Small point - maybe we could call this something like jupyterHubServiceDefinition
or something like that instead?
It's a bit confusing if it's called a load balancer but it's not necessarily doing any non-trivial load balancing.
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.
That's a fair point. In the ClusterIP configuration jupyterHubLoadBalancer
is almost a duplicate of the existing jupyterHubService
. I don't know the reasoning behind having two services for Jupyter, but if someone could give me some pointers i could just put a flag into ksonnet whether or not to deploy jupyterHubLoadBalancer
in the first place.
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.
See #99 for why two services.
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
I went ahead and merged this since the PR as is fixed the issue; feel free to send another PR if think changing the names is worth cleaning up. |
@elsonrodriguez: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
commit f10c384 Author: Jeremy Lewi <[email protected]> Date: Thu Jan 25 17:11:18 2018 -0800 Need to set authenticator param. commit 705067f Author: Jeremy Lewi <[email protected]> Date: Thu Jan 25 15:53:08 2018 -0800 Fix some bugs and start a user guide. commit cbf67a3 Merge: c60c245 fed20a1 Author: Jeremy Lewi <[email protected]> Date: Thu Jan 25 14:31:46 2018 -0800 Sync to head. commit c60c245 Author: Jeremy Lewi <[email protected]> Date: Thu Jan 25 14:20:47 2018 -0800 * Use Envoy as a reverse proxy and for JWT verification. * Don't run Envoy as a side car in the JupyterHub pod. This is cleaner and allows us to have a single reverse proxy for all services. commit fed20a1 Author: Elson Rodriguez <[email protected]> Date: Thu Jan 25 06:28:22 2018 -0800 Changing Jupyter service to ClusterIP by default. (kubeflow#139) * Changing default Service type for Jupyterhub to Cluster IP * Exposing services publicly is a security risk so we want to avoid recommending that since people may not understand the implications * Updated documentation to reflect ClusterIP change for Jupyter. commit 8aaa393 Author: lluunn <[email protected]> Date: Thu Jan 25 06:21:55 2018 -0800 fix guide typo (kubeflow#144) commit 7bb642c Author: Robert Wilkins III <[email protected]> Date: Tue Jan 23 23:05:10 2018 -0600 Update README.md (kubeflow#142) Revised grammer and punctuation changes to the document as detailed below: Added (ML) after "Machine Learning" on line 3 to avoid ambiguation over the "ML" reference on line 18. Changed "best of breed" to "best-of-breed" on line 3 to match language used in Kubernetes/Kubernetes. Removed the comma after GPUs on line 6 to avoid treating a regular sentence as a run-on sentence. Changed "it is" to "it's" on line 23. There's no need to get formal here if the rest of the document has a light conversational feel. Removed an unnecessary comma from Line 23 after (within reason). Added dashes in "easy to use" on line 25 to match formatting from line 3. Added a colon after "using Kubeflow if" on line 30 to match the format previously established in the previous section called "The Kubeflow Mission". Capitalized Kubeflow on line 34 to match casing from line 30. Changed the sentence structure and added a period for line 36 to make more sense rather than a strange run-on sentence. Removed unnecessary newline between lines 36 and 37 to match format established in line 34. Added a period after (see below) on line 39. Added a comma after "GPUs" to avoid a run-on sentence on line 47. Changed the ending comma on line 58 to a colon to follow the established format used previously in this document. Added a comma after Kubeflow on line 83 and added a period to finish the sentence. Moved the comma after "using" to after "GKE" in line 109for the sentence to make sense. Added a dash for "in depth" on line to follow the document's established format. commit 80cb162 Author: Neeraj Kashyap <[email protected]> Date: Mon Jan 22 05:31:32 2018 -0800 Client script for inception model server (kubeflow#92) Added a script that allows users to run the hosted inception model on images on their local filesystems or on Google Cloud Storage. This is, with only very slight modifications for readability, the same as the client provided by TensorFlow Serving - https://github.com/tensorflow/serving/blob/master/tensorflow_serving/example/inception_client.py As such, I am completely okay with us just linking to their script. My initial intention was to make this as a notebook, but the problem is that the tensorflow-serving-api Python package is only available for Python 2 and the kubeflow-core environment only offers a Python 3 backend for the Jupyter notebook. This is therefore a stopgap until I can introduce an appropriate image in place of the one used by kubeflow-core. * Changed model serving service type to ClusterIP from LoadBalancer * Added instructions for exposing service IP commit bde2ddc Author: Putra Manggala <[email protected]> Date: Fri Jan 19 17:34:59 2018 -0500 Fix datascientists typo (kubeflow#137) commit 698cc67 Author: Jeremy Lewi <[email protected]> Date: Thu Jan 18 20:45:49 2018 -0800 Make Argo UI available publicly at testing-argo.kubeflow.ui (kubeflow#132) * We use Argo to run our E2E tests so the UI is very useful for debugging tests. * Add an ingress with a static IP to expose it publicly. * Fix kubeflow#131 commit 55c220d Merge: 11d989c ca95a0d Author: Jeremy Lewi <[email protected]> Date: Wed Jan 17 21:24:58 2018 -0800 Merge remote-tracking branch 'github/iap' into iap commit 11d989c Merge: 8e6fb87 4c9217d Author: Jeremy Lewi <[email protected]> Date: Wed Jan 17 21:24:34 2018 -0800 Resolve conflicts. commit 4c9217d Author: Jeremy Lewi <[email protected]> Date: Wed Jan 17 20:50:24 2018 -0800 Fix TfJob operator roles and TfCNN prototype (kubeflow#130) * Fix the TFCNN prototype; the termination policy wasn't being properly set * Create service accounts and role bindings for the TfJob operator and UI * Fix kubeflow#129 TfCnn template doesn't set termination policy correctly * Fix kubeflow#125 Missing roles for tf-job operator * Fix kubeflow#95; presubmits/postsubmits need to use the code at the commit we checked out *We do this by replacing the directory in vendor with a symbolic link to where we checked out the source. * It looks like using "--as" with ksonnet leads to strange errors about the server not being able to create the config map * If we don't use "--as" need to fetch credentials a second time or else we get RBAC issues creating the cluster
run on presubmit instead of postsubmit adds workflow for kfctl go test adds util.libsonnet adds workflow libsonnet from kubeflow/kubeflow adds kfctl go test to params.libsonnet minor fix git fetch kubeflow/kubeflow set version to master for workflow
No description provided.