Add recovery infrastructure hook to work with older Lucene indices by ywelsch · Pull Request #81056 · elastic/elasticsearch

ywelsch · 2021-11-25T15:22:13Z

This is a simple infrastructure PR for more interesting follow-ups.

We add a simple hook to the snapshot recovery infrastructure that allows making changes to the incoming Lucene files before they are processed the core system. This allows a plugin to make changes to the incoming Lucene files before they are exposed to the rest of the system, e.g. upgrade old Lucene version files to a newer version.

We also add a setting for snapshot repositories to allow restoring older indices (only available in snapshot builds).

elasticmachine · 2021-11-25T15:22:16Z

Pinging @elastic/es-distributed (Team:Distributed)

ywelsch · 2021-11-25T15:26:11Z

server/build.gradle

  ignoreClasses 'org.apache.lucene.index.LazySoftDeletesDirectoryReaderWrapper'
+
+  // only used temporarily, will be removed in a follow-up
+  ignoreClasses 'org.apache.lucene.index.SegmentInfosHelper'


The SegmentInfosHelper class is used by this PR to load ES 6 (= Lucene 7) indices, which allows us to test that the recovery infrastructure as added by the PR works as intended. The class here will be removed in a follow-up step that will take a different, but more involved approach to load older indices (it's out of focus for this PR).

ywelsch · 2021-11-25T15:29:08Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

                    } else if ("mappings".equals(currentFieldName)) {
                        // don't try to parse these for now
                        parser.skipChildren();
+                    } else if ("in_sync_allocations".equals(currentFieldName)) {


This is just copy paste from the regular IndexMetadata parsing code. in_sync_allocations are required to exist in IndexMetadata for a restore to go through.

ywelsch · 2021-11-25T15:30:52Z

server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java

                // store can still have existing files but they will be deleted just before being
                // restored.
-                recoveryTargetMetadata = store.getMetadata(null, true);
+                if (isSearchableSnapshotStore(store.indexSettings().getSettings())) {


We skip some of these steps here for searchable snapshots (as they are effectively useless, and require opening up the index, which we can defer to a later stage by special-casing this here).

ywelsch · 2021-11-25T15:35:19Z

server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java

-    private void afterRestore(SnapshotFiles snapshotFiles, Store store, StoreFileMetadata restoredSegmentsFile) {
-        try {
-            if (isSearchableSnapshotStore(store.indexSettings().getSettings()) == false) {
-                Lucene.pruneUnreferencedFiles(restoredSegmentsFile.name(), store.directory());


This code does not look to do anything, and is actually in the way as it opens the Lucene index before it's converted. No tests are breaking with this code, and it's unclear whether it still serves a purpose (even after some historic digging)

Yes, it looks like we clean up according to the snapshot contents anyway - this second cleanup was added later on in #8969 but there's no indication why it was needed. I think you're right anyway, no need to open the index here.

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

ywelsch · 2021-11-29T12:41:41Z

x-pack/qa/repository-old-versions/build.gradle

- * 2.0 and the Server Side Public License, v 1; you may not use this file except
- * in compliance with, at your election, the Elastic License 2.0 or the Server
- * Side Public License, v 1.
+ * 2.0; you may not use this file except in compliance with the Elastic License


this code got moved from qa to x-pack:qa

DaveCTurner

Mainly looked at the changes to snapshots-of-searchable-snapshots code, and I like this change. It's so much easier now that we can write searchable-snapshot-specific code in :server.

I left a couple of other comments too.

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

ywelsch · 2021-11-29T13:52:25Z

...in/old-lucene-versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/OldSegmentInfos.java

+ */
+@SuppressWarnings("CheckStyle")
+@SuppressForbidden(reason = "Lucene class")
+public class OldSegmentInfos implements Cloneable, Iterable<SegmentCommitInfo> {


This class is just temporarily here until we have proper shaded versions of Lucene. It just helps embedding ES 6 indices in order to let the tests pass.

No need for a review on this one.

server/src/main/java/org/elasticsearch/index/CompositeIndexEventListener.java

server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java

tlrx · 2021-11-29T14:37:15Z

.../old-lucene-versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/OldLuceneVersions.java

+
+    @Override
+    public void onIndexModule(IndexModule indexModule) {
+        indexModule.addIndexEventListener(new IndexEventListener() {


I suppose that we can be more restrictive here and only register the IndexEventListener once for snapshot builds with ALLOW_BWC_INDICES_SETTING setting?

We only ran the listener for snapshot builds. I've now moved the isSnapshotBuild check in 3357ec3 to the registration of the listener. At this point, we don't know anything about the repository, so making it conditional on the ALLOW_BWC_INDICES_SETTING won't work. As this conditional logic will go away (and be replaced by license checks etc. in follow-ups) I think we're good here.

tlrx · 2021-11-29T15:50:58Z

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

            final ImmutableOpenMap.Builder<ShardId, ShardRestoreStatus> shardsBuilder = ImmutableOpenMap.builder();

-            final Version minIndexCompatibilityVersion = currentState.getNodes().getMaxNodeVersion().minimumIndexCompatibilityVersion();
+            final Version minIndexCompatibilityVersion = skipVersionChecks(repositoryMetadata)


We also need some special handling in allocation deciders to only restore old Lucene indices on nodes >= 8.1

This PR is not addressing allocation-related bits, focusing on recovery bits only. There are quite a few more things to do for these indices to be properly handled at the allocation level. For example, restarting a node with a BWC index will currently fail (will detect old index and refuse to start up). This will be addressed in follow-ups.

tlrx · 2021-11-29T15:52:50Z

.../repository-old-versions/src/test/java/org/elasticsearch/oldrepos/OldRepositoryAccessIT.java

+                // so far only added basic infrastructure for reading 6.0.0+ indices
+                if (Build.CURRENT.isSnapshot() && oldVersion.onOrAfter(Version.fromString("6.0.0"))) {
+                    // restore index
+                    RestoreSnapshotResponse restoreSnapshotResponse = client.snapshot()


Can we also test the restore and mount of old Lucene indices over closed indices too?

I've extended the test to cover this.

tlrx

LGTM - I only made minor comments that you can feel free to address or not.

I did not review the pure Lucene parts, I'd expect someone more knowledgeable to review it.

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

server/src/main/java/org/elasticsearch/repositories/blobstore/FileRestoreContext.java

tlrx · 2021-12-02T11:30:49Z

...n/old-lucene-versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/BWCLucene70Codec.java

+import java.util.Iterator;
+import java.util.List;
+
+public class BWCLucene70Codec extends Codec {


Can we add some documentation about what this class does?

The Lucene bits are temporary and will be completely reworked in a follow-up. Will address it there.

tlrx · 2021-12-02T11:45:41Z

.../old-lucene-versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/OldLuceneVersions.java

+        final String segmentFileName = SegmentInfos.getLastCommitSegmentsFileName(directory);
+        if (segmentFileName != null) {
+            long generation = SegmentInfos.generationFromSegmentsFileName(segmentFileName);
+            try (ChecksumIndexInput input = directory.openChecksumInput(segmentFileName, IOContext.READ)) {


I'm not sure why a ChecksumIndexInput is used here? It seems that a regular IndexInput can be used, or maybe there's something I miss.

This method is going away in a follow-up.

tlrx · 2021-12-02T11:46:29Z

.../old-lucene-versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/OldLuceneVersions.java

+                CodecUtil.checkIndexHeaderSuffix(input, Long.toString(generation, Character.MAX_RADIX));
+
+                Version luceneVersion = Version.fromBits(input.readVInt(), input.readVInt(), input.readVInt());
+                int indexCreatedVersion = input.readVInt();


It is unused

going away in a follow-up.

tlrx · 2021-12-02T11:47:21Z

.../old-lucene-versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/OldLuceneVersions.java

+                int indexCreatedVersion = input.readVInt();
+                return luceneVersion;
+            } catch (Exception e) {
+                // ignore


Should we fail the shard if we cannot read the Lucene version?

addressed in follow-up

tlrx · 2021-12-02T11:52:10Z

...main/java/org/elasticsearch/xpack/searchablesnapshots/store/InMemoryNoOpCommitDirectory.java

 public class InMemoryNoOpCommitDirectory extends FilterDirectory {

    private final Directory realDirectory;
+    private final Set<String> deletedFiles = new HashSet<>();


Can we make this a concurrent set?

++ good point

tlrx · 2021-12-02T11:54:07Z

.../repository-old-versions/src/test/java/org/elasticsearch/oldrepos/OldRepositoryAccessIT.java

                assertThat(snapshotStatus.getStats().getTotalFileCount(), greaterThan(0));
+
+                // so far only added basic infrastructure for reading 6.0.0+ indices
+                if (Build.CURRENT.isSnapshot() && oldVersion.onOrAfter(Version.fromString("6.0.0"))) {


At some point we could reintroduce the Version constants for old Elasticsearch versions.

This logic will completely change in the follow-up.

tlrx · 2021-12-02T11:55:33Z

.../repository-old-versions/src/test/java/org/elasticsearch/oldrepos/OldRepositoryAccessIT.java

+        // mount as searchable snapshot
+        RestoreSnapshotResponse mountSnapshotResponse = client.searchableSnapshots()
+            .mountSnapshot(
+                new MountSnapshotRequest("testrepo", "snap1", "test").storage(MountSnapshotRequest.Storage.FULL_COPY)


Can we randomize this between full/partial?

needed another small tweak. I have this already in the follow-up.

ywelsch · 2021-12-02T15:02:09Z

Thanks @tlrx and @DaveCTurner !

ywelsch added 15 commits November 22, 2021 17:41

Use searchable snapshots with old repos

8e1e74e

add recovery hook

16787b0

Basic infra for restoring old snaps

5eb1652

Merge remote-tracking branch 'elastic/master' into search-snap-old-repos

0573748

oops

34e3249

cjbjvø≈

2b81012

jgjh

8bcfd6c

it's nullable

74209b5

some reverting

0a2d9c7

renames

01fb461

asdafafa

72fd881

fixo

e9b8039

asdaf

63f6fec

Merge remote-tracking branch 'elastic/master' into search-snap-old-repos

38537c0

asda

737421a

ywelsch added >non-issue :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.1.0 labels Nov 25, 2021

elasticmachine added the Team:Distributed Meta label for distributed team. label Nov 25, 2021

ywelsch commented Nov 25, 2021

View reviewed changes

ywelsch added 4 commits November 25, 2021 16:52

fix test

67eb0d1

fix test

f9ef94b

Merge remote-tracking branch 'elastic/master' into search-snap-old-repos

b3e91e6

moar test fix

43ab5e3

ywelsch commented Nov 29, 2021

View reviewed changes

ywelsch requested review from DaveCTurner and tlrx November 29, 2021 12:42

DaveCTurner reviewed Nov 29, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java Outdated Show resolved Hide resolved

no further split packages

01debd3

disallow usage of "allow_bwc_indices" repo setting in release buildso

1680afa

ywelsch commented Nov 29, 2021

View reviewed changes

oh no, a newline

f6b4450

tlrx reviewed Nov 29, 2021

View reviewed changes

ywelsch added 6 commits November 30, 2021 10:37

Tanguy's review feedbacko

3357ec3

Merge remote-tracking branch 'elastic/master' into search-snap-old-repos

a134574

quality checks as usual, spotless

833e2e0

warnings...

697b0f0

...

1878a0d

Check source and simple runtime fields query

13dcf38

ywelsch requested a review from tlrx November 30, 2021 11:41

ywelsch mentioned this pull request Dec 1, 2021

Snapshots as simple archives #81210

Closed

32 tasks

tlrx approved these changes Dec 2, 2021

View reviewed changes

ywelsch added 2 commits December 2, 2021 14:27

review comments

bab959e

Merge remote-tracking branch 'elastic/master' into search-snap-old-repos

8132cee

ywelsch merged commit 1d3f684 into elastic:master Dec 2, 2021

Conversation

ywelsch commented Nov 25, 2021

Uh oh!

elasticmachine commented Nov 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywelsch Nov 25, 2021 • 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!

ywelsch Nov 25, 2021 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ywelsch Nov 29, 2021 • 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!

Uh oh!

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!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

ywelsch commented Dec 2, 2021

Uh oh!

ywelsch Nov 25, 2021 •

edited

Loading

ywelsch Nov 25, 2021 •

edited

Loading

ywelsch Nov 29, 2021 •

edited

Loading