Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Zen2] Implement state recovery #36013

Merged
merged 15 commits into from
Dec 4, 2018
Merged

Conversation

andrershov
Copy link
Contributor

This PR implements proper metadata recovery for Zen2.

GatewayServiceis responsible for recovery. In Zen1 GatewayService
creates aninstance of Gateway, that is used to read out to other cluster
nodes, get their state and calculate the most up-to-date state based on
versions. After that Gateway passes this restored state to GatewayService.GatewayRecoveryListener that mixes up current state
and restored state, removes state not recovered block, creates the
routing table and performs re-routing.

For Zen2 most of these steps could be omitted because currentState is
what should be used. However, Zen2 still needs to remove state not
recovered block, create routing table and perform re-routing.
This PR abstracts things to be done for recovery as a Runnable created
based on discovery type. Also GatewayService.RecoverStateUpdateTask
class is created, which is submitted for Zen2 case. In case of Zen1,
submitted task extends RecoverStateUpdateTask.

This PR also switches all tests that are already using Zen2 from
InMemoryPersistedState to GatewayMetaState.

@andrershov andrershov self-assigned this Nov 28, 2018
@andrershov andrershov added >enhancement :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Nov 28, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some comments. Gateway does a little more which we have to take into account as well, e.g. upgrading and archiving unknown or invalid settings, and import indices as closed for which the index metadata has some issues.

@ywelsch ywelsch changed the title [Zen2] Implement proper recovery [Zen2] Implement state recovery Nov 29, 2018
@andrershov
Copy link
Contributor Author

@ywelsch I've sufficiently reworked my PR, to address things that were missing for Zen2 (upgradeAndArchiveUnknownOrInvalidSettings and close bad indices). Now there are a bunch of ClusterState updaters (essentially a function from ClusterState to ClusterState) that could be found in GatewayService.Updaters class. upgradeAndArchiveUnknownOrInvalidSettings and closeBadIndices are among them (so Gateway is no longer responsible for this work). Also, there is a recoverClusterBlock updater, which previously was inlined in GatewayRecoveryListener for Zen1 and in GatewayMetaStateService for Zen2. I think the resulting code is much cleaner.
As a side effect of this, since all of these updaters are static functions, each of them could be easily unit-tested and you can find tests in GatewayServiceTests (two of them, namely, testUpgradePersistentSettings and testUpgradeTransientSettings have migrated from GatewayTests).
Also, I've made variables final whenever possible in parts of the code that I've touched.
Could you please make the second pass?

@andrershov
Copy link
Contributor Author

run gradle build tests 1

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the iteration @andrershov.

import static org.hamcrest.Matchers.equalTo;

@ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0)
public class PersistedStateIT extends ESIntegTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this class is now gone, can you activate some of the tests that do full cluster restarts? For example, PersistentTasksExecutorFullRestartIT. Find other test class that are referencing TestZenDiscovery.USE_ZEN2 and see if they have a comment about "no state persistence yet" or something similar, enable them and see if they reliably pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of tests that need to be enabled and a lot of them fail and require code changes. I've spent most time of the day to analyze failures, I suggest to address them in a follow-up PR:

Tests that pass w/o code changes:

  1. CreateIndexIT
  2. ClusterStatsIT
  3. FlushIT
  4. CorruptedFileIT
  5. QuorumGatewayIT
  6. GatewayIndexIT (except testDanglingIndices)

Tests that require (known) code changes to pass:

  1. IngestRestartIT - IngestService.innerUpdatePipelines should be updated.
  2. RecoverAfterNodesIT - this tests sets autoMinMasterNodes to false and in this case auto-cluster bootstrap is not pefromed. Changes are required to the test code.
  3. PersistentTaskExecutorFullRestartIT - PersistentTaskNodeService.clusterChanged should be updated.

Tests that fail and require further investigation:

  1. EnableAssignmentDecidedIT - changes to PersistentTaskNodeService.clusterChanged are not enough, for some reason persisent tasks assignment is performed, before ClusterSettings reflect that assingment is disabled.
  2. PrimaryAllocationIT
  3. GatewayIndexIT.testDanglingIndices
  4. RemoveCorruptedShardCommandIT

@andrershov
Copy link
Contributor Author

@ywelsch Thank you, I've made required changes, I've also commented on enabling zen2 tests, I strongly believe this should be addressed by a follow-up PR.

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at the tests. I've asked for two tiny changes, looks good otherwise.

.nodes(DiscoveryNodes.builder().add(localNode).localNodeId(localNode.getId()).build())
.build();
public void applyClusterStateUpdaters() {
assert clusterStateUpdatersApplied == false : "applyClusterStateUpdaters must only be called once";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the previousClusterState.nodes().getLocalNode() == null to adding another mutable field to this class.
For extra safety, you can assert that transportService.getLocalNode() != null when this is called.

@@ -196,7 +222,7 @@ public long getCurrentTerm() {

@Override
public ClusterState getLastAcceptedState() {
assert previousClusterState.nodes().getLocalNode() != null : "Call setLocalNode before calling this method";
assert clusterStateUpdatersApplied : "Call applyClusterStateUpdaters before calling this method";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe say here that cluster state is not fully recovered / built yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. >enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants