From 20a051b6cb9a98c330cabe41fb6dcb576d414bb7 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Thu, 31 Mar 2022 20:16:04 +0800 Subject: [PATCH 01/13] add RepositoriesDuplicatedCreationIT --- .../RepositoriesDuplicatedCreationIT.java | 121 ++++++++++++++++++ .../cluster/service/MasterService.java | 4 +- 2 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java diff --git a/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java b/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java new file mode 100644 index 0000000000000..ea8d01d41c55d --- /dev/null +++ b/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java @@ -0,0 +1,121 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.repositories; + +import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesResponse; +import org.elasticsearch.cluster.metadata.RepositoryMetadata; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.settings.Setting; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.BigArrays; +import org.elasticsearch.env.Environment; +import org.elasticsearch.indices.recovery.RecoverySettings; +import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.plugins.RepositoryPlugin; +import org.elasticsearch.snapshots.mockstore.MockRepository; +import org.elasticsearch.test.ESIntegTestCase; +import org.elasticsearch.xcontent.NamedXContentRegistry; + +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; + +@ESIntegTestCase.ClusterScope(supportsDedicatedMasters = false, numDataNodes = 3) +public class RepositoriesDuplicatedCreationIT extends ESIntegTestCase { + @Override + protected Collection> nodePlugins() { + return Collections.singletonList(UnstableRepository.Plugin.class); + } + + public static class UnstableRepository extends MockRepository { + public static final String TYPE = "unstable"; + public static final Setting> UNSTABLE_NODES = Setting.stringListSetting( + "repository.unstable_nodes", + Setting.Property.NodeScope, + Setting.Property.Dynamic + ); + + public UnstableRepository( + RepositoryMetadata metadata, + Environment environment, + NamedXContentRegistry namedXContentRegistry, + ClusterService clusterService, + BigArrays bigArrays, + RecoverySettings recoverySettings + ) { + super(metadata, environment, namedXContentRegistry, clusterService, bigArrays, recoverySettings); + List unstableNodes = UNSTABLE_NODES.get(metadata.settings()); + if (unstableNodes.contains(clusterService.getNodeName())) { + throw new RepositoryException(metadata.name(), "Failed to create repository: current node is not stable"); + } + } + + public static class Plugin extends org.elasticsearch.plugins.Plugin implements RepositoryPlugin { + @Override + public Map getRepositories( + Environment env, + NamedXContentRegistry namedXContentRegistry, + ClusterService clusterService, + BigArrays bigArrays, + RecoverySettings recoverySettings + ) { + return Collections.singletonMap( + TYPE, + (metadata) -> new UnstableRepository(metadata, env, namedXContentRegistry, clusterService, bigArrays, recoverySettings) + ); + } + + @Override + public List> getSettings() { + return List.of(UNSTABLE_NODES); + } + } + } + + public void testDuplicateCreateRepository() throws Exception { + final String repositoryName = "test-duplicate-create-repo"; + String unstableNode = Strings.arrayToCommaDelimitedString( + Arrays.stream(internalCluster().getNodeNames()) + .filter(name -> name.equals(internalCluster().getMasterName()) == false) + .toArray() + ); + + // put repository for the first time: only let master node create repository successfully + createRepository( + repositoryName, + UnstableRepository.TYPE, + Settings.builder().put("location", randomRepoPath()).put(UnstableRepository.UNSTABLE_NODES.getKey(), unstableNode) + ); + + // restart master + internalCluster().restartNode(internalCluster().getMasterName()); + ensureGreen(); + + // put repository again: let all node can create repository successfully + createRepository(repositoryName, UnstableRepository.TYPE, Settings.builder().put("location", randomRepoPath())); + } + + private void createRepository(String name, String type, Settings.Builder settings) { + // create + assertAcked(client().admin().cluster().preparePutRepository(name).setType(type).setVerify(false).setSettings(settings).get()); + // get + final GetRepositoriesResponse updatedGetRepositoriesResponse = client().admin().cluster().prepareGetRepositories(name).get(); + // assert + assertThat(updatedGetRepositoriesResponse.repositories(), hasSize(1)); + final RepositoryMetadata updatedRepositoryMetadata = updatedGetRepositoriesResponse.repositories().get(0); + assertThat(updatedRepositoryMetadata.type(), equalTo(type)); + } +} diff --git a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java index e9e2574ea6661..9c12c59967314 100644 --- a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java +++ b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java @@ -899,7 +899,7 @@ private static ClusterState innerExecuteTasks( final var taskContexts = castTaskContexts(executionResults); try { return executor.execute(previousClusterState, taskContexts); - } catch (Exception e) { + } catch (Throwable e) { logger.trace( () -> new ParameterizedMessage( "failed to execute cluster state update (on version: [{}], uuid: [{}]) for [{}]\n{}{}{}", @@ -913,7 +913,7 @@ private static ClusterState innerExecuteTasks( e ); for (final var executionResult : executionResults) { - executionResult.onBatchFailure(e); + executionResult.onBatchFailure(new Exception(e)); } return previousClusterState; } From 5660a786ed5299a449b6c7b13587d17332e85ba9 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Thu, 31 Mar 2022 20:36:42 +0800 Subject: [PATCH 02/13] fix NPE in RepositoriesService --- .../repositories/RepositoriesService.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java index 84b628d1ecc2f..b23e74950acf2 100644 --- a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java @@ -213,8 +213,7 @@ public ClusterState execute(ClusterState currentState) { if (existing == null) { existing = RepositoriesService.this.internalRepositories.get(request.name()); } - assert existing != null : "repository [" + newRepositoryMetadata.name() + "] must exist"; - assert existing.getMetadata() == repositoryMetadata; + assert existing == null || existing.getMetadata() == repositoryMetadata; final RepositoryMetadata updatedMetadata; if (canUpdateInPlace(newRepositoryMetadata, existing)) { if (repositoryMetadata.settings().equals(newRepositoryMetadata.settings())) { @@ -540,9 +539,10 @@ public void applyClusterState(ClusterChangedEvent event) { } private static boolean canUpdateInPlace(RepositoryMetadata updatedMetadata, Repository repository) { - assert updatedMetadata.name().equals(repository.getMetadata().name()); - return repository.getMetadata().type().equals(updatedMetadata.type()) - && repository.canUpdateInPlace(updatedMetadata.settings(), Collections.emptySet()); + assert repository == null || updatedMetadata.name().equals(repository.getMetadata().name()); + return repository != null + || repository.getMetadata().type().equals(updatedMetadata.type()) + && repository.canUpdateInPlace(updatedMetadata.settings(), Collections.emptySet()); } /** From 6fe56a1a8e6f0f6865866c185d6e01eda41478e0 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Fri, 1 Apr 2022 17:21:20 +0800 Subject: [PATCH 03/13] revert MasterService change --- .../java/org/elasticsearch/cluster/service/MasterService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java index 9c12c59967314..e9e2574ea6661 100644 --- a/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java +++ b/server/src/main/java/org/elasticsearch/cluster/service/MasterService.java @@ -899,7 +899,7 @@ private static ClusterState innerExecuteTasks( final var taskContexts = castTaskContexts(executionResults); try { return executor.execute(previousClusterState, taskContexts); - } catch (Throwable e) { + } catch (Exception e) { logger.trace( () -> new ParameterizedMessage( "failed to execute cluster state update (on version: [{}], uuid: [{}]) for [{}]\n{}{}{}", @@ -913,7 +913,7 @@ private static ClusterState innerExecuteTasks( e ); for (final var executionResult : executionResults) { - executionResult.onBatchFailure(new Exception(e)); + executionResult.onBatchFailure(e); } return previousClusterState; } From 076f79090b58146d4976e29c05c65d0cfbb69be2 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Fri, 1 Apr 2022 21:46:05 +0800 Subject: [PATCH 04/13] create DummyRepository when repository creation is failed --- .../repositories/DummyRepository.java | 187 ++++++++++++++++++ .../repositories/RepositoriesService.java | 17 +- .../repositories/DummyRepositoryTests.java | 32 +++ .../RepositoriesServiceTests.java | 83 ++++++++ 4 files changed, 311 insertions(+), 8 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/repositories/DummyRepository.java create mode 100644 server/src/test/java/org/elasticsearch/repositories/DummyRepositoryTests.java diff --git a/server/src/main/java/org/elasticsearch/repositories/DummyRepository.java b/server/src/main/java/org/elasticsearch/repositories/DummyRepository.java new file mode 100644 index 0000000000000..529b0aa57fc27 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/repositories/DummyRepository.java @@ -0,0 +1,187 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.repositories; + +import org.elasticsearch.Version; +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.ClusterStateUpdateTask; +import org.elasticsearch.cluster.metadata.IndexMetadata; +import org.elasticsearch.cluster.metadata.Metadata; +import org.elasticsearch.cluster.metadata.RepositoryMetadata; +import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.common.component.AbstractLifecycleComponent; +import org.elasticsearch.index.shard.ShardId; +import org.elasticsearch.index.snapshots.IndexShardSnapshotStatus; +import org.elasticsearch.index.store.Store; +import org.elasticsearch.indices.recovery.RecoveryState; +import org.elasticsearch.snapshots.SnapshotId; + +import java.io.IOException; +import java.util.Collection; +import java.util.function.Consumer; +import java.util.function.Function; + +/** + * This class represents a repository that failed during creation. + * This could happen when repository plugin is not stable enough to guarantee repository creation success all the time + */ +public class DummyRepository extends AbstractLifecycleComponent implements Repository { + + private final RepositoryMetadata repositoryMetadata; + + public DummyRepository(RepositoryMetadata repositoryMetadata) { + this.repositoryMetadata = repositoryMetadata; + } + + private RepositoryException createCreationException() { + return new RepositoryException( + repositoryMetadata.name(), + "repository type [" + repositoryMetadata.type() + "] failed to create on current node" + ); + } + + @Override + public RepositoryMetadata getMetadata() { + return repositoryMetadata; + } + + @Override + public void getSnapshotInfo(GetSnapshotInfoContext context) { + throw createCreationException(); + } + + @Override + public Metadata getSnapshotGlobalMetadata(SnapshotId snapshotId) { + throw createCreationException(); + } + + @Override + public IndexMetadata getSnapshotIndexMetaData(RepositoryData repositoryData, SnapshotId snapshotId, IndexId index) throws IOException { + throw createCreationException(); + } + + @Override + public void getRepositoryData(ActionListener listener) { + listener.onFailure(createCreationException()); + } + + @Override + public void finalizeSnapshot(FinalizeSnapshotContext finalizeSnapshotContext) { + finalizeSnapshotContext.onFailure(createCreationException()); + } + + @Override + public void deleteSnapshots( + Collection snapshotIds, + long repositoryStateId, + Version repositoryMetaVersion, + ActionListener listener + ) { + listener.onFailure(createCreationException()); + } + + @Override + public long getSnapshotThrottleTimeInNanos() { + throw createCreationException(); + } + + @Override + public long getRestoreThrottleTimeInNanos() { + throw createCreationException(); + } + + @Override + public String startVerification() { + throw createCreationException(); + } + + @Override + public void endVerification(String verificationToken) { + throw createCreationException(); + } + + @Override + public void verify(String verificationToken, DiscoveryNode localNode) { + throw createCreationException(); + } + + @Override + public boolean isReadOnly() { + // this repository is assumed writable to bypass read-only check and fail with exception produced by this class + return false; + } + + @Override + public void snapshotShard(SnapshotShardContext snapshotShardContext) { + snapshotShardContext.onFailure(createCreationException()); + } + + @Override + public void restoreShard( + Store store, + SnapshotId snapshotId, + IndexId indexId, + ShardId snapshotShardId, + RecoveryState recoveryState, + ActionListener listener + ) { + listener.onFailure(createCreationException()); + } + + @Override + public IndexShardSnapshotStatus getShardSnapshotStatus(SnapshotId snapshotId, IndexId indexId, ShardId shardId) { + throw createCreationException(); + } + + @Override + public void updateState(ClusterState state) { + + } + + @Override + public void executeConsistentStateUpdate( + Function createUpdateTask, + String source, + Consumer onFailure + ) { + onFailure.accept(createCreationException()); + } + + @Override + public void cloneShardSnapshot( + SnapshotId source, + SnapshotId target, + RepositoryShardId shardId, + ShardGeneration shardGeneration, + ActionListener listener + ) { + listener.onFailure(createCreationException()); + } + + @Override + public void awaitIdle() { + + } + + @Override + protected void doStart() { + + } + + @Override + protected void doStop() { + + } + + @Override + protected void doClose() throws IOException { + + } +} diff --git a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java index b23e74950acf2..92d4735ea1ece 100644 --- a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java @@ -213,7 +213,8 @@ public ClusterState execute(ClusterState currentState) { if (existing == null) { existing = RepositoriesService.this.internalRepositories.get(request.name()); } - assert existing == null || existing.getMetadata() == repositoryMetadata; + assert existing != null : "repository [" + newRepositoryMetadata.name() + "] must exist"; + assert existing.getMetadata() == repositoryMetadata; final RepositoryMetadata updatedMetadata; if (canUpdateInPlace(newRepositoryMetadata, existing)) { if (repositoryMetadata.settings().equals(newRepositoryMetadata.settings())) { @@ -523,10 +524,11 @@ public void applyClusterState(ClusterChangedEvent event) { logger.warn(() -> new ParameterizedMessage("failed to create repository [{}]", repositoryMetadata.name()), ex); } } - if (repository != null) { - logger.debug("registering repository [{}]", repositoryMetadata.name()); - builder.put(repositoryMetadata.name(), repository); + if (repository == null) { + repository = new DummyRepository(repositoryMetadata); } + logger.debug("registering repository [{}]", repositoryMetadata.name()); + builder.put(repositoryMetadata.name(), repository); } for (Repository repo : builder.values()) { repo.updateState(state); @@ -539,10 +541,9 @@ public void applyClusterState(ClusterChangedEvent event) { } private static boolean canUpdateInPlace(RepositoryMetadata updatedMetadata, Repository repository) { - assert repository == null || updatedMetadata.name().equals(repository.getMetadata().name()); - return repository != null - || repository.getMetadata().type().equals(updatedMetadata.type()) - && repository.canUpdateInPlace(updatedMetadata.settings(), Collections.emptySet()); + assert updatedMetadata.name().equals(repository.getMetadata().name()); + return repository.getMetadata().type().equals(updatedMetadata.type()) + && repository.canUpdateInPlace(updatedMetadata.settings(), Collections.emptySet()); } /** diff --git a/server/src/test/java/org/elasticsearch/repositories/DummyRepositoryTests.java b/server/src/test/java/org/elasticsearch/repositories/DummyRepositoryTests.java new file mode 100644 index 0000000000000..cd3783a003ddf --- /dev/null +++ b/server/src/test/java/org/elasticsearch/repositories/DummyRepositoryTests.java @@ -0,0 +1,32 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.repositories; + +import org.elasticsearch.cluster.metadata.RepositoryMetadata; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.snapshots.SnapshotId; +import org.elasticsearch.test.ESTestCase; + +public class DummyRepositoryTests extends ESTestCase { + + private DummyRepository repository = new DummyRepository(new RepositoryMetadata("name", "type", Settings.EMPTY)); + + public void testShouldThrowWhenGettingMetadata() { + expectThrows(RepositoryException.class, () -> repository.getSnapshotGlobalMetadata(new SnapshotId("name", "uuid"))); + } + + public void testShouldNotThrowWhenApplyingLifecycleChanges() { + repository.start(); + repository.stop(); + } + + public void testShouldNotThrowWhenClosingToAllowRemovingRepo() { + repository.close(); + } +} diff --git a/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java b/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java index d4ec767e5817c..187683500a3f1 100644 --- a/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java @@ -85,6 +85,8 @@ public void setUp() throws Exception { Map typesRegistry = Map.of( TestRepository.TYPE, TestRepository::new, + UnstableRepository.TYPE, + UnstableRepository::new, MeteredRepositoryTypeA.TYPE, metadata -> new MeteredRepositoryTypeA(metadata, clusterService), MeteredRepositoryTypeB.TYPE, @@ -212,6 +214,78 @@ public void onFailure(Exception e) { }); } + // test DummyRepository is returned if repository failed to create + public void testHandlesCreationFailureWhenApplyingClusterState() { + var repoName = randomAlphaOfLengthBetween(10, 25); + + var clusterState = createClusterStateWithRepo(repoName, UnstableRepository.TYPE); + repositoriesService.applyClusterState(new ClusterChangedEvent("put unstable repository", clusterState, emptyState())); + + var repo = repositoriesService.repository(repoName); + assertThat(repo, isA(DummyRepository.class)); + } + + // test DummyRepository can be replaced if current repo is created successfully + public void testReplaceDummyRepositoryWhenCreationSuccess() { + var repoName = randomAlphaOfLengthBetween(10, 25); + + var clusterState = createClusterStateWithRepo(repoName, UnstableRepository.TYPE); + repositoriesService.applyClusterState(new ClusterChangedEvent("put unstable repository", clusterState, emptyState())); + + var repo = repositoriesService.repository(repoName); + assertThat(repo, isA(DummyRepository.class)); + + clusterState = createClusterStateWithRepo(repoName, TestRepository.TYPE); + repositoriesService.applyClusterState(new ClusterChangedEvent("put test repository", clusterState, emptyState())); + repo = repositoriesService.repository(repoName); + assertThat(repo, isA(TestRepository.class)); + } + + // test remove DummyRepository when current repo is removed in cluster state + public void testRemoveDummyRepositoryTypeWhenApplyingClusterState() { + var repoName = randomAlphaOfLengthBetween(10, 25); + + var clusterState = createClusterStateWithRepo(repoName, UnstableRepository.TYPE); + repositoriesService.applyClusterState(new ClusterChangedEvent("put unstable repository", clusterState, emptyState())); + repositoriesService.applyClusterState(new ClusterChangedEvent("removing repo", emptyState(), clusterState)); + + assertThatThrows( + () -> repositoriesService.repository(repoName), + RepositoryMissingException.class, + equalTo("[" + repoName + "] missing") + ); + } + + // DummyRepository is created when current node is non-master node and failed to create repository by applying cluster state from master + // When current node become master node later and same repository is put again, current node can create repository successfully and + // replace previous DummyRepository + public void testRegisterRepositorySuccessAfterCreationFailed() { + // 1. repository creation failed when current node is non-master node and apply cluster state from master + var repoName = randomAlphaOfLengthBetween(10, 25); + + var clusterState = createClusterStateWithRepo(repoName, UnstableRepository.TYPE); + repositoriesService.applyClusterState(new ClusterChangedEvent("put unstable repository", clusterState, emptyState())); + + var repo = repositoriesService.repository(repoName); + assertThat(repo, isA(DummyRepository.class)); + + // 2. repository creation successfully when current node become master node and repository is put again + var request = new PutRepositoryRequest().name(repoName).type(TestRepository.TYPE); + + repositoriesService.registerRepository(request, new ActionListener<>() { + @Override + public void onResponse(AcknowledgedResponse acknowledgedResponse) { + assertTrue(acknowledgedResponse.isAcknowledged()); + assertThat(repositoriesService.repository(repoName), isA(TestRepository.class)); + } + + @Override + public void onFailure(Exception e) { + fail("[" + repoName + "] repository type [test] faield to create"); + } + }); + } + private ClusterState createClusterStateWithRepo(String repoName, String repoType) { ClusterState.Builder state = ClusterState.builder(new ClusterName("test")); Metadata.Builder mdBuilder = Metadata.builder(); @@ -391,6 +465,15 @@ public void close() { } } + private static class UnstableRepository extends TestRepository { + private static final String TYPE = "unstable"; + + private UnstableRepository(RepositoryMetadata metadata) { + super(metadata); + throw new RepositoryException(TYPE, "failed to create unstable repository"); + } + } + private static class MeteredRepositoryTypeA extends MeteredBlobStoreRepository { private static final String TYPE = "type-a"; private static final RepositoryStats STATS = new RepositoryStats(Map.of("GET", 10L)); From 8ca6af5d0c88f02dc9bf549642f5e484474b695a Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Fri, 1 Apr 2022 22:12:27 +0800 Subject: [PATCH 05/13] add changelog file --- docs/changelog/85551.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/85551.yaml diff --git a/docs/changelog/85551.yaml b/docs/changelog/85551.yaml new file mode 100644 index 0000000000000..c85b2e11d6d98 --- /dev/null +++ b/docs/changelog/85551.yaml @@ -0,0 +1,5 @@ +pr: 85534 +summary: "Fix NPE in RepositoriesService" +area: Snapshot/Restore +type: bug +issues: [85550] From db544c1faf32ee4ec2ea75008ef1b1e9d8c16e97 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Fri, 1 Apr 2022 23:44:24 +0800 Subject: [PATCH 06/13] optimize code --- .../RepositoriesDuplicatedCreationIT.java | 25 +++++++++++++++++++ ...Repository.java => InvalidRepository.java} | 9 ++++--- .../repositories/RepositoriesService.java | 9 ++++--- ...Tests.java => InvalidRepositoryTests.java} | 7 ++++-- .../RepositoriesServiceTests.java | 22 ++++++++-------- 5 files changed, 52 insertions(+), 20 deletions(-) rename server/src/main/java/org/elasticsearch/repositories/{DummyRepository.java => InvalidRepository.java} (93%) rename server/src/test/java/org/elasticsearch/repositories/{DummyRepositoryTests.java => InvalidRepositoryTests.java} (79%) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java b/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java index ea8d01d41c55d..cebc161e74af7 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java @@ -9,6 +9,7 @@ package org.elasticsearch.repositories; import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesResponse; +import org.elasticsearch.action.admin.cluster.repositories.verify.VerifyRepositoryResponse; import org.elasticsearch.cluster.metadata.RepositoryMetadata; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Strings; @@ -30,8 +31,10 @@ import java.util.Map; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.elasticsearch.test.hamcrest.ThrowableAssertions.assertThatException; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.isA; @ESIntegTestCase.ClusterScope(supportsDedicatedMasters = false, numDataNodes = 3) public class RepositoriesDuplicatedCreationIT extends ESIntegTestCase { @@ -99,6 +102,24 @@ public void testDuplicateCreateRepository() throws Exception { UnstableRepository.TYPE, Settings.builder().put("location", randomRepoPath()).put(UnstableRepository.UNSTABLE_NODES.getKey(), unstableNode) ); + // verification should fail with some node has InvalidRepository + try { + client().admin().cluster().prepareVerifyRepository(repositoryName).get(); + fail("verification should fail when some node failed to create repository"); + } catch (Exception e) { + assertThat(e, isA(RepositoryVerificationException.class)); + assertEquals(e.getSuppressed().length, 2); + assertThatException( + e.getSuppressed()[0].getCause(), + RepositoryException.class, + equalTo("[" + repositoryName + "] repository type [" + UnstableRepository.TYPE + "] failed to create on current node") + ); + assertThatException( + e.getSuppressed()[1].getCause(), + RepositoryException.class, + equalTo("[" + repositoryName + "] repository type [" + UnstableRepository.TYPE + "] failed to create on current node") + ); + } // restart master internalCluster().restartNode(internalCluster().getMasterName()); @@ -106,6 +127,10 @@ public void testDuplicateCreateRepository() throws Exception { // put repository again: let all node can create repository successfully createRepository(repositoryName, UnstableRepository.TYPE, Settings.builder().put("location", randomRepoPath())); + // verification should succeed with all node create repository successfully + VerifyRepositoryResponse verifyRepositoryResponse = client().admin().cluster().prepareVerifyRepository(repositoryName).get(); + assertEquals(verifyRepositoryResponse.getNodes().size(), 3); + } private void createRepository(String name, String type, Settings.Builder settings) { diff --git a/server/src/main/java/org/elasticsearch/repositories/DummyRepository.java b/server/src/main/java/org/elasticsearch/repositories/InvalidRepository.java similarity index 93% rename from server/src/main/java/org/elasticsearch/repositories/DummyRepository.java rename to server/src/main/java/org/elasticsearch/repositories/InvalidRepository.java index 529b0aa57fc27..408446ca73fde 100644 --- a/server/src/main/java/org/elasticsearch/repositories/DummyRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/InvalidRepository.java @@ -32,18 +32,21 @@ * This class represents a repository that failed during creation. * This could happen when repository plugin is not stable enough to guarantee repository creation success all the time */ -public class DummyRepository extends AbstractLifecycleComponent implements Repository { +public class InvalidRepository extends AbstractLifecycleComponent implements Repository { private final RepositoryMetadata repositoryMetadata; + private final RepositoryException creationException; - public DummyRepository(RepositoryMetadata repositoryMetadata) { + public InvalidRepository(RepositoryMetadata repositoryMetadata, RepositoryException creationException) { this.repositoryMetadata = repositoryMetadata; + this.creationException = creationException; } private RepositoryException createCreationException() { return new RepositoryException( repositoryMetadata.name(), - "repository type [" + repositoryMetadata.type() + "] failed to create on current node" + "repository type [" + repositoryMetadata.type() + "] failed to create on current node", + creationException ); } diff --git a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java index 92d4735ea1ece..ae430bd4244cb 100644 --- a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java @@ -515,6 +515,7 @@ public void applyClusterState(ClusterChangedEvent event) { // TODO: this catch is bogus, it means the old repo is already closed, // but we have nothing to replace it logger.warn(() -> new ParameterizedMessage("failed to change repository [{}]", repositoryMetadata.name()), ex); + repository = new InvalidRepository(repositoryMetadata, ex); } } } else { @@ -522,13 +523,13 @@ public void applyClusterState(ClusterChangedEvent event) { repository = createRepository(repositoryMetadata, typesRegistry, RepositoriesService::createUnknownTypeRepository); } catch (RepositoryException ex) { logger.warn(() -> new ParameterizedMessage("failed to create repository [{}]", repositoryMetadata.name()), ex); + repository = new InvalidRepository(repositoryMetadata, ex); } } - if (repository == null) { - repository = new DummyRepository(repositoryMetadata); + if (repository != null) { + logger.debug("registering repository [{}]", repositoryMetadata.name()); + builder.put(repositoryMetadata.name(), repository); } - logger.debug("registering repository [{}]", repositoryMetadata.name()); - builder.put(repositoryMetadata.name(), repository); } for (Repository repo : builder.values()) { repo.updateState(state); diff --git a/server/src/test/java/org/elasticsearch/repositories/DummyRepositoryTests.java b/server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java similarity index 79% rename from server/src/test/java/org/elasticsearch/repositories/DummyRepositoryTests.java rename to server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java index cd3783a003ddf..2b74f27438485 100644 --- a/server/src/test/java/org/elasticsearch/repositories/DummyRepositoryTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java @@ -13,9 +13,12 @@ import org.elasticsearch.snapshots.SnapshotId; import org.elasticsearch.test.ESTestCase; -public class DummyRepositoryTests extends ESTestCase { +public class InvalidRepositoryTests extends ESTestCase { - private DummyRepository repository = new DummyRepository(new RepositoryMetadata("name", "type", Settings.EMPTY)); + private InvalidRepository repository = new InvalidRepository( + new RepositoryMetadata("name", "type", Settings.EMPTY), + new RepositoryException("name", "failed to create repository") + ); public void testShouldThrowWhenGettingMetadata() { expectThrows(RepositoryException.class, () -> repository.getSnapshotGlobalMetadata(new SnapshotId("name", "uuid"))); diff --git a/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java b/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java index 187683500a3f1..7f84dceb73621 100644 --- a/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java @@ -214,7 +214,7 @@ public void onFailure(Exception e) { }); } - // test DummyRepository is returned if repository failed to create + // test InvalidRepository is returned if repository failed to create public void testHandlesCreationFailureWhenApplyingClusterState() { var repoName = randomAlphaOfLengthBetween(10, 25); @@ -222,18 +222,18 @@ public void testHandlesCreationFailureWhenApplyingClusterState() { repositoriesService.applyClusterState(new ClusterChangedEvent("put unstable repository", clusterState, emptyState())); var repo = repositoriesService.repository(repoName); - assertThat(repo, isA(DummyRepository.class)); + assertThat(repo, isA(InvalidRepository.class)); } - // test DummyRepository can be replaced if current repo is created successfully - public void testReplaceDummyRepositoryWhenCreationSuccess() { + // test InvalidRepository can be replaced if current repo is created successfully + public void testReplaceInvalidRepositoryWhenCreationSuccess() { var repoName = randomAlphaOfLengthBetween(10, 25); var clusterState = createClusterStateWithRepo(repoName, UnstableRepository.TYPE); repositoriesService.applyClusterState(new ClusterChangedEvent("put unstable repository", clusterState, emptyState())); var repo = repositoriesService.repository(repoName); - assertThat(repo, isA(DummyRepository.class)); + assertThat(repo, isA(InvalidRepository.class)); clusterState = createClusterStateWithRepo(repoName, TestRepository.TYPE); repositoriesService.applyClusterState(new ClusterChangedEvent("put test repository", clusterState, emptyState())); @@ -241,8 +241,8 @@ public void testReplaceDummyRepositoryWhenCreationSuccess() { assertThat(repo, isA(TestRepository.class)); } - // test remove DummyRepository when current repo is removed in cluster state - public void testRemoveDummyRepositoryTypeWhenApplyingClusterState() { + // test remove InvalidRepository when current repo is removed in cluster state + public void testRemoveInvalidRepositoryTypeWhenApplyingClusterState() { var repoName = randomAlphaOfLengthBetween(10, 25); var clusterState = createClusterStateWithRepo(repoName, UnstableRepository.TYPE); @@ -256,9 +256,9 @@ public void testRemoveDummyRepositoryTypeWhenApplyingClusterState() { ); } - // DummyRepository is created when current node is non-master node and failed to create repository by applying cluster state from master - // When current node become master node later and same repository is put again, current node can create repository successfully and - // replace previous DummyRepository + // InvalidRepository is created when current node is non-master node and failed to create repository by applying cluster state from + // master. When current node become master node later and same repository is put again, current node can create repository successfully + // and replace previous InvalidRepository public void testRegisterRepositorySuccessAfterCreationFailed() { // 1. repository creation failed when current node is non-master node and apply cluster state from master var repoName = randomAlphaOfLengthBetween(10, 25); @@ -267,7 +267,7 @@ public void testRegisterRepositorySuccessAfterCreationFailed() { repositoriesService.applyClusterState(new ClusterChangedEvent("put unstable repository", clusterState, emptyState())); var repo = repositoriesService.repository(repoName); - assertThat(repo, isA(DummyRepository.class)); + assertThat(repo, isA(InvalidRepository.class)); // 2. repository creation successfully when current node become master node and repository is put again var request = new PutRepositoryRequest().name(repoName).type(TestRepository.TYPE); From c9759e3d875ba56a864537ffc005c419f414fcf6 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Sat, 2 Apr 2022 00:36:30 +0800 Subject: [PATCH 07/13] add fail after code.run in ThrowableAssertions#assertThatThrows --- .../org/elasticsearch/test/hamcrest/ThrowableAssertions.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java b/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java index 2aa43444af72d..a8a37ca23a89a 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java +++ b/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java @@ -12,6 +12,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.isA; +import static org.junit.Assert.fail; /** * Assertions for exceptions and their messages @@ -25,6 +26,7 @@ public static void assertThatThrows( ) { try { code.run(); + fail("Exception with type " + exceptionType.getName() + "should be thrown"); } catch (Throwable e) { assertThatException(e, exceptionType, messageMatcher); } From 87011b765127434df1734f54695b5572174dd556 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Sat, 2 Apr 2022 11:12:30 +0800 Subject: [PATCH 08/13] fix shard failure reason in EncryptedRepositorySecretIntegTests --- .../EncryptedRepositorySecretIntegTests.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedRepositorySecretIntegTests.java b/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedRepositorySecretIntegTests.java index 98e780d2ea4df..27d97f849d78e 100644 --- a/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedRepositorySecretIntegTests.java +++ b/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedRepositorySecretIntegTests.java @@ -420,7 +420,16 @@ public void testSnapshotIsPartialForMissingPassword() throws Exception { incompleteSnapshotResponse.getSnapshotInfo() .shardFailures() .stream() - .allMatch(shardFailure -> shardFailure.reason().contains("[" + repositoryName + "] missing")) + .allMatch( + shardFailure -> shardFailure.reason() + .contains( + "RepositoryException[[" + + repositoryName + + "] failed to create repository]; nested: IllegalArgumentException[Secure setting [repository.encrypted." + + repositoryName + + ".password] must be set]" + ) + ) ); assertThat( incompleteSnapshotResponse.getSnapshotInfo().userMetadata(), From 323aa3ecfbd0eb4ca1c93aed28b8ff1802091579 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Sat, 2 Apr 2022 22:53:40 +0800 Subject: [PATCH 09/13] update --- docs/changelog/85551.yaml | 4 +- .../RepositoriesDuplicatedCreationIT.java | 45 ++++++++++--------- .../repositories/InvalidRepository.java | 3 +- .../repositories/RepositoriesService.java | 7 ++- .../repositories/InvalidRepositoryTests.java | 20 ++++----- .../RepositoriesServiceTests.java | 2 +- .../test/hamcrest/ThrowableAssertions.java | 24 +++++++++- .../EncryptedRepositorySecretIntegTests.java | 8 +--- 8 files changed, 65 insertions(+), 48 deletions(-) diff --git a/docs/changelog/85551.yaml b/docs/changelog/85551.yaml index c85b2e11d6d98..82ec1ed648325 100644 --- a/docs/changelog/85551.yaml +++ b/docs/changelog/85551.yaml @@ -1,5 +1,5 @@ -pr: 85534 -summary: "Fix NPE in RepositoriesService" +pr: 85551 +summary: "Distinguish missing and invalid repositories" area: Snapshot/Restore type: bug issues: [85550] diff --git a/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java b/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java index cebc161e74af7..8ffc4a6abc5cf 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java @@ -12,7 +12,6 @@ import org.elasticsearch.action.admin.cluster.repositories.verify.VerifyRepositoryResponse; import org.elasticsearch.cluster.metadata.RepositoryMetadata; import org.elasticsearch.cluster.service.ClusterService; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.BigArrays; @@ -88,38 +87,44 @@ public List> getSettings() { } } - public void testDuplicateCreateRepository() throws Exception { + public void testCreateInvalidRepository() throws Exception { final String repositoryName = "test-duplicate-create-repo"; - String unstableNode = Strings.arrayToCommaDelimitedString( - Arrays.stream(internalCluster().getNodeNames()) - .filter(name -> name.equals(internalCluster().getMasterName()) == false) - .toArray() - ); // put repository for the first time: only let master node create repository successfully createRepository( repositoryName, UnstableRepository.TYPE, - Settings.builder().put("location", randomRepoPath()).put(UnstableRepository.UNSTABLE_NODES.getKey(), unstableNode) + Settings.builder() + .put("location", randomRepoPath()) + .putList( + UnstableRepository.UNSTABLE_NODES.getKey(), + Arrays.stream(internalCluster().getNodeNames()) + .filter(name -> name.equals(internalCluster().getMasterName()) == false) + .toList() + ) ); // verification should fail with some node has InvalidRepository + boolean verifyPass = false; try { client().admin().cluster().prepareVerifyRepository(repositoryName).get(); - fail("verification should fail when some node failed to create repository"); + verifyPass = true; } catch (Exception e) { assertThat(e, isA(RepositoryVerificationException.class)); - assertEquals(e.getSuppressed().length, 2); - assertThatException( - e.getSuppressed()[0].getCause(), - RepositoryException.class, - equalTo("[" + repositoryName + "] repository type [" + UnstableRepository.TYPE + "] failed to create on current node") - ); - assertThatException( - e.getSuppressed()[1].getCause(), - RepositoryException.class, - equalTo("[" + repositoryName + "] repository type [" + UnstableRepository.TYPE + "] failed to create on current node") - ); + assertEquals(e.getSuppressed().length, internalCluster().numDataAndMasterNodes() - 1); + for (Throwable suppressed : e.getSuppressed()) { + assertThatException( + suppressed.getCause(), + RepositoryException.class, + equalTo("[" + repositoryName + "] repository type [" + UnstableRepository.TYPE + "] failed to create on current node") + ); + assertThatException( + suppressed.getCause().getCause().getCause(), + RepositoryException.class, + equalTo("[" + repositoryName + "] Failed to create repository: current node is not stable") + ); + } } + assertFalse("verification should fail when some node failed to create repository", verifyPass); // restart master internalCluster().restartNode(internalCluster().getMasterName()); diff --git a/server/src/main/java/org/elasticsearch/repositories/InvalidRepository.java b/server/src/main/java/org/elasticsearch/repositories/InvalidRepository.java index 408446ca73fde..1f11def3c8010 100644 --- a/server/src/main/java/org/elasticsearch/repositories/InvalidRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/InvalidRepository.java @@ -29,8 +29,7 @@ import java.util.function.Function; /** - * This class represents a repository that failed during creation. - * This could happen when repository plugin is not stable enough to guarantee repository creation success all the time + * Represents a repository that exists in the cluster state but could not be instantiated on a node, typically due to invalid configuration. */ public class InvalidRepository extends AbstractLifecycleComponent implements Repository { diff --git a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java index ae430bd4244cb..0ecd1a6d32c6e 100644 --- a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java @@ -526,10 +526,9 @@ public void applyClusterState(ClusterChangedEvent event) { repository = new InvalidRepository(repositoryMetadata, ex); } } - if (repository != null) { - logger.debug("registering repository [{}]", repositoryMetadata.name()); - builder.put(repositoryMetadata.name(), repository); - } + assert repository != null : "repository should not be null here"; + logger.debug("registering repository [{}]", repositoryMetadata.name()); + builder.put(repositoryMetadata.name(), repository); } for (Repository repo : builder.values()) { repo.updateState(state); diff --git a/server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java b/server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java index 2b74f27438485..0a6c4c5e9f8b2 100644 --- a/server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java @@ -13,6 +13,9 @@ import org.elasticsearch.snapshots.SnapshotId; import org.elasticsearch.test.ESTestCase; +import static org.elasticsearch.test.hamcrest.ThrowableAssertions.assertThatThrows; +import static org.hamcrest.Matchers.equalTo; + public class InvalidRepositoryTests extends ESTestCase { private InvalidRepository repository = new InvalidRepository( @@ -21,15 +24,12 @@ public class InvalidRepositoryTests extends ESTestCase { ); public void testShouldThrowWhenGettingMetadata() { - expectThrows(RepositoryException.class, () -> repository.getSnapshotGlobalMetadata(new SnapshotId("name", "uuid"))); - } - - public void testShouldNotThrowWhenApplyingLifecycleChanges() { - repository.start(); - repository.stop(); - } - - public void testShouldNotThrowWhenClosingToAllowRemovingRepo() { - repository.close(); + assertThatThrows( + () -> repository.getSnapshotGlobalMetadata(new SnapshotId("name", "uuid")), + RepositoryException.class, + equalTo("[name] repository type [type] failed to create on current node"), + RepositoryException.class, + equalTo("[name] failed to create repository") + ); } } diff --git a/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java b/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java index 7f84dceb73621..aa96a5e73838c 100644 --- a/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java @@ -281,7 +281,7 @@ public void onResponse(AcknowledgedResponse acknowledgedResponse) { @Override public void onFailure(Exception e) { - fail("[" + repoName + "] repository type [test] faield to create"); + assert false : e; } }); } diff --git a/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java b/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java index a8a37ca23a89a..404378131a095 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java +++ b/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java @@ -12,7 +12,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.isA; -import static org.junit.Assert.fail; +import static org.junit.Assert.assertFalse; /** * Assertions for exceptions and their messages @@ -24,12 +24,32 @@ public static void assertThatThrows( Class exceptionType, Matcher messageMatcher ) { + boolean success = false; try { code.run(); - fail("Exception with type " + exceptionType.getName() + "should be thrown"); + success = true; } catch (Throwable e) { assertThatException(e, exceptionType, messageMatcher); } + assertFalse("Exception with type " + exceptionType.getName() + "should be thrown", success); + } + + public static void assertThatThrows( + LuceneTestCase.ThrowingRunnable code, + Class exceptionType, + Matcher messageMatcher, + Class causeExceptionType, + Matcher causeMessageMatcher + ) { + boolean success = false; + try { + code.run(); + success = true; + } catch (Throwable e) { + assertThatException(e, exceptionType, messageMatcher); + assertThatException(e.getCause(), causeExceptionType, causeMessageMatcher); + } + assertFalse("Exception with type " + exceptionType.getName() + "should be thrown", success); } public static void assertThatException(Throwable exception, Class exceptionType, Matcher messageMatcher) { diff --git a/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedRepositorySecretIntegTests.java b/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedRepositorySecretIntegTests.java index 27d97f849d78e..579af5f4905cb 100644 --- a/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedRepositorySecretIntegTests.java +++ b/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedRepositorySecretIntegTests.java @@ -422,13 +422,7 @@ public void testSnapshotIsPartialForMissingPassword() throws Exception { .stream() .allMatch( shardFailure -> shardFailure.reason() - .contains( - "RepositoryException[[" - + repositoryName - + "] failed to create repository]; nested: IllegalArgumentException[Secure setting [repository.encrypted." - + repositoryName - + ".password] must be set]" - ) + .contains("Secure setting [repository.encrypted." + repositoryName + ".password] must be set") ) ); assertThat( From d3f881a5b0c84f34c5f38518551f705cf511fd12 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Sat, 2 Apr 2022 23:07:02 +0800 Subject: [PATCH 10/13] remove cluster Scope settings in InvalidRepositoryIT --- ...iesDuplicatedCreationIT.java => InvalidRepositoryIT.java} | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) rename server/src/internalClusterTest/java/org/elasticsearch/repositories/{RepositoriesDuplicatedCreationIT.java => InvalidRepositoryIT.java} (97%) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java b/server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java similarity index 97% rename from server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java rename to server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java index 8ffc4a6abc5cf..ce72b5f529761 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/repositories/RepositoriesDuplicatedCreationIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java @@ -35,8 +35,7 @@ import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.isA; -@ESIntegTestCase.ClusterScope(supportsDedicatedMasters = false, numDataNodes = 3) -public class RepositoriesDuplicatedCreationIT extends ESIntegTestCase { +public class InvalidRepositoryIT extends ESIntegTestCase { @Override protected Collection> nodePlugins() { return Collections.singletonList(UnstableRepository.Plugin.class); @@ -134,7 +133,7 @@ public void testCreateInvalidRepository() throws Exception { createRepository(repositoryName, UnstableRepository.TYPE, Settings.builder().put("location", randomRepoPath())); // verification should succeed with all node create repository successfully VerifyRepositoryResponse verifyRepositoryResponse = client().admin().cluster().prepareVerifyRepository(repositoryName).get(); - assertEquals(verifyRepositoryResponse.getNodes().size(), 3); + assertEquals(verifyRepositoryResponse.getNodes().size(), internalCluster().numDataAndMasterNodes()); } From a6527cb917a842b3473e6a8586f1932867d487f4 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Sat, 2 Apr 2022 23:13:06 +0800 Subject: [PATCH 11/13] we should ensure at least two node in cluster --- .../java/org/elasticsearch/repositories/InvalidRepositoryIT.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java b/server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java index ce72b5f529761..edde2c5834198 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java @@ -87,6 +87,7 @@ public List> getSettings() { } public void testCreateInvalidRepository() throws Exception { + internalCluster().ensureAtLeastNumDataNodes(2); final String repositoryName = "test-duplicate-create-repo"; // put repository for the first time: only let master node create repository successfully From 59b8faa4f1cb823360184a6326217abfd045b665 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Sun, 3 Apr 2022 00:16:15 +0800 Subject: [PATCH 12/13] fix typo in UnkonwnTypeRepository --- .../org/elasticsearch/repositories/UnknownTypeRepository.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/repositories/UnknownTypeRepository.java b/server/src/main/java/org/elasticsearch/repositories/UnknownTypeRepository.java index b634f09502f1e..70f764dd9b5e0 100644 --- a/server/src/main/java/org/elasticsearch/repositories/UnknownTypeRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/UnknownTypeRepository.java @@ -30,7 +30,7 @@ /** * This class represents a repository that could not be initialized due to unknown type. - * This could happen whe a user creates a snapshot repository using a type from a plugin and then removes the plugin. + * This could happen when a user creates a snapshot repository using a type from a plugin and then removes the plugin. */ public class UnknownTypeRepository extends AbstractLifecycleComponent implements Repository { From f2b55757da7ba810e3d7a365f02f6c40fe412556 Mon Sep 17 00:00:00 2001 From: mushaoqiong Date: Mon, 4 Apr 2022 22:24:04 +0800 Subject: [PATCH 13/13] Use expectThrows instead of ThrowableAssertions --- .../repositories/InvalidRepositoryIT.java | 38 +++++++++---------- .../repositories/InvalidRepositoryTests.java | 12 +++--- .../RepositoriesServiceTests.java | 6 +-- .../test/hamcrest/ThrowableAssertions.java | 22 ----------- 4 files changed, 25 insertions(+), 53 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java b/server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java index edde2c5834198..217784cd127e8 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/repositories/InvalidRepositoryIT.java @@ -30,7 +30,6 @@ import java.util.Map; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; -import static org.elasticsearch.test.hamcrest.ThrowableAssertions.assertThatException; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.isA; @@ -104,27 +103,24 @@ public void testCreateInvalidRepository() throws Exception { ) ); // verification should fail with some node has InvalidRepository - boolean verifyPass = false; - try { - client().admin().cluster().prepareVerifyRepository(repositoryName).get(); - verifyPass = true; - } catch (Exception e) { - assertThat(e, isA(RepositoryVerificationException.class)); - assertEquals(e.getSuppressed().length, internalCluster().numDataAndMasterNodes() - 1); - for (Throwable suppressed : e.getSuppressed()) { - assertThatException( - suppressed.getCause(), - RepositoryException.class, - equalTo("[" + repositoryName + "] repository type [" + UnstableRepository.TYPE + "] failed to create on current node") - ); - assertThatException( - suppressed.getCause().getCause().getCause(), - RepositoryException.class, - equalTo("[" + repositoryName + "] Failed to create repository: current node is not stable") - ); - } + final var expectedException = expectThrows( + RepositoryVerificationException.class, + () -> client().admin().cluster().prepareVerifyRepository(repositoryName).get() + ); + for (Throwable suppressed : expectedException.getSuppressed()) { + Throwable outerCause = suppressed.getCause(); + assertThat(outerCause, isA(RepositoryException.class)); + assertThat( + outerCause.getMessage(), + equalTo("[" + repositoryName + "] repository type [" + UnstableRepository.TYPE + "] failed to create on current node") + ); + Throwable innerCause = suppressed.getCause().getCause().getCause(); + assertThat(innerCause, isA(RepositoryException.class)); + assertThat( + innerCause.getMessage(), + equalTo("[" + repositoryName + "] Failed to create repository: current node is not stable") + ); } - assertFalse("verification should fail when some node failed to create repository", verifyPass); // restart master internalCluster().restartNode(internalCluster().getMasterName()); diff --git a/server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java b/server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java index 0a6c4c5e9f8b2..d6f3d3a8d0383 100644 --- a/server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/InvalidRepositoryTests.java @@ -13,8 +13,8 @@ import org.elasticsearch.snapshots.SnapshotId; import org.elasticsearch.test.ESTestCase; -import static org.elasticsearch.test.hamcrest.ThrowableAssertions.assertThatThrows; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.isA; public class InvalidRepositoryTests extends ESTestCase { @@ -24,12 +24,12 @@ public class InvalidRepositoryTests extends ESTestCase { ); public void testShouldThrowWhenGettingMetadata() { - assertThatThrows( - () -> repository.getSnapshotGlobalMetadata(new SnapshotId("name", "uuid")), + final var expectedException = expectThrows( RepositoryException.class, - equalTo("[name] repository type [type] failed to create on current node"), - RepositoryException.class, - equalTo("[name] failed to create repository") + () -> repository.getSnapshotGlobalMetadata(new SnapshotId("name", "uuid")) ); + assertThat(expectedException.getMessage(), equalTo("[name] repository type [type] failed to create on current node")); + assertThat(expectedException.getCause(), isA(RepositoryException.class)); + assertThat(expectedException.getCause().getMessage(), equalTo("[name] failed to create repository")); } } diff --git a/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java b/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java index aa96a5e73838c..102baacb64bf3 100644 --- a/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java @@ -248,10 +248,8 @@ public void testRemoveInvalidRepositoryTypeWhenApplyingClusterState() { var clusterState = createClusterStateWithRepo(repoName, UnstableRepository.TYPE); repositoriesService.applyClusterState(new ClusterChangedEvent("put unstable repository", clusterState, emptyState())); repositoriesService.applyClusterState(new ClusterChangedEvent("removing repo", emptyState(), clusterState)); - - assertThatThrows( - () -> repositoriesService.repository(repoName), - RepositoryMissingException.class, + assertThat( + expectThrows(RepositoryMissingException.class, () -> repositoriesService.repository(repoName)).getMessage(), equalTo("[" + repoName + "] missing") ); } diff --git a/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java b/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java index 404378131a095..2aa43444af72d 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java +++ b/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ThrowableAssertions.java @@ -12,7 +12,6 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.isA; -import static org.junit.Assert.assertFalse; /** * Assertions for exceptions and their messages @@ -24,32 +23,11 @@ public static void assertThatThrows( Class exceptionType, Matcher messageMatcher ) { - boolean success = false; try { code.run(); - success = true; } catch (Throwable e) { assertThatException(e, exceptionType, messageMatcher); } - assertFalse("Exception with type " + exceptionType.getName() + "should be thrown", success); - } - - public static void assertThatThrows( - LuceneTestCase.ThrowingRunnable code, - Class exceptionType, - Matcher messageMatcher, - Class causeExceptionType, - Matcher causeMessageMatcher - ) { - boolean success = false; - try { - code.run(); - success = true; - } catch (Throwable e) { - assertThatException(e, exceptionType, messageMatcher); - assertThatException(e.getCause(), causeExceptionType, causeMessageMatcher); - } - assertFalse("Exception with type " + exceptionType.getName() + "should be thrown", success); } public static void assertThatException(Throwable exception, Class exceptionType, Matcher messageMatcher) {