Scope tf-operator to a namespace #789

ankushagarwal · 2018-08-21T04:50:42Z

Based on the KUBEFLOW_NAMESPACE, tf-operator will be scoped to a
particular namespace.

If --namespace flag is not set or left as empty, tf-operator will be
scoped to cluster-wide. If it is set to a namespace value, tf-operator
will only operate in the namespace

Fixes #759

/cc @gaocegege @jlewi

This change is

Based on the KUBEFLOW_NAMESPACE, tf-operator will be scoped to a particular namespace. If KUBEFLOW_NAMESPACE is not set or left as empty, tf-operator will be scoped to cluster-wide. If it is set to a namespace value, tf-operator will only operate in the namespace

ankushagarwal · 2018-08-21T04:54:16Z

Once we merge this, we can create a new image and then I will update the tfjob prototype to parametrize this.

ankushagarwal · 2018-08-21T05:10:34Z

/retest

gaocegege · 2018-08-21T06:14:07Z

/lgtm

gaocegege · 2018-08-21T06:14:20Z

/retest

jlewi · 2018-08-21T12:23:15Z

Why an environment variable and not a command line option?

A command line option seems more discoverable to me; e.g. run the binary with --help.

And in the intended use case we aren't really inheriting namespace from the environment. I think of the namespace as something we will explicitly set as opposed to something that will be inherited e.g. the NAMESPACE of the pod.

I don't think we want the namespace the operator is handling to be the same as the namespace the operator is running in. This should allow further separation between "core kubeflow" and user space.

ankushagarwal · 2018-08-21T20:14:00Z

Done.

gaocegege · 2018-08-22T02:46:55Z

/retest

gaocegege · 2018-08-22T02:47:17Z

/lgtm

ScorpioCPH

Generally LGTM.

ScorpioCPH · 2018-08-22T03:19:08Z

cmd/tf-operator.v2/app/server.go

@@ -65,6 +65,12 @@ func Run(opt *options.ServerOption) error {
 		log.Infof("EnvKubeflowNamespace not set, use default namespace")
 		namespace = metav1.NamespaceDefault
 	}
+	if len(opt.Namespace) == 0 {
+		opt.Namespace = v1.NamespaceAll


How about set the default value in fs.StringVar()?

ScorpioCPH · 2018-08-22T03:19:47Z

cmd/tf-operator.v2/app/options/options.go

@@ -40,6 +41,10 @@ func (s *ServerOption) AddFlags(fs *flag.FlagSet) {
 		`The url of the Kubernetes API server,
 		 will overrides any value in kubeconfig, only required if out-of-cluster.`)

+	fs.StringVar(&s.Namespace, "namespace", "",


We can set default value as v1.NamespaceAll here.

jlewi · 2018-08-22T12:26:16Z

nit: please update the PR description to reflect the fact its an argument now not an environment variable.

ankushagarwal · 2018-08-22T15:50:34Z

Addressed comments. PTAL.

jlewi · 2018-08-22T17:20:24Z

Looks like some transient SSL error failure

Traceback (most recent call last):
 File "/usr/lib/python2.7/runpy.py", line 174, in _run_module_as_main
   "__main__", fname, loader, pkg_name)
 File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
   exec code in run_globals
 File "/mnt/test-data-volume/kubeflow-tf-operator-presubmit-v1-789-b5ed4cb-957-1c49/src/kubeflow/tf-operator/py/deploy.py", line 360, in <module>
   main()
 File "/mnt/test-data-volume/kubeflow-tf-operator-presubmit-v1-789-b5ed4cb-957-1c49/src/kubeflow/tf-operator/py/deploy.py", line 356, in main
   args.func(args)
 File "/mnt/test-data-volume/kubeflow-tf-operator-presubmit-v1-789-b5ed4cb-957-1c49/src/kubeflow/tf-operator/py/deploy.py", line 139, in setup_cluster
   util.create_cluster(gke, project, zone, cluster_request)
 File "/mnt/test-data-volume/kubeflow-tf-operator-presubmit-v1-789-b5ed4cb-957-1c49/src/kubeflow/testing/py/kubeflow/testing/util.py", line 170, in create
_cluster
   create_op = wait_for_operation(gke, project, zone, response["name"])
 File "/mnt/test-data-volume/kubeflow-tf-operator-presubmit-v1-789-b5ed4cb-957-1c49/src/kubeflow/testing/py/kubeflow/testing/util.py", line 235, in wait_f
or_operation
   projectId=project, zone=zone, operationId=op_id).execute()
 File "/usr/local/lib/python2.7/dist-packages/oauth2client/_helpers.py", line 133, in positional_wrapper
   return wrapped(*args, **kwargs)
 File "/usr/local/lib/python2.7/dist-packages/googleapiclient/http.py", line 839, in execute
   method=str(self.method), body=self.body, headers=self.headers)
 File "/usr/local/lib/python2.7/dist-packages/googleapiclient/http.py", line 179, in _retry_request
   raise exception
