[
https://issues.apache.org/jira/browse/HADOOP-20045?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18135367#comment-18135367
]
ASF GitHub Bot commented on HADOOP-20045:
-----------------------------------------
szetszwo commented on code in PR #8888:
URL: https://github.com/apache/hadoop/pull/8888#discussion_r4418350830
##########
dev-support/benchmarks/kms-http-benchmark.py:
##########
@@ -0,0 +1,160 @@
+#!/usr/bin/env python3
Review Comment:
Let's move the benchmark and readme to `
hadoop-common-project/hadoop-common/src/test/scripts/kms-http-benchmark`.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -561,31 +561,49 @@ private void removeExpiredKeys() {
protected byte[] createPassword(TokenIdent identifier) {
this.apiLock.writeLock().lock();
try {
- int sequenceNum;
- long now = Time.now();
- sequenceNum = incrementDelegationTokenSeqNum();
- identifier.setIssueDate(now);
- identifier.setMaxDate(now + tokenMaxLifetime);
- identifier.setMasterKeyId(currentKey.getKeyId());
- identifier.setSequenceNumber(sequenceNum);
- LOG.info("Creating password for identifier: " +
formatTokenId(identifier) +
- ", currentKey: " + currentKey.getKeyId());
- byte[] password = createPassword(identifier.getBytes(),
currentKey.getKey());
- DelegationTokenInformation tokenInfo =
- new DelegationTokenInformation(now + tokenRenewInterval, password,
- getTrackingIdIfEnabled(identifier));
+ DelegationTokenInformation tokenInfo = prepareToken(identifier,
+ incrementDelegationTokenSeqNum());
try {
METRICS.trackStoreToken(() -> storeToken(identifier, tokenInfo));
} catch (IOException ioe) {
LOG.error("Could not store token " + formatTokenId(identifier) + "!!",
ioe);
}
- return password;
+ return tokenInfo.getPassword();
} finally {
this.apiLock.writeLock().unlock();
}
}
-
+ /**
+ * Initialize an identifier and its password using a consistent signing key.
+ * Sequence allocation and persistence are the caller's responsibility and
+ * must not be performed inside this critical section by external stores.
+ * The caller must not modify the identifier after preparation.
+ *
+ * @param identifier identifier to initialize
+ * @param sequenceNumber previously allocated unique sequence number
+ * @return prepared token information
+ */
+ protected final DelegationTokenInformation prepareToken(TokenIdent
identifier,
+ int sequenceNumber) {
+ DelegationTokenInformation tokenInfo;
+ this.apiLock.writeLock().lock();
+ try {
+ long now = Time.now();
+ identifier.setIssueDate(now);
+ identifier.setMaxDate(now + tokenMaxLifetime);
+ identifier.setMasterKeyId(currentKey.getKeyId());
+ identifier.setSequenceNumber(sequenceNumber);
+ byte[] password = createPassword(identifier.getBytes(),
currentKey.getKey());
+ tokenInfo = new DelegationTokenInformation(now + tokenRenewInterval,
password,
+ getTrackingIdIfEnabled(identifier));
+ } finally {
+ this.apiLock.writeLock().unlock();
+ }
+ LOG.info("Creating password for identifier: " + formatTokenId(identifier)
+ + ", currentKey: " + identifier.getMasterKeyId());
Review Comment:
AbstractDelegationTokenSecretManager.createPassword(..) already has acquired
the lock before calling this method. I wonder if we can print this log message
outside the lock.
> Avoid global write-lock contention during ZooKeeper delegation token creation
> and add a KMS HTTP benchmark
> ----------------------------------------------------------------------------------------------------------
>
> Key: HADOOP-20045
> URL: https://issues.apache.org/jira/browse/HADOOP-20045
> Project: Hadoop Common
> Issue Type: Improvement
> Components: common, kms
> Reporter: Wei-Chiu Chuang
> Assignee: Wei-Chiu Chuang
> Priority: Major
> Labels: pull-request-available
> Attachments: kms-http-latency.png, kms-http-throughput.png,
> zk-token-creation-lock-flow.png
>
>
> h3. Problem
> KMS requests authenticated with delegation tokens can stall behind a request
> issuing a new token.
> HADOOP-18851 replaced synchronization in AbstractDelegationTokenSecretManager
> with a read-write lock, allowing concurrent verification. However,
> createPassword() still holds the global write lock while allocating a
> sequence-number range and persisting the token in ZooKeeper. A slow ZooKeeper
> request therefore blocks unrelated requests that need the read lock to verify
> cached tokens.
> A thread dump from an affected KMS instance shows the lock holder waiting in:
> {noformat}
> AbstractDelegationTokenSecretManager.createPassword()
> -> ZKDelegationTokenSecretManager.storeToken()
> -> ZKDelegationTokenSecretManager.addOrUpdateToken()
> -> ZooKeeper.create()
> -> ClientCnxn.submitRequest()
> -> Object.wait()
> {noformat}
> KMS uses DelegationTokenManager.ZKSecretManager, which extends the common
> ZKDelegationTokenSecretManager. The contention fix therefore belongs in the
> common secret-manager implementation; the Router implementation is an
> additional consumer.
> h3. Proposed change
> Separate the ZooKeeper creation path into three stages:
> # Allocate a sequence number under a dedicated allocator lock, reserving a
> new range when necessary.
> # Initialize the identifier and compute its password under a short global
> write-lock section.
> # Persist the prepared token synchronously outside the global lock, returning
> success only after persistence completes.
> Preserve existing storage-hook locking for non-ZooKeeper implementations. An
> interrupted sequence-range refill must abort issuance rather than fall
> through and consume an unreserved number.
> The HDFS Router ZooKeeper subclass also needs an adjustment: creation must
> not insert a speculative cache entry before persistence or unconditionally
> insert one afterward. Existing watcher, cache-miss, and rebuild paths
> populate newly created tokens. This avoids a creator restoring stale local
> state after a concurrent cancellation.
> !zk-token-creation-lock-flow.png|width=1000!
> This change addresses creation-induced contention. Renewal, cancellation, key
> rotation, and existing distributed-cache ordering races remain separate work.
> It does not claim immediate cross-manager revocation or complete distributed
> consistency.
> h3. Correctness validation
> The change adds tests covering:
> * Verification of an existing cached token while creation is paused during
> allocation or persistence.
> * Unique sequence numbers during concurrent issuance across two managers.
> * Persistence failure without speculative cache publication.
> * Interrupted sequence-range refill.
> * Key rotation between token preparation and persistence.
> * Router creation and immediate verification with watchers disabled.
> The two blocking regression tests time out against the original code and pass
> with the fix. The selected common and Router validation comprised 43 passing
> tests. The HTTP benchmark's scheduler and latency-accounting tests and the
> existing KMS authentication-filter test also passed.
> h3. End-to-end KMS HTTP benchmark
> Add an opt-in benchmark in hadoop-kms and a comparison runner using:
> * One KMS server JVM, one standalone ZooKeeper JVM, and a separate
> load-generator JVM.
> * 32 concurrent HTTP requests; no concurrency sweep.
> * Sequence-number batch size 100, unchanged between versions.
> * No injected latency or sleep in the request/storage path.
> * Verify:create request mixes of 100:0, 99:1, 95:5, and 90:10.
> * Five seconds of warmup and fifteen seconds of measurement per case, with
> three repetitions.
> * Fresh server processes/data per case and alternating original/fixed pair
> order across repetitions.
> Verification traffic consists of metadata GET requests authenticated using
> existing delegation tokens. Creation traffic uses GETDELEGATIONTOKEN with
> simple primary authentication. Cookie reuse, redirects, and automatic retries
> are disabled to preserve the intended authentication paths and request mix.
> Before measuring, the harness checks real delegation-token authentication,
> rejection of an invalid token and an unauthenticated request, and the actual
> ZKSecretManager implementation. Diagnostic counters are disabled before load.
> Loaded secret-manager class fingerprints are checked against the selected
> original/fixed common jars in every case.
> Submitted mixes are enforced in shuffled blocks of 100. Block-tail
> submissions and drained completions are recorded separately; post-cutoff
> completions do not inflate measurement-window goodput. Token-node growth must
> equal the number of completed create operations. The harness records
> throughput, errors, per-operation latency, JVM CPU/GC snapshots, and
> ZooKeeper statistics, including real fsync activity.
> h3. Results
> Baseline: Apache trunk commit {{a2d306c65a84bdf11a6eb5251cfff1bfe110546e}}.
> Both versions use the same KMS/client benchmark and dependencies, differing
> in the selected common secret-manager implementation. JDK 17.0.15 was used.
> Median successful total HTTP requests per second across three repetitions:
> || Verify:create || Original requests/s || Fixed requests/s || Fixed/original
> ||
> | 100:0 | 30,811 | 30,150 | 0.98x |
> | 99:1 | 17,672 | 26,885 | 1.52x |
> | 95:5 | 5,349 | 23,949 | 4.48x |
> | 90:10 | 3,103 | 23,174 | 7.47x |
> !kms-http-throughput.png|width=1000!
> All 24 cases completed with zero HTTP or response-validation errors and
> passed loaded-artifact, submitted-ratio, and token-count checks. Whiskers in
> the charts show observed min-max ranges, not confidence intervals.
> Median per-run p99 latency in milliseconds:
> || Verify:create || Original verify p99 || Fixed verify p99 || Original
> create p99 || Fixed create p99 ||
> | 100:0 | 4.14 | 4.48 | N/A | N/A |
> | 99:1 | 5.31 | 4.18 | 7.08 | 18.97 |
> | 95:5 | 14.22 | 4.31 | 17.18 | 21.38 |
> | 90:10 | 21.81 | 3.39 | 22.47 | 19.35 |
> !kms-http-latency.png|width=1100!
> The 100:0 control showed variability, including one lower fixed-build
> observation; its median was 2.1 percent lower with the fix. Three short
> repetitions do not establish statistical significance for small differences.
> Creation p99 increased at 99:1 and 95:5 under the higher achieved load. These
> are closed-loop capacity comparisons, not matched-arrival-rate latency
> comparisons, and the results do not establish uniformly improved creation
> latency.
> The workload measures authenticated metadata requests with simple
> authentication for issuance and local standalone ZooKeeper. It does not
> establish production throughput for Kerberos, TLS, encryption operations, or
> a ZooKeeper quorum. No performance threshold is imposed in normal CI.
> h3. Reproduction
> The HTTP benchmark is opt-in; ordinary test runs execute only the lightweight
> scheduler and latency-accounting checks. Defaults match the matrix above.
> {noformat}
> mvn -pl hadoop-common-project/hadoop-kms \
> -Dtest=TestKMSHttpBenchmark -Dkms.benchmark=true \
> -Dmaven.test.failure.ignore=false test
> {noformat}
> For controlled comparison, build original and fixed hadoop-common jars with
> otherwise identical source/dependency versions, then run:
> {noformat}
> python3 dev-support/benchmarks/kms-http-benchmark.py \
> --baseline-common /path/to/original/hadoop-common.jar \
> --fixed-common /path/to/fixed/hadoop-common.jar \
> --output /path/to/fresh/results-directory
> {noformat}
> The runner saves incremental comparison JSON and per-case commands, logs,
> configuration, and metrics. Maven 3.9.15 or newer and JDK 17 are required for
> this trunk checkout. No native profile is required.
> Related: [HADOOP-18851|https://issues.apache.org/jira/browse/HADOOP-18851].
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
