From c589a31512c795466673afbee8524d78a9f60b77 Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 20 Jan 2021 20:36:59 +0000 Subject: [PATCH] Assert getRepositoryData only on master node A trap for the uninitiated: only the master should be calling `getRepositoryData()`, but today this isn't checked anywhere so there's a risk that we inadvertently introduce some code that gets the repository data on other nodes too. This commit introduces an assertion to catch that. Second attempt at #67780 which was reverted due to test failures. --- .../repositories/blobstore/BlobStoreRepository.java | 6 ++++++ .../repositories/blobstore/BlobStoreTestUtil.java | 1 + .../SearchableSnapshotRecoveryStateIntegrationTests.java | 4 +++- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java index 203236570a0c5..7d0c32a153559 100644 --- a/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java @@ -1344,6 +1344,12 @@ RepositoryData repositoryData() { @Override public void getRepositoryData(ActionListener listener) { + // RepositoryData is the responsibility of the elected master: we shouldn't be loading it on other nodes as we don't have good + // consistency guarantees there, but electedness is too ephemeral to assert. We can say for sure that this node should be + // master-eligible, which is almost as strong since all other snapshot-related activity happens on data nodes whether they be + // master-eligible or not. + assert clusterService.localNode().isMasterNode() : "should only load repository data on master nodes"; + if (latestKnownRepoGen.get() == RepositoryData.CORRUPTED_REPO_GEN) { listener.onFailure(corruptedStateException(null)); return; diff --git a/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreTestUtil.java b/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreTestUtil.java index 0241e116a6638..da385bdae95ed 100644 --- a/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreTestUtil.java +++ b/test/framework/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreTestUtil.java @@ -305,6 +305,7 @@ private static ClusterService mockClusterService(ClusterState initialState) { when(clusterService.getClusterApplierService()).thenReturn(clusterApplierService); // Setting local node as master so it may update the repository metadata in the cluster state final DiscoveryNode localNode = new DiscoveryNode("", buildNewFakeTransportAddress(), Version.CURRENT); + when(clusterService.localNode()).thenReturn(localNode); final AtomicReference currentState = new AtomicReference<>( ClusterState.builder(initialState).nodes( DiscoveryNodes.builder().add(localNode).masterNodeId(localNode.getId()).localNodeId(localNode.getId()).build()).build()); diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotRecoveryStateIntegrationTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotRecoveryStateIntegrationTests.java index 0122e494520b9..ee67ae67a3601 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotRecoveryStateIntegrationTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotRecoveryStateIntegrationTests.java @@ -182,7 +182,9 @@ public void testFilesStoredInThePersistentCacheAreMarkedAsReusedInRecoveryState( assertThat(repository, instanceOf(BlobStoreRepository.class)); final BlobStoreRepository blobStoreRepository = (BlobStoreRepository) repository; - final RepositoryData repositoryData = ESBlobStoreRepositoryIntegTestCase.getRepositoryData(repository); + final RepositoryData repositoryData = ESBlobStoreRepositoryIntegTestCase.getRepositoryData( + internalCluster().getCurrentMasterNodeInstance(RepositoriesService.class).repository(fsRepoName) + ); final IndexId indexId = repositoryData.resolveIndexId(indexName); long inMemoryCacheSize = 0; long expectedPhysicalCacheSize = 0;