Add leader election by pmorie · Pull Request #6683 · knative/serving

pmorie · 2020-01-29T22:27:48Z

Proposed Changes

depends on add leader election support to sharedmain pkg#1019 for changes to sharedmain, which is used by:
- controller
- autoscaler-hpa
- networking-certmanager
- networking-istio
- networking-ns-cert
- webhook
/config is changed so that it is simple to enable leader election, but it is not enabled by default

Release Note

Add support for leader election

mattmoor

I'll TAL at the pkg PR next.

mattmoor · 2020-02-04T20:34:57Z

 [[override]]
  name = "knative.dev/pkg"
-  branch = "master"
+  source = "github.com/pmorie/pkg"


Pro tip: I usually throw a // TODO(mattmoor): DO NOT SUBMIT around this, so that the bot catches it and leaves a comment.

mattmoor · 2020-02-04T21:09:13Z

+	if !leConfig.LeaderElect {
+		log.Printf("autoscaler will not run in leader-elected mode")
+		run(ctx)
+		panic("unreachable")


is this not hit when the context is cancelled?

(which happens when K8s sends SIGTERM)

mattmoor · 2020-02-04T21:10:33Z

-		return err == nil, nil
-	}); perr != nil {
-		log.Fatal("Timed out attempting to get k8s version: ", err)
-	}


If you'd left this as is, then you could save some kubeclient.Get calls below.

mattmoor · 2020-02-04T21:14:25Z

+	if err != nil {
+		logger.Fatalw("Failed to get hostname for leader election", zap.Error(err))
+	}
+	id = id + "_" + string(uuid.NewUUID())


I think hoisting this little bit into a helper would document it in the most straightforward way. At first I read the comment above and was like: "That's not what os.Hostname() does...?" :)

mattmoor · 2020-02-04T21:16:57Z

+    # giving up; 10 seconds is the value used by core kubernetes controllers.
+    renewDeadline: "10s"
+    # retryPeriod is how long the leader election client waits between tries of
+    # actions; 2 seconds is the value used by core kuberntes controllers.


Suggested change

# actions; 2 seconds is the value used by core kuberntes controllers.

# actions; 2 seconds is the value used by core kubernetes controllers.

This still seems to be a problem

mattmoor · 2020-02-04T21:18:27Z

+				logger.Fatalw("leaderelection lost")
+			},
+		},
+		// WatchDog: electionChecker,


What's this?

mattmoor · 2020-02-04T21:19:06Z

-	if err := eg.Wait(); err != nil && err != http.ErrServerClosed {
-		logger.Errorw("Error while running server", zap.Error(err))
-	}
+	panic("unreachable")


I'd use logger.Fatal in place of panic

mattmoor · 2020-02-04T21:19:43Z

