Skip to content
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 feature flag to remove use of "INSERT RETURNING" in NewOrderAndAuthzs #7739

Merged
merged 3 commits into from
Oct 4, 2024

Conversation

aarongable
Copy link
Contributor

@aarongable aarongable commented Oct 2, 2024

This is our only use of MariaDB's "INSERT ... RETURNING" syntax, which does not exist in MySQL and Vitess. Add a feature flag which removes our use of this feature, so that we can easily disable it and then re-enable it if it turns out to be too much of a performance hit.

Also add a benchmark showing that the serial-insertion approach is slower, but perhaps not debilitatingly so.

CPS Compliance Review: This feature flag does not risk noncompliance with our CP/CPS.
IN-10737 tracks the corresponding SRE changes to enable this flag.

Part of #7718

@aarongable aarongable requested a review from a team as a code owner October 2, 2024 20:09
@aarongable aarongable requested a review from jsha October 2, 2024 20:09
Copy link
Contributor

github-actions bot commented Oct 2, 2024

@aarongable, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values.

Copy link
Contributor

github-actions bot commented Oct 2, 2024

@aarongable, this PR adds one or more new feature flags: InsertAuthzsIndividually. As such, this PR must be accompanied by a review of the Let's Encrypt CP/CPS to ensure that our behavior both before and after this flag is flipped is compliant with that document.

Please conduct such a review, then add your findings to the PR description in a paragraph beginning with "CPS Compliance Review:".

@aarongable
Copy link
Contributor Author

Benchmarks, where true/false indicates whether the new feature flag is enabled, and the integer represents how many new authzs were being inserted in a single batch. The results show that the flag-true codepath is statistically slower, by about 15% in the most extreme case:

$ docker compose run boulder go test ./sa -bench=BenchmarkNewOrderAndAuthzs
goos: linux
goarch: amd64
pkg: github.com/letsencrypt/boulder/sa
cpu: AMD Ryzen 9 3900X 12-Core Processor            
BenchmarkNewOrderAndAuthzs/false/1-24           1000000000               0.1718 ns/op
BenchmarkNewOrderAndAuthzs/false/2-24           1000000000               0.1712 ns/op
BenchmarkNewOrderAndAuthzs/false/5-24           1000000000               0.1718 ns/op
BenchmarkNewOrderAndAuthzs/false/10-24          1000000000               0.1736 ns/op
BenchmarkNewOrderAndAuthzs/false/20-24          1000000000               0.1766 ns/op
BenchmarkNewOrderAndAuthzs/false/50-24          1000000000               0.1805 ns/op
BenchmarkNewOrderAndAuthzs/false/100-24         1000000000               0.1805 ns/op
BenchmarkNewOrderAndAuthzs/true/1-24            1000000000               0.1763 ns/op
BenchmarkNewOrderAndAuthzs/true/2-24            1000000000               0.1730 ns/op
BenchmarkNewOrderAndAuthzs/true/5-24            1000000000               0.1741 ns/op
BenchmarkNewOrderAndAuthzs/true/10-24           1000000000               0.1809 ns/op
BenchmarkNewOrderAndAuthzs/true/20-24           1000000000               0.1770 ns/op
BenchmarkNewOrderAndAuthzs/true/50-24           1000000000               0.1849 ns/op
BenchmarkNewOrderAndAuthzs/true/100-24          1000000000               0.2063 ns/op
PASS
ok      github.com/letsencrypt/boulder/sa       80.791s

@beautifulentropy
Copy link
Member

Benchmarks, where true/false indicates whether the new feature flag is enabled, and the integer represents how many new authzs were being inserted in a single batch. The results show that the flag-true codepath is statistically slower, by about 15% in the most extreme case:

On my own machine, an Apple M2 Max, I'm seeing ~15% in the most extreme case as well.

Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good. I'm surprised this shows only a 15% slowdown in worst case (100 authzs to create). I suspect that's because the mariadb instance is on the same machine and therefore has very low latency.

I think it's okay to land this as-is because it's behind a feature flag, but I suspect we'll see a much worse worst-case slowdown in realistic situations and wind up deciding to do the insertions somewhat in parallel (less than our current parallelism, because spiky connection counts, but more than 1).

We could also simulate slowdown a little more accurately by using latency distribution stats from prod.

@aarongable aarongable requested review from a team and jprenken and removed request for a team October 4, 2024 00:32
nil,
nil,
})
err = tx.Insert(ctx, am)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One difference between this codepath and the current codepath is that the MultiInserter code below hardcodes nil for four fields: Attempted, AttemptedAt, ValidationRecord, and ValidationError. These fields should never be set in a NewOrderAndAuthzs request from the RA. Is it worth adding some extra lines of code to this new codepath to retain the same defense-in-depth as the old codepath has? Or are we happy abandoning that teensy bit of extra safety?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like either way is OK; defense in depth is great and I wouldn't say no to it, but this bad case feels impossible.

@aarongable aarongable merged commit 7b032a6 into main Oct 4, 2024
20 checks passed
@aarongable aarongable deleted the authz-multi-flag branch October 4, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants