Skip to content

Commit

Permalink
Assert getRepositoryData only on master node (elastic#67806)
Browse files Browse the repository at this point in the history
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 elastic#67780 which was reverted due to test failures.
  • Loading branch information
DaveCTurner committed Jan 21, 2021
1 parent 1f89cf7 commit 5f225d7
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -1360,6 +1360,12 @@ RepositoryData repositoryData() {

@Override
public void getRepositoryData(ActionListener<RepositoryData> 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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,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<ClusterState> currentState = new AtomicReference<>(
ClusterState.builder(initialState).nodes(
DiscoveryNodes.builder().add(localNode).masterNodeId(localNode.getId()).localNodeId(localNode.getId()).build()).build());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 5f225d7

Please sign in to comment.