From 03c11206d5fac820e1bca2fdcf469e4e2a42dc95 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Sat, 16 Nov 2019 22:17:57 +0100 Subject: [PATCH 1/4] Fix RepoCleanup not Removed on Master-Failover The logic for `cleanupInProgress()` was backwards everywhere (method itself and all but one user). Also, we weren't checking it when removing a repository. This lead to a bug (in the one spot that didn't use the method backwards) that prevented the cleanup cluster state entry from ever being removed from the cluster state if master failed over during the cleanup process. This change corrects the backwards logic, adds a test that makes sure the cleanup is always removed and adds a check that prevents repository removal during cleanup to the repositories service. --- .../TransportCleanupRepositoryAction.java | 4 +- .../cluster/RepositoryCleanupInProgress.java | 10 +++- .../repositories/RepositoriesService.java | 3 +- .../snapshots/SnapshotsService.java | 12 ++++- .../DedicatedClusterSnapshotRestoreIT.java | 54 ++++++++++++++++++- .../xpack/slm/SnapshotRetentionTask.java | 2 +- 6 files changed, 76 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java index 738b261d7fd8e..c0d076d7c917f 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java @@ -122,7 +122,7 @@ private static ClusterState removeInProgressCleanup(final ClusterState currentSt RepositoryCleanupInProgress cleanupInProgress = currentState.custom(RepositoryCleanupInProgress.TYPE); if (cleanupInProgress != null) { boolean changed = false; - if (cleanupInProgress.cleanupInProgress() == false) { + if (cleanupInProgress.cleanupInProgress()) { cleanupInProgress = new RepositoryCleanupInProgress(); changed = true; } @@ -175,7 +175,7 @@ private void cleanupRepo(String repositoryName, ActionListener entries() { + return List.copyOf(entries); } @Override @@ -106,6 +110,10 @@ public Entry(String repository, long repositoryStateId) { this.repositoryStateId = repositoryStateId; } + public String repository() { + return repository; + } + @Override public void writeTo(StreamOutput out) throws IOException { out.writeString(repository); diff --git a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java index ab14e7e291577..20f083dcff0fe 100644 --- a/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java +++ b/server/src/main/java/org/elasticsearch/repositories/RepositoriesService.java @@ -435,8 +435,7 @@ private static void validate(final String repositoryName) { } } - - private void ensureRepositoryNotInUse(ClusterState clusterState, String repository) { + private static void ensureRepositoryNotInUse(ClusterState clusterState, String repository) { if (SnapshotsService.isRepositoryInUse(clusterState, repository) || RestoreService.isRepositoryInUse(clusterState, repository)) { throw new IllegalStateException("trying to modify or unregister repository that is currently used "); } diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java index e942c4ac2d33c..c2e5a64f4b324 100644 --- a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java +++ b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java @@ -275,7 +275,7 @@ public ClusterState execute(ClusterState currentState) { "cannot snapshot while a snapshot deletion is in-progress"); } final RepositoryCleanupInProgress repositoryCleanupInProgress = currentState.custom(RepositoryCleanupInProgress.TYPE); - if (repositoryCleanupInProgress != null && repositoryCleanupInProgress.cleanupInProgress() == false) { + if (repositoryCleanupInProgress != null && repositoryCleanupInProgress.cleanupInProgress()) { throw new ConcurrentSnapshotExecutionException(repositoryName, snapshotName, "cannot snapshot while a repository cleanup is in-progress"); } @@ -1185,7 +1185,7 @@ public ClusterState execute(ClusterState currentState) { "cannot delete - another snapshot is currently being deleted"); } final RepositoryCleanupInProgress repositoryCleanupInProgress = currentState.custom(RepositoryCleanupInProgress.TYPE); - if (repositoryCleanupInProgress != null && repositoryCleanupInProgress.cleanupInProgress() == false) { + if (repositoryCleanupInProgress != null && repositoryCleanupInProgress.cleanupInProgress()) { throw new ConcurrentSnapshotExecutionException(snapshot.getRepository(), snapshot.getSnapshotId().getName(), "cannot delete snapshot while a repository cleanup is in-progress"); } @@ -1343,6 +1343,14 @@ public static boolean isRepositoryInUse(ClusterState clusterState, String reposi } } } + final RepositoryCleanupInProgress repositoryCleanupInProgress = clusterState.custom(RepositoryCleanupInProgress.TYPE); + if (repositoryCleanupInProgress != null) { + for (RepositoryCleanupInProgress.Entry entry : repositoryCleanupInProgress.entries()) { + if (entry.repository().equals(repository)) { + return true; + } + } + } return false; } diff --git a/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java b/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java index a11fb9d13bc04..bb83d736e2177 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java +++ b/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java @@ -23,6 +23,8 @@ import com.carrotsearch.hppc.IntSet; import org.elasticsearch.Version; import org.elasticsearch.action.ActionFuture; +import org.elasticsearch.action.ActionRunnable; +import org.elasticsearch.action.admin.cluster.repositories.cleanup.CleanupRepositoryResponse; import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse; import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse; @@ -32,6 +34,7 @@ import org.elasticsearch.action.admin.indices.stats.ShardStats; import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.support.ActiveShardCount; +import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.action.support.master.AcknowledgedResponse; import org.elasticsearch.client.AdminClient; import org.elasticsearch.client.Client; @@ -39,6 +42,7 @@ import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.ClusterStateUpdateTask; import org.elasticsearch.cluster.NamedDiff; +import org.elasticsearch.cluster.RepositoryCleanupInProgress; import org.elasticsearch.cluster.SnapshotsInProgress; import org.elasticsearch.cluster.health.ClusterHealthStatus; import org.elasticsearch.cluster.metadata.IndexMetaData; @@ -69,7 +73,9 @@ import org.elasticsearch.indices.recovery.RecoveryState; import org.elasticsearch.node.Node; import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.repositories.RepositoriesService; import org.elasticsearch.repositories.RepositoryMissingException; +import org.elasticsearch.repositories.blobstore.BlobStoreRepository; import org.elasticsearch.rest.AbstractRestChannel; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; @@ -86,6 +92,7 @@ import org.elasticsearch.test.disruption.ServiceDisruptionScheme; import org.elasticsearch.test.rest.FakeRestRequest; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.UncheckedIOException; import java.nio.file.FileVisitResult; @@ -993,6 +1000,52 @@ public void testMasterShutdownDuringFailedSnapshot() throws Exception { assertEquals(SnapshotState.FAILED, snapshotInfo.state()); } + public void testMasterFailoverDuringCleanup() throws Exception { + logger.info("--> starting two master nodes and one data node"); + internalCluster().startMasterOnlyNodes(2); + internalCluster().startDataOnlyNodes(1); + + logger.info("--> creating repository"); + assertAcked(client().admin().cluster().preparePutRepository("test-repo") + .setType("mock").setSettings(Settings.builder() + .put("location", randomRepoPath()) + .put("compress", randomBoolean()) + .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES))); + + logger.info("--> snapshot"); + client().admin().cluster().prepareCreateSnapshot("test-repo", "test-snap") + .setWaitForCompletion(true).get(); + + final RepositoriesService service = internalCluster().getInstance(RepositoriesService.class, internalCluster().getMasterName()); + final BlobStoreRepository repository = (BlobStoreRepository) service.repository("test-repo"); + + logger.info("--> creating a garbage data blob"); + final PlainActionFuture garbageFuture = PlainActionFuture.newFuture(); + repository.threadPool().generic().execute(ActionRunnable.run(garbageFuture, () -> repository.blobStore() + .blobContainer(repository.basePath()).writeBlob("snap-foo.dat", new ByteArrayInputStream(new byte[1]), 1, true))); + garbageFuture.get(); + + final String masterNode = blockMasterFromFinalizingSnapshotOnIndexFile("test-repo"); + + logger.info("--> starting repository cleanup"); + final ActionFuture cleanupFuture = + client().admin().cluster().prepareCleanupRepository("test-repo").execute(); + + logger.info("--> waiting for block to kick in on " + masterNode); + waitForBlock(masterNode, "test-repo", TimeValue.timeValueSeconds(60)); + + logger.info("--> stopping master node"); + internalCluster().stopCurrentMasterNode(); + + logger.info("--> wait for cleanup to finish and disappear from cluster state"); + assertBusy(() -> { + assertTrue(cleanupFuture.isDone()); + RepositoryCleanupInProgress cleanupInProgress = + client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); + assertFalse(cleanupInProgress.cleanupInProgress()); + }, 30, TimeUnit.SECONDS); + } + /** * Tests that a shrunken index (created via the shrink APIs) and subsequently snapshotted * can be restored when the node the shrunken index was created on is no longer part of @@ -1555,5 +1608,4 @@ public EnumSet context() { return EnumSet.of(MetaData.XContentContext.GATEWAY, MetaData.XContentContext.SNAPSHOT); } } - } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SnapshotRetentionTask.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SnapshotRetentionTask.java index e32f926fe8217..09990a64e325c 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SnapshotRetentionTask.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SnapshotRetentionTask.java @@ -440,7 +440,7 @@ public static boolean okayToDeleteSnapshots(ClusterState state) { // Cannot delete while a repository is being cleaned final RepositoryCleanupInProgress repositoryCleanupInProgress = state.custom(RepositoryCleanupInProgress.TYPE); - if (repositoryCleanupInProgress != null && repositoryCleanupInProgress.cleanupInProgress() == false) { + if (repositoryCleanupInProgress != null && repositoryCleanupInProgress.cleanupInProgress()) { return false; } From b2b74f04e6fa4a1da470265cfb0381d22f6142bd Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Sat, 16 Nov 2019 22:44:04 +0100 Subject: [PATCH 2/4] moar fixes --- .../TransportCleanupRepositoryAction.java | 9 +++ .../DedicatedClusterSnapshotRestoreIT.java | 57 +++++++++++++++++-- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java index c0d076d7c917f..dbd9751b618f3 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java @@ -172,6 +172,9 @@ private void cleanupRepo(String repositoryName, ActionListener blobStoreRepository.cleanup( @@ -215,6 +219,11 @@ private void after(@Nullable Exception failure, @Nullable RepositoryCleanupResul "Failed to finish repository cleanup operations on [{}][{}]", repositoryName, repositoryStateId), failure); } assert failure != null || result != null; + if (startedCleanup == false) { + logger.debug("No cleanup task to remove from cluster state because we failed to start one", failure); + listener.onFailure(failure); + return; + } clusterService.submitStateUpdateTask( "remove repository cleanup task [" + repositoryName + "][" + repositoryStateId + ']', new ClusterStateUpdateTask() { diff --git a/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java b/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java index bb83d736e2177..d6acb4e02d968 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java +++ b/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java @@ -24,7 +24,6 @@ import org.elasticsearch.Version; import org.elasticsearch.action.ActionFuture; import org.elasticsearch.action.ActionRunnable; -import org.elasticsearch.action.admin.cluster.repositories.cleanup.CleanupRepositoryResponse; import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse; import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse; @@ -1028,8 +1027,7 @@ public void testMasterFailoverDuringCleanup() throws Exception { final String masterNode = blockMasterFromFinalizingSnapshotOnIndexFile("test-repo"); logger.info("--> starting repository cleanup"); - final ActionFuture cleanupFuture = - client().admin().cluster().prepareCleanupRepository("test-repo").execute(); + client().admin().cluster().prepareCleanupRepository("test-repo").execute(); logger.info("--> waiting for block to kick in on " + masterNode); waitForBlock(masterNode, "test-repo", TimeValue.timeValueSeconds(60)); @@ -1039,7 +1037,58 @@ public void testMasterFailoverDuringCleanup() throws Exception { logger.info("--> wait for cleanup to finish and disappear from cluster state"); assertBusy(() -> { - assertTrue(cleanupFuture.isDone()); + RepositoryCleanupInProgress cleanupInProgress = + client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); + assertFalse(cleanupInProgress.cleanupInProgress()); + }, 30, TimeUnit.SECONDS); + } + + public void testRepeatCleanupsDontRemove() throws Exception { + logger.info("--> starting two master nodes and one data node"); + internalCluster().startMasterOnlyNodes(2); + internalCluster().startDataOnlyNodes(1); + + logger.info("--> creating repository"); + assertAcked(client().admin().cluster().preparePutRepository("test-repo") + .setType("mock").setSettings(Settings.builder() + .put("location", randomRepoPath()) + .put("compress", randomBoolean()) + .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES))); + + logger.info("--> snapshot"); + client().admin().cluster().prepareCreateSnapshot("test-repo", "test-snap") + .setWaitForCompletion(true).get(); + + final RepositoriesService service = internalCluster().getInstance(RepositoriesService.class, internalCluster().getMasterName()); + final BlobStoreRepository repository = (BlobStoreRepository) service.repository("test-repo"); + + logger.info("--> creating a garbage data blob"); + final PlainActionFuture garbageFuture = PlainActionFuture.newFuture(); + repository.threadPool().generic().execute(ActionRunnable.run(garbageFuture, () -> repository.blobStore() + .blobContainer(repository.basePath()).writeBlob("snap-foo.dat", new ByteArrayInputStream(new byte[1]), 1, true))); + garbageFuture.get(); + + final String masterNode = blockMasterFromFinalizingSnapshotOnIndexFile("test-repo"); + + logger.info("--> starting repository cleanup"); + client().admin().cluster().prepareCleanupRepository("test-repo").execute(); + + logger.info("--> waiting for block to kick in on " + masterNode); + waitForBlock(masterNode, "test-repo", TimeValue.timeValueSeconds(60)); + try { + logger.info("--> sending another cleanup"); + assertThrows(client().admin().cluster().prepareCleanupRepository("test-repo").execute(), IllegalStateException.class); + logger.info("--> ensure cleanup is still in progress"); + final RepositoryCleanupInProgress cleanup = + client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); + assertTrue(cleanup.cleanupInProgress()); + } finally { + logger.info("--> unblocking master node"); + unblockNode("test-repo", masterNode); + } + + logger.info("--> wait for cleanup to finish and disappear from cluster state"); + assertBusy(() -> { RepositoryCleanupInProgress cleanupInProgress = client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); assertFalse(cleanupInProgress.cleanupInProgress()); From 45f30fce22ea600fb3f6e8b1caee7086b9f0153e Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Sun, 17 Nov 2019 09:41:53 +0100 Subject: [PATCH 3/4] dry up testing --- .../BlobStoreRepositoryCleanupIT.java | 110 ++++++++++++++++++ .../DedicatedClusterSnapshotRestoreIT.java | 103 +--------------- 2 files changed, 111 insertions(+), 102 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryCleanupIT.java diff --git a/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryCleanupIT.java b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryCleanupIT.java new file mode 100644 index 0000000000000..d20670044619f --- /dev/null +++ b/server/src/test/java/org/elasticsearch/repositories/blobstore/BlobStoreRepositoryCleanupIT.java @@ -0,0 +1,110 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.repositories.blobstore; + +import org.elasticsearch.action.ActionRunnable; +import org.elasticsearch.action.support.PlainActionFuture; +import org.elasticsearch.cluster.RepositoryCleanupInProgress; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.unit.ByteSizeUnit; +import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.repositories.RepositoriesService; +import org.elasticsearch.snapshots.AbstractSnapshotIntegTestCase; +import org.elasticsearch.test.ESIntegTestCase; + +import java.io.ByteArrayInputStream; +import java.util.concurrent.TimeUnit; + +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows; + +@ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0) +public class BlobStoreRepositoryCleanupIT extends AbstractSnapshotIntegTestCase { + + public void testMasterFailoverDuringCleanup() throws Exception { + startBlockedCleanup("test-repo"); + + logger.info("--> stopping master node"); + internalCluster().stopCurrentMasterNode(); + + logger.info("--> wait for cleanup to finish and disappear from cluster state"); + assertBusy(() -> { + RepositoryCleanupInProgress cleanupInProgress = + client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); + assertFalse(cleanupInProgress.cleanupInProgress()); + }, 30, TimeUnit.SECONDS); + } + + public void testRepeatCleanupsDontRemove() throws Exception { + final String masterNode = startBlockedCleanup("test-repo"); + + logger.info("--> sending another cleanup"); + assertThrows(client().admin().cluster().prepareCleanupRepository("test-repo").execute(), IllegalStateException.class); + + logger.info("--> ensure cleanup is still in progress"); + final RepositoryCleanupInProgress cleanup = + client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); + assertTrue(cleanup.cleanupInProgress()); + + logger.info("--> unblocking master node"); + unblockNode("test-repo", masterNode); + + logger.info("--> wait for cleanup to finish and disappear from cluster state"); + assertBusy(() -> { + RepositoryCleanupInProgress cleanupInProgress = + client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); + assertFalse(cleanupInProgress.cleanupInProgress()); + }, 30, TimeUnit.SECONDS); + } + + private String startBlockedCleanup(String repoName) throws Exception { + logger.info("--> starting two master nodes and one data node"); + internalCluster().startMasterOnlyNodes(2); + internalCluster().startDataOnlyNodes(1); + + logger.info("--> creating repository"); + assertAcked(client().admin().cluster().preparePutRepository(repoName) + .setType("mock").setSettings(Settings.builder() + .put("location", randomRepoPath()) + .put("compress", randomBoolean()) + .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES))); + + logger.info("--> snapshot"); + client().admin().cluster().prepareCreateSnapshot(repoName, "test-snap") + .setWaitForCompletion(true).get(); + + final RepositoriesService service = internalCluster().getInstance(RepositoriesService.class, internalCluster().getMasterName()); + final BlobStoreRepository repository = (BlobStoreRepository) service.repository(repoName); + + logger.info("--> creating a garbage data blob"); + final PlainActionFuture garbageFuture = PlainActionFuture.newFuture(); + repository.threadPool().generic().execute(ActionRunnable.run(garbageFuture, () -> repository.blobStore() + .blobContainer(repository.basePath()).writeBlob("snap-foo.dat", new ByteArrayInputStream(new byte[1]), 1, true))); + garbageFuture.get(); + + final String masterNode = blockMasterFromFinalizingSnapshotOnIndexFile(repoName); + + logger.info("--> starting repository cleanup"); + client().admin().cluster().prepareCleanupRepository(repoName).execute(); + + logger.info("--> waiting for block to kick in on " + masterNode); + waitForBlock(masterNode, repoName, TimeValue.timeValueSeconds(60)); + return masterNode; + } +} diff --git a/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java b/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java index d6acb4e02d968..a11fb9d13bc04 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java +++ b/server/src/test/java/org/elasticsearch/snapshots/DedicatedClusterSnapshotRestoreIT.java @@ -23,7 +23,6 @@ import com.carrotsearch.hppc.IntSet; import org.elasticsearch.Version; import org.elasticsearch.action.ActionFuture; -import org.elasticsearch.action.ActionRunnable; import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse; import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse; import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse; @@ -33,7 +32,6 @@ import org.elasticsearch.action.admin.indices.stats.ShardStats; import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.support.ActiveShardCount; -import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.action.support.master.AcknowledgedResponse; import org.elasticsearch.client.AdminClient; import org.elasticsearch.client.Client; @@ -41,7 +39,6 @@ import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.ClusterStateUpdateTask; import org.elasticsearch.cluster.NamedDiff; -import org.elasticsearch.cluster.RepositoryCleanupInProgress; import org.elasticsearch.cluster.SnapshotsInProgress; import org.elasticsearch.cluster.health.ClusterHealthStatus; import org.elasticsearch.cluster.metadata.IndexMetaData; @@ -72,9 +69,7 @@ import org.elasticsearch.indices.recovery.RecoveryState; import org.elasticsearch.node.Node; import org.elasticsearch.plugins.Plugin; -import org.elasticsearch.repositories.RepositoriesService; import org.elasticsearch.repositories.RepositoryMissingException; -import org.elasticsearch.repositories.blobstore.BlobStoreRepository; import org.elasticsearch.rest.AbstractRestChannel; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; @@ -91,7 +86,6 @@ import org.elasticsearch.test.disruption.ServiceDisruptionScheme; import org.elasticsearch.test.rest.FakeRestRequest; -import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.UncheckedIOException; import java.nio.file.FileVisitResult; @@ -999,102 +993,6 @@ public void testMasterShutdownDuringFailedSnapshot() throws Exception { assertEquals(SnapshotState.FAILED, snapshotInfo.state()); } - public void testMasterFailoverDuringCleanup() throws Exception { - logger.info("--> starting two master nodes and one data node"); - internalCluster().startMasterOnlyNodes(2); - internalCluster().startDataOnlyNodes(1); - - logger.info("--> creating repository"); - assertAcked(client().admin().cluster().preparePutRepository("test-repo") - .setType("mock").setSettings(Settings.builder() - .put("location", randomRepoPath()) - .put("compress", randomBoolean()) - .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES))); - - logger.info("--> snapshot"); - client().admin().cluster().prepareCreateSnapshot("test-repo", "test-snap") - .setWaitForCompletion(true).get(); - - final RepositoriesService service = internalCluster().getInstance(RepositoriesService.class, internalCluster().getMasterName()); - final BlobStoreRepository repository = (BlobStoreRepository) service.repository("test-repo"); - - logger.info("--> creating a garbage data blob"); - final PlainActionFuture garbageFuture = PlainActionFuture.newFuture(); - repository.threadPool().generic().execute(ActionRunnable.run(garbageFuture, () -> repository.blobStore() - .blobContainer(repository.basePath()).writeBlob("snap-foo.dat", new ByteArrayInputStream(new byte[1]), 1, true))); - garbageFuture.get(); - - final String masterNode = blockMasterFromFinalizingSnapshotOnIndexFile("test-repo"); - - logger.info("--> starting repository cleanup"); - client().admin().cluster().prepareCleanupRepository("test-repo").execute(); - - logger.info("--> waiting for block to kick in on " + masterNode); - waitForBlock(masterNode, "test-repo", TimeValue.timeValueSeconds(60)); - - logger.info("--> stopping master node"); - internalCluster().stopCurrentMasterNode(); - - logger.info("--> wait for cleanup to finish and disappear from cluster state"); - assertBusy(() -> { - RepositoryCleanupInProgress cleanupInProgress = - client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); - assertFalse(cleanupInProgress.cleanupInProgress()); - }, 30, TimeUnit.SECONDS); - } - - public void testRepeatCleanupsDontRemove() throws Exception { - logger.info("--> starting two master nodes and one data node"); - internalCluster().startMasterOnlyNodes(2); - internalCluster().startDataOnlyNodes(1); - - logger.info("--> creating repository"); - assertAcked(client().admin().cluster().preparePutRepository("test-repo") - .setType("mock").setSettings(Settings.builder() - .put("location", randomRepoPath()) - .put("compress", randomBoolean()) - .put("chunk_size", randomIntBetween(100, 1000), ByteSizeUnit.BYTES))); - - logger.info("--> snapshot"); - client().admin().cluster().prepareCreateSnapshot("test-repo", "test-snap") - .setWaitForCompletion(true).get(); - - final RepositoriesService service = internalCluster().getInstance(RepositoriesService.class, internalCluster().getMasterName()); - final BlobStoreRepository repository = (BlobStoreRepository) service.repository("test-repo"); - - logger.info("--> creating a garbage data blob"); - final PlainActionFuture garbageFuture = PlainActionFuture.newFuture(); - repository.threadPool().generic().execute(ActionRunnable.run(garbageFuture, () -> repository.blobStore() - .blobContainer(repository.basePath()).writeBlob("snap-foo.dat", new ByteArrayInputStream(new byte[1]), 1, true))); - garbageFuture.get(); - - final String masterNode = blockMasterFromFinalizingSnapshotOnIndexFile("test-repo"); - - logger.info("--> starting repository cleanup"); - client().admin().cluster().prepareCleanupRepository("test-repo").execute(); - - logger.info("--> waiting for block to kick in on " + masterNode); - waitForBlock(masterNode, "test-repo", TimeValue.timeValueSeconds(60)); - try { - logger.info("--> sending another cleanup"); - assertThrows(client().admin().cluster().prepareCleanupRepository("test-repo").execute(), IllegalStateException.class); - logger.info("--> ensure cleanup is still in progress"); - final RepositoryCleanupInProgress cleanup = - client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); - assertTrue(cleanup.cleanupInProgress()); - } finally { - logger.info("--> unblocking master node"); - unblockNode("test-repo", masterNode); - } - - logger.info("--> wait for cleanup to finish and disappear from cluster state"); - assertBusy(() -> { - RepositoryCleanupInProgress cleanupInProgress = - client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); - assertFalse(cleanupInProgress.cleanupInProgress()); - }, 30, TimeUnit.SECONDS); - } - /** * Tests that a shrunken index (created via the shrink APIs) and subsequently snapshotted * can be restored when the node the shrunken index was created on is no longer part of @@ -1657,4 +1555,5 @@ public EnumSet context() { return EnumSet.of(MetaData.XContentContext.GATEWAY, MetaData.XContentContext.SNAPSHOT); } } + } From 05eb21fd2ba41476574d06b13475fc7a93e47154 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Mon, 18 Nov 2019 11:59:17 +0100 Subject: [PATCH 4/4] CR: renaming --- .../cleanup/TransportCleanupRepositoryAction.java | 6 +++--- .../elasticsearch/cluster/RepositoryCleanupInProgress.java | 2 +- .../java/org/elasticsearch/snapshots/SnapshotsService.java | 4 ++-- .../blobstore/BlobStoreRepositoryCleanupIT.java | 6 +++--- .../org/elasticsearch/xpack/slm/SnapshotRetentionTask.java | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java index dbd9751b618f3..2cd0d06e67969 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/repositories/cleanup/TransportCleanupRepositoryAction.java @@ -93,7 +93,7 @@ public TransportCleanupRepositoryAction(TransportService transportService, Clust clusterService.addStateApplier(event -> { if (event.localNodeMaster() && event.previousState().nodes().isLocalNodeElectedMaster() == false) { final RepositoryCleanupInProgress repositoryCleanupInProgress = event.state().custom(RepositoryCleanupInProgress.TYPE); - if (repositoryCleanupInProgress == null || repositoryCleanupInProgress.cleanupInProgress() == false) { + if (repositoryCleanupInProgress == null || repositoryCleanupInProgress.hasCleanupInProgress() == false) { return; } clusterService.submitStateUpdateTask("clean up repository cleanup task after master failover", @@ -122,7 +122,7 @@ private static ClusterState removeInProgressCleanup(final ClusterState currentSt RepositoryCleanupInProgress cleanupInProgress = currentState.custom(RepositoryCleanupInProgress.TYPE); if (cleanupInProgress != null) { boolean changed = false; - if (cleanupInProgress.cleanupInProgress()) { + if (cleanupInProgress.hasCleanupInProgress()) { cleanupInProgress = new RepositoryCleanupInProgress(); changed = true; } @@ -178,7 +178,7 @@ private void cleanupRepo(String repositoryName, ActionListener { RepositoryCleanupInProgress cleanupInProgress = client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); - assertFalse(cleanupInProgress.cleanupInProgress()); + assertFalse(cleanupInProgress.hasCleanupInProgress()); }, 30, TimeUnit.SECONDS); } @@ -60,7 +60,7 @@ public void testRepeatCleanupsDontRemove() throws Exception { logger.info("--> ensure cleanup is still in progress"); final RepositoryCleanupInProgress cleanup = client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); - assertTrue(cleanup.cleanupInProgress()); + assertTrue(cleanup.hasCleanupInProgress()); logger.info("--> unblocking master node"); unblockNode("test-repo", masterNode); @@ -69,7 +69,7 @@ public void testRepeatCleanupsDontRemove() throws Exception { assertBusy(() -> { RepositoryCleanupInProgress cleanupInProgress = client().admin().cluster().prepareState().get().getState().custom(RepositoryCleanupInProgress.TYPE); - assertFalse(cleanupInProgress.cleanupInProgress()); + assertFalse(cleanupInProgress.hasCleanupInProgress()); }, 30, TimeUnit.SECONDS); } diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SnapshotRetentionTask.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SnapshotRetentionTask.java index 09990a64e325c..25de192e76d04 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SnapshotRetentionTask.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SnapshotRetentionTask.java @@ -440,7 +440,7 @@ public static boolean okayToDeleteSnapshots(ClusterState state) { // Cannot delete while a repository is being cleaned final RepositoryCleanupInProgress repositoryCleanupInProgress = state.custom(RepositoryCleanupInProgress.TYPE); - if (repositoryCleanupInProgress != null && repositoryCleanupInProgress.cleanupInProgress()) { + if (repositoryCleanupInProgress != null && repositoryCleanupInProgress.hasCleanupInProgress()) { return false; }