HDDS-7941. Race condition in getFileStatus causes flaky testObjectStoreCreateWithO3fs

[devmadhuu]

What changes were proposed in this pull request?

This PR is fixing the race condition possibility in getFIleStatus call where keyTable cache iterator cache flush can happen while iterating table iterator in method org.apache.hadoop.ozone.om.KeyManagerImpl#createFakeDirIfShould

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7941

How was this patch tested?

This patch was tested with multiple CI job runs to test flakiness in test failure of this JIRA due to above mentioned cause. Here is the green CI run after applying patch.

[devmadhuu]

@sumitagrawl @sadanand48 @smengcl Pls review.

[hemantk-12]

I don't understand how this patch is solving the issue. Because to me, nothing is changing. Instead of using cache iterator, you are just directly getting value form cache.
Just to do a simple check, I ran the test 25 times, with and without fix and it passed all the time.

It would be great if you can add more details how this change will fix it.

[devmadhuu]

I don't understand how this patch is solving the issue. Because to me, nothing is changing. Instead of using cache iterator, you are just directly getting value form cache. Just to do a simple check, I ran the test 25 times, with and without fix and it passed all the time.

It would be great if you can add more details how this change will fix it.

This test case fails in checkPath method of TestOzoneFSWithObjectStoreCreate.testObjectStoreCreateWithO3fs test case where even after deleting a key on a given path , getFileStatus API returns valid fileStatus, so after analyzing the code flow for getFIleStatus, it was found that isKeyDeleted method is the problem where after getting cache iterator on table, cache flush is unpredictably be flushed and can give wrong result check of key deletion. Here in. this test case, we are doing some rename operation and after that all original set of key paths were sent in batch and each key path by key path , code flow hits createFakeDirIfShould and check for existence of key here below:

if (exists
          && cacheKey.startsWith(targetKey)
          && !Objects.equals(cacheKey, targetKey)) {
        LOG.debug("Fake dir {} required for {}", targetKey, cacheKey);
        return createDirectoryKey(cacheValue.getCacheValue(), dirKey);
      }

Now here for a given key, control is returning from here in if check and after iterating all keys in batch, if a key not found in cache , we don't want to loose deleted keys entry here because after this while loop, if we don't find the keys in cache, we iterate in keyTable and during this iteration we want to make sure that we don't use isKeyDeleted method for key existence in cache due to unpredictable nature of cache flush. So we are making list of deleted keys as well in while loop..

Here is the failed CI run before the patch and green CI run after applying this patch.

[smengcl]

This can be dumping a lot of keyTable cache entries into the temp set deletedKeys. And this method can be called frequently from getFileStatus / getOzoneFileStatus.

Is there a better way to fix this? Since the only caller is already holding a BUCKET_LOCK:

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

Line 1127 in e4cbc20

metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,

It might be fine for now since keyTable cache should only hold a few entries that are not flushed yet.

[devmadhuu]

Thanks @smengcl for your review. Another way to fix is this #5093 PR. Here a table.isExist() API being used, but my only concern is that cache is always faster and isExist is being called in #5093 PR till we find the target key as well it is being called inside getNextKey() call. Pls have look #5093 PR

[devmadhuu]

@smengcl Kindly advise.

[smengcl]

Alright. Let's merge this PR now for a fix. We could improve this further in #5093 .

[smengcl]

+1 pending CI

[devmadhuu]

+1 pending CI

Thanks @smengcl for review. CI all green.

[sumitagrawl]

@devmadhuu Thanks for working over this, LGTM +1