New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce one trillian Map caller at a time with sequencer election #1009

Merged
merged 15 commits into from Dec 5, 2018

Conversation

Projects
5 participants
@gdbelvin
Collaborator

gdbelvin commented Jul 5, 2018

Ensure that there is exactly one KT Sequencer running for each directory.

  1. Each sequencer instance polls the domains database periodically, looking for new domains.
  2. When new domains are found, an election object is created for them.
  3. On a separate thread, (and much more frequently), the sequencer asks what directories it is currently master for and attempts to do a sequencing run on them.

Fixes #565

Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
@codecov

This comment has been minimized.

codecov bot commented Jul 5, 2018

Codecov Report

Merging #1009 into master will decrease coverage by 2.12%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1009      +/-   ##
==========================================
- Coverage   66.82%   64.69%   -2.13%     
==========================================
  Files          40       41       +1     
  Lines        3008     3107      +99     
==========================================
  Hits         2010     2010              
- Misses        658      757      +99     
  Partials      340      340
Impacted Files Coverage Δ
core/sequencer/sequencer.go 20.93% <0%> (-8.11%) ⬇️
core/sequencer/election/tracker.go 0% <0%> (ø)

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 3fa5d84...d2a9c39. Read the comment docs.

@gdbelvin gdbelvin requested a review from pavelkalinnikov Jul 6, 2018

@pavelkalinnikov

Flushing some initial comments.

Show resolved Hide resolved cmd/keytransparency-sequencer/main.go Outdated
Show resolved Hide resolved cmd/keytransparency-sequencer/main.go Outdated
Show resolved Hide resolved cmd/keytransparency-sequencer/main.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
@Martin2112

Please also address the comments that Pavel made.

Show resolved Hide resolved cmd/keytransparency-sequencer/main.go Outdated
Show resolved Hide resolved cmd/keytransparency-sequencer/main.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
Show resolved Hide resolved impl/integration/env.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated

@gdbelvin gdbelvin force-pushed the gdbelvin:f/election branch from e1988ca to 064f6f8 Jul 18, 2018

@pavelkalinnikov

Flushing some more comments.

Show resolved Hide resolved cmd/keytransparency-sequencer/main.go Outdated
Show resolved Hide resolved cmd/keytransparency-sequencer/main.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
@pavelkalinnikov

Commented on the design questions.

Show resolved Hide resolved core/sequencer/election.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go Outdated

@gdbelvin gdbelvin added this to In progress in Retryable Sequencer Nov 26, 2018

@gdbelvin gdbelvin force-pushed the gdbelvin:f/election branch from 546994a to dd06ec7 Nov 30, 2018

const resourceLabel = "resource"
var (

This comment has been minimized.

@golangcibot

golangcibot Nov 30, 2018

once is a global variable (from gochecknoglobals)

Show resolved Hide resolved core/sequencer/tracker/tracker.go Outdated

@gdbelvin gdbelvin force-pushed the gdbelvin:f/election branch from dd06ec7 to b0d01d0 Nov 30, 2018

@gdbelvin gdbelvin added for review and removed do not merge labels Nov 30, 2018

@gdbelvin

This comment has been minimized.

Collaborator

gdbelvin commented Nov 30, 2018

Updated with completely new mastership tracker

@gdbelvin gdbelvin moved this from In progress to Needs review in Retryable Sequencer Nov 30, 2018

@pavelkalinnikov

Mostly nits. Please update the RP description (e.g. domain -> directory).

Show resolved Hide resolved core/sequencer/tracker/tracker.go Outdated
Show resolved Hide resolved core/sequencer/tracker/tracker.go Outdated
Show resolved Hide resolved core/sequencer/tracker/tracker.go Outdated
Show resolved Hide resolved core/sequencer/tracker/tracker.go Outdated
Show resolved Hide resolved core/sequencer/tracker/tracker.go Outdated
Show resolved Hide resolved core/sequencer/tracker/tracker.go Outdated
Show resolved Hide resolved core/sequencer/tracker/tracker.go Outdated
Show resolved Hide resolved core/sequencer/tracker/tracker.go Outdated
Show resolved Hide resolved core/sequencer/tracker/tracker.go Outdated
Show resolved Hide resolved core/sequencer/tracker/tracker.go Outdated

gdbelvin added some commits Nov 30, 2018

@gdbelvin gdbelvin force-pushed the gdbelvin:f/election branch from 4a10e62 to d355e45 Dec 4, 2018

@gdbelvin gdbelvin force-pushed the gdbelvin:f/election branch from 6d58d10 to 08134fc Dec 5, 2018

Merge branch 'master' into f/election
* master:
  Fix deployment scripts (#1143)
@Martin2112

This comment has been minimized.

Martin2112 commented Dec 5, 2018

Approving subject to other reviews being happy.

Show resolved Hide resolved core/sequencer/election/tracker.go Outdated
Show resolved Hide resolved core/sequencer/sequencer.go
Show resolved Hide resolved core/sequencer/election/tracker.go Outdated
Show resolved Hide resolved core/sequencer/election/tracker.go Outdated
Show resolved Hide resolved core/sequencer/election/tracker.go
Show resolved Hide resolved core/sequencer/election/tracker.go

gdbelvin added some commits Dec 5, 2018

const resourceLabel = "resource"
var (

This comment has been minimized.

@golangcibot

golangcibot Dec 5, 2018

once is a global variable (from gochecknoglobals)

@gdbelvin

This comment has been minimized.

Collaborator

gdbelvin commented Dec 5, 2018

PTAL

Retryable Sequencer automation moved this from Needs review to Reviewer approved Dec 5, 2018

@pavelkalinnikov

LGTM

Show resolved Hide resolved core/sequencer/election/tracker.go
return err
}
defer func() {
if err := e.Close(ctx); err != nil {

This comment has been minimized.

@pavelkalinnikov

pavelkalinnikov Dec 5, 2018

Is there a condition under which this call can race with WithMastership in line 194? Looks okay to me, as watchOnce returns errors only when the election object is not visible to line 194, so should be no race.

This comment has been minimized.

@pavelkalinnikov

pavelkalinnikov Dec 5, 2018

Hm, unless the loop 107 stops because ctx is canceled...

This comment has been minimized.

@gdbelvin

gdbelvin Dec 5, 2018

Collaborator

defer mt.setNotMaster() in watchOnce will always be called before e.Close().

const resourceLabel = "resource"
var (

This comment has been minimized.

@golangcibot

golangcibot Dec 5, 2018

once is a global variable (from gochecknoglobals)

@gdbelvin gdbelvin merged commit dc66c58 into google:master Dec 5, 2018

2 of 5 checks passed

GolangCI 1 issue found
Details
codecov/patch 0% of diff hit (target 66.82%)
Details
codecov/project 64.69% (-2.13%) compared to 3fa5d84
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Retryable Sequencer automation moved this from Reviewer approved to Done Dec 5, 2018

@gdbelvin gdbelvin deleted the gdbelvin:f/election branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment