add leader election support to sharedmain by pmorie · Pull Request #1019 · knative/pkg

pmorie · 2020-01-29T22:23:19Z

For #1007; this PR adds WIP support for leader election to sharedmain.

vagababov

/lint

vagababov · 2020-01-30T01:10:18Z

-		logger.Fatalw("Version check failed", zap.Error(err))
-	}
+	// run is the main controller flow
+	run := func(ctx context.Context) {


This is quite big. Why not extract it to a separate function?

I was following the constructions in k8s controller manager, decided to save any cosmetics or discretionary factoring for after it worked. Since that function captures many variables from the enclosing context, extracting a function would be more laborious than it might appear - since you would have to introduce a type or function factory to inject those variables.

I didn't look closely, but I noticed logger. If it's just 2-3, then passing via args should be alright.
But, sure, we can brush it up later on.

Frankly, I don't think it's worth it, but you're welcome to do it as a follow-on PR.

vagababov · 2020-01-30T01:11:05Z

+		run(ctx)
+		panic("unreachable")
+	} else {
+		// create a unique identifier so that two controllers on the same host don't


Suggested change

// create a unique identifier so that two controllers on the same host don't

// Create a unique identifier so that two controllers on the same host don't

vagababov · 2020-01-30T01:11:51Z

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


Suggested change

id = id + "_" + string(uuid.NewUUID())

id += "_" + string(uuid.NewUUID())

vagababov · 2020-01-30T01:12:12Z

+			logger.Fatalw("Failed to get hostname for leader election", zap.Error(err))
+		}
+		id = id + "_" + string(uuid.NewUUID())
+		log.Printf("%v will run in leader-elected mode with id %v", component, id)


rm? or logger.Infof().

vagababov · 2020-01-30T01:12:42Z

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


Suggested change

logger.Fatalw("error creating lock: %v", err)

logger.Fatalw("Error creating lock: %v", zap.Error(err))

vagababov · 2020-01-30T01:13:05Z

+			Callbacks: leaderelection.LeaderCallbacks{
+				OnStartedLeading: run,
+				OnStoppedLeading: func() {
+					logger.Fatalw("leaderelection lost")


Suggested change

logger.Fatalw("leaderelection lost")

logger.Fatal("leaderelection lost")

should work?

If I understand this code correctly, the controller will start up, and try to acquire the leader lock. If it participates in the election and is not elected, it will sit and continue participating in the leader election. If it wins the election, it will serve (run the specified controllers) and will exit if it ever loses an election after that.

Is that correct?

vagababov · 2020-01-30T01:13:40Z

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


I'd go with logger.Fatal() instead, rather than panic. Same below.

knative-prow-robot

@vagababov: 11 warnings.

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.

knative-prow-robot · 2020-01-30T01:14:16Z

+	corev1 "k8s.io/api/core/v1"
+)
+
+const ConfigMapNameEnv = "CONFIG_LEADERELECTION_NAME"


Golint comments: exported const ConfigMapNameEnv should have comment or be unexported. More info.

probably unexported.

knative-prow-robot · 2020-01-30T01:14:16Z

+	EnabledComponents map[string]bool
+}
+
+func (c *Config) GetComponentConfig(name string) ComponentConfig {


Golint comments: exported method Config.GetComponentConfig should have comment or be unexported. More info.

knative-prow-robot · 2020-01-30T01:14:17Z

+		},
+	}
+
+	for i, _ := range cases {


Suggested change

for i, _ := range cases {

for i := range cases {

Golint range-loop: should omit 2nd value from range; this loop is equivalent to for i := range ....

knative-prow-robot · 2020-01-30T01:14:18Z

+		},
+	}
+
+	for i, _ := range cases {


Suggested change

for i, _ := range cases {

for i := range cases {

Golint range-loop: should omit 2nd value from range; this loop is equivalent to for i := range ....

knative-prow-robot · 2020-01-30T01:14:18Z

+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return kle.NewConfigFromMap(nil)
+		} else {


Golint indent: if block ends with a return statement, so drop this else and outdent its block. More info.

+1 to this comment.

skaslev · 2020-01-30T13:13:08Z

@@ -0,0 +1,4 @@
+# See the OWNERS docs at https://go.k8s.io/owners


I thought ./hack/update-deps.sh deletes OWNERS files in vendor/.

If you run it ;-)

Yep, if you run it applies :)

vaikas

Thanks for adding this!

vaikas · 2020-02-04T20:09:51Z

+					logger.Fatalw("leaderelection lost")
+				},
+			},
+			// TODO: investigate using watchdog


