Add SQL election module #3318

evankanderson · 2024-01-27T00:13:31Z

Adds a generic SQL leader election module, as discussed in #3219; it sounds like Sigstore is looking at possibly using this for simplicity.

Fixes #3219

Note that I haven't yet wired this up into Trillian nor added database table declarations -- I'm happy to do that in this PR or a follow-up. I'm not including this in the changelog until it's wired in, but I wanted to get feedback on how to do that first.

Checklist

I have updated the CHANGELOG.
I have updated documentation accordingly (including the feature implementation matrix).

util/election2/sql/election.go

JAORMX · 2024-01-27T06:42:05Z

util/election2/sql/election.go

+	}
+	defer tx.Rollback()
+	row := tx.QueryRow(
+		"SELECT leader, last_update FROM leader_election WHERE resource_id = $1",


Wouldn't the $<num> format be problematic in mysql?

Yep, switched to ?, which probably makes postgres-likes unhappy, but since sqlite is happy with both and there's already a MySQL backend for trillian, that makes sense.

JAORMX · 2024-01-27T06:45:25Z

util/election2/sql/election.go

+}
+
+func (e *Election) initializeLock(ctx context.Context) error {
+	insert, err := e.db.Prepare("INSERT INTO leader_election (resource_id, leader, last_update) VALUES ($1, $2, $3)")


Wouldn't we need to handle the case where more than one replica executes an initialization at the same time? Or is the idea is to crash loop, restart and continue?

Thanks for the reminder to actually handle the error in NewElection. The idea is that leader_election has a primary-key index on resource_id, so attempting a second INSERT with the same resource_id will simply fail with a conflict. But we should handle other SQL errors if there's a good way to do so.

Note that we also don't have a good way to clean up resource_id rows after they've been created... that's probably not a problem, as resource_ids are probably pretty stable, but still...

And, fixing the lint errors and adding a test to cover this case, I realize that "INSERT had a conflict" is not a standardized error, so I need to do the query first to look for sql.ErrNoRows, which is standardized. ¯_(ツ)_/¯

roger2hk · 2024-01-28T09:05:51Z

/gcbrun

JAORMX · 2024-01-29T06:49:29Z

/gcbrun

evankanderson · 2024-01-29T21:45:06Z

I fixed the remaining lint issue and added what I think is the right integration hooks. I'm still working on running the integration tests -- in particular, adding a test that uses the --election_backend=mysql option.

evankanderson · 2024-01-29T21:45:21Z

/gcbrun

mhutchinson · 2024-01-30T09:41:26Z

cmd/trillian_log_signer/main.go

@@ -47,13 +47,15 @@ import (
 	"github.com/google/trillian/util/election"
 	"github.com/google/trillian/util/election2"
 	etcdelect "github.com/google/trillian/util/election2/etcd"
+	"github.com/google/trillian/util/election2/sql"


This comment relates to the package of the new code. I recommend mysql over sql to be clear at which implementation it is targeting, and for consistency with the flag names used in this file to pick it.

JAORMX

LGTM

JAORMX · 2024-01-30T17:57:37Z

Ah! Linter found something

JAORMX · 2024-01-30T17:58:03Z

/gcbrun

phbnf · 2024-01-30T13:37:11Z

cmd/trillian_log_signer/main.go

@@ -71,6 +73,7 @@ var (
 	numSeqFlag               = flag.Int("num_sequencers", 10, "Number of sequencer workers to run in parallel")
 	sequencerGuardWindowFlag = flag.Duration("sequencer_guard_window", 0, "If set, the time elapsed before submitted leaves are eligible for sequencing")
 	forceMaster              = flag.Bool("force_master", false, "If true, assume master for all logs")
+	electionBackend          = flag.String("election_backend", "etcd", fmt.Sprintf("Election backend to use. One of: mysql, etcd, noop"))


nit: rename to electionSystem to match with quotaSystem

phbnf · 2024-01-30T18:51:25Z

cmd/trillian_log_signer/main.go

@@ -143,13 +146,22 @@ func main() {
 	instanceID := fmt.Sprintf("%s.%d", hostname, os.Getpid())
 	var electionFactory election2.Factory
 	switch {
-	case *forceMaster:
+	case *forceMaster || *electionBackend == "noop":


It's not fantastic that we can now control this behavior with two different flags, but I also cannot thing of a better way forward for now. On the long run, we could / should probably deprecate forceMaster. No action required!

phbnf · 2024-01-30T18:56:41Z

util/election2/mysql/election.go

+	currentLeader leaderData
+	leaderLock    sync.Cond
+
+	// If a channel is supplied with the cancel, it will be signalled when the election routine has exited.


In practice, cancel is always created by Await, and Resign always sends a message there? I'm not sure I understand what's conditional.

I wrote this before realizing that I could re-use Resign for Close, and I think one of those did not wait for the error in the first implementation. I can remove the comment, and change the type from *chan error to chan error probably...

Gotcha - makes sense to me to remove the comment then yes!

phbnf · 2024-01-30T19:12:21Z

util/election2/mysql/election.go

+}
+
+// Close implements election2.Election
+func (e *Election) Close(ctx context.Context) error {


Given this is essentially a wrapper around Resign, this election object could be re-used for a new election, which is slightly at odds with the interface definition. In practice, I don't think anything relies on this behaviour, and there aren't any resources that can be released, so I think it's safe. Does that match your understanding?

Yes.

Do you want me to make the object unusable, to catch errors that might occur by calling other methods after Close?

If you can think of a way of an elegant way of doing this, sure thing. I don't think it's super super important, it's just some extra care to make sure it's aligned with the interface definition :)

roger2hk · 2024-02-02T00:20:36Z

util/election2/mysql/election.go

+// WithMastership implements election2.Election
+func (e *Election) WithMastership(ctx context.Context) (context.Context, error) {


Having the inclusivity and diversity in mind, may I suggest replacing master with another inclusive term?

https://developers.google.com/style/word-list#master

I can rewrite the election2.Election interface if you'd like. 😁

Otherwise, I think those are the only two instances of the word "master" rather than "leader".

Add SQL election module

219fc8d

evankanderson requested a review from a team as a code owner January 27, 2024 00:13

evankanderson requested a review from phbnf January 27, 2024 00:13

JAORMX reviewed Jan 27, 2024

View reviewed changes

JAORMX requested review from roger2hk and AlCutter January 28, 2024 08:58

Fix lint errors, address JAORMX comments

5f4b850

evankanderson added 2 commits January 29, 2024 12:13

Fix errcheck lint warning in test

4d31437

Add DB initialization script and integrate with log_signer

c2b24eb

mhutchinson reviewed Jan 30, 2024

View reviewed changes

Update to use mysql module, fix factory invocation

05b113d

JAORMX approved these changes Jan 30, 2024

View reviewed changes

phbnf reviewed Jan 30, 2024

View reviewed changes

roger2hk reviewed Feb 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SQL election module #3318

Add SQL election module #3318

evankanderson commented Jan 27, 2024

JAORMX Jan 27, 2024

evankanderson Jan 29, 2024

JAORMX Jan 27, 2024

evankanderson Jan 29, 2024

evankanderson Jan 29, 2024

roger2hk commented Jan 28, 2024

JAORMX commented Jan 29, 2024

evankanderson commented Jan 29, 2024

evankanderson commented Jan 29, 2024

mhutchinson Jan 30, 2024

JAORMX left a comment

JAORMX commented Jan 30, 2024

JAORMX commented Jan 30, 2024

phbnf Jan 30, 2024

phbnf Jan 30, 2024

phbnf Jan 30, 2024

evankanderson Feb 2, 2024

phbnf Feb 5, 2024

phbnf Jan 30, 2024

evankanderson Feb 2, 2024

phbnf Feb 5, 2024

roger2hk Feb 2, 2024

evankanderson Feb 2, 2024

		// WithMastership implements election2.Election
		func (e *Election) WithMastership(ctx context.Context) (context.Context, error) {

Add SQL election module #3318

Are you sure you want to change the base?

Add SQL election module #3318

Conversation

evankanderson commented Jan 27, 2024

Checklist

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

roger2hk commented Jan 28, 2024

JAORMX commented Jan 29, 2024

evankanderson commented Jan 29, 2024

evankanderson commented Jan 29, 2024

Choose a reason for hiding this comment

JAORMX left a comment

Choose a reason for hiding this comment

JAORMX commented Jan 30, 2024

JAORMX commented Jan 30, 2024

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

Choose a reason for hiding this comment