Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix simultaneously creating a snapshot and updating the repository can potentially trigger an infinite loop #17532

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),

### Fixed
- Fix bytes parameter on `_cat/recovery` ([#17598](https://github.com/opensearch-project/OpenSearch/pull/17598))
- Fix simultaneously creating a snapshot and updating the repository can potentially trigger an infinite loop ([#17532](https://github.com/opensearch-project/OpenSearch/pull/17532))

### Security

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,12 @@
import java.util.Collection;
import java.util.Collections;

import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS;
import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.hasToString;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.sameInstance;
Expand Down Expand Up @@ -122,4 +126,66 @@ public void testSystemRepositoryCantBeCreated() {

assertThrows(RepositoryException.class, () -> createRepository(repositoryName, FsRepository.TYPE, repoSettings));
}

public void testCreatSnapAndUpdateReposityCauseInfiniteLoop() throws InterruptedException {
// create index
internalCluster();
String indexName = "test-index";
createIndex(indexName, Settings.builder().put(SETTING_NUMBER_OF_REPLICAS, 0).put(SETTING_NUMBER_OF_SHARDS, 1).build());
index(indexName, "_doc", "1", Collections.singletonMap("user", generateRandomStringArray(1, 10, false, false)));
flush(indexName);

// create repository
final String repositoryName = "test-repo";
Settings.Builder repoSettings = Settings.builder()
.put("location", randomRepoPath())
.put("max_snapshot_bytes_per_sec", "10mb")
.put("max_restore_bytes_per_sec", "10mb");
OpenSearchIntegTestCase.putRepositoryWithNoSettingOverrides(
client().admin().cluster(),
repositoryName,
FsRepository.TYPE,
true,
repoSettings
);

String snapshotName = "test-snapshot";
Runnable createSnapshot = () -> {
logger.info("--> begining snapshot");
client().admin()
.cluster()
.prepareCreateSnapshot(repositoryName, snapshotName)
.setWaitForCompletion(true)
.setIndices(indexName)
.get();
logger.info("--> finishing snapshot");
};

// snapshot mab be failed when updating repository
Thread thread = new Thread(() -> {
try {
createSnapshot.run();
} catch (Exception e) {
assertThat(e, instanceOf(RepositoryException.class));
assertThat(e, hasToString(containsString(("the repository is closed"))));
}
});
thread.start();

logger.info("--> begin to reset repository");
repoSettings = Settings.builder().put("location", randomRepoPath()).put("max_snapshot_bytes_per_sec", "300mb");
OpenSearchIntegTestCase.putRepositoryWithNoSettingOverrides(
client().admin().cluster(),
repositoryName,
FsRepository.TYPE,
true,
repoSettings
);
logger.info("--> finish to reset repository");

// after updating repository, snapshot should be success
createSnapshot.run();

thread.join();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,8 @@
*/
protected volatile int bufferSize;

private volatile boolean closed;

/**
* Constructs new BlobStoreRepository
* @param repositoryMetadata The metadata for this repository including name and settings
Expand Down Expand Up @@ -630,6 +632,7 @@
}
if (store != null) {
try {
closed = true;
store.close();
} catch (Exception t) {
logger.warn("cannot close blob store", t);
Expand All @@ -643,6 +646,10 @@
String source,
Consumer<Exception> onFailure
) {
if (this.closed) {
onFailure.accept(new RepositoryException(metadata.name(), "the repository is closed, maybe updated or deleted, please retry"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to throw exception here, we just need to notify the user that the repo have changed and let the user decide whether to retry or not.

return;

Check warning on line 651 in server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java#L650-L651

Added lines #L650 - L651 were not covered by tests
}
final RepositoryMetadata repositoryMetadataStart = metadata;
getRepositoryData(ActionListener.wrap(repositoryData -> {
final ClusterStateUpdateTask updateTask = createUpdateTask.apply(repositoryData);
Expand Down
Loading