[Searchable Snapshots] Correctly clone IndexInputs to avoid race #6367

andrross · 2023-02-18T02:04:06Z

In PR #6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen after this point to ensure that each thread gets its own clone.

I've added a unit test that will reliably fail without the fix.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2023-02-18T02:30:13Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11334/
CommitID: c004d97
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

andrross · 2023-02-20T17:14:25Z

Fixed up the spotless errors. Also, I ran the integration test in a loop over the weekend (which is how I found this bug) and it passed a total 11,440 of iterations with no failures, so I'm confident this is the fix the race I observed.

reta · 2023-02-20T17:27:29Z

server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java

+            final IndexInput origin = originFuture.get();
+            // The origin instances stays in the cache with a ref count of zero
+            // and must be cloned before being returned.
+            return origin.clone();


@andrross I am struggling to understand this change: fetchBlob used to return cloned version but now there is a blocking call and, well, cloning. The only effect I could deduct from that is that cloning would happen in different threads, was that the issue?

The crux of the issue is that invocationLinearizer.linearize() can result in concurrent calls being returned the same instance, so that means the clone call must happen after the linearize() call. Each concurrent thread must independently clone the result, and that wasn't happening before.

I did move the blocking call from OnDemandBlockSnapshotIndexInput to here. That wasn't strictly necessary, but the clone call must happen after redeeming the future and I wanted to keep all the cloning logic inside this class.

@reta The sequence was like this:

download blob

clone

wrap in future and return (potentially to multiple threads)

get result from future

It has changed to this:

download blob

wrap in future and return (potentially to multiple threads)

get result from future

clone

Thanks @andrross it looks like just adding whenComplete(r -> r.clone()) on the future would have done the same?

@reta That was my first instinct but I think each concurrent thread is given the same CompletableFuture instance, so for n threads only one clone would be made and shared across threads.

@reta What I could have done is just move the clone call from TransferManager::fetchBlob to OnDemandBlockSnapshotIndexInput::fetchBlob, but I felt that TransferManager is the one that should be keeping track of what is cloned and should not be returning things that are not safe to use directly, which led to a slightly larger refactoring.

Oh I see what you mean, no I think the composition should be fine:

return asyncFetchBlob(blobFetchRequest.getFilePath(), () -> { try { return fetchBlob(blobFetchRequest); } catch (IOException e) { throw new IllegalStateException(e); } }).thenApply(r -> r.clone());

Indeed, fetchBlob would return a value from first concurrent execution (if any), but each invoker would get an unique clone of the value.

@andrross I agree that TransferManager should keep these guarantees

I think the composition should be fine

You're right, thenApply will do exactly what I want since each invocation will create a new future instance with the thenApply function applied to it.

github-actions · 2023-02-20T17:33:56Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11421/
CommitID: 4495659
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-02-20T17:37:48Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11422/
CommitID: 87ddd4a
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

andrross · 2023-02-20T18:32:31Z

REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.index.store.remote.utils.TransferManagerTests.testConcurrentAccess" -Dtests.seed=8617C6A8B33B1557 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=ar-SA -Dtests.timezone=America/St_Lucia -Druntime.java=19

org.opensearch.index.store.remote.utils.TransferManagerTests > testConcurrentAccess FAILED
    java.lang.IllegalStateException: FileSystem Cache per segment capacity is less than single IndexInput default block size
        at __randomizedtesting.SeedInfo.seed([8617C6A8B33B1557:490AF9A9D2BEE804]:0)
        at org.opensearch.index.store.remote.filecache.FileCacheFactory.createFileCache(FileCacheFactory.java:56)
        at org.opensearch.index.store.remote.filecache.FileCacheFactory.createConcurrentLRUFileCache(FileCacheFactory.java:41)
        at org.opensearch.index.store.remote.utils.TransferManagerTests.<init>(TransferManagerTests.java:40)

The number of cache segments is based off of Runtime.getAvailableProcessors and wow we have some large machines running the CI workflows :). I'll change this to remove the non-determinism here.

