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.

Sign up for GitHub

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

Jump to bottom

Deprecate the ENV MY_POD_NAME and use default namespace #348

Merged
merged 1 commit into from
Feb 2, 2018
Merged

Deprecate the ENV MY_POD_NAME and use default namespace #348

merged 1 commit into from
Feb 2, 2018

Conversation

ScorpioCPH
Copy link
Member

@ScorpioCPH ScorpioCPH commented Jan 26, 2018 •
edited by jlewi
Loading

Hi, this PR fixes #341:

  • Deprecate the ENV MY_POD_NAMESPACE and MY_POD_NAME
  • And use NamespaceDefault by default

@jlewi PTAL, thanks!

This change is Reviewable

@coveralls
Copy link

coveralls commented Jan 26, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 31.856% when pulling 9f993a8 on ScorpioCPH:deprecate-the-env into 330eb92 on tensorflow:master.

jlewi
jlewi reviewed Jan 26, 2018
View reviewed changes
cmd/tf_operator/app/server.go Outdated
@@ -103,7 +98,7 @@ func Run(opt *options.ServerOption) error {

rl := &resourcelock.EndpointsLock{
EndpointsMeta: metav1.ObjectMeta{
Namespace: namespace,
Namespace: metav1.NamespaceDefault,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is NamespaceDefault the default namespace? Why do you use the default Namespace and not the pod's namespace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is resourcelock‘s namespace.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we be using the namespace that the controller is deployed in for the resource lock? What if we want to deploy different instances of the controller in different namespaces? Wouldn't the end up trying to share the same resource lock causing problems?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this resource lock? to make sure that there is only one instance to create the controller in the namespace at any time?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlewi I'm not sure about this, will take a look at it, and file a issue to follow up.

@gaocegege It is used for HA.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be ok with getting rid of MY_POD_NAME in this PR but leaving MY_POD_NAMESPACE until we can resolve the issue.

jlewi
jlewi reviewed Jan 26, 2018
View reviewed changes
Copy link
Contributor

@jlewi jlewi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there were some test failures.

@ScorpioCPH
Copy link
Member Author

/test tf-k8s-presubmit

@ScorpioCPH
Copy link
Member Author

@jlewi Is the test machine out of disk?

@jlewi
Copy link
Contributor

jlewi commented Jan 26, 2018

It was the other day but I cleaned it up so it shouldn't be anymore; let me check.

@jlewi
Copy link
Contributor

jlewi commented Jan 26, 2018

Test project ran out of IP addressess

[2018-01-26 13:11:36,970] {base_task_runner.py:98} INFO - Subtask: googleapiclient.errors.HttpError: <HttpError 403 when requesting https://container.googleapis.com/v1/projects/mlkube-testing/zones/us-east1-d/clusters?alt=json returned "Insufficient regional quota to satisfy request for resource: "IN_USE_ADDRESSES". The request requires '1.0' and is short '1.0'. The regional quota is '8.0' with '0.0' available.">
[2018-01-26 13:11:36,971] {base_task_runner.py:98} INFO - Subtask: 

Looks like we leaked some K8s clusters consuming all the addresses. I'm deleting the clusters.

@ScorpioCPH
Copy link
Member Author

/test tf-k8s-presubmit

@ScorpioCPH
Copy link
Member Author

@jlewi Ping.

@ScorpioCPH
Copy link
Member Author

@jlewi PTAL, thanks!

@ScorpioCPH ScorpioCPH mentioned this pull request Jan 30, 2018
Closed
@ScorpioCPH ScorpioCPH changed the title Deprecate the ENV MY_POD_NAMESPACE and MY_POD_NAME Deprecate the ENV MY_POD_NAME and use default namespace Feb 2, 2018
@ScorpioCPH
Copy link
Member Author

Not sure what is this error The command "gometalinter --config=linter_config.json ./pkg/..." exited with 1.

gaocegege
gaocegege requested changes Feb 2, 2018
View reviewed changes
Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is caused by goimports: the name of the package should be the same name in the source. There is a related issue: joefitzgerald/go-plus#52 (comment)

pkg/util/util.go Outdated
@@ -10,6 +10,11 @@ import (
log "github.com/golang/glog"
Copy link
Member

@gaocegege gaocegege Feb 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this line to "github.com/golang/glog" could solve the errors in Travis CI

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much, addressed your comments.

@jlewi jlewi merged commit e2b13e0 into kubeflow:master Feb 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaocegege gaocegege gaocegege approved these changes

@jlewi jlewi jlewi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate the ENV MY_POD_NAMESPACE and MY_POD_NAME
4 participants
@ScorpioCPH @coveralls @jlewi @gaocegege