PSMDB-1997: Add deferred encryption key cleanup to avoid race with checkpoint#1786
PSMDB-1997: Add deferred encryption key cleanup to avoid race with checkpoint#1786
Conversation
There was a problem hiding this comment.
Pull request overview
Adds infrastructure to defer deletion of per-database encryption keys (Percona KeyDB) and periodically clean up orphaned keys after verifying they are no longer referenced by any WiredTiger ident, to avoid races with checkpoint/drop-pending cleanup.
Changes:
- Added parsing helper to extract
encryption.keyidfrom WiredTiger metadata configs, with unit tests. - Extended the KeyDB/KVEngine API and WiredTigerKVEngine implementation to enumerate keys, delete keys, and scan WT metadata for keyIds currently in use.
- Added a timestamp-monitor-driven background cleanup in
StorageEngineImpl, gated by new server parameters.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mongo/db/storage/wiredtiger/wiredtiger_util.h | Declares helper to extract encryption keyId from WT config strings. |
| src/mongo/db/storage/wiredtiger/wiredtiger_util.cpp | Implements getEncryptionKeyId() via WiredTigerConfigParser. |
| src/mongo/db/storage/wiredtiger/wiredtiger_util_test.cpp | Adds coverage for keyId extraction across config variants. |
| src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.h | Exposes new key-management methods on the engine. |
| src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine.cpp | Implements key enumeration/deletion and “keys in use” scan. |
| src/mongo/db/storage/wiredtiger/wiredtiger_kv_engine_encryption_key_test.cpp | Adds tests for key enumeration/deletion + server parameter toggling. |
| src/mongo/db/storage/wiredtiger/wiredtiger_global_options.idl | Introduces encryptionKeyCleanupDeferred + cleanup interval parameters. |
| src/mongo/db/storage/wiredtiger/encryption_keydb.h | Declares KeyDB enumeration API. |
| src/mongo/db/storage/wiredtiger/encryption_keydb.cpp | Implements getAllKeyIds() cursor scan; guards null encryptor pointers. |
| src/mongo/db/storage/storage_engine_impl.h | Adds cleanup listener + last-run tracking. |
| src/mongo/db/storage/storage_engine_impl.cpp | Implements periodic orphaned key cleanup with catalog + WT metadata checks. |
| src/mongo/db/storage/keydb_api.h | Extends KeyDB API surface for deferred cleanup support. |
| src/mongo/db/shard_role/shard_catalog/collection_catalog_helper.cpp | Removes immediate key deletion from dropDatabase path; documents deferred cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b2b3f91 to
ef48a8e
Compare
fea52cd to
765829e
Compare
|
The |
e7848db to
85e552a
Compare
…eckpoint This commit implements deferred encryption key cleanup to prevent race conditions between encryption key deletion and checkpoint cleanup during dropDatabase operations.
Add unit tests for the new encryption key management functionality:
Add two test files and corresponding test suites for verifying deferred encryption key cleanup functionality: - deferred_key_cleanup_drop_database.js: Tests basic database create/drop operations with deferred cleanup enabled - deferred_key_cleanup_stress.js: Stress test with 16 parallel threads performing 200 iterations each of database create/drop Test suites run with both AES256-GCM and AES256-CBC cipher modes. The stress test uses db.getSiblingDB() instead of conn.getDB() to ensure the database connection works correctly when the worker function is serialized and executed in a parallel shell context.
8d290db to
e6005b9
Compare
3ed6b1d to
6a1589b
Compare
6a1589b to
d9e50d4
Compare
igorsol
left a comment
There was a problem hiding this comment.
- Why do we need encryptionKeyCleanupDeferred server parameter? And why default is false? Why not enable cleanup unconditionally?
- LOGV2: 29060-29069, 29071-74 ids are occupied by other code. Please use unoccupied range
- wiredtiger_kv_engine.cpp: remove formatting changes in code unrelated to this PR
|
|
||
| for (const auto& keyId : keyIds) { | ||
| // Skip special keys | ||
| if (EncryptionKeyDB::isSpecialKeyId(keyId)) { |
There was a problem hiding this comment.
Redundant check: EncryptionKeyDB::isSpecialKeyId already excludes special key ids
|
|
||
| // CRITICAL CHECK: If ANY ident in storage uses this key, do NOT delete it. | ||
| // This includes drop-pending idents that haven't been physically removed yet. | ||
| if (keyIdsInUse.count(keyId) > 0) { |
There was a problem hiding this comment.
count() is O(n) operation. In case of thousands of idents this check might get pretty expensive.
Consider sorting both keyIds and keyIdsInUse vectors and implementing a single-pass algorithm traversing both vectors.
Code reviewFound 1 issue:
percona-server-mongodb/src/mongo/db/storage/storage_engine_impl.cpp Lines 1042 to 1047 in d9e50d4 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
No description provided.