[
https://issues.apache.org/jira/browse/HDDS-17216?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17216:
------------------------------
Attachment: MC-2-keep-s3-secret-cache-entries-until-own-index-flushed.patch
> S3 secret cache cleanup can remove the entry of an unflushed transaction, so
> OMs can commit different S3SecretTable contents
> ----------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17216
> URL: https://issues.apache.org/jira/browse/HDDS-17216
> Project: Apache Ozone
> Issue Type: Bug
> Reporter: Siyao Meng
> Priority: Major
> Attachments:
> MC-2-keep-s3-secret-cache-entries-until-own-index-flushed.patch
>
>
> h3. Mechanism
> Each write to an S3 secret puts an entry carrying its transaction index into
> {{S3InMemoryCache}}. After the double buffer has committed a batch, the flush
> thread calls {{S3InMemoryCache.clearCache}} with the batch's indexes, which
> removes the entries carrying those indexes, while the apply thread keeps
> applying later transactions. A read that misses the cache loads the committed
> row and caches it with index 0. The cache is consistent as long as every
> applied but unflushed write keeps its entry until its own index is flushed.
> Three places do not ensure that:
> # {{S3RevokeSecretRequest}} replaces the entry with
> {{S3SecretValue.deleted()}}, which keeps the index of the entry it replaces
> instead of the revoke's index. When the revoke is applied after the batch
> holding the previous write to that access ID was committed and before that
> batch's cleanup, the cleanup removes the deleted entry although the revoke is
> not flushed.
> # {{OMTenantRevokeUserAccessIdRequest}} calls {{invalidateCacheEntry}}, which
> only replaces an entry that is already cached ({{computeIfPresent}}), and
> calls it without the access ID's {{S3_SECRET_LOCK}}. If the access ID has no
> cache entry, the tenant revoke leaves none.
> # {{clearCache}} maps each cached index to its access ID in a first pass and
> removes each mapped access ID in a second pass, without checking the entry's
> index again. {{S3SecretLockedManager.clearS3Cache}} holds the
> {{S3_SECRET_LOCK}} stripe of the key "cache", not of the access IDs, so a
> write to the access ID can be applied between the two passes, and its entry
> is removed.
> In each case a read that misses the cache before the newer transaction is
> flushed caches the older row with index 0, which the cleanup of the newer
> transaction does not match. After that flush, the OM's cache entry for the
> access ID no longer matches its committed row.
> Which OMs are affected depends on when each OM's double buffer flush runs
> relative to its apply thread. That timing is local to each OM and is not part
> of the Ratis log.
> h3. Impact
> * *OMs can commit different S3SecretTable contents.* Two OMs apply the same
> log create@1, revoke@2, create@3 for one access ID. On an OM that flushes all
> three in one batch, create@3 ({{GetS3Secret}}) returns OK and commits the new
> secret. On an OM that applies revoke@2 between the commit and the cleanup of
> batch [1], create@3 reads the older row, returns {{S3_SECRET_ALREADY_EXISTS}}
> and writes nothing, so after both OMs have flushed index 3 one holds the new
> secret and the other holds no row for the access ID. Nothing compares or
> repairs the tables of OMs at the same applied index, so the difference
> survives restarts. A later {{GetS3Secret}} or {{SetS3Secret}} for that access
> ID can likewise get a different result on an OM whose cache entry no longer
> matches its committed row than on the others.
> * *Cache entry and committed row disagree.* After path 3 the DB holds the
> newer write, but the OM's cache entry for the access ID does not match its
> committed row until the OM restarts or the access ID is written again.
> Affected versions: the deleted entries and the index based cleanup (paths 1
> and 3) exist since HDDS-9444, path 2 since the tenant revoke request. The
> missing cleanup registration described under Patch is on master only, since
> HDDS-16643.
> h3. Reproduction
> There is no standalone reproduction attached. The regression tests are in the
> patch, in the existing {{TestS3GetSecretRequest}}: real
> {{OmMetadataManagerImpl}} (RocksDB), {{S3SecretLockedManager}} over
> {{S3SecretManagerImpl}} with the in-memory cache, requests applied through
> {{preExecute}} and {{validateAndUpdateCache}}, and each flush done in two
> steps by the test (commit the responses with {{checkAndUpdateDB}}, then
> {{clearS3Cache}} for the responses that registered the S3 secret table), so
> the interleavings above become a plain sequence of calls. All four fail
> without the change:
> * {{testRevokeKeptInCacheWhenEarlierIndexIsCleanedUp}} (path 1): commit [1],
> apply revoke@2, clean up [1]; expects the deleted entry with index 2 to stay
> until the revoke is flushed.
> * {{testCreateAfterRevokeWhenEarlierIndexIsCleanedUp}} (path 1, the diverging
> OM): the same, then {{GetS3Secret}}@3; fails with "expected: OK but was:
> S3_SECRET_ALREADY_EXISTS".
> * {{testTenantRevokeOfUncachedSecretIsKeptInCache}} (path 2): the secret is
> committed and not cached, tenant revoke@1 is applied; expects a deleted entry
> until the revoke is flushed.
> * {{testCacheCleanupKeepsEntryReplacedDuringCleanup}} (path 3):
> {{SetS3Secret}}@2 is applied at the moment {{clearCache([1])}} reads the
> index of the create's entry (a Mockito spy on that cache value); expects the
> set's entry to stay.
> The two OM case and paths 1 and 3 were also run through the flush method of a
> real {{OzoneManagerDoubleBuffer}} (the one its daemon calls) on two OM stacks
> in one JVM, with the results described under Impact. These runs show that the
> interleavings exist, not how often they occur.
> h3. Patch
> [^MC-2-keep-s3-secret-cache-entries-until-own-index-flushed.patch], against
> df04bc0e309f625a7dcdcd60790a42f34e2743d9. It also applies to master at
> 2cb1b6dd4e9.
> The patch keeps every cache entry until its own transaction is flushed:
> * {{S3SecretCache.invalidate(id, transactionLogIndex)}} (new, with a
> {{S3SecretManager.invalidateCacheEntry}} counterpart) always puts a deleted
> entry carrying the revoking transaction's index, whether or not the access ID
> is cached. {{S3RevokeSecretRequest}} passes {{context.getIndex()}}.
> {{OMTenantRevokeUserAccessIdRequest}} passes its {{transactionLogIndex}} and
> now calls it under the access ID's lock through {{doUnderLock}}, as
> {{OMTenantAssignUserAccessIdRequest}} already does. The index is the Ratis
> index every OM applies the entry with, so no request or proto change is
> needed.
> * {{S3InMemoryCache.clearCache}} removes an entry only if it still carries
> the flushed index ({{computeIfPresent}}).
> * The new {{invalidateCacheEntry}} records the S3 secret table with
> {{TableCacheUpdateTracker}}, as {{updateCache}} does, so the double buffer
> runs the cleanup for the revoke's index. Since HDDS-16643 a revoke whose
> secret was already cached did not register that table.
> For stores with batch support (the default RocksDB store), every unflushed
> write is then represented by an entry that no earlier cleanup removes, and
> reads and writes of an access ID are serialized by its lock, so a cache miss
> means the committed row is current and an entry loaded with index 0 holds it.
> On stores without batch support the store is written at flush time; that is
> addressed by HDDS-17214, which also edits {{S3RevokeSecretRequest}},
> {{OMTenantRevokeUserAccessIdRequest}} and {{TestS3GetSecretRequest}}, so the
> two patches need a rebase whichever lands second. The existing one argument
> {{invalidate}} is kept unchanged for its remaining caller, the flush time
> {{S3SecretManagerImpl.revokeSecret}} of those stores.
> The lock in {{OMTenantRevokeUserAccessIdRequest}} is argued from the code (a
> read that missed the cache could otherwise overwrite the new deleted entry
> with the committed row) and is not covered by a failing test.
> The patch includes the {{S3SecretManagerImpl.getSecretString}} change
> proposed in HDDS-17200 so that it is complete on its own; drop that hunk if
> HDDS-17200 is committed first.
> Without the races the behaviour is unchanged, except that a deleted entry is
> now removed when the revoke itself is flushed instead of when the replaced
> entry's index is flushed.
> With the patch the four tests pass, the ozone-manager unit suites for S3
> requests and responses, the double buffer, the delegation token secret
> manager and multi tenancy pass (315 tests), {{TestMultiTenantVolume}},
> {{TestSecureOzoneCluster.testGetSetRevokeS3Secret}} and
> {{TestSecureOzoneRpcClient.testS3Auth}} pass on a mini cluster (7 tests), and
> checkstyle is clean.
> Found by TLA+ model checking and code review of the OM S3 secret cache and
> external S3 secret store paths under HDDS-15926, on commit
> df04bc0e309f625a7dcdcd60790a42f34e2743d9. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-17200 (deleted cache entries in {{getSecretString}}),
> HDDS-9444 (introduced the index based cleanup and the deleted entries),
> HDDS-16643 (removed the static cleanup table declarations) and HDDS-5358 (an
> earlier invalidation fix). The attached patch is a proposal for review.
> Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
