Remove MDP from PersistedClusterStateService by rjernst · Pull Request #72278 · elastic/elasticsearch

rjernst · 2021-04-26T22:00:41Z

This commit removes multiple data path support from the
PersistedClusterStateService. Most notably, many error cases are no
longer possible where duplicate/conflicting data could previously exist
across multiple paths, there is now only one path.

relates #71205

This commit removes multiple data path support from the PersistedClusterStateService. Most notably, many error cases are no longer possible where duplicate/conflicting data could previously exist across multiple paths, there is now only one path. relates elastic#71205

elasticmachine · 2021-04-26T22:00:44Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

rjernst · 2021-04-26T22:02:44Z

Note that to avoid an enormous PR removing MDP support, this PR tries to narrowly remove support from a single service. There are many examples where the calling code creates a service for testing with multiple paths, but just uses the first path. This is ok for as an intermediate step since we already block any actual uses of MDP in real nodes.

DaveCTurner

This is ok for as an intermediate step since we already block any actual uses of MDP in real nodes.

I left a suggestion at https://github.com/elastic/elasticsearch/pull/72282/files#r621070239 for making this clearer from the code, and a few other observations.

server/src/main/java/org/elasticsearch/gateway/GatewayMetaState.java

DaveCTurner · 2021-04-27T10:00:34Z

server/src/main/java/org/elasticsearch/gateway/PersistedClusterStateService.java

            final Metadata metadata = Metadata.Builder.fromXContent(XContentFactory.xContent(XContentType.SMILE)
                .createParser(namedXContentRegistry, LoggingDeprecationHandler.INSTANCE, bytes.bytes, bytes.offset, bytes.length));
            logger.trace("found global metadata with last-accepted term [{}]", metadata.coordinationMetadata().term());
-            if (builderReference.get() != null) {


This change seems unrelated, it's here to catch a case where someone meddled with the metadata and left us with two copies of the global metadata in the same data path. I think we should keep this.

DaveCTurner · 2021-04-27T10:01:01Z

server/src/main/java/org/elasticsearch/gateway/PersistedClusterStateService.java

            final IndexMetadata indexMetadata = IndexMetadata.fromXContent(XContentFactory.xContent(XContentType.SMILE)
                .createParser(namedXContentRegistry, LoggingDeprecationHandler.INSTANCE, bytes.bytes, bytes.offset, bytes.length));
            logger.trace("found index metadata for {}", indexMetadata.getIndex());
-            if (indexUUIDs.add(indexMetadata.getIndexUUID()) == false) {


This change seems unrelated, it's here to catch a case where someone meddled with the metadata and left us with two copies of the metadata for the same index in the same data path. I think we should keep this.

DaveCTurner · 2021-04-27T10:02:53Z

server/src/test/java/org/elasticsearch/gateway/PersistedClusterStateServiceTests.java

        }
    }

