refactor the controller logic #234

wackxu · 2017-12-19T14:52:10Z

This PR is mainly about refactoring the structure of controller keep style with kubernetes controller. And the main logic of package trainer is not modified. We can continue do refactor next step to make the controller stateless.

This change is

googlebot · 2017-12-19T14:52:13Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

k8s-ci-robot · 2017-12-19T14:52:26Z

Hi @wackxu. Thanks for your PR.

I'm waiting for a kubernetes or tensorflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

wackxu · 2017-12-19T14:55:52Z

The work is going on. Then it is ok to review, I will call yo @jlewi

zjj2wry · 2017-12-20T10:09:09Z

cool, will help review when it ready

gaocegege · 2017-12-24T13:01:13Z

Awesome, plz @ me if the PR is ready since I implemented a similar controller before and I am glad to help to review it :-)

ScorpioCPH

Can you just submit the last one commit refactor logic, i think the other commits are the same as #215

wackxu · 2017-12-29T07:07:06Z

This PR is almost done, I need to wait #215 (almost get merge) get merged. Then I will done the following work.

zjj2wry · 2017-12-29T07:28:02Z

cmd/tf_operator/app/options/options.go

+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/


we need this header? @jlewi

I don't think so.

zjj2wry · 2017-12-29T07:34:16Z

cmd/tf_operator/app/server.go

