Factor out election Runner #1177

pav-kv · 2018-05-31T13:25:42Z

Distilled version of #1144.

daviddrysdale · 2018-05-31T13:44:57Z

This would be a lot easier to review if it didn't mix together factoring out electionRunner with a bunch of behavioural changes -- is it possible to split things up?

codecov-io · 2018-05-31T17:23:22Z

Codecov Report

Merging #1177 into master will increase coverage by 0.18%.
The diff coverage is 88.37%.

@@            Coverage Diff             @@
##           master    #1177      +/-   ##
==========================================
+ Coverage   58.34%   58.52%   +0.18%     
==========================================
  Files         109      110       +1     
  Lines        9424     9415       -9     
==========================================
+ Hits         5498     5510      +12     
+ Misses       3363     3344      -19     
+ Partials      563      561       -2

Impacted Files	Coverage Δ
server/log_operation_manager.go	`76.16% <86.79%> (+5.46%)`	⬆️
util/election/runner.go	`90.9% <90.9%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b736197...f53a5ab. Read the comment docs.

pav-kv

Removed the new TTL behavior, and tried to make the resigning delay behavior somewhat compatible with the old one. Do you think there are other behavioral changes I should remove from the PR?

pav-kv · 2018-06-01T09:56:30Z

server/trillian_log_signer/main.go

+			MasterCheckInterval: *masterCheckInterval,
+		},
+		MasterHoldInterval: *masterHoldInterval,
+		// TODO(pavelkalinnikov): ResignOdds initially set a geometric


Leaving the old flag name for compatibility reasons. Could change it in this PR though.

Yes, I'd leave it for now and we can do smaller PRs to migrate it.

pav-kv · 2018-06-01T09:56:45Z

server/log_operation_manager.go

-		glog.Errorf("%d: failed to resign mastership: %v", r.er.logID, err)
+// getResignDelay computes master resignation delay based on operation
+// parameters.
+func getResignDelay(info *LogOperationInfo) time.Duration {


This is not exactly equivalent to the previous ResignOdds behavior, although could be simpler?

The old one was a geometric distribution: the probability of resigning on k-th master check iteration was (1-p)^(k-1) * p, where p=1/ResignOdds. Expected number of iterations was ResignOdds. So the code here could look like:

p := 1.0 / float64(info.ResignOdds) x := rand.Float64() checks := int64(math.Log2(1.0-x) / math.Log2(1.0-p)) return delay + time.Duration(checks)*info.ElectionConfig.MasterCheckInterval

The new one is just uniform with expected number of extra waiting iterations equal to ResignSpread/2 (could make it ResignSpread to be even more compatible with the old behaviour).

I don't think the exact behaviour is important at this level. We want to be able to transfer mastership to allow things to balance out across multiple tasks but not too often and (probabilistically likely) not for all the logs at the same time.

pav-kv · 2018-06-01T10:09:50Z

server/log_operation_manager.go

+				delay := getResignDelay(&l.info)
+				// Note: The loop is to allow blocking by a mocked TimeSource.
+				for until := start.Add(delay); l.info.TimeSource.Now().Before(until); {
+					if !util.Sleep(run.Done, delay) {


Should be util.SleepContext(run.Ctx, delay) != nil.

pav-kv · 2018-06-01T10:18:00Z

util/election/runner.go

+				glog.Warningf("%sIsMaster: false", r.prefix)
+				return
+			} else if !isMaster {
+				glog.Warningf("%sIsMaster: %v", r.prefix, err)


This should be swapped with line 130.

pav-kv · 2018-06-01T11:32:20Z

To give some more context on why I'm doing this, here is how I'm going to use Runner in CT's Migrillian: ct-go#264, see RunWithElection method.

Martin2112 · 2020-11-02T14:08:18Z

server/log_operation_manager.go

-		glog.Errorf("%d: failed to resign mastership: %v", r.er.logID, err)
+// getResignDelay computes master resignation delay based on operation
+// parameters.
+func getResignDelay(info *LogOperationInfo) time.Duration {


I don't think the exact behaviour is important at this level. We want to be able to transfer mastership to allow things to balance out across multiple tasks but not too often and (probabilistically likely) not for all the logs at the same time.

Martin2112 · 2020-11-02T14:10:54Z

server/log_operation_manager.go

-		glog.Errorf("%d: failed to resign mastership: %v", r.er.logID, err)
+// getResignDelay computes master resignation delay based on operation
+// parameters.
+func getResignDelay(info *LogOperationInfo) time.Duration {


Can you rename this to something not starting with 'get', just resignDelay for example.

Martin2112 · 2020-11-02T14:14:35Z

server/log_operation_manager.go

 func fixupElectionInfo(info LogOperationInfo) LogOperationInfo {
-	if info.PreElectionPause < minPreElectionPause {
-		info.PreElectionPause = minPreElectionPause
+	cfg := &info.ElectionConfig


If the other two parameters (hold interval and spread) were moved into ElectionConfig then this could just be passed an election config.

I haven't fully examined the impact of this change though.

Martin2112 · 2020-11-02T14:16:51Z

server/log_operation_manager.go

+		runner := election.NewRunner(&l.info.ElectionConfig, el, l.info.TimeSource, label+": ")
+		l.electionRunner[logID] = runner
+
+		//   beMaster := func(ctx context.Context) error {


Is this meant to be here? Duplicates one in runner.go. If intended as documentation it should maybe be moved outside to the function doc.

Martin2112 · 2020-11-02T14:20:14Z

server/trillian_log_signer/main.go

+			MasterCheckInterval: *masterCheckInterval,
+		},
+		MasterHoldInterval: *masterHoldInterval,
+		// TODO(pavelkalinnikov): ResignOdds initially set a geometric


Yes, I'd leave it for now and we can do smaller PRs to migrate it.

Martin2112 · 2020-11-02T14:22:56Z

util/election/runner.go

+	return r.me.Resign(ctx)
+}
+
+// Close permanently stops this Runner from participating in election.


Nit: should probably be "elections".

Martin2112 · 2020-11-02T14:23:46Z

util/election/runner_test.go

+// expectNotClosed checks that done is not closed within some timeout.
+func expectNotClosed(t *testing.T, done <-chan struct{}, msg string) {
+	t.Helper()
+	if !util.Sleep(done, decentDur) {


This sort of construct can be flaky but maybe can't be avoided here?

pav-kv · 2020-11-02T14:38:45Z

Oh, don't look at this. I was just backing up my local branches :)

Martin2112 · 2020-11-02T14:44:57Z

OK, didn't look too closely and reviewed it.

pphaneuf · 2021-01-15T11:18:57Z

There's a "convert to draft" feature now!

Factor out election Runner

42c3b65

googlebot added the cla: yes label May 31, 2018

pav-kv requested a review from daviddrysdale May 31, 2018 13:32

pav-kv added 2 commits May 31, 2018 17:54

Make linter happy

15fa50f

Remove TTL code for later PR

027aa19

Bring resign_odds back

6f3680b

pav-kv commented Jun 1, 2018

View reviewed changes

Minor fixes

f53a5ab

pav-kv mentioned this pull request Jun 5, 2018

Support watch-based master election implementations #1191

Closed

daviddrysdale mentioned this pull request Jun 7, 2018

[For discussion] Factor out election.Runner #1199

Merged

tmp

6257c7e

RJPercival assigned pav-kv Sep 3, 2018

daviddrysdale removed their request for review May 30, 2019 09:03

pav-kv requested a review from a team as a code owner November 2, 2020 13:37

pav-kv requested a review from Martin2112 November 2, 2020 13:37

Martin2112 reviewed Nov 2, 2020

View reviewed changes

pav-kv marked this pull request as draft January 15, 2021 11:28

pav-kv closed this Jan 22, 2021

pav-kv deleted the election_runner branch January 22, 2021 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out election Runner #1177

Factor out election Runner #1177

pav-kv commented May 31, 2018 •

edited

Loading

daviddrysdale commented May 31, 2018

codecov-io commented May 31, 2018 •

edited

Loading

pav-kv left a comment

pav-kv Jun 1, 2018

Martin2112 Nov 2, 2020

pav-kv Jun 1, 2018

Martin2112 Nov 2, 2020

pav-kv Jun 1, 2018

pav-kv Jun 1, 2018

pav-kv Jun 1, 2018

pav-kv Jun 1, 2018

pav-kv commented Jun 1, 2018

Martin2112 Nov 2, 2020

Martin2112 Nov 2, 2020

Martin2112 Nov 2, 2020

Martin2112 Nov 2, 2020

Martin2112 Nov 2, 2020

Martin2112 Nov 2, 2020

Martin2112 Nov 2, 2020

pav-kv commented Nov 2, 2020

Martin2112 commented Nov 2, 2020

pphaneuf commented Jan 15, 2021

Factor out election Runner #1177

Factor out election Runner #1177

Conversation

pav-kv commented May 31, 2018 • edited Loading

daviddrysdale commented May 31, 2018

codecov-io commented May 31, 2018 • edited Loading

Codecov Report

pav-kv 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

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

pav-kv commented Jun 1, 2018

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

pav-kv commented Nov 2, 2020

Martin2112 commented Nov 2, 2020

pphaneuf commented Jan 15, 2021

pav-kv commented May 31, 2018 •

edited

Loading

codecov-io commented May 31, 2018 •

edited

Loading