fix: Improve Azure Key Vault bulk load performance - #1218
Conversation
Signed-off-by: Mael Regnery <mael@mqli.fr>
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 6ff268e. Configure here.
| recordFailure(e.getClass().getSimpleName(), nameOf.apply(item), e); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Deadline does not bound load
Medium Severity
--azure-bulk-load-timeout is documented as an overall time budget, but the deadline is only checked before limiter.acquire(). That acquire can block past the deadline with no timeout, work is still submitted afterward, and after abandoning dispatch ExecutorService.close() waits for in-flight tasks with no deadline-aware bound or cancellation. Startup can therefore continue well beyond the configured budget whenever Azure requests are slow or saturated.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6ff268e. Configure here.
There was a problem hiding this comment.
Pull request overview
Improves Azure Key Vault bulk-loading performance through bounded concurrent processing and configurable timeouts.
Changes:
- Adds concurrent, deadline-aware bulk-loading infrastructure.
- Integrates concurrency settings into Azure loading paths and CLI options.
- Adds loader and Azure mapping tests plus release documentation.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Documents the performance improvement and options. |
commandline/build.gradle |
Adds the keystorage dependency. |
commandline/.../PicoCliAzureKeyVaultParameters.java |
Defines Azure bulk-load CLI options. |
core/.../Eth1Runner.java |
Logs configured Azure concurrency. |
core/.../Eth2Runner.java |
Passes and logs bulk-load options. |
keystorage/.../AzureKeyVault.java |
Uses concurrent loading for keys and secrets. |
keystorage/.../BulkLoadOptions.java |
Defines concurrency and deadline settings. |
keystorage/.../ConcurrentBulkLoader.java |
Implements bounded concurrent loading. |
keystorage/.../AzureKeyVaultTest.java |
Updates Azure mapping tests. |
keystorage/.../ConcurrentBulkLoaderTest.java |
Tests concurrency, failures, deadlines, and interrupts. |
signing/.../SecpAzureBulkLoader.java |
Passes bulk-load options to key mapping. |
signing/.../AzureKeyVaultParameters.java |
Exposes default bulk-load configuration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (loggedFailures.size() < MAX_LOGGED_FAILURES) { | ||
| loggedFailures.add(name + ": " + label); | ||
| LOG.warn("{}: failed to load '{}' - {}", description, name, label, cause); | ||
| } else { | ||
| LOG.debug("{}: failed to load '{}' - {}", description, name, label, cause); | ||
| } |
| try { | ||
| limiter.acquire(); |
| "Maximum number of concurrent requests to Azure Key Vault during bulk key loading " | ||
| + "(Default: ${DEFAULT-VALUE})", | ||
| paramLabel = "<MAX_CONCURRENCY>") | ||
| private int maxConcurrency = BulkLoadOptions.DEFAULT_MAX_CONCURRENCY; |
MqllR
changed the title
Summary
Fixes #1217
Secrets and keys are now fetched concurrently, up to a configurable limit, while the vault is still being listed, instead of one 25-item page at a time with per-page
parallelStream()(which barely parallelizes under a constrained CPU limit, sinceparallelStream()sizes offRuntime.availableProcessors()).ConcurrentBulkLoader/BulkLoadOptionsinkeystorage.azure: bounded-concurrency dispatch over a lazily-consumed listing stream, with a deadline, interrupt handling, and per-item error accounting.AzureKeyVault.mapSecrets/mapKeyPropertiesuse the loader instead of page-by-pageparallelStream().--azure-bulk-load-max-concurrency(default 20) and--azure-bulk-load-timeout(default 900 seconds).Performance
Same environment as #1217 (container limited to
cpu: "1",memory: 2Gi; requestscpu: 500m,memory: 1Gi; vault with 20001 secrets):Test plan
./gradlew :keystorage:build— unit tests forConcurrentBulkLoaderandAzureKeyVault./gradlew :commandline:build— CLI parameter tests./gradlew :signing:build./gradlew :core:compileJavaNote
Medium Risk
Changes affect startup/reload key loading for all Azure bulk users; mis-tuned concurrency or timeouts could increase errors or leave keys unloaded, though defaults and explicit error reporting mitigate silent partial loads.
Overview
Azure Key Vault bulk loading is reworked for much faster startup when many secrets or keys are loaded (eth1 and eth2).
Listing and fetching now overlap:
mapSecretsandmapKeyPropertiesdrive a newConcurrentBulkLoaderover lazily consumed SDK streams, with a semaphore-capped concurrency (virtual threads), periodic progress logs, and strict accounting for failures, deadlines, interrupts, and incomplete listing.Operators can tune behavior via
--azure-bulk-load-max-concurrency(default 20) and--azure-bulk-load-timeout(default 900s).BulkLoadOptionsis wired throughAzureKeyVaultParameters/ CLI into eth1 and eth2 bulk load paths. Transient Azure errors rely on the SDK retry policy only (no custom retry layer in the loader).Reviewed by Cursor Bugbot for commit 6ff268e. Bugbot is set up for automated code reviews on this repo. Configure here.