-
-
Notifications
You must be signed in to change notification settings - Fork 603
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
Add stagger to CT log submissions. #3794
Conversation
This allows each log a chance to respond before we move onto the next, spreading our load more evenly across the logs in a log group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small nit to request fixed. This is a small PR but I would feel better if there was a unit test as well if that's doable within the constraints at hand. It seems like setting a really high Stagger for one of two logs in a test configuration and then asserting that only the first log had a submission before timing out a shorter context would be sufficient.
ctpolicy/ctpolicy.go
Outdated
@@ -79,7 +85,15 @@ func (ctp *CTPolicy) race(ctx context.Context, cert core.CertDER, group cmd.CTGr | |||
// so we maximize the distribution of logs we get SCTs from. | |||
for _, i := range rand.Perm(len(group.Logs)) { | |||
l := group.Logs[i] | |||
go func(l cmd.LogDescription) { | |||
go func(i int, l cmd.LogDescription) { | |||
// Each submission waits 500ms longer than the previous one, to give the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the 500ms part of this comment isn't necessarily accurate since group.Stagger.Duration
is configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the test. 👍
ctpolicy/ctpolicy_test.go
Outdated
ctp := New(countingPub, []cmd.CTGroup{ | ||
{ | ||
Name: "a", | ||
Stagger: cmd.ConfigDuration{500 * time.Millisecond}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go vet is flagging this in CI:
# github.com/letsencrypt/boulder/ctpolicy
ctpolicy/ctpolicy_test.go:186: github.com/letsencrypt/boulder/cmd.ConfigDuration composite literal uses unkeyed fields
Merging with one review based on dev availability. |
This allows each log a chance to respond before we move onto the next,
spreading our load more evenly across the logs in a log group.