+		Client: leaderElectionClient.CoreV1(),
+		LockConfig: resourcelock.ResourceLockConfig{
+			Identity:      id,
+			EventRecorder: &record.FakeRecorder{},


we can use real event broadcast? we can get some events in tf-operator endpoint when tf-operator failover.

i created a PR #260 fix this.

zjj2wry · 2017-12-29T07:34:40Z

cmd/tf_operator/app/server.go

+
+	id, err := os.Hostname()
+	if err != nil {
+		return fmt.Errorf("failed to get hostname: %v", err)


Failed ... would be better

Please fix.

zjj2wry · 2017-12-29T07:36:42Z

cmd/tf_operator/app/options/options.go

+	fs.IntVar(&s.ChaosLevel, "chaos-level", -1, "DO NOT USE IN PRODUCTION - level of chaos injected into the TfJob created by the operator.")
+	fs.BoolVar(&s.PrintVersion, "version", false, "Show version and quit")
+	fs.DurationVar(&s.GCInterval, "gc-interval", 10*time.Minute, "GC interval")
+	fs.StringVar(&s.ControllerConfigFile, "controller_config_file", "", "Path to file containing the controller config.")


let's fix controller_config_file to controller-config-file in this PR?

@wackxu i created a PR #259 fix this. let this PR focus on controller. what you think?

zjj2wry · 2017-12-29T07:37:24Z

cmd/tf_operator/app/server.go

+	name := os.Getenv("MY_POD_NAME")
+	if len(name) == 0 {
+		glog.Fatalf("must set env MY_POD_NAME")
+	}


can we move this in validate func?

zjj2wry · 2017-12-29T07:42:43Z

@wackxu will review controller logic when #215 get merged

zjj2wry · 2017-12-29T07:44:44Z

pkg/util/k8sutil/tf_job_client.go

@@ -1,147 +0,0 @@
-// Licensed under the Apache License, Version 2.0 (the "License");


i suspect remove this will break dashboard...

ScorpioCPH

Hi, I think #215 has been merged, could me mind remove the repeated commits please?

wackxu · 2018-01-02T06:59:13Z

@ScorpioCPH Done， PTAL

coveralls · 2018-01-02T07:30:19Z

Coverage increased (+2.0%) to 30.515% when pulling 5a2e58f on wackxu:refcon into ed7cae7 on tensorflow:master.

ScorpioCPH

@wackxu Thanks very much for your refactor work! Would you please add more description about what this PR do and why we need this PR, Thanks!

@jlewi Hi, I suggested that we need more design doc/discussion before the following PR review. It will be helpful to reduce duplicating efforts.

ScorpioCPH · 2018-01-02T07:15:50Z

cmd/tf_operator/app/options/options.go

+
+import (
+	"github.com/spf13/pflag"
+	"time"


Please use gofmt to format your source code.

Move built-in packages to the beginning of import {} maybe better.

Thanks for your review, Will address it.

Anything update?

@ScorpioCPH I have run go fmt and it is the right order.

Add a new line between built-in package time and github.com/spf13/pflag, and retry.

ScorpioCPH · 2018-01-02T07:17:03Z

cmd/tf_operator/app/server.go

+	"time"
+	"k8s.io/client-go/rest"
+	clientset "k8s.io/client-go/kubernetes"
+)


Please format this import{} statement.

ScorpioCPH · 2018-01-02T07:19:48Z

cmd/tf_operator/app/server.go

+	}
+	name := os.Getenv("MY_POD_NAME")
+	if len(name) == 0 {
+		glog.Fatalf("must set env MY_POD_NAME")


Why must we set a pod name? I think this is no matter for users, in another word, Kubeflow users don't know what is Pod.

Agree, I would like to remove it.

I think we can remove it. I think it's a legacy of the original etcd code.

ScorpioCPH · 2018-01-02T08:42:55Z

cmd/tf_operator/main.go

@@ -1,194 +1,22 @@
 package main


Please add copyright.

ScorpioCPH · 2018-01-02T09:08:42Z

cmd/tf_operator/app/server.go

+		return fmt.Errorf("failed to get hostname: %v", err)
+	}
+
+	rl := &resourcelock.EndpointsLock{


What this resourcelock used for?

I think it is for high availability when we run more than one tensorflow operator.

resoucelock have endpoint and configmap type. tf_operator use endpoint type. tf_operator endpoint use annotation store kubernetes leader election information as a optimistic lock

There's some discussion in #263.

ScorpioCPH · 2018-01-02T09:11:45Z

cmd/tf_operator/app/server.go

+		},
+	}
+
+	election.RunOrDie(election.LeaderElectionConfig{


Will we have more than one Kubeflow controller in a cluster?

Yeah, we can run more than one tensorflow operator for high availability as kubernetes controller manage

only a replecas work. so it is a Cold Backup

jlewi · 2018-01-03T06:10:25Z

@wackxu agree with @ScorpioCPH

Can you explain how using informers works and how it compares to the current model?

High level question. Does this refactor make the controller stateless?

The initial design of the controller was stateful such that we maintained state about each TfJob in memory.

This was convenient because it meant that every time the job controller first loaded a job we could perform some expensive operation to determine the state of the job. Then when we periodically (every O(seconds) check the state of the job) we could take advantage of the known state. If the controller is stateless then this could be more problematic because either we'd need to 1) store this state in the ApiServer or 2) recompute the state on each update which could be prohibitively expensive.

wackxu · 2018-01-03T08:57:22Z

@jlewi You can get details information about how informers works from https://engineering.bitnami.com/articles/a-deep-dive-into-kubernetes-controllers.html

wackxu · 2018-01-03T09:04:55Z

@jlewi In this refactor, the controller is still stateful. If we want to keep style with kubernetes controller, I think the change is so big that we can do it next step. I just modify the structure of controller. And the main logic of package trainer is not modified

jlewi · 2018-01-03T13:46:02Z

@wackxu Thanks for the info its very helpful. Can you update the initial comment in this PR to explain what refactoring this PR is doing and why?

wackxu · 2018-01-05T01:54:49Z

@jlewi @zjj2wry @ScorpioCPH I think we can first merge it and It just modify the structure for controller and next step @gaocegege will focus on refactor the logic about reconcile

coveralls · 2018-01-05T01:58:58Z

Coverage increased (+2.0%) to 30.336% when pulling 4a050fc on wackxu:refcon into f7803f1 on tensorflow:master.

coveralls · 2018-01-05T02:02:12Z

Coverage increased (+2.0%) to 30.336% when pulling 4a050fc on wackxu:refcon into f7803f1 on tensorflow:master.

zjj2wry · 2018-01-05T02:14:08Z

/ok-to-test

jlewi · 2018-01-05T04:50:40Z

@wackxu Thank you I think my high level questions have been addressed. Your plan seems reasonable.

@zjj2wry @ScorpioCPH I'll let you take the lead on reviewing this.

ScorpioCPH · 2018-01-05T08:07:10Z

@jlewi LGTM, let's move forward :)

jlewi · 2018-01-05T15:23:16Z

@wackxu looks like there are some lint issues. Can you please fix?

jlewi

One nit but otherwise this is ready to go once lint issues are fixed and travis is passing.

jlewi · 2018-01-03T05:54:49Z

cmd/tf_operator/app/server.go

+	}
+	name := os.Getenv("MY_POD_NAME")
+	if len(name) == 0 {
+		glog.Fatalf("must set env MY_POD_NAME")


I think we can remove it. I think it's a legacy of the original etcd code.

jlewi · 2018-01-03T05:56:55Z

cmd/tf_operator/app/server.go

+		return fmt.Errorf("failed to get hostname: %v", err)
+	}
+
+	rl := &resourcelock.EndpointsLock{


There's some discussion in #263.

jlewi · 2018-01-05T15:26:14Z

cmd/tf_operator/app/server.go

+
+	id, err := os.Hostname()
+	if err != nil {
+		return fmt.Errorf("failed to get hostname: %v", err)


Please fix.

wackxu · 2018-01-08T02:47:09Z

/retest

coveralls · 2018-01-08T03:00:56Z

Coverage increased (+2.09%) to 30.462% when pulling 43e3fb3 on wackxu:refcon into 190394d on tensorflow:master.

coveralls · 2018-01-08T03:20:49Z

Coverage increased (+2.2%) to 30.567% when pulling 0bded69 on wackxu:refcon into 190394d on tensorflow:master.

wackxu · 2018-01-08T03:53:05Z

@jlewi Done, PTAL

jlewi · 2018-01-08T13:52:41Z

Fixed some merge conflicts; waiting for the tests to rerun before merging.

coveralls · 2018-01-08T14:01:28Z

Coverage increased (+2.1%) to 30.551% when pulling 3f8c5f8 on wackxu:refcon into 1f85e46 on tensorflow:master.

…orker. * This was added in kubeflow#221 and accidentally removed in the refactor in kubeflow#234.

…roken (#308) * In syncTfJob when checking whether a work queue item corresponds to a TrainingJob already in the map we need to check the UID. Otherwise we will not properly handle the case where a training job is deleted and then a new job is recreated with the same name. * We need to make sure that the Replicas field in TrainingJob is always properly set; * We were only initializing replicas in setup which was problematic in the case where the TfJob controller gets restarted because on restarted setup won't be invoked because the job is past that phase and as a result the replicas won't be reinitialized. * test_runner needs to ignore case when checking whether the job succeeded otherwise we conclude that successful jobs failed * The controller should only forget about job after the job has been cleaned up; not when it is marked as succeeded or failed. * Add back code to support termination policies use the worker and not the master as the chief *This was added in #221 and accidentally removed in the refactor in #234.

k8s-ci-robot added the needs-ok-to-test label Dec 19, 2017

wackxu mentioned this pull request Dec 19, 2017

Refactor the TfJob to use K8s libraries #215

Merged

ScorpioCPH reviewed Dec 25, 2017

View reviewed changes

wackxu changed the title ~~[WIP]refactor the controller logic~~ refactor the controller logic Dec 29, 2017

zjj2wry reviewed Dec 29, 2017

View reviewed changes

ScorpioCPH reviewed Dec 30, 2017

View reviewed changes

ScorpioCPH reviewed Jan 2, 2018

View reviewed changes

gaocegege mentioned this pull request Jan 3, 2018

refactor the TfJob to use Informer and Controller #206

Closed

k8s-ci-robot removed the needs-ok-to-test label Jan 5, 2018

jlewi reviewed Jan 5, 2018

View reviewed changes

refactor controller logic

0bded69

jlewi added 2 commits January 8, 2018 05:50

Merge branch 'master' into refcon

ae72db4

Merge branch 'master' into refcon

3f8c5f8

jlewi merged commit da1a861 into kubeflow:master Jan 8, 2018

jlewi mentioned this pull request Jan 10, 2018

Regression in flag name: controller-config-file #279

Closed

jlewi added a commit to jlewi/k8s that referenced this pull request Jan 14, 2018

* Add back code to support termination policies which don't use the w…

29358ab

…orker. * This was added in kubeflow#221 and accidentally removed in the refactor in kubeflow#234.

jlewi mentioned this pull request Jan 14, 2018

Fix a bunch of problems in TfJob CRD that crept in while tests were broken #308

Merged

		@@ -1,147 +0,0 @@
		// Licensed under the Apache License, Version 2.0 (the "License");

refactor the controller logic #234

refactor the controller logic #234

Conversation

wackxu commented Dec 19, 2017 • edited Loading

googlebot commented Dec 19, 2017

k8s-ci-robot commented Dec 19, 2017

wackxu commented Dec 19, 2017

zjj2wry commented Dec 20, 2017

gaocegege commented Dec 24, 2017 • edited Loading

ScorpioCPH left a comment • edited Loading

Choose a reason for hiding this comment

wackxu commented Dec 29, 2017

zjj2wry Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjj2wry Dec 29, 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

zjj2wry Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

zjj2wry Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

zjj2wry Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

zjj2wry commented Dec 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScorpioCPH left a comment

Choose a reason for hiding this comment

wackxu commented Jan 2, 2018

coveralls commented Jan 2, 2018 • edited Loading

ScorpioCPH left a comment • 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

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

zjj2wry Jan 2, 2018 • 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

jlewi commented Jan 3, 2018

wackxu commented Jan 3, 2018

wackxu commented Jan 3, 2018

jlewi commented Jan 3, 2018

wackxu commented Jan 5, 2018

coveralls commented Jan 5, 2018 • edited Loading

coveralls commented Jan 5, 2018 • edited Loading

zjj2wry commented Jan 5, 2018

jlewi commented Jan 5, 2018

ScorpioCPH commented Jan 5, 2018

jlewi commented Jan 5, 2018

jlewi 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

wackxu commented Jan 8, 2018

coveralls commented Jan 8, 2018 • edited Loading

coveralls commented Jan 8, 2018 • edited Loading

wackxu commented Jan 8, 2018

jlewi commented Jan 8, 2018

coveralls commented Jan 8, 2018 • edited Loading

wackxu commented Dec 19, 2017 •

edited

Loading

gaocegege commented Dec 24, 2017 •

edited

Loading

ScorpioCPH left a comment •

edited

Loading

zjj2wry Dec 29, 2017 •

edited

Loading

zjj2wry Dec 29, 2017 •

edited

Loading

zjj2wry Dec 29, 2017 •

edited

Loading

zjj2wry Jan 2, 2018 •

edited

Loading

zjj2wry Dec 29, 2017 •

edited

Loading

coveralls commented Jan 2, 2018 •

edited

Loading

ScorpioCPH left a comment •

edited

Loading

zjj2wry Jan 2, 2018 •

edited

Loading

coveralls commented Jan 5, 2018 •

edited

Loading

coveralls commented Jan 5, 2018 •

edited

Loading

coveralls commented Jan 8, 2018 •

edited

Loading

coveralls commented Jan 8, 2018 •

edited

Loading

coveralls commented Jan 8, 2018 •

edited

Loading