SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11221 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11221/)
HDFS-11210. Enhance key rolling to guarantee new KeyVersion is returned (xiao: rev 2007e0cf2ad371e2dbf533c367f09c1f5acd1c0b)

• (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KeyAuthorizationKeyProvider.java
• (edit) hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSRESTConstants.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyProvider.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/ValueQueue.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyProviderExtension.java
• (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/EagerKeyGeneratorKeyProviderCryptoExtension.java
• (edit) hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMS.java
• (edit) hadoop-common-project/hadoop-kms/src/site/markdown/index.md.vm
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/LoadBalancingKMSClientProvider.java
• (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyShell.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/CachingKeyProvider.java
• (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyShell.java
• (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java

Added the one line comment on top of patch 5, and committed to trunk.

Thanks Andrew for the great comments!

Thanks a lot Andrew! Test failure look unrelated and passes locally.
Will be adding the code comment and committing soon.

-1 overall

This message was automatically generated.

Gotcha, I figured it was related to negative numbers, though my quick tests with python returned positive numbers. I guess Java's behavior is different; definitely a quirky area for languages.

Anyway I'm +1 pending Jenkins (and the comment improvement if you want). Great work here!

Thanks Andrew, if you have any other comments, please don't hold back.

Noticed you changed indexFor from modulo to a mask. What was the reason for this? The masking only works when the array size is a power of 2.

Because I forgot to change the symbol bit to 0, hence all those test failures when int hashCode return a negative value. Instead of modulo then 0-out the first bit, I figured masking it once is more elegant.
True it'll be problematic when array size isn't 2, maybe add a comment for future since array size is hardcoded currently?

And finally we have the item in NN:

The Client instance inside NN(s), and corresponding cache(s). When hadoop key roll is succeeded, the client provider inside NN should be drained too.

Still think my 1st comment of invalidateCache when receiving re-encrypt is the way to go, and should probably add that line to the mighty HDFS-10899....

LGTM overall, I think we can just commit this as is if you want.

• IMO making the # of locks configurable is optional. The ideal number is typically a multiple of the # of cores, and 16 seems like a very reasonable default considering we also typically don't have that many keys.
• Noticed you changed indexFor from modulo to a mask. What was the reason for this? The masking only works when the array size is a power of 2.

Patch 5 to implement indexFor better

-1 overall

This message was automatically generated.

Thanks a lot for the review Andrew! Exactly the 'quality review' I mentioned in the last comment.

locks

Used a fixed-sized preallocated array for the lock.
Researched a bit but think we should be able to live with the fixed-sized locks in this case. Resizing an array is easy, but seems a little messy to do for the array of stripped locks without a synchronized block on the ValueQueue object. (IIUC we need to writelock all stripes before switching to a new-sized array, to prevent the same key that's locked at index 1 unprotected due to its new index 2).
If this looks good to you, I can make the size configurable (and potentially modify ValueQueue to use a builder instead of adding the 7th param to ctor)

Do you know why UniqueKeyBlockingQueue#put is synchronized, but the other methods aren't?

Good q, I'm not sure. Looking at the code + comments, the only explanation IMHO is we want to make sure no more than 1 runnable per key is created. And the only thing the async thread do is put, so kinda make sense... We could ping the authors for the answer.

it looks like we'll invalidate multiple times since there's an invalidateCache call in both KMSCP and LBKMSCP. Do you think we can remove the invalidates in LBKMSCP?

I don't think it'd be safe to remove it from LBKMSCP, because ValueQueue is per KMSCP. So 1 KMSCP#rollNewVersion call should really invalidates all KMSCPs' caches for a LBKMSCP group.
We may be able to only invalidate the rest of the KMSCPs in the LBKMSCP group, so each KMSCP only invalidates once. But I don't think this double invalidation will practically create problems, so would like to go for the current KISS way for code cleanness. Thoughts?

Other comments are all addressed.

Thanks for the rev Xiao, looks pretty close, mostly nitty stuff and questions:

• index.md.vm, new entry on getMetadata looks incomplete?
• TestKMS, typo MockVersinoName
• Since readLock and writeLock both need to call into a synchronized block to do anything, it might limit performance. I don't know if ValueQueue is actually performance critical, but one approach to solve this is preallocating an array of locks and using striped locking. Essentially, hash the key, and use the lock at that array location. This spreads out load across the locks well assuming the hash function is good. Resizing the array of locks is an exercise left to the reader.
• Are the createLockIfNonexist calls in readLock and writeLock sufficient for safety, or do we need the other calls too? Would prefer to remove the other calls for clarity if they're not necessary.
• Do you know why UniqueKeyBlockingQueue#put is synchronized, but the other methods aren't?
• KMSClientProvider#invalidateCache, little nit, please move the comment up above where we invalidate the server cache
• Final q, it looks like we'll invalidate multiple times since there's an invalidateCache call in both KMSCP and LBKMSCP. Do you think we can remove the invalidates in LBKMSCP?

Patch 3 to fix the 2 related test failure: invalidate cache after rollover in cachingkeyprovider; not getCurrentKey when logging invalidateCache, since that's under another ACL...

-1 overall

This message was automatically generated.

Thanks for the review Andrew, really appreciate the quality review of yours.

Good call to invalidate cache, don't know what I was thinking when naming it...

High-level, is KeyProvider#clearCache intended for use by end-users?

Since invalidateCache is added to KMS, and the keyprovider API, end-users can definitely use it. I try to hide this detail as much as possible, but agree hadoop key command addition would be handy, and no harm. So added this and a test in patch 2.

All comments addressed I think. Also added a new test in TestKMS with mock, to fail-before-pass-after the invalidation. The rollover draining test is race by nature, so IMHO the separate mock testing would be clearer.

Thanks for working on this Xiao, sorry for the slow review, comments and questions:

High-level, is KeyProvider#clearCache intended for use by end-users? It looks like we call clear after rollover in LBKMSCP automatically, but I could see a "hadoop key" command being useful if there was a failure and you want to retry.

• I'd prefer "invalidate" to "clear", since "cache invalidation" is a known term. e.g. invalidateCache, INVALIDATE_CACHE, etc.
• KMS.java, the debug log says "Rolling key with name", should probably say "Invalidating cache for key with name" ?
• KMSCP, why was the drain in the rollover removed? I see a drain was added in LBKMSCP, but what if we're not using that wrapper?
• KMSCP, since clearCache isn't implemented users can't call this except via rollover (which calls clearServerCache). Is this intentional? I removed the clearCache in TestKMS and it still passed.
• KeyProvider, should specify what the param is for clearCache in the Javadoc. Also I think you meant "return the new key version" at the end of the description.
• ValueQueue, you could have readLock(keyName) readUnlock(keyName) wrappers to make it more concise, and also create the lock automatically.
• ValueQueue, do we need to worry about races to create locks in keyQueueLocks?

Preliminary patch 1 to express the idea. This should take care of all things KMS.

Regarding the last point about the provider instance (or specifically, KeyProviderCryptoExtension) inside NN:
since no one else is aware of this client, I'm thinking when a re-encrypt command is submitted, we need to get the keyname, and do a clearCache always. Considering re-encrypt should be an infrequent command executed during maintenance window, this should be acceptable. Better approaches welcome!