Could we add an issue to track this and add a pointer to that here?

Opened #1048

vaikas · 2020-02-04T20:13:17Z

+	}
+
+	if leaseDurationStr, ok := data["leaseDuration"]; ok {
+		if leaseDuration, err := time.ParseDuration(leaseDurationStr); err == nil {


unless I'm totes reading this wrong, if a user specifies an invalid value here, it will not take effect and they will have no visibility that what they wanted didn't happen?
Should we at the very least log this error?
Just worrying about the lack of visibility here.

That's a good point; I think that in the presence of an invalid value, you should receive the default value and have a log message. WDYT

Yes, it gets the default value now, my worry was the lack of feedback, which the log message would provide.

Three choices here:

If you supply an invalid value, it fails loudly (i.e. logger.Fatal) This is most appropriate for startup conditions, where it might block something like a rolling update.

If an invalid config is supplied, the entire config should be considered invalid and rejected. In this case, this would be adding an else to return nil, err

If a partially-invalid config is supplied, do the best you can, and log errors about the parts that didn't work.

I'd most prefer option 1, but given that we're driving this from dynamically-updated configmaps, I think option 2 would be better than option 3, because it makes the application of the ConfigMap much more deterministic (all or nothing, vs all-except-my-typos).

So... @nimakaviani added a validating webhook that allows us to register configmaps for validation and reject them outright if it returns an error. So the right answer here is multi-part:

This should return an error

Each downstream webhook should add this to the list of configmaps to validate.

Then the configmap update will be outright rejected and never saved to etcd.

evankanderson · 2020-02-04T20:57:23Z

-	} else if !apierrors.IsNotFound(err) {
-		logger.With(zap.Error(err)).Fatalf("Error reading ConfigMap %q", logging.ConfigMapName())
-	}
+		// Watch the logging config map and dynamically update logging levels.


Do we want this and the observability configmap to be subject to leader election, or do we want them to apply even when the controller is not leader-elected?

Since these are configured as part of the leader-elected code, I think that there is no notion where these configs apply when the controller is not the leader. If the controller is running and not the leader it is waiting to be the leader. Note, there is an exceptional case where the leader election lock may be lost but the process still running due to deadlock which will be addressed by #1048, but I don't think it changes the equation, and I think it would be fine to have logging and observability config watches stay within the leader-elected section.

These watches are used to report information like Go resource usage and where to write structured logs (which may be written for background work even when not leader elected).

Similarly, I worry that the client version check on line 172 could lead to N leader candidates which will exit as soon as they are elected leader because they don't actually like the version of Kubernetes apiserver that they are attached to.

I think I'd prefer to see the leader election more narrowly wrapped around the controller start on 234-235 (and maybe subsequent).

evankanderson · 2020-02-04T20:58:02Z

+		logger.Info("Starting controllers...")
+		go controller.StartAll(ctx.Done(), controllers...)
+
+		profilingServer := profiling.NewServer(profilingHandler)


Ditto on profiling. Do we want the profiler to only run on the elected leader, or do we want to be able to check profiling on all the participants?

here I think on all

evankanderson · 2020-02-04T20:59:35Z

+
+		// If we have one or more admission controllers, then start the webhook
+		// and pass them in.
+		if len(webhooks) > 0 {


Same question on the webhooks. In particular, one could imagine in leader-election mode having the webhooks run only only on the non-elected nodes, to offload some CPU work from the leader. I could also see just running the webhook on all of them so that the webhook has higher availability.

I expect that webhooks will not be run leader-elected, as they should be horizontally scalable, they are already accessed behind a service, and they do not directly mutate the API server state.

I agree with all your statements about webhooks -- my suggestion was to take this code out from inside run() and put it back in the general shared main.

I think @mattmoor 's "mink" distribution combines the webhooks and controllers into a single deployment, for example. It seems surprising to have leader election control this code.

evankanderson · 2020-02-04T21:00:34Z

+			// Register webhook metrics
+			webhook.RegisterMetrics()
+
+			// possible bug? egCtx


This looks like a new comment. Should @vagababov or @mattmoor comment on whether this ctx is correct?

I'm pretty sure that this was intentional because the ctx here is being passed into work that is enqueued on the errgroup. So it should key off of the SIGTERM not the first failure in the errgroup

I believe it, it just looked a little fishy to me superficially, hence the note to myself.

evankanderson · 2020-02-04T21:03:19Z

+			Callbacks: leaderelection.LeaderCallbacks{
+				OnStartedLeading: run,
+				OnStoppedLeading: func() {
+					logger.Fatalw("leaderelection lost")


If I understand this code correctly, the controller will start up, and try to acquire the leader lock. If it participates in the election and is not elected, it will sit and continue participating in the leader election. If it wins the election, it will serve (run the specified controllers) and will exit if it ever loses an election after that.

Is that correct?

evankanderson · 2020-02-04T21:07:34Z

+	}
+
+	if leaseDurationStr, ok := data["leaseDuration"]; ok {
+		if leaseDuration, err := time.ParseDuration(leaseDurationStr); err == nil {


Three choices here:

If you supply an invalid value, it fails loudly (i.e. logger.Fatal) This is most appropriate for startup conditions, where it might block something like a rolling update.

If an invalid config is supplied, the entire config should be considered invalid and rejected. In this case, this would be adding an else to return nil, err

If a partially-invalid config is supplied, do the best you can, and log errors about the parts that didn't work.

I'd most prefer option 1, but given that we're driving this from dynamically-updated configmaps, I think option 2 would be better than option 3, because it makes the application of the ConfigMap much more deterministic (all or nothing, vs all-except-my-typos).

evankanderson · 2020-02-04T21:08:09Z

+
+	if enabledComponents, ok := data["enabledComponents"]; ok {
+		tokens := strings.Split(enabledComponents, ",")
+		for i, _ := range tokens {


evankanderson · 2020-02-04T21:11:47Z

+		for i, _ := range tokens {
+			str := tokens[i]
+
+			if str == "" {


Do we need this check?

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

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


Can we make this a helper in the leaderelection package? Seems like you use it in serving as well.

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

+	RenewDeadline time.Duration
+	RetryPeriod   time.Duration
+
+	EnabledComponents map[string]bool


use sets.String from apimachinery

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

+
+	if enabledComponents, ok := data["enabledComponents"]; ok {
+		tokens := strings.Split(enabledComponents, ",")
+		for i, _ := range tokens {


If you use sets.String as I suggest below, then the entire loop can be sets.NewString(tokens...)

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

+	ResourceLock  string        `json:"resourceLock"`
+	LeaseDuration time.Duration `json:"leaseDuration"`
+	RenewDeadline time.Duration `json:"renewDeadline"`
+	RetryPeriod   time.Duration `json:"retryPeriod"`


I don't think we need the json encodings here?

correct, i'll remove them

pmorie · 2020-02-04T22:34:23Z

@evankanderson

If I understand this code correctly, the controller will start up, and try to acquire the leader lock. If it participates in the election and is not elected, it will sit and continue participating in the leader election. If it wins the election, it will serve (run the specified controllers) and will exit if it ever loses an election after that.

Is that correct?

yes, if leader election is enabled for the controller by name

vagababov · 2020-02-04T22:30:20Z

 }

+// GetLeaderElectionConfig gets the leader election config.
+func GetLeaderElectionConfig(ctx context.Context) (*kle.Config, error) {


Does this method need to be public?

It is used in controllers that do not use sharedmain, as are other methods from this file that are exported.

vagababov · 2020-02-04T22:36:20Z

+		logger.Info("Starting controllers...")
+		go controller.StartAll(ctx.Done(), controllers...)
+
+		profilingServer := profiling.NewServer(profilingHandler)


here I think on all

vagababov · 2020-02-04T22:39:31Z

+				EventRecorder: recorder,
+			})
+		if err != nil {
+			logger.Fatalw("Error creating lock: %v", err)


Suggested change

logger.Fatalw("Error creating lock: %v", err)

logger.Fatalw("Error creating lock", zap.Error(err))

vagababov · 2020-02-04T22:40:44Z

+	corev1 "k8s.io/api/core/v1"
+)
+
+const ConfigMapNameEnv = "CONFIG_LEADERELECTION_NAME"


probably unexported.

vagababov · 2020-02-04T22:42:14Z

+	return defaultComponentConfig()
+}
+
+func defaultConfig() Config {


Suggested change

func defaultConfig() Config {

func defaultConfig() *Config {

vagababov · 2020-02-04T22:42:34Z

+}
+
+func defaultConfig() Config {
+	return Config{


Suggested change

return Config{

return &Config{

This way you don't have to take address of the returned object every time

pmorie · 2020-02-04T23:27:15Z

I'd most prefer option 1, but given that we're driving this from dynamically-updated configmaps, I think option 2 would be better than option 3, because it makes the application of the ConfigMap much more deterministic (all or nothing, vs all-except-my-typos).

After some thought, I am leaning toward option 1, to error loudly if there is an invalid value and refuse to start. The configmap will be protected by a validating webhook. In most cases this will prevent the configmap from being mutated into an invalid configuration, but it's still possible to get into a state whether the configmap contains invalid data. Since we believe it will be an exceptional condition for this to happen, I think it is okay to refuse to start. If the configmap contains invalid data, and the system is otherwise healthy, the configmap will only ever be able to be updated to a valid state. Similarly, we exit the controller during startup if there's an error parsing the log config.

Note, currently this code does not reload a running process' configuration based on changes to the configmap. I spent a bit of time investigating whether the leader election code would tolerate being reconfigured at runtime (the fields of the config are exported and referenced within the code instead of unpacked from the config). I believe it would be possible to investigate whether this would work, but I do not believe the code is designed for that and it is not tested for it.

I think we have a few options:

Attempt to make the controller reload itself without exiting of the leader election config changes
Make the controller exit if the leader election config changes
Retain the existing behavior (read once and only once)

Many changes of configuration would likely be benign in isolation (ie, no other changes to deployed resources for knative controllers), but some could result in controller downtime or other negative effects (like disabling leader election without changing number of controller replicas).

In general I expect that operators of knative will rarely change this configuration, and in my experience leader elected controllers frequently move between being the leader, to waiting for the lock, and back again. Since that is the case, i'm leaning toward retaining the existing behavior and reading only once OR making the controller exit (since they frequently exit and are restarted by the kubelet anyway due to losing the leader election lock).

pmorie · 2020-02-21T22:53:08Z

/retest

vagababov

Not sure if I left the same comments before, but those are mostly stylistical changes.

vagababov · 2020-02-26T19:28:55Z

+		<-egCtx.Done()
+
+		profilingServer.Shutdown(context.Background())
+		// Don't forward ErrServerClosed as that indicates we're already shutting down.


Nit: comment here doesn't make much sense, since we're not forwarding anything, logging at best.

vagababov · 2020-02-26T19:31:18Z

+	if !leConfig.LeaderElect {
+		logger.Infof("%v will not run in leader-elected mode", component)
+		run(ctx)
+		logger.Fatal("unreachable")


won't this be reachable when <-ctx.Done() triggers?

Yeah, this no longer applies, I don't think. I'll remove it.

vagababov · 2020-02-26T19:32:27Z

+
+	if resourceLock, ok := data["resourceLock"]; ok {
+		if !validResourceLocks.Has(resourceLock) {
+			return nil, fmt.Errorf("resourceLock: invalid value %q: valid values are \"leases\",\"configmaps\",\"endpoints\"", resourceLock)


Suggested change

return nil, fmt.Errorf("resourceLock: invalid value %q: valid values are \"leases\",\"configmaps\",\"endpoints\"", resourceLock)

return nil, fmt.Errorf(`resourceLock: invalid value %q: valid values are "leases", "configmaps", "endpoints"`, resourceLock)

vagababov · 2020-02-26T19:35:42Z

+		EnabledComponents: sets.NewString(),
+	}
+
+	if resourceLock, ok := data["resourceLock"]; ok {


Here and for the checks below, I'd totally support simplifying this to:

if x := data["resourceLock"]; !validResourceLocks(x) { return fmt.Errorf(.... }

"" is not a valid value is just as good as must not be empty, imo. But will shorten the file in half.

vagababov · 2020-02-26T19:37:38Z

+	return defaultComponentConfig()
+}
+
+func defaultConfig() Config {


why wouldn't we return pointer from the getgo?

I think we can actually delete this method entirely now.

knative-metrics-robot · 2020-02-26T21:37:07Z

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

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

vagababov

/lgtm

evankanderson

A few small comments; the only big one is that I still think the logging and metrics setup should be outside the leader election (so that we can monitor non-elected controllers).

evankanderson · 2020-02-26T21:14:26Z

 	profilingServer := profiling.NewServer(profilingHandler)
 	eg, egCtx := errgroup.WithContext(ctx)
 	eg.Go(profilingServer.ListenAndServe)
+	go func() {


Just wondering why this is a goroutine, rather than a defer?

The method won't exit until the server is done or crashes. Is there a downside to using a goroutine here?

It was just a bit unusual, but it looks like the only leader election option is RunOrDie, so this is fine.

evankanderson · 2020-02-26T21:15:43Z

+	run := func(ctx context.Context) {
+		cmw := SetupConfigMapWatchOrDie(ctx, logger)
+		controllers, _ := ControllersAndWebhooksFromCtors(ctx, cmw, ctors...)
+		WatchLoggingConfigOrDie(ctx, cmw, logger, atomicLevel, component)


I'm still wondering why we only want to configure logging and monitoring after being elected leader, rather than applying the logging and monitoring configs continuously.

I was following the pattern in the k8s controller manager, which is to basically do absolutely nothing until you're the leader. I don't see an advantage to watching these configs until you're the leader, but if you want me to change it, I will. Do you want me to change it?

The idea would be to have logs and metrics potentially exporting to a remote destination even for non-leader controllers (so you could count the number of standby working controllers, for example).

But this can be done later, so I'll approve as-is.

evankanderson · 2020-02-26T21:17:39Z

+	}
+
+	RunLeaderElected(ctx, logger, run, component, leConfig)
+	logger.Fatal("unreachable")


Why logger.Fatal here, rather than allowing RunLeaderElected to exit at shutdown if necessary?

evankanderson · 2020-02-26T21:31:12Z

+
+	// Create a unique identifier so that two controllers on the same host don't
+	// race.
+	id, err := kle.UniqueID()


Not for this PR

It seems like we might want to include a metric here indicating the leader-election status.

Can you drop a // TODO: add monitoring for leader election status to this bit of the code (and maybe file a good-first-issue for it)?

evankanderson · 2020-02-26T21:47:58Z

+	}
+
+	if leaseDurationStr, ok := data["leaseDuration"]; ok {
+		if leaseDuration, err := time.ParseDuration(leaseDurationStr); err == nil {


Combining with Victor's comment above, what about:

var err error if config.LeaseDuration, err = time.ParseDuration(data["leaseDuration"]); err != nil { return nil, fmt.Errorf("leaseDuration: %+v", err) }

evankanderson · 2020-02-26T21:52:21Z

+		return &config, nil
+	}
+
+	return NewConfigFromMap(configMap.Data)


It seems like we should pass the config from defaultConfig through NewConfigFromMap to make sure that it meets the expected invariants. WDYT?

evankanderson · 2020-02-26T21:54:57Z

+
+func defaultComponentConfig() ComponentConfig {
+	return ComponentConfig{
+		LeaderElect: false,


This is the default value. I assume you're just trying to make the default very clear?

evankanderson · 2020-02-26T21:57:40Z

+	cm := os.Getenv(ConfigMapNameEnv)
+	if cm == "" {
+		return "config-leader-election"
+	}
+	return cm


Suggested change

cm := os.Getenv(ConfigMapNameEnv)

if cm == "" {

return "config-leader-election"

}

return cm

if cm := os.Getenv(ConfigMapNameEnv); cm != "" {

return cm

}

return "config-leader-election"

evankanderson

/approve

evankanderson · 2020-02-26T22:38:07Z

+	run := func(ctx context.Context) {
+		cmw := SetupConfigMapWatchOrDie(ctx, logger)
+		controllers, _ := ControllersAndWebhooksFromCtors(ctx, cmw, ctors...)
+		WatchLoggingConfigOrDie(ctx, cmw, logger, atomicLevel, component)


The idea would be to have logs and metrics potentially exporting to a remote destination even for non-leader controllers (so you could count the number of standby working controllers, for example).

But this can be done later, so I'll approve as-is.

evankanderson · 2020-02-26T22:38:40Z

 	profilingServer := profiling.NewServer(profilingHandler)
 	eg, egCtx := errgroup.WithContext(ctx)
 	eg.Go(profilingServer.ListenAndServe)
+	go func() {


It was just a bit unusual, but it looks like the only leader election option is RunOrDie, so this is fine.

knative-prow-robot · 2020-02-26T22:39:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, 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~~ [evankanderson]

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

* Add leader election config and to sharedmain * Add new dependencies * Extract method for RunLeaderElected * Make leader election config constructor validate * Rename leader election files * Always start profiling server whether component has LE lock or not * Fix entering unreachable section when leader election is disabled * Address PR feedback

* add leader election support to sharedmain (#1019) * Add leader election config and to sharedmain * Add new dependencies * Extract method for RunLeaderElected * Make leader election config constructor validate * Rename leader election files * Always start profiling server whether component has LE lock or not * Fix entering unreachable section when leader election is disabled * Address PR feedback * Fix missing import

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

mattmoor reviewed Jan 29, 2020

View reviewed changes

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

knative-prow-robot requested review from mattmoor and vaikas January 29, 2020 22:23

pmorie mentioned this pull request Jan 29, 2020

Add leader election knative/serving#6683

Merged

vagababov reviewed Jan 30, 2020

View reviewed changes

knative-prow-robot reviewed Jan 30, 2020

View reviewed changes

skaslev reviewed Jan 30, 2020

View reviewed changes

pmorie force-pushed the leader-election branch from 2ccb64d to 2ff59b4 Compare January 30, 2020 22:17

mattmoor reviewed Jan 30, 2020

View reviewed changes

pmorie force-pushed the leader-election branch from 2ff59b4 to 5dbd8d0 Compare January 30, 2020 22:19

mattmoor reviewed Jan 30, 2020

View reviewed changes

pmorie force-pushed the leader-election branch from 5dbd8d0 to 7c06a49 Compare January 30, 2020 22:25

mattmoor reviewed Jan 30, 2020

View reviewed changes

pmorie force-pushed the leader-election branch 2 times, most recently from f78a2c2 to e28f996 Compare February 4, 2020 19:12

pmorie mentioned this pull request Feb 4, 2020

Add leader election knative/eventing#2501

Merged

vaikas reviewed Feb 4, 2020

View reviewed changes

pmorie force-pushed the leader-election branch from e28f996 to b4201f2 Compare February 4, 2020 21:00

evankanderson reviewed Feb 4, 2020

View reviewed changes

mattmoor reviewed Feb 4, 2020

View reviewed changes

pmorie force-pushed the leader-election branch from b4201f2 to a3b3051 Compare February 4, 2020 22:01

vagababov reviewed Feb 4, 2020

View reviewed changes

knative-prow-robot added the area/test-and-release label Feb 21, 2020

pmorie force-pushed the leader-election branch 3 times, most recently from 98c490c to 0c44ffe Compare February 25, 2020 15:40

pmorie mentioned this pull request Feb 25, 2020

Add support for leader election knative/eventing-contrib#966

Merged

pmorie force-pushed the leader-election branch from 0c44ffe to 18aad98 Compare February 25, 2020 21:09

pmorie added 5 commits February 26, 2020 11:08

Add leader election config and to sharedmain

24e165c

Add new dependencies

3a369ea

Extract method for RunLeaderElected

92ea15b

Make leader election config constructor validate

2c7427c

Rename leader election files

7aba263

pmorie force-pushed the leader-election branch from 18aad98 to 7aba263 Compare February 26, 2020 16:09

pmorie added 2 commits February 26, 2020 12:00

Always start profiling server whether component has LE lock or not

fb698c2

Fix entering unreachable section when leader election is disabled

51e0726

vagababov reviewed Feb 26, 2020

View reviewed changes

Address PR feedback

29186e5

vagababov reviewed Feb 26, 2020

View reviewed changes

knative-prow-robot assigned vagababov Feb 26, 2020

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

evankanderson reviewed Feb 26, 2020

View reviewed changes

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

knative-prow-robot merged commit ca35cb8 into knative:master Feb 26, 2020

pmorie mentioned this pull request Feb 26, 2020

release-0.13: add leader election support to sharedmain (#1019) #1132

Merged

pmorie mentioned this pull request Mar 3, 2020

Add leader election knative/serving-operator#321

Merged

	// create a unique identifier so that two controllers on the same host don't
	// Create a unique identifier so that two controllers on the same host don't

	id = id + "_" + string(uuid.NewUUID())
	id += "_" + string(uuid.NewUUID())

	logger.Fatalw("error creating lock: %v", err)
	logger.Fatalw("Error creating lock: %v", zap.Error(err))

	logger.Fatalw("leaderelection lost")
	logger.Fatal("leaderelection lost")

		@@ -0,0 +1,4 @@
		# See the OWNERS docs at https://go.k8s.io/owners

	logger.Fatalw("Error creating lock: %v", err)
	logger.Fatalw("Error creating lock", zap.Error(err))

	return nil, fmt.Errorf("resourceLock: invalid value %q: valid values are \"leases\",\"configmaps\",\"endpoints\"", resourceLock)
	return nil, fmt.Errorf(`resourceLock: invalid value %q: valid values are "leases", "configmaps", "endpoints"`, resourceLock)

Conversation

pmorie commented Jan 29, 2020

Uh oh!

vagababov 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!

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

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!

Uh oh!

Uh oh!

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!

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!

vaikas 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!

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