-    public void testFailsIfGlobalMetadataIsDuplicated() throws IOException {


This test is still meaningful, although we'll need to work out a more creative way to duplicate that document.

Here is the same test but using two distinct NodeEnvironment objects to get the metadata indices that we mash together. This works on master as it stands today, obviously needs adjusting for the single-data-path case.

public void testFailsIfGlobalMetadataIsDuplicated() throws IOException { // if someone attempted surgery on the metadata index by hand, e.g. deleting broken segments, then maybe the global metadata // is duplicated final Path[] dataPaths1 = createDataPaths(); final Path[] dataPaths2 = createDataPaths(); try (NodeEnvironment nodeEnvironment1 = newNodeEnvironment(dataPaths1); NodeEnvironment nodeEnvironment2 = newNodeEnvironment(dataPaths2)) { try (Writer writer1 = newPersistedClusterStateService(nodeEnvironment1).createWriter(); Writer writer2 = newPersistedClusterStateService(nodeEnvironment2).createWriter()) { final ClusterState oldClusterState = loadPersistedClusterState(newPersistedClusterStateService(nodeEnvironment1)); final long newVersion = randomLongBetween(1L, Long.MAX_VALUE); final ClusterState newClusterState = ClusterState.builder(oldClusterState).version(newVersion).build(); writeState(writer1, 0L, newClusterState, oldClusterState); writeState(writer2, 0L, newClusterState, oldClusterState); } final Path brokenPath = randomFrom(nodeEnvironment1.nodeDataPaths()); final Path dupPath = randomFrom(nodeEnvironment2.nodeDataPaths()); try (Directory directory = new SimpleFSDirectory(brokenPath.resolve(PersistedClusterStateService.METADATA_DIRECTORY_NAME)); Directory dupDirectory = new SimpleFSDirectory(dupPath.resolve(PersistedClusterStateService.METADATA_DIRECTORY_NAME))) { try (IndexWriter indexWriter = new IndexWriter(directory, new IndexWriterConfig())) { indexWriter.addIndexes(dupDirectory); indexWriter.commit(); } } final String message = expectThrows(IllegalStateException.class, () -> newPersistedClusterStateService(nodeEnvironment1).loadBestOnDiskState()).getMessage(); assertThat(message, allOf(containsString("duplicate global metadata found"), containsString(brokenPath.toString()))); } }

DaveCTurner · 2021-04-27T10:03:01Z

server/src/test/java/org/elasticsearch/gateway/PersistedClusterStateServiceTests.java

-        }
-    }
-
-    public void testFailsIfIndexMetadataIsDuplicated() throws IOException {


This test is still meaningful, although we'll need to work out a more creative way to duplicate that document.

Here is the same test but using two distinct NodeEnvironment objects to get the metadata indices that we mash together. This works on master as it stands today, obviously needs adjusting for the single-data-path case.

public void testFailsIfIndexMetadataIsDuplicated() throws IOException { // if someone attempted surgery on the metadata index by hand, e.g. deleting broken segments, then maybe some index metadata // is duplicated final Path[] dataPaths1 = createDataPaths(); final Path[] dataPaths2 = createDataPaths(); try (NodeEnvironment nodeEnvironment1 = newNodeEnvironment(dataPaths1); NodeEnvironment nodeEnvironment2 = newNodeEnvironment(dataPaths2)) { final String indexUUID = UUIDs.randomBase64UUID(random()); final String indexName = randomAlphaOfLength(10); try (Writer writer1 = newPersistedClusterStateService(nodeEnvironment1).createWriter(); Writer writer2 = newPersistedClusterStateService(nodeEnvironment2).createWriter()) { final ClusterState oldClusterState = loadPersistedClusterState(newPersistedClusterStateService(nodeEnvironment1)); final ClusterState newClusterState = ClusterState.builder(oldClusterState) .metadata(Metadata.builder(oldClusterState.metadata()) .version(1L) .coordinationMetadata(CoordinationMetadata.builder(oldClusterState.coordinationMetadata()).term(1L).build()) .put(IndexMetadata.builder(indexName) .version(1L) .settings(Settings.builder() .put(IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), 1) .put(IndexMetadata.INDEX_NUMBER_OF_REPLICAS_SETTING.getKey(), 0) .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) .put(IndexMetadata.SETTING_INDEX_UUID, indexUUID)))) .incrementVersion().build(); writeState(writer1, 0L, newClusterState, oldClusterState); writeState(writer2, 0L, newClusterState, oldClusterState); } final Path brokenPath = randomFrom(nodeEnvironment1.nodeDataPaths()); final Path dupPath = randomFrom(nodeEnvironment2.nodeDataPaths()); try (Directory directory = new SimpleFSDirectory(brokenPath.resolve(PersistedClusterStateService.METADATA_DIRECTORY_NAME)); Directory dupDirectory = new SimpleFSDirectory(dupPath.resolve(PersistedClusterStateService.METADATA_DIRECTORY_NAME))) { try (IndexWriter indexWriter = new IndexWriter(directory, new IndexWriterConfig())) { indexWriter.deleteDocuments(new Term("type", "global")); // do not duplicate global metadata indexWriter.addIndexes(dupDirectory); indexWriter.commit(); } } final String message = expectThrows(IllegalStateException.class, () -> newPersistedClusterStateService(nodeEnvironment1).loadBestOnDiskState()).getMessage(); assertThat(message, allOf( containsString("duplicate metadata found"), containsString(brokenPath.toString()), containsString(indexName), containsString(indexUUID))); } }

The path.data setting is now a singular string, but the method dataFiles() that gives access to the path is still an array. This commit renames the method and makes the return type a single Path. relates elastic#71205

rjernst · 2021-05-04T16:33:30Z

Thanks for laying out the modified tests @DaveCTurner. I've added them back as suggested. I think this PR is ready for review again.

DaveCTurner

LGTM, I noted scope for one tiny extra cleanup too but no need for another round.

DaveCTurner · 2021-05-04T18:20:27Z

server/src/test/java/org/elasticsearch/env/NodeRepurposeCommandTests.java

            nodePaths = new Path[] { nodeEnvironment.nodeDataPath() };
            final String nodeId = randomAlphaOfLength(10);
-            try (PersistedClusterStateService.Writer writer = new PersistedClusterStateService(nodePaths, nodeId,
+            try (PersistedClusterStateService.Writer writer = new PersistedClusterStateService(nodePaths[0], nodeId,


We can fix nodePaths here too, indeed it's only used in this one method so can just be a local variable of type Path.

This reverts commit 2dfaf7a. The revert was not clean, it required merging with a few changes to loadOnDiskState since the initial removal of MDP support. relates elastic#71205

This reverts commit 2dfaf7a. The revert was not clean, it required merging with a few changes to loadOnDiskState since the initial removal of MDP support. relates #71205

rjernst added :Core/Infra/Core Core issues without another label >refactoring v8.0.0 labels Apr 26, 2021

rjernst requested review from DaveCTurner and jasontedor April 26, 2021 22:00

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Apr 26, 2021

checkstyle

4e91dac

DaveCTurner reviewed Apr 27, 2021

View reviewed changes

DaveCTurner mentioned this pull request Apr 27, 2021

Convert path.data to String setting instead of List #72282

Merged

rjernst added 8 commits April 27, 2021 08:31

Merge branch 'master' into mdp10

e122de7

restore dup tests and handling code

658724e

Make Environment.dataFiles singular

f201c19

The path.data setting is now a singular string, but the method dataFiles() that gives access to the path is still an array. This commit renames the method and makes the return type a single Path. relates elastic#71205

checkstyle

0d1cb0c

Merge branch 'master' into mdp12

c869867

Merge branch 'mdp12' into mdp10

f927e18

Merge branch 'master' into mdp10

7674f3f

add back tests and fixup

c1ba7dd

checkstyle

40af74d

DaveCTurner approved these changes May 4, 2021

View reviewed changes

simplify

0fa6b83

rjernst merged commit 2dfaf7a into elastic:master May 5, 2021

rjernst deleted the mdp10 branch May 5, 2021 18:51

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

rjernst mentioned this pull request Sep 30, 2021

Restore Multiple Data Paths for 8.0 #78525

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove MDP from PersistedClusterStateService#72278

Remove MDP from PersistedClusterStateService#72278
rjernst merged 12 commits intoelastic:masterfrom
rjernst:mdp10

rjernst commented Apr 26, 2021

Uh oh!

elasticmachine commented Apr 26, 2021

Uh oh!

rjernst commented Apr 26, 2021

Uh oh!

DaveCTurner left a comment

Uh oh!

Uh oh!

DaveCTurner Apr 27, 2021

Uh oh!

DaveCTurner Apr 27, 2021

Uh oh!

DaveCTurner Apr 27, 2021

Uh oh!

DaveCTurner Apr 27, 2021

Uh oh!

DaveCTurner Apr 27, 2021

Uh oh!

DaveCTurner Apr 27, 2021

Uh oh!

rjernst commented May 4, 2021

Uh oh!

DaveCTurner left a comment

Uh oh!

DaveCTurner May 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rjernst commented Apr 26, 2021

Uh oh!

elasticmachine commented Apr 26, 2021

Uh oh!

rjernst commented Apr 26, 2021

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaveCTurner Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

rjernst commented May 4, 2021

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 4, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants