-
Notifications
You must be signed in to change notification settings - Fork 69
Parallel signing #1468
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
Parallel signing #1468
Conversation
This is (especially) useful with rekor 2 where the service only responds after log inclusion: We would prefer to get all signatures in the same inclusion batch. The change still affects both rekor v1 and rekor v2. This commit is a rebase of a bunch of Ramons commits. Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com> Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* Session thread safety is ambiguous: it may now be safe but situation is unclear * Use a single Session per create_entry() call: This may have a little more overhead but seems like the safest option * Note that RekorLog (v1) other methods may not be thread safe: I only reviewed the create_entry() flow Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
It's a little unclear if Session is now thread safe or not: avoid reuse just in case Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* Separate the thread method so it's easier to see potential safety issues
* Using print() with the same file object is generally not thread safe: Avoid
it from the threaded method
The output remains effectively the same except:
* The b64 encoded signature is no longer printed to terminal
* Some print()s are now logger.info(): e.g.
Transparency log entry created at index: 5562
* Other print()s happen in a batch now, after he signing has finished
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
|
The included test only signs a few artifacts at a time but I have run this on more pathological cases: Runtime on current main branch
Runtime on parallel-sign branch
The time differences here are not that big but I think that's mostly because staging rekor 2 is currently configured with small and quick batches: With longer batching times the rekor2 difference will only grow |
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Make every RekorLog have a Session of their own by default. This means RekorClient no longer needs to manage that. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Colleen confirmed this for me: tessera checkpoint interval is currently configured at 2 seconds in staging, this might get bumped up to 10 seconds The effect of this with the 25 artifact rekor2 test (with the current 10 thread limit) is :
The timings for this branch depend on the max number of threads: current 10 threads means 25 artifacts splits to 3 checkpoints. The takeaways to me are:
|
woodruffw
left a comment
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 @jku! I left two small comments/nitpicks 🙂
This number does affect the number of concurrent rekor POST requests we have in flight, but we are unlikely to hit rate limits as they are defined in "requests from same host per minute". Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
woodruffw
left a comment
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.
LGTM, great work @jku!
One thing I was thinking about it whether the increased latency with Rekor v2 means that it might make sense to show a progress bar while signing (i.e. when we can detect an interactive user terminal). Do you have thoughts on that? Not a blocker here either way.
I'd like it... but I don't think it's possible to implement one that is useful: for the typical case (signing less than
|
This reverts commit 3a26b7d. The windows tests started to fail with this one: Reverting until there is a fix Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* Sign multiple artifacts in threads
This is (especially) useful with rekor 2 where the service only
responds after log inclusion: We would prefer to get all signatures
in the same inclusion batch.
The change still affects both rekor v1 and rekor v2.
This commit is a rebase of a bunch of Ramons commits.
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* rekor: Make create_entry() conservative WRT session use
* Session thread safety is ambiguous: it may now be safe but situation
is unclear
* Use a single Session per create_entry() call: This may have a little
more overhead but seems like the safest option
* Note that RekorLog (v1) other methods may not be thread safe: I only
reviewed the create_entry() flow
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* timestamp: Dont reuse session
It's a little unclear if Session is now thread safe or not: avoid reuse
just in case
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* CLI: Refactor terminal output
* Separate the thread method so it's easier to see potential safety issues
* Using print() with the same file object is generally not thread safe: Avoid
it from the threaded method
The output remains effectively the same except:
* The b64 encoded signature is no longer printed to terminal
* Some print()s are now logger.info(): e.g.
Transparency log entry created at index: 5562
* Other print()s happen in a batch now, after he signing has finished
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* tests: Test signing multiple artifacts
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* Add test that signs multiple artifacts with rekor2
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* tests: lint fixes
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* rekor: Refactor Session handling in RekorClient
Make every RekorLog have a Session of their own by default.
This means RekorClient no longer needs to manage that.
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* cli: Let Python pick number of signing threads
This number does affect the number of concurrent rekor POST requests
we have in flight, but we are unlikely to hit rate limits as
they are defined in "requests from same host per minute".
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
---------
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Co-authored-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
This uses threads to sign multiple artifacts in parallel. The issue that is being solved here is that rekor v2 only responds to the entry request after the entry has been included in the log and the inclusion proof is available. When entry requests are done in parallel, they all get responses in the next "inclusion batch" (instead of one response per batch), speeding up multi-artifact signing by a lot. A practical example of the use case could be cpython releases where they sign ~10 artifacts.
There are multiple changes:
print()in threaded methodrequestswithurllib3#1040 we can likely get rid of this.Signer.sign_artifact()andSigner.sign_dsse()are now expected to be thread safe.