katib ui: adapt environment in which cluster role is unavailable #1141

sperlingxx · 2020-04-13T11:16:54Z

Katib UI fails to get experiment list when cluster role is unavailable. This problem is fixed in this PR through additional environment variable representing available namespaces.

kubeflow-bot · 2020-04-13T11:16:59Z

This change is

sperlingxx · 2020-04-13T11:20:30Z

@andreyvelich @johnugeorge Could you help to review this PR?

c-bata · 2020-04-13T11:50:44Z

pkg/ui/v1alpha3/const.go

+	availableNameSpaces, hasClusterRole = func() ([]string, bool) {
+		ns := env.GetEnvOrDefault(AvailableNameSpaceEnvName, ClusterRoleKey)
+		if ns == ClusterRoleKey {
+			return []string{""}, true


I'm not confident but do you mistake it for []string{}?

And I think you can use init() function like:

var ( availableNameSpaces []string hasClusterRole bool ) func init() { ns := env.GetEnvOrDefault(AvailableNameSpaceEnvName, ClusterRoleKey) ... }

@c-bata Thanks for your suggestions!

It should be []string{""} which means no namespace options(restriction). If []string{}, it will use default name space defined by controller.v1alpha3/consts.DefaultKatibNamespace.

I have replaced with init function

I see. Thank you for your explanation!

pkg/ui/v1alpha3/backend.go

johnugeorge · 2020-04-13T12:45:30Z

pkg/ui/v1alpha3/const.go

+)
+
+func init() {
+	ns := env.GetEnvOrDefault(AvailableNameSpaceEnvName, ClusterRoleKey)


Couple of questions here

Is it right to get the namespaces during init function? What happens if env variable is changed later?

GetEnvOrDefault has fallback value as the second argument. Did you mean to keep ClusterRoleKey as the default value?

@johnugeorge

To me, this env variable should be set via manifest yaml file. I can't see any motivation to modify this env variable later.

Yes.

johnugeorge · 2020-04-13T12:45:55Z

pkg/ui/v1alpha3/nas.go

-	// Use "" to get experiments in all namespaces.
-	jobs, err := k.getExperimentList("", JobTypeNAS)
+	// Get NAS-related experiments in all available namespaces.
+	jobs, err := k.getExperimentList(availableNameSpaces, JobTypeNAS)


What do you expect when you have multiple namespaces passed?

Currently, in Katib Client: https://github.com/kubeflow/katib/blob/master/pkg/util/v1alpha3/katibclient/katib_client.go#L210, we extract only first namespace from the list, so it will not give you all available namespaces.

@andreyvelich Yes, but value of namespace is fixed on "" (https://github.com/kubeflow/katib/blob/master/pkg/ui/v1alpha3/hp.go#L22) which leads to list experiments in cluster scope.

@johnugeorge It seems only first namespace will be taken. It is a problem.

johnugeorge · 2020-04-13T12:48:38Z

Btwn, what is your use case? Can you explain the deployment requirement?

sperlingxx · 2020-04-13T15:19:14Z

Btwn, what is your use case? Can you explain the deployment requirement?
In internal k8s clusters of Antfin, service accounts of third party operators (like kubeflow operators) have no access to ClusterRole and ClusterRoleBinding for security reasons.

andreyvelich · 2020-04-13T19:53:37Z

Btwn, what is your use case? Can you explain the deployment requirement?
In internal k8s clusters of Antfin, service accounts of third party operators (like kubeflow operators) have no access to ClusterRole and ClusterRoleBinding for security reasons.

For this reason, do you have access only for the namespace where Katib UI was deployed?
Also, in your case, how Katib controller works? Since it is also required ClusterRole to deploy Jobs not in DefaultNamespace?

sperlingxx · 2020-04-14T00:38:25Z

Btwn, what is your use case? Can you explain the deployment requirement?
In internal k8s clusters of Antfin, service accounts of third party operators (like kubeflow operators) have no access to ClusterRole and ClusterRoleBinding for security reasons.

For this reason, do you have access only for the namespace where Katib UI was deployed?
Also, in your case, how Katib controller works? Since it is also required ClusterRole to deploy Jobs not in DefaultNamespace?

In my company, all machine learning related resources are shared the same namespace. So, cluster role is not required.

andreyvelich · 2020-04-14T01:03:59Z

If Katib UI can't fetch all namespaces, just get Experiments and Namespaces only from namespace where Katib UI is deployed.
What do you think about it @sperlingxx ?

sperlingxx · 2020-04-14T04:56:35Z

If Katib UI can't fetch all namespaces, just get Experiments and Namespaces only from namespace where Katib UI is deployed.
What do you think about it @sperlingxx ?

Good idea!

johnugeorge · 2020-04-14T13:15:27Z

If Katib UI can't fetch all namespaces, just get Experiments and Namespaces only from namespace where Katib UI is deployed.
What do you think about it @sperlingxx ?

Good idea!

In this case, do you really need extra environment variable? If you do not have permissions, get experiments from own namespace else do the current way.

sperlingxx · 2020-04-14T13:27:33Z

@johnugeorge I think we need know from env variable whether cluster role is permitted. In alternative, katib will try to get objects in cluster scope at first. If failed then fallback to get objects from own namespace.

johnugeorge · 2020-04-14T13:29:14Z

@johnugeorge I think we need whether has permission from env variable. In alternative, katib will try to get objects in cluster scope at first. If failed then fallback to get objects from own namespace.

+1 to this.

andreyvelich · 2020-04-14T13:53:27Z

In alternative, katib will try to get objects in cluster scope at first. If failed then fallback to get objects from own namespace.

I agree with it. Katib UI should try to get objects from all namespaces and if it fails, just get object from own namespace.

sperlingxx · 2020-04-15T12:05:52Z

@johnugeorge @andreyvelich I think current PR is ready.

andreyvelich

@sperlingxx Thanks!
/lgtm
/cc @johnugeorge

johnugeorge · 2020-04-16T18:30:24Z

/approve

k8s-ci-robot · 2020-04-16T18:30:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge

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~~ [johnugeorge]

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

…eflow#1141) * katib ui: adapt environments which cluster role is unavailable * use init_fn * fix gofmt * update * fix error message * fallback plan

katib ui: adapt environments which cluster role is unavailable

7785b6d

k8s-ci-robot requested review from Akado2009 and jinan-zhou April 13, 2020 11:16

k8s-ci-robot added the size/M label Apr 13, 2020

c-bata reviewed Apr 13, 2020

View reviewed changes

sperlingxx added 2 commits April 13, 2020 20:04

use init_fn

a70c01b

fix gofmt

37a6dfb

johnugeorge reviewed Apr 13, 2020

View reviewed changes

pkg/ui/v1alpha3/backend.go Outdated Show resolved Hide resolved

johnugeorge reviewed Apr 13, 2020

View reviewed changes

sperlingxx added 2 commits April 14, 2020 13:56

update

fbe69c1

fix error message

345bbe0

fallback plan

3b06cdc

andreyvelich approved these changes Apr 15, 2020

View reviewed changes

k8s-ci-robot requested a review from johnugeorge April 15, 2020 19:23

k8s-ci-robot assigned andreyvelich Apr 15, 2020

k8s-ci-robot added the lgtm label Apr 15, 2020

k8s-ci-robot added the approved label Apr 16, 2020

k8s-ci-robot merged commit 109d8f8 into kubeflow:master Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

katib ui: adapt environment in which cluster role is unavailable #1141

katib ui: adapt environment in which cluster role is unavailable #1141

sperlingxx commented Apr 13, 2020

kubeflow-bot commented Apr 13, 2020

sperlingxx commented Apr 13, 2020 •

edited

Loading

c-bata Apr 13, 2020

sperlingxx Apr 13, 2020

c-bata Apr 13, 2020

johnugeorge Apr 13, 2020

sperlingxx Apr 13, 2020

johnugeorge Apr 13, 2020

andreyvelich Apr 13, 2020

sperlingxx Apr 13, 2020

sperlingxx Apr 13, 2020

johnugeorge commented Apr 13, 2020

sperlingxx commented Apr 13, 2020

andreyvelich commented Apr 13, 2020

sperlingxx commented Apr 14, 2020

andreyvelich commented Apr 14, 2020

sperlingxx commented Apr 14, 2020

johnugeorge commented Apr 14, 2020

sperlingxx commented Apr 14, 2020 •

edited

Loading

johnugeorge commented Apr 14, 2020

andreyvelich commented Apr 14, 2020

sperlingxx commented Apr 15, 2020

andreyvelich left a comment

johnugeorge commented Apr 16, 2020

k8s-ci-robot commented Apr 16, 2020

katib ui: adapt environment in which cluster role is unavailable #1141

katib ui: adapt environment in which cluster role is unavailable #1141

Conversation

sperlingxx commented Apr 13, 2020

kubeflow-bot commented Apr 13, 2020

sperlingxx commented Apr 13, 2020 • 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

johnugeorge commented Apr 13, 2020

sperlingxx commented Apr 13, 2020

andreyvelich commented Apr 13, 2020

sperlingxx commented Apr 14, 2020

andreyvelich commented Apr 14, 2020

sperlingxx commented Apr 14, 2020

johnugeorge commented Apr 14, 2020

sperlingxx commented Apr 14, 2020 • edited Loading

johnugeorge commented Apr 14, 2020

andreyvelich commented Apr 14, 2020

sperlingxx commented Apr 15, 2020

andreyvelich left a comment

Choose a reason for hiding this comment

johnugeorge commented Apr 16, 2020

k8s-ci-robot commented Apr 16, 2020

sperlingxx commented Apr 13, 2020 •

edited

Loading

sperlingxx commented Apr 14, 2020 •

edited

Loading