scheduler: refactor main entry Run() #38572

hongchaodeng · 2016-12-11T04:58:17Z

The kube-scheduler/app.Run() is the main entry of scheduler program.
It's enormous.
This PR tries to clean it up. Should be more modular and readable.

k8s-reviewable · 2016-12-11T04:58:23Z

This change is

hongchaodeng · 2016-12-11T05:18:30Z

cc @timothysc @xiang90
Can you help review this?

xiang90 · 2016-12-11T19:32:02Z

plugin/cmd/kube-scheduler/scheduler.go

@@ -36,5 +37,8 @@ func main() {

 	verflag.PrintAndExitIfRequested()

-	app.Run(s)
+	if err := app.Run(s); err != nil {
+		glog.Errorf("scheduler app failed to run: %v", err)


glog.Fatal here? or we can reach line 43.

xiang90 · 2016-12-11T19:32:55Z

plugin/cmd/kube-scheduler/scheduler.go

+	if err := app.Run(s); err != nil {
+		glog.Errorf("scheduler app failed to run: %v", err)
+	}
+	glog.Fatal("this statement shouldn't be reached")


we should panic on a unreachable line not fatal. usually fatal on unrecoverable external error, and panic on unrecoverable internal error.

xiang90 · 2016-12-11T19:34:09Z

plugin/cmd/kube-scheduler/app/server.go

 	run := func(_ <-chan struct{}) {
 		sched.Run()
 		select {}
 	}

 	if !s.LeaderElection.LeaderElect {
 		run(nil)
-		glog.Fatal("this statement is unreachable")
-		panic("unreachable")
+		return nil


probably should still panic internally.

xiang90 · 2016-12-11T19:34:51Z

plugin/cmd/kube-scheduler/app/server.go

+
+	cli, err := clientset.NewForConfig(restclient.AddUserAgent(kubeconfig, "leader-election"))
+	if err != nil {
+		return nil, fmt.Errorf("Invalid API configuration: %v", err)


invalid API configuration: %v

xiang90 · 2016-12-11T19:35:13Z

plugin/cmd/kube-scheduler/app/server.go

+func createClient(s *options.SchedulerServer) (*clientset.Clientset, error) {
+	kubeconfig, err := clientcmd.BuildConfigFromFlags(s.Master, s.Kubeconfig)
+	if err != nil {
+		glog.Errorf("unable to build config from flags: %v", err)


we should log error in the caller?

xiang90 · 2016-12-11T19:35:56Z

Minor issues. LGTM after fixing them.

hongchaodeng · 2016-12-12T00:35:06Z

Fixed minor issues

xiang90 · 2016-12-12T00:38:57Z

@hongchaodeng Awesome. Thanks!

hongchaodeng · 2016-12-12T02:24:35Z

@k8s-bot unit test this #38594

hongchaodeng · 2016-12-12T02:25:09Z

@k8s-bot cri e2e test this #38556

hongchaodeng · 2016-12-12T06:36:22Z

@k8s-bot cri e2e test this

k8s-ci-robot · 2016-12-12T07:47:44Z

Jenkins CRI GCE e2e failed for commit 7bd580caf9f1e492624af42d0fb3864dcb0cead5. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

hongchaodeng · 2016-12-12T16:44:21Z

@wojtek

Can you help review?

timothysc · 2016-12-12T19:38:35Z

plugin/cmd/kube-scheduler/app/server.go

+			goruntime.SetBlockProfileRate(1)
+		}
+	}
+	if c, err := configz.New("componentconfig"); err == nil {


It's difficult to tell, but did you ensure this was the 1st place configz was used in the shuffling. B4 it was the 1st line before refactoring.

Pretty sure it's fine. Tested and verified.
If you looked at the commit 5ec02bd, it actually wasn't used anywhere else.

Not sure why it needs to be put 1st line.
cc original author @mikedanese

timothysc · 2016-12-12T19:53:00Z

plugin/cmd/kube-scheduler/scheduler.go

+	if err := app.Run(s); err != nil {
+		glog.Fatalf("scheduler app failed to run: %v", err)
+	}
+	panic("this statement shouldn't be reached")


I don't think this logic makes much sense.. of course we can reach this point via a number of ways. Shouldn't we just exit gracefully?

I will remove this one.

of course we can reach this point via a number of ways.

IIRC, no way to reach here? Can you clarify?

timothysc · 2016-12-12T19:55:15Z

plugin/cmd/kube-scheduler/app/server.go

 	if err != nil {
-		glog.Fatalf("Invalid API configuration: %v", err)
+		return fmt.Errorf("unable to create kube client: %v", err)


Does it make sense to re-wrap the error or just return them? I just debate the utility if we're refactoring.

The previous code usually Fatalf() right there. Now We Fatalf() outside, and thus it's better to wrap them.

timothysc

Please answer the minor comments, otherwise lgtm

k8s-ci-robot · 2016-12-12T21:26:00Z

Jenkins GCI GKE smoke e2e failed for commit a6bfeae99555a59b6c201bd13dab5c3ce70f2712. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-12T21:26:22Z

Jenkins GCE etcd3 e2e failed for commit a6bfeae99555a59b6c201bd13dab5c3ce70f2712. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-12T21:27:13Z

Jenkins CRI GCE Node e2e failed for commit a6bfeae99555a59b6c201bd13dab5c3ce70f2712. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-12T21:28:05Z

Jenkins verification failed for commit a6bfeae99555a59b6c201bd13dab5c3ce70f2712. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-12T21:28:06Z

Jenkins GCE e2e failed for commit a6bfeae99555a59b6c201bd13dab5c3ce70f2712. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-12T21:28:57Z

Jenkins unit/integration failed for commit a6bfeae99555a59b6c201bd13dab5c3ce70f2712. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-12T21:29:09Z

Jenkins GKE smoke e2e failed for commit a6bfeae99555a59b6c201bd13dab5c3ce70f2712. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-12T21:29:11Z

Jenkins GCE Node e2e failed for commit a6bfeae99555a59b6c201bd13dab5c3ce70f2712. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-12T21:29:11Z

Jenkins Kubemark GCE e2e failed for commit a6bfeae99555a59b6c201bd13dab5c3ce70f2712. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-12T21:29:22Z

Jenkins GCI GCE e2e failed for commit a6bfeae99555a59b6c201bd13dab5c3ce70f2712. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

timothysc · 2016-12-12T22:16:23Z

lgtm.

k8s-github-robot · 2016-12-13T03:42:34Z

Automatic merge from submit-queue (batch tested with PRs 38315, 38624, 38572, 38544)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 11, 2016

k8s-github-robot assigned davidopp Dec 11, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Dec 11, 2016

hongchaodeng force-pushed the sr branch from b8b7ceb to 5277c12 Compare December 11, 2016 05:14

davidopp assigned wojtek-t and unassigned davidopp Dec 11, 2016

xiang90 reviewed Dec 11, 2016

View reviewed changes

hongchaodeng force-pushed the sr branch from f3efab4 to a010eb9 Compare December 12, 2016 00:34

hongchaodeng force-pushed the sr branch from a010eb9 to 7bd580c Compare December 12, 2016 00:41

timothysc reviewed Dec 12, 2016

View reviewed changes

timothysc suggested changes Dec 12, 2016

View reviewed changes

hongchaodeng force-pushed the sr branch from 7bd580c to a6bfeae Compare December 12, 2016 21:22

scheduler: refactor main entry Run()

a565648

bazel update

bcd3aac

hongchaodeng force-pushed the sr branch from a6bfeae to bcd3aac Compare December 12, 2016 21:23

timothysc approved these changes Dec 12, 2016

View reviewed changes

timothysc added release-note-none Denotes a PR that doesn't merit a release note. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed release-note-label-needed labels Dec 12, 2016

timothysc added this to the v1.6 milestone Dec 12, 2016

timothysc self-assigned this Dec 12, 2016

k8s-github-robot merged commit 65238bc into kubernetes:master Dec 13, 2016

hongchaodeng deleted the sr branch December 13, 2016 17:39

scheduler: refactor main entry Run() #38572

scheduler: refactor main entry Run() #38572

Conversation

hongchaodeng commented Dec 11, 2016 • edited

k8s-reviewable commented Dec 11, 2016

hongchaodeng commented Dec 11, 2016

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

xiang90 commented Dec 11, 2016

hongchaodeng commented Dec 12, 2016

xiang90 commented Dec 12, 2016

hongchaodeng commented Dec 12, 2016

hongchaodeng commented Dec 12, 2016

hongchaodeng commented Dec 12, 2016

k8s-ci-robot commented Dec 12, 2016

hongchaodeng commented Dec 12, 2016

Choose a reason for hiding this comment

hongchaodeng Dec 12, 2016 • edited

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

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 12, 2016

k8s-ci-robot commented Dec 12, 2016

k8s-ci-robot commented Dec 12, 2016

k8s-ci-robot commented Dec 12, 2016

k8s-ci-robot commented Dec 12, 2016

k8s-ci-robot commented Dec 12, 2016

k8s-ci-robot commented Dec 12, 2016

k8s-ci-robot commented Dec 12, 2016

k8s-ci-robot commented Dec 12, 2016

k8s-ci-robot commented Dec 12, 2016

timothysc commented Dec 12, 2016

k8s-github-robot commented Dec 13, 2016

hongchaodeng commented Dec 11, 2016 •

edited

hongchaodeng Dec 12, 2016 •

edited