ssl.SSLError: ('The read operation timed out',)

/test all

jlewi · 2018-08-22T17:20:54Z

/lgtm
/approve

k8s-ci-robot · 2018-08-22T17:20:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jlewi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from gaocegege and jlewi August 21, 2018 04:50

k8s-ci-robot added the size/S label Aug 21, 2018

k8s-ci-robot assigned gaocegege Aug 21, 2018

k8s-ci-robot added the lgtm label Aug 21, 2018

Update to use a flag instead of environment variable

7f3b213

k8s-ci-robot removed the lgtm label Aug 21, 2018

k8s-ci-robot added the lgtm label Aug 22, 2018

ScorpioCPH approved these changes Aug 22, 2018

View reviewed changes

Address review comment

b5ed4cb

k8s-ci-robot removed the lgtm label Aug 22, 2018

k8s-ci-robot assigned jlewi Aug 22, 2018

k8s-ci-robot added the lgtm label Aug 22, 2018

k8s-ci-robot added the approved label Aug 22, 2018

k8s-ci-robot merged commit b576c25 into kubeflow:master Aug 22, 2018

ankushagarwal mentioned this pull request Aug 22, 2018

Update tf-job-operator prototype to support namespace-scoped deployment kubeflow/kubeflow#1403

Merged

jlewi mentioned this pull request Aug 24, 2018

Restrict privilege of Kubeflow services accounts such as tf-job-operator to namespace level kubeflow/kubeflow#1213

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scope tf-operator to a namespace #789

Scope tf-operator to a namespace #789

ankushagarwal commented Aug 21, 2018 •

edited

Loading

ankushagarwal commented Aug 21, 2018

ankushagarwal commented Aug 21, 2018

gaocegege commented Aug 21, 2018

gaocegege commented Aug 21, 2018

jlewi commented Aug 21, 2018

ankushagarwal commented Aug 21, 2018

gaocegege commented Aug 22, 2018

gaocegege commented Aug 22, 2018

ScorpioCPH left a comment

ScorpioCPH Aug 22, 2018

ankushagarwal Aug 22, 2018

ScorpioCPH Aug 22, 2018

jlewi commented Aug 22, 2018

ankushagarwal commented Aug 22, 2018

jlewi commented Aug 22, 2018

jlewi commented Aug 22, 2018

k8s-ci-robot commented Aug 22, 2018

Scope tf-operator to a namespace #789

Scope tf-operator to a namespace #789

Conversation

ankushagarwal commented Aug 21, 2018 • edited Loading

ankushagarwal commented Aug 21, 2018

ankushagarwal commented Aug 21, 2018

gaocegege commented Aug 21, 2018

gaocegege commented Aug 21, 2018

jlewi commented Aug 21, 2018

ankushagarwal commented Aug 21, 2018

gaocegege commented Aug 22, 2018

gaocegege commented Aug 22, 2018

ScorpioCPH left a comment

Choose a reason for hiding this comment

ScorpioCPH Aug 22, 2018

Choose a reason for hiding this comment

ankushagarwal Aug 22, 2018

Choose a reason for hiding this comment

ScorpioCPH Aug 22, 2018

Choose a reason for hiding this comment

jlewi commented Aug 22, 2018

ankushagarwal commented Aug 22, 2018

jlewi commented Aug 22, 2018

jlewi commented Aug 22, 2018

k8s-ci-robot commented Aug 22, 2018

ankushagarwal commented Aug 21, 2018 •

edited

Loading