-	eg, egCtx := errgroup.WithContext(ctx)
-	eg.Go(func() error {
-		return customMetricsAdapter.Run(ctx.Done())
+	leaderelection.RunOrDie(ctx, leaderelection.LeaderElectionConfig{


IIUC we get all of this for free in the other binaries due to their use of sharedmain?

mattmoor · 2020-02-04T21:23:20Z

+			})
 		if err != nil {
-			logger.Fatalw("Failed to create admission controller", zap.Error(err))
+			logger.Fatalw("error creating lock: %v", err)


Are we leaking resources on uninstall?

pmorie · 2020-02-24T19:34:11Z

/retest

pmorie · 2020-02-25T15:32:47Z

/retest

vagababov

/lint

knative-prow-robot

@vagababov: 1 warning.

Details

In response to this:

/lint

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.

vagababov

/lgtm

mattmoor

/lgtm
/approve
/hold

Mostly nits. If we want to get this baking, feel free to /unhold and follow-up, or I can restamp if you want to knock these off now.

mattmoor · 2020-02-27T02:27:27Z

 	// config validation constructors
 	tracingconfig "knative.dev/pkg/tracing/config"
 	defaultconfig "knative.dev/serving/pkg/apis/config"
+	configvalidation "knative.dev/serving/pkg/apis/config/validation"


This is sort of an odd place to put this. Can we make it knative.dev/serving/pkg/leaderelection?

Note: the defaulting config is here because defaulting happens as part of the pkg/apis stuff

That's fair. I had something similar in eventing that already went in, mind if we do this in a follow-up?

mattmoor · 2020-02-27T02:27:45Z

@@ -0,0 +1,69 @@
+# Copyright 2018 The Knative Authors


Suggested change

# Copyright 2018 The Knative Authors

# Copyright 2020 The Knative Authors

mattmoor · 2020-02-27T02:28:35Z

+  resourceLock: "leases"
+  leaseDuration: "15s"
+  renewDeadline: "10s"
+  retryPeriod: "2s"


All of the above should come from defaults, right? If so we generally let the defaulting provide them so that folks can safely tweak the knobs via kubectl edit and not upset the three-way merge.

Ack, I will do that in a follow-up.

mattmoor · 2020-02-27T02:28:45Z

+    # giving up; 10 seconds is the value used by core kubernetes controllers.
+    renewDeadline: "10s"
+    # retryPeriod is how long the leader election client waits between tries of
+    # actions; 2 seconds is the value used by core kuberntes controllers.


This still seems to be a problem

mattmoor · 2020-02-27T02:29:49Z

+
+// ValidateLeaderElectionConfig enriches the leader election config validation
+// with extra validations specific to serving.
+func ValidateLeaderElectionConfig(configMap *corev1.ConfigMap) (*kle.Config, error) {


If we go with pkg/leaderelection (comment above) then we should rename ValidateConfig to avoid stutter.

knative-prow-robot · 2020-02-27T02:32:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, pmorie

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mattmoor]

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

pmorie · 2020-02-27T03:38:20Z

/unhold

pmorie · 2020-02-27T03:47:14Z

/retest

Looks like the failure in auto tls tests may be related to #7018

knative-prow-robot · 2020-02-27T03:50:30Z

@pmorie: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-serving-integration-tests-go114	`9799bb1`	link	`/test pull-knative-serving-integration-tests-go114`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

knative-metrics-robot · 2020-02-27T03:54:52Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/leaderelection/config.go	Do not exist	85.7%

mattmoor

/lgtm

pmorie · 2020-02-27T16:21:15Z

/retest

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 29, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 29, 2020

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 29, 2020

mattmoor reviewed Jan 29, 2020

View reviewed changes

knative-prow-robot added the area/autoscale label Jan 29, 2020

knative-prow-robot requested review from tcnghia and yanweiguo January 29, 2020 22:28

pmorie force-pushed the leader-election branch from e0ad305 to 7b5ee47 Compare January 30, 2020 22:23

mattmoor reviewed Jan 30, 2020

View reviewed changes

pmorie force-pushed the leader-election branch from 7b5ee47 to 26fb8d9 Compare January 30, 2020 22:33

mattmoor reviewed Jan 30, 2020

View reviewed changes

pmorie force-pushed the leader-election branch from 26fb8d9 to 210423a Compare January 30, 2020 22:46

mattmoor reviewed Jan 30, 2020

View reviewed changes

pmorie force-pushed the leader-election branch from 210423a to 8eae52e Compare January 30, 2020 22:53

mattmoor reviewed Jan 30, 2020

View reviewed changes

Comment thread config/config-leader-election.yaml Outdated

pmorie commented Jan 30, 2020

View reviewed changes

Comment thread config/config-leader-election.yaml Outdated

mattmoor reviewed Feb 4, 2020

View reviewed changes

pmorie force-pushed the leader-election branch from 8eae52e to 9799bb1 Compare February 6, 2020 17:33

knative-prow-robot added the area/API API objects and controllers label Feb 6, 2020

mattmoor reviewed Feb 6, 2020

View reviewed changes

pmorie force-pushed the leader-election branch from 9799bb1 to a303651 Compare February 24, 2020 18:49

mattmoor reviewed Feb 24, 2020

View reviewed changes

pmorie changed the title ~~WIP: Add leader election~~ Add leader election Feb 24, 2020

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 24, 2020

pmorie force-pushed the leader-election branch from a303651 to 58a39e5 Compare February 25, 2020 01:09

mattmoor reviewed Feb 25, 2020

View reviewed changes

pmorie closed this Feb 25, 2020

pmorie reopened this Feb 25, 2020

pmorie force-pushed the leader-election branch from 9aef37a to 1e9ef61 Compare February 26, 2020 23:34

mattmoor reviewed Feb 26, 2020

View reviewed changes

vagababov reviewed Feb 27, 2020

View reviewed changes

knative-prow-robot reviewed Feb 27, 2020

View reviewed changes

Comment thread pkg/apis/config/validation/leader_election.go Outdated

vagababov reviewed Feb 27, 2020

View reviewed changes

Comment thread pkg/apis/config/validation/leader_election.go

Add comment for exported validation method

abc628f

mattmoor reviewed Feb 27, 2020

View reviewed changes

vagababov reviewed Feb 27, 2020

View reviewed changes

knative-prow-robot assigned vagababov Feb 27, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2020

mattmoor reviewed Feb 27, 2020

View reviewed changes

knative-prow-robot assigned mattmoor Feb 27, 2020

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2020

pmorie added 2 commits February 26, 2020 22:27

Move leader election config -> pkg/leaderelection

4ab0cf5

Nits in leader election config

71359df

knative-prow-robot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Feb 27, 2020

mattmoor reviewed Feb 27, 2020

View reviewed changes

Rename leader election config validation method

70e90c9

mattmoor reviewed Feb 27, 2020

View reviewed changes

Comment thread config/config-leader-election.yaml

mattmoor reviewed Feb 27, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2020

knative-prow-robot merged commit 37852e6 into knative:master Feb 27, 2020

	# actions; 2 seconds is the value used by core kuberntes controllers.
	# actions; 2 seconds is the value used by core kubernetes controllers.

	# Copyright 2018 The Knative Authors
	# Copyright 2020 The Knative Authors

Conversation

pmorie commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmorie commented Feb 24, 2020

Uh oh!

pmorie commented Feb 25, 2020

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Feb 27, 2020

Uh oh!

pmorie commented Feb 27, 2020

Uh oh!

pmorie commented Feb 27, 2020

Uh oh!

Uh oh!

knative-prow-robot commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-metrics-robot commented Feb 27, 2020

Uh oh!

mattmoor left a comment

pmorie commented Jan 29, 2020 •

edited

Loading

knative-prow-robot commented Feb 27, 2020 •

edited

Loading