github-actions · 2023-02-20T19:03:28Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.search.SearchWeightedRoutingIT.testSearchAggregationWithNetworkDisruption_FailOpenEnabled

URL: https://build.ci.opensearch.org/job/gradle-check/11426/
CommitID: 35ece51
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

andrross · 2023-02-20T19:15:02Z

org.opensearch.search.SearchWeightedRoutingIT.testSearchAggregationWithNetworkDisruption_FailOpenEnabled

#5957

reta · 2023-02-20T21:55:45Z

server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java

-
-    private CompletableFuture<IndexInput> asyncFetchBlob(Path path, Supplier<IndexInput> indexInputSupplier) {
-        return invocationLinearizer.linearize(path, p -> indexInputSupplier.get());
+        }).thenApply(IndexInput::clone);


In PR opensearch-project#6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>

github-actions · 2023-02-20T22:09:05Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11439/
CommitID: cc23e5a
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-02-20T22:33:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/11440/
CommitID: c168022

In PR #6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com> (cherry picked from commit 5e5c83b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

In PR #6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. (cherry picked from commit 5e5c83b) Signed-off-by: Andrew Ross <andrross@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

andrross added backport 2.x Backport to 2.x branch skip-changelog labels Feb 18, 2023

andrross requested review from reta, anasalkouz, Bukhtawar, CEHENKLE, dblock, gbbafna, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, adnapibar, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners February 18, 2023 02:04

andrross force-pushed the clone-bug branch from c004d97 to 4495659 Compare February 20, 2023 17:05

reta reviewed Feb 20, 2023

View reviewed changes

kotwanikunal approved these changes Feb 20, 2023

View reviewed changes

andrross force-pushed the clone-bug branch from 35ece51 to cc23e5a Compare February 20, 2023 21:48

reta reviewed Feb 20, 2023

View reviewed changes

reta approved these changes Feb 20, 2023

View reviewed changes

andrross force-pushed the clone-bug branch from cc23e5a to c168022 Compare February 20, 2023 22:05

andrross merged commit 5e5c83b into opensearch-project:main Feb 20, 2023

andrross deleted the clone-bug branch February 20, 2023 22:36

opensearch-trigger-bot bot mentioned this pull request Feb 20, 2023

[Backport 2.x] [Searchable Snapshots] Correctly clone IndexInputs to avoid race #6393

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Searchable Snapshots] Correctly clone IndexInputs to avoid race #6367

[Searchable Snapshots] Correctly clone IndexInputs to avoid race #6367

andrross commented Feb 18, 2023

github-actions bot commented Feb 18, 2023

andrross commented Feb 20, 2023

reta Feb 20, 2023

andrross Feb 20, 2023 •

edited

Loading

andrross Feb 20, 2023

reta Feb 20, 2023

andrross Feb 20, 2023

andrross Feb 20, 2023

reta Feb 20, 2023

reta Feb 20, 2023

andrross Feb 20, 2023

github-actions bot commented Feb 20, 2023

github-actions bot commented Feb 20, 2023

andrross commented Feb 20, 2023

github-actions bot commented Feb 20, 2023

andrross commented Feb 20, 2023

reta Feb 20, 2023

github-actions bot commented Feb 20, 2023

github-actions bot commented Feb 20, 2023

[Searchable Snapshots] Correctly clone IndexInputs to avoid race #6367

[Searchable Snapshots] Correctly clone IndexInputs to avoid race #6367

Conversation

andrross commented Feb 18, 2023

Check List

github-actions bot commented Feb 18, 2023

Gradle Check (Jenkins) Run Completed with:

andrross commented Feb 20, 2023

Choose a reason for hiding this comment

andrross Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Feb 20, 2023

Gradle Check (Jenkins) Run Completed with:

andrross commented Feb 20, 2023

github-actions bot commented Feb 20, 2023

Gradle Check (Jenkins) Run Completed with:

andrross commented Feb 20, 2023

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Feb 20, 2023

Gradle Check (Jenkins) Run Completed with:

andrross Feb 20, 2023 •

edited

Loading