From eab1bda48469b5b828eab84028193d8d2f302e40 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Thu, 4 Feb 2021 16:23:52 +0100 Subject: [PATCH] Fix Encrypted Repo Test and RepositoriesService Logging (#68469) (#68527) This test creates a strange situation when it restarts the two master nodes back to back without strong guarantees about whether or not the cluster has cleanly formed again. Since we really just want to make sure that the master is after the restart is actually running with the changed password, it seemed easiest to simply do a full restart rather than the current loop. Also, this commit fixes the logging around repository creation and deletion which made this hard to debug and by logging before a CS update has actually been processed would also log incorrectly. Lastly, I removed the now unnecessary hacks of using the generic pool on a node to fetch repository data. This logic is not needed any longer and the repository will now automatically use the correct thread under the hood. The hack effectively just adds another possible failure mode around node restarts since the generic pool quietly rejects tasks after shutdown. Closes #67834 --- .../repositories/RepositoriesService.java | 26 ++++++++++++++---- ...ryptedFSBlobStoreRepositoryIntegTests.java | 5 +--- .../EncryptedRepositorySecretIntegTests.java | 27 +++++-------------- 3 files changed, 28 insertions(+), 30 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java index 22559d6f72f6d..f2013bcd51f51 100644 --- a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java @@ -168,13 +168,15 @@ public void registerRepository(final PutRepositoryRequest request, final ActionL clusterService.submitStateUpdateTask("put_repository [" + request.name() + "]", new AckedClusterStateUpdateTask(request, acknowledgementStep) { + private boolean found = false; + private boolean changed = false; + @Override public ClusterState execute(ClusterState currentState) { ensureRepositoryNotInUse(currentState, request.name()); Metadata metadata = currentState.metadata(); Metadata.Builder mdBuilder = Metadata.builder(currentState.metadata()); RepositoriesMetadata repositories = metadata.custom(RepositoriesMetadata.TYPE, RepositoriesMetadata.EMPTY); - boolean found = false; List repositoriesMetadata = new ArrayList<>(repositories.repositories().size() + 1); for (RepositoryMetadata repositoryMetadata : repositories.repositories()) { if (repositoryMetadata.name().equals(newRepositoryMetadata.name())) { @@ -189,13 +191,11 @@ public ClusterState execute(ClusterState currentState) { } } if (found == false) { - logger.info("put repository [{}]", request.name()); repositoriesMetadata.add(new RepositoryMetadata(request.name(), request.type(), request.settings())); - } else { - logger.info("update repository [{}]", request.name()); } repositories = new RepositoriesMetadata(repositoriesMetadata); mdBuilder.putCustom(RepositoriesMetadata.TYPE, repositories); + changed = true; return ClusterState.builder(currentState).metadata(mdBuilder).build(); } @@ -214,6 +214,13 @@ public boolean mustAck(DiscoveryNode discoveryNode) { @Override public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { + if (changed) { + if (found) { + logger.info("updated repository [{}]", request.name()); + } else { + logger.info("put repository [{}]", request.name()); + } + } publicationStep.onResponse(null); } }); @@ -288,6 +295,8 @@ public void unregisterRepository(final DeleteRepositoryRequest request, final Ac clusterService.submitStateUpdateTask("delete_repository [" + request.name() + "]", new AckedClusterStateUpdateTask(request, listener) { + private final List deletedRepositories = new ArrayList<>(); + @Override public ClusterState execute(ClusterState currentState) { Metadata metadata = currentState.metadata(); @@ -299,7 +308,7 @@ public ClusterState execute(ClusterState currentState) { for (RepositoryMetadata repositoryMetadata : repositories.repositories()) { if (Regex.simpleMatch(request.name(), repositoryMetadata.name())) { ensureRepositoryNotInUse(currentState, repositoryMetadata.name()); - logger.info("delete repository [{}]", repositoryMetadata.name()); + deletedRepositories.add(repositoryMetadata.name()); changed = true; } else { repositoriesMetadata.add(repositoryMetadata); @@ -317,6 +326,13 @@ public ClusterState execute(ClusterState currentState) { throw new RepositoryMissingException(request.name()); } + @Override + public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { + if (deletedRepositories.isEmpty() == false) { + logger.info("deleted repositories [{}]", deletedRepositories); + } + } + @Override public boolean mustAck(DiscoveryNode discoveryNode) { // repository was created on both master and data nodes diff --git a/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedFSBlobStoreRepositoryIntegTests.java b/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedFSBlobStoreRepositoryIntegTests.java index ee5da734b14af..508749039fe40 100644 --- a/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedFSBlobStoreRepositoryIntegTests.java +++ b/x-pack/plugin/repository-encrypted/src/internalClusterTest/java/org/elasticsearch/repositories/encrypted/EncryptedFSBlobStoreRepositoryIntegTests.java @@ -6,7 +6,6 @@ */ package org.elasticsearch.repositories.encrypted; -import org.elasticsearch.action.ActionRunnable; import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.common.settings.MockSecureSettings; import org.elasticsearch.common.settings.Settings; @@ -158,9 +157,7 @@ public void testTamperedEncryptionMetadata() throws Exception { assertThat( expectThrows( RepositoryException.class, - () -> PlainActionFuture.get( - f -> blobStoreRepository.threadPool().generic().execute(ActionRunnable.wrap(f, blobStoreRepository::getRepositoryData)) - ) + () -> PlainActionFuture.get(blobStoreRepository::getRepositoryData) ).getMessage(), containsString("the encryption metadata in the repository has been corrupted") ); 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 140644e8d7469..1bbbc3581a575 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 @@ -8,7 +8,6 @@ package org.elasticsearch.repositories.encrypted; import org.elasticsearch.ElasticsearchSecurityException; -import org.elasticsearch.action.ActionRunnable; import org.elasticsearch.action.admin.cluster.repositories.verify.VerifyRepositoryResponse; import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse; @@ -40,7 +39,6 @@ import java.util.Arrays; import java.util.Collection; -import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Set; @@ -549,7 +547,6 @@ public void testWrongRepositoryPassword() throws Exception { internalCluster().startNodes(2, Settings.builder().setSecureSettings(secureSettingsWithPassword).build()); ensureStableCluster(2); createRepository(repositoryName, repositorySettings, true); - // create empty smapshot final String snapshotName = randomName(); logger.info("--> create empty snapshot {}:{}", repositoryName, snapshotName); CreateSnapshotResponse createSnapshotResponse = client().admin() @@ -575,14 +572,9 @@ public void testWrongRepositoryPassword() throws Exception { EncryptedRepositoryPlugin.ENCRYPTION_PASSWORD_SETTING.getConcreteSettingForNamespace(repositoryName).getKey(), wrongPassword ); - Set nodesWithWrongPassword = new HashSet<>(); - do { - String masterNodeName = internalCluster().getMasterName(); - logger.info("--> restart master node {}", masterNodeName); - internalCluster().restartNode(masterNodeName, new InternalTestCluster.RestartCallback()); - nodesWithWrongPassword.add(masterNodeName); - ensureStableCluster(2); - } while (false == nodesWithWrongPassword.contains(internalCluster().getMasterName())); + + internalCluster().fullRestart(); + ensureStableCluster(2); // maybe recreate the repository if (randomBoolean()) { deleteRepository(repositoryName); @@ -594,9 +586,7 @@ public void testWrongRepositoryPassword() throws Exception { ).repository(repositoryName); RepositoryException e = expectThrows( RepositoryException.class, - () -> PlainActionFuture.get( - f -> blobStoreRepository.threadPool().generic().execute(ActionRunnable.wrap(f, blobStoreRepository::getRepositoryData)) - ) + () -> PlainActionFuture.get(blobStoreRepository::getRepositoryData) ); assertThat(e.getCause().getMessage(), containsString("repository password is incorrect")); e = expectThrows( @@ -621,13 +611,8 @@ public void testWrongRepositoryPassword() throws Exception { EncryptedRepositoryPlugin.ENCRYPTION_PASSWORD_SETTING.getConcreteSettingForNamespace(repositoryName).getKey(), goodPassword ); - do { - String masterNodeName = internalCluster().getMasterName(); - logger.info("--> restart master node {}", masterNodeName); - internalCluster().restartNode(masterNodeName, new InternalTestCluster.RestartCallback()); - nodesWithWrongPassword.remove(masterNodeName); - ensureStableCluster(2); - } while (nodesWithWrongPassword.contains(internalCluster().getMasterName())); + internalCluster().fullRestart(); + ensureStableCluster(2); // ensure get snapshot works GetSnapshotsResponse getSnapshotResponse = client().admin().cluster().prepareGetSnapshots(repositoryName).get(); assertThat(getSnapshotResponse.getSnapshots(), hasSize(1));