Implement basic `CcrRepository` restore #36287

Tim-Brooks · 2018-12-06T00:15:29Z

This is related to #35975. It implements a basic restore functionality
for the CcrRepository. When the restore process is kicked off, it
configures the new index as expected for a follower index. This means
that the index has a different uuid, the version is not incremented, and
the Ccr metadata is installed.

When the restore shard method is called, an empty shard is initialized.

elasticmachine · 2018-12-06T00:15:31Z

Pinging @elastic/es-distributed

Tim-Brooks · 2018-12-06T00:16:24Z

This pulls over the basic restore functionality from #35719. This is so that we can start implementing the restore process without worrying about the concurrent restore issue.

ywelsch

What's the purpose of having incrementIndexVersion = false?

ywelsch · 2018-12-06T08:39:31Z

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

                @Override
-                public ClusterState execute(ClusterState currentState) {
+                public ClusterState execute(ClusterState currentState) throws Exception {
+                    // TODO: Evaluate if we want to keep once restore at a time behavior


I don't understand this comment. What do you mean here?

Removed. That was just an earlier comment about concurrent restores.

ywelsch · 2018-12-06T08:55:31Z

test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java

+    /**
+     * Returns an Iterable to all instances for the given class &gt;T&lt; for the cluster's master node.
+     */
+    public synchronized <T> T getMasterNodeInstance(Class<T> clazz) {


If you merge latest master, you will notice that there is already a method with same name, but different semantics. Maybe call this one getCurrentMasterNodeInstance(...)

ywelsch

I've left more comments, especially on what SnapshotInfo should represent. Can you please also reply to my comments when pushing changes, so that it's easier for me to see how you addressed them? Look for example at #35678 or #35488

ywelsch · 2018-12-06T18:56:02Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRepository.java

-        throw new UnsupportedOperationException("Unsupported for repository of type: " + TYPE);
+        assert SNAPSHOT_UUID.equals(snapshotId.getUUID()) : "RemoteClusterRepository only supports the _latest_ as the UUID";
+        Client remoteClient = client.getRemoteClusterClient(remoteClusterAlias);
+        ClusterStateResponse response = remoteClient.admin().cluster().prepareState().clear().setMetaData(true).get();


is there a way to filter out the index metadata here? We just want the global metadata.

It looks like if you set indices to an empty array it returns all of the indices. I could like put a single random string for an index name in there?

it's a little ugly but might work, and save on transferring a lot of index metadata over the wire.

I added:

.setIndices("dummy_index_name") // We set a single dummy index name to avoid fetching all the index data

ywelsch · 2018-12-06T18:57:25Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRepository.java

    public SnapshotInfo getSnapshotInfo(SnapshotId snapshotId) {
-        throw new UnsupportedOperationException("Unsupported for repository of type: " + TYPE);
+        assert SNAPSHOT_UUID.equals(snapshotId.getUUID()) : "RemoteClusterRepository only supports the _latest_ as the UUID";
+        return new SnapshotInfo(snapshotId, Collections.singletonList(snapshotId.getName()), SnapshotState.SUCCESS, version);


I wonder if we should return all indices of the remote cluster here. That would more naturally map to the notion of the state of the remote cluster representing a snapshot.

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRepository.java

ywelsch · 2018-12-06T19:08:49Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRepository.java

+    // The snapshot will be compatible because remote cluster connections must be compatible with our cluster
+    // version. So the snapshot version is not too important.
+    // TODO: Evaluate where we eventually want to pull the version from
+    private final Version version = Version.CURRENT;


the version here matters. As an initial approximation, we can take the maximum version of data nodes in the leader cluster.

private void validateSnapshotRestorable(final String repository, final SnapshotInfo snapshotInfo) { if (!snapshotInfo.state().restorable()) { throw new SnapshotRestoreException(new Snapshot(repository, snapshotInfo.snapshotId()), "unsupported snapshot state [" + snapshotInfo.state() + "]"); } if (Version.CURRENT.before(snapshotInfo.version())) { throw new SnapshotRestoreException(new Snapshot(repository, snapshotInfo.snapshotId()), "the snapshot was created with Elasticsearch version [" + snapshotInfo.version() + "] which is higher than the version of this node [" + Version.CURRENT + "]"); } }

Here is the validation performed based on this version. So if the leader cluster is a newer version than the follower cluster, it will be marked as incompatible. So I will take the approach of the maximum version from the data nodes in the leader cluster if you are happy with that.

So I will take the approach of the maximum version from the data nodes in the leader cluster if you are happy with that.

The validation logic you mentioned above (validateSnapshotRestorable) will not guarantee us that there isn't a data node in the follower cluster that is older than the master that runs the validation logic there, but it's at least better than no validation at all. I think it's the best check we can do for now. Can you add an item to the meta-issue that we need to think a bit more about BWC constraints?

I will add a note.

ywelsch · 2018-12-06T19:10:36Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRepository.java

+
+        ImmutableOpenMap<String, IndexMetaData> remoteIndices = remoteMetaData.getIndices();
+        for (String indexName : remoteMetaData.getConcreteAllIndices()) {
+            SnapshotId snapshotId = new SnapshotId(indexName, SNAPSHOT_UUID);


instead of one snapshot per index, I think it's more natural to have a fixed SnapshotID(latest, latest) and all the indices of the cluster then as indices that are part of the snapshot.

ywelsch · 2018-12-06T19:16:38Z

x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/CcrRepositoryIT.java

+
+        PlainActionFuture<RestoreService.RestoreCompletionResponse> future = PlainActionFuture.newFuture();
+        restoreService.restoreSnapshot(restoreRequest, future);
+        future.actionGet();


check that response says that all shards successfully restored?

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRepository.java

ywelsch

Some nits, looks good o.w.

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRepository.java

This is related to elastic#35975. It implements a basic restore functionality for the CcrRepository. When the restore process is kicked off, it configures the new index as expected for a follower index. This means that the index has a different uuid, the version is not incremented, and the Ccr metadata is installed. When the restore shard method is called, an empty shard is initialized.

This is related to #35975. It implements a basic restore functionality for the CcrRepository. When the restore process is kicked off, it configures the new index as expected for a follower index. This means that the index has a different uuid, the version is not incremented, and the Ccr metadata is installed. When the restore shard method is called, an empty shard is initialized.

Tim-Brooks added 2 commits December 5, 2018 17:05

WIP

ae3bf3a

WIP

3554484

Tim-Brooks added >non-issue v7.0.0 :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v6.6.0 labels Dec 6, 2018

Tim-Brooks requested review from bleskes, martijnvg and ywelsch December 6, 2018 00:15

Tim-Brooks added 2 commits December 5, 2018 17:41

Merge remote-tracking branch 'upstream/master' into ccr_repo_work

4a5a7de

Merge remote-tracking branch 'upstream/master' into ccr_repo_work

e6c481f

ywelsch reviewed Dec 6, 2018

View reviewed changes

Tim-Brooks added 3 commits December 6, 2018 09:36

Merge remote-tracking branch 'upstream/master' into ccr_repo_work

d874031

Changes

b07b7c8

Fix test

0582d51

Tim-Brooks requested a review from ywelsch December 6, 2018 17:03

ywelsch suggested changes Dec 6, 2018

View reviewed changes

Tim-Brooks added 2 commits December 6, 2018 18:05

Changes

ceb59ce

Cleanup

b5c3cff

Tim-Brooks requested a review from ywelsch December 7, 2018 16:25

Make change

279990e

ywelsch approved these changes Dec 7, 2018

View reviewed changes

Changes

f8b7bc1

Tim-Brooks merged commit 8a53f2b into elastic:master Dec 7, 2018

Tim-Brooks added the backport pending label Dec 7, 2018

Tim-Brooks removed the backport pending label Dec 12, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Tim-Brooks deleted the ccr_repo_work branch December 18, 2019 14:46

Implement basic CcrRepository restore #36287

Implement basic CcrRepository restore #36287

Uh oh!

Conversation

Tim-Brooks commented Dec 6, 2018

Uh oh!

elasticmachine commented Dec 6, 2018

Uh oh!

Tim-Brooks commented Dec 6, 2018

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Implement basic `CcrRepository` restore #36287

Implement basic `CcrRepository` restore #36287

Tim-Brooks Dec 6, 2018 •

edited

Loading