Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for parallel loading of ssl_multicert.yaml certificates, controlled by a new records.config setting, to speed up certificate (re)configuration in ATS’s TLS / QUIC certificate loaders.
Changes:
- Introduces
proxy.config.ssl.server.multicert.concurrency(records + docs) and threads the value throughSSLConfigParams. - Updates
SSLMultiCertConfigLoaderto optionally load multicert items in parallel usingstd::thread, with locking for sharedSSLCertLookupmutations. - Extends the gold test to cover the new parallel-loading behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gold_tests/tls/ssl_multicert_loader.test.py | Adds a new gold test section intended to validate parallel loading behavior. |
| src/records/RecordsConfig.cc | Registers the new proxy.config.ssl.server.multicert.concurrency record. |
| src/iocore/net/SSLUtils.cc | Implements the parallel multicert loading logic and adds locking around shared lookup mutation. |
| src/iocore/net/SSLConfig.cc | Reads the new concurrency record into SSLConfigParams and passes “first load” state into the loader. |
| src/iocore/net/QUICMultiCertConfigLoader.cc | Updates QUIC cert reload path to pass “first load” state into the loader. |
| src/iocore/net/P_SSLConfig.h | Adds configLoadConcurrency to SSLConfigParams. |
| include/iocore/net/SSLMultiCertConfigLoader.h | Updates loader API to accept firstLoad and adds _load_items() + mutex. |
| doc/admin-guide/files/records.yaml.en.rst | Documents the new configuration record and its behavior. |
zwoop
left a comment
zwoop
left a comment
There was a problem hiding this comment.
This generally looks good now, couple of nitpicks, at your discretion to address or not.
Change to just do a clamp as well if firstLoad
bryancall
left a comment
bryancall
left a comment
There was a problem hiding this comment.
Looks good overall, thanks for the cleanup and follow-up fixes.
One test improvement suggestion: the new parallel-load test currently checks for 'loaded N certs', which can pass in both threaded and single-threaded paths. To validate the new behavior more directly, consider asserting the reload-path log line with a fixed configured concurrency (for example, set multicert concurrency to 2, run traffic_ctl config reload, and check for 'loading N certs with 2 threads' and 'loaded N certs in 2 threads').
* Resurrecting #7877 Parallel ssl cert loading Added updates, with tests, logging, settings and a benchmark script which shows 2x improvement on load times for certs (on my macbook) Removing the -1, 0 will autoselect, default is now 1 for old behavior * Add license to benchmark --------- Co-authored-by: Evan Zelkowitz <e_zelkowitz@apple.com>
|
Added to 10.2.x via #13043 |
Adds a new config proxy.config.ssl.server.multicert.concurrency
Defaults to 1 for single core usage, however even with that on first load it will use all cores for cert loading. After that it will use the specified value 0(auto)/1(default)/N(number of threads) when doing reloads of certs