drop `index.shard.check_on_startup: fix` #32279

vladimirdolzhenko · 2018-07-23T12:41:10Z

drop index.shard.check_on_startup: fix setting
put a corrupt marker if it is detected during start up check that index

Relates #31389

elasticmachine · 2018-07-23T12:41:12Z

Pinging @elastic/es-distributed

Relates elastic#31389

DaveCTurner

Looks good, and I'm pleased that there can be some tests for these settings. I asked a few questions about the tests.

DaveCTurner · 2018-08-20T13:31:38Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+        final IndexMetaData indexMetaData = IndexMetaData.builder(indexShard.indexSettings().getIndexMetaData())
+            .settings(Settings.builder()
+                .put(indexShard.indexSettings.getSettings())
+                .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), "checksum"))


Does this test also work if we set this to true? If so, could we choose randomly between these two settings, and rename the test appropriately?

In case of corrupted index IndexShard with checksum option creates corrupted marker file, while in case of true it does not create it - seems due to https://github.com/elastic/elasticsearch/pull/32279/files#diff-49e1a1b834b522f4ae6997c5defe9eb0R1939 - as IllegalStateException is not subclass of IOException https://github.com/elastic/elasticsearch/pull/32279/files#diff-49e1a1b834b522f4ae6997c5defe9eb0R1894

Is it worth to change to IOException as well ?

It sounds to me like it's doing the wrong thing here, but these things are subtle. @ywelsch might know: if check_on_startup: true finds a corruption it doesn't currently mark the shard as corrupted. I think it should. WDYT?

@DaveCTurner @ywelsch
https://www.elastic.co/guide/en/elasticsearch/reference/current/index-modules.html

checksum - Check for physical corruption.

true - Check for both physical and logical corruption. This is much more expensive in terms of CPU and memory usage.

true relies on Lucene CheckIndex check - that is indeed heavier operation - I would say we have to create a corruption marker

DaveCTurner · 2018-08-20T13:35:56Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+        // start shard with checksum - it has to pass successfully
+
+        final ShardRouting shardRouting = ShardRoutingHelper.initWithSameId(indexShard.routingEntry(),
+            primary ? RecoverySource.StoreRecoverySource.EXISTING_STORE_INSTANCE : RecoverySource.PeerRecoverySource.INSTANCE


primary is always true here.

DaveCTurner · 2018-08-20T13:38:21Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+        assertThat("store is clean",
+            corruptedMarkerCount.get(), equalTo(0));
+
+        // storeProvider that does not perform check index on close - it is corrupted


I don't fully understand this comment, or what this storeProvider is for. The heart of this test seems to be:

expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard, primary));

We also want the corruption marker to exist as soon as this is thrown, not after the shard is subsequently closed. If we need to test that opening a shard with a corruption marker fails, and that the corruption marker isn't removed when it's closed, then I think that should be a separate test.

Indeed, that heart of the test is
expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard, primary));

the problem with default store provider - that it checks index on close. In case of corrupted index - it fails on close (as it's corrupted) - and we have to close it to avoid resource leakage.

therefore the key point of adjusted storeProvider is

baseDirectoryWrapper.setCheckIndexOnClose(false);

Agree to split it into 2 tests - the 2nd one with opening a shard with a corruption marker fails

DaveCTurner · 2018-08-20T13:48:35Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+            Files.walk(indexPath)
+                .filter(p -> Files.isRegularFile(p) && IndexWriter.WRITE_LOCK_NAME.equals(p.getFileName().toString()) == false)
+                .toArray(Path[]::new);
+        CorruptionUtils.corruptFile(random(), filesToCorrupt);


Does this definitely corrupt a file that will be checked? It doesn't, for instance, hit a translog file in a generation that's early enough that it's no longer needed?

It indeed corrupts only index file due to
final Path indexPath = shardPath.getDataPath().resolve("index");

translog files are stored in another place - shardPath.getDataPath().resolve("translog");

…heckChecksum into two tests: testIndexCheckOnStartup, testShardDoesNotStartIfCorruptedMarkerIsPresent; clean up

vladimirdolzhenko · 2018-08-21T11:26:01Z

@elasticmachine test this please

vladimirdolzhenko · 2018-08-21T13:29:32Z

@DaveCTurner I addressed your comments - tried ran test twice but it fails on completely irrelevant part (passed on private CI) - could you please have another look into this PR ?

DaveCTurner

Looks good. I asked for a change to one of the tests and a whitespace nit. Also, if you've picked up a flaky commit from master then I recommend trying to merge a later, good, one in.

DaveCTurner · 2018-08-21T13:42:14Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+                .put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), randomFrom("true", "checksum")))
+            .build();
+
+        final Path indexPath = corruptIndexFile(shardPath);


I think this test should create the corruption marker directly, rather than relying on the check-on-startup behaviour. Essentially, it should fail to start even in the presence of a corruption marker, even if the index itself is not corrupt.

DaveCTurner · 2018-08-21T13:49:04Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+        };
+        Files.walkFileTree(indexPath, corruptedVisitor);
+
+        assertThat("store is clean",


I think I'd prefer statements to go all on one line if possible. There's a few places where that could be done, such as this one.

DaveCTurner · 2018-08-21T13:54:14Z

I think this should go into 6.x too - your labels indicate this will only be merged into 7.0. We also need a PR to update the breaking changes docs for 6.x to note that fix is no longer supported. I'm not sure if we should note that in the 6.0 breaking changes, because it's not worked since then.

vladimirdolzhenko · 2018-08-21T14:11:44Z

@DaveCTurner this PR has to go together with #32281 - so it depends mostly on that PR - as far as it is ready we can target version and back port to 6.x

…check shard does not start if corruption marker is there; drop unnecessary new lines

DaveCTurner

I did another pass and this is nearly there - I left just a few requests to tidy up comments and assertion messages, and spotted one missing assertion.

DaveCTurner · 2018-08-27T09:47:00Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+            null, null, indexShard.engineFactory,
+            indexShard.getGlobalCheckpointSyncer(), EMPTY_EVENT_LISTENER);
+
+        expectThrows(IndexShardRecoveryException.class, () -> newStartedShard(p -> corruptedShard, true));


Please could you assert something about the thrown exception - ideally that its message describes the affected shard and contains an appropriate message?

DaveCTurner · 2018-08-27T09:47:57Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+        indexShard.flush(new FlushRequest());
+        closeShards(indexShard);
+
+        // start shard with checksum - it has to pass successfully


This comment seems misleading: we don't start the shard here, and we expect it not to pass successfully when we do.

makes sense, addressed your comments

DaveCTurner · 2018-08-27T09:49:25Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+
+        final Path indexPath = corruptIndexFile(shardPath);
+
+        // check that corrupt marker is *NOT* there


I don't think this comment is necessary here. The check is the assertion a few lines further down, and it should be clear what that assertion is doing. I think the message in the assertion could use the phrase "corruption marker" rather than "clean" for consistency.

DaveCTurner · 2018-08-27T09:50:05Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+        assertThat(exception1.getCause().getMessage(), equalTo(corruptionMessage + " (resource=preexisting_corruption)"));
+        closeShards(corruptedShard);
+
+        // check that corrupt marker is there


Similarly - this comment isn't needed here.

DaveCTurner · 2018-08-27T09:50:12Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+
+        // create corrupted marker
+        final String corruptionMessage = "fake ioexception";
+        try(Store store = createStore(indexShard.indexSettings(), shardPath)){


Nit: whitespace

DaveCTurner · 2018-08-27T09:54:14Z

Also please update the labels on this to indicate how it will be backported. I think it should go into 6.5 too.

vladimirdolzhenko · 2018-08-27T10:28:42Z

it has to be backported to 6.5 together with #32281

DaveCTurner · 2018-08-27T11:52:59Z

We discussed back-porting in another channel and agreed:

this change can be made independently of add RemoveCorruptedShardDataCommand #32281
for ease of review this PR should be made backportable to 6.5 with a small follow-up to actually remove the option in 7.0.

DaveCTurner

Actually, we need a deprecation warning on using fix so that we can remove it in 7.0.

vladimirdolzhenko · 2018-08-28T10:25:10Z

I will add deprecation message for fix

DaveCTurner

There's a simpler way to write the test - see comments. Also suggested adding "which has no effect" to the deprecation message, and to use the word "accepted" rather than "supported".

DaveCTurner · 2018-08-28T12:00:43Z

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java

@@ -299,6 +299,10 @@ public IndexShard(
        logger.debug("state: [CREATED]");

        this.checkIndexOnStartup = indexSettings.getValue(IndexSettings.INDEX_CHECK_ON_STARTUP);
+        if ("fix".equals(checkIndexOnStartup)) {
+            deprecationLogger.deprecated("Setting [index.shard.check_on_startup] is set to deprecated value [fix], "
+                + "which will be unsupported in future");


Suggest

which has no effect and will not be accepted in future

DaveCTurner · 2018-08-28T12:01:22Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardDeprecatedSettingTests.java

+        return false;
+    }
+
+    public void testCheckOnStartupDeprecatedValue() throws Exception {


I think this could be simplified to:

public void testCheckOnStartupDeprecatedValue() throws Exception { final IndexShard newShard = newShard(true, Settings.builder().put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), "fix").build()); assertWarnings("Setting [index.shard.check_on_startup] is set to deprecated value [fix], which will be unsupported in future"); closeShards(newShard); }

(and using assertWarnings means it doesn't need to be in its own test fixture too)

already figured that out ;)

DaveCTurner · 2018-08-28T12:01:32Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardDeprecatedSettingTests.java

+    public void testCheckOnStartupDeprecatedValue() throws Exception {
+        final Settings settings = Settings.builder().put(IndexSettings.INDEX_CHECK_ON_STARTUP.getKey(), "fix").build();
+
+        try(ThreadContext threadContext = new ThreadContext(Settings.EMPTY)) {


Nit: missing whitespace after the try.

DaveCTurner

Thanks, LGTM now.

vladimirdolzhenko · 2018-08-28T15:16:04Z

@elasticmachine test this please

…ster' into fix/31389_1

vladimirdolzhenko · 2018-08-31T10:56:35Z

thanks @DaveCTurner 👍

Relates #31389 (cherry picked from commit 3d82a30)

completely drop `index.shard.check_on_startup: fix` for 7.0 (#33194) Relates to #32279

Relates to #32279

* master: Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test Adjust soft-deletes version after backport into 6.5 completely drop `index.shard.check_on_startup: fix` for 7.0 (#33194) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi drop `index.shard.check_on_startup: fix` (#32279) tracked at [DOCS] Moves ml folder from x-pack/docs to docs (#33248) [DOCS] Move rollup APIs to docs (#31450) [DOCS] Rename X-Pack Commands section (#33005) TEST: Disable soft-deletes in ParentChildTestCase Fixes SecurityIntegTestCase so it always adds at least one alias (#33296) Fix pom for build-tools (#33300) Lazy evaluate java9home (#33301) SQL: test coverage for JdbcResultSet (#32813) Work around to be able to generate eclipse projects (#33295) Highlight that index_phrases only works if no slop is used (#33303) Different handling for security specific errors in the CLI. Fix for #33230 (#33255) [ML] Refactor delimited file structure detection (#33233) SQL: Support multi-index format as table identifier (#33278) MINOR: Remove Dead Code from PathTrie (#33280) Enable forbiddenapis server java9 (#33245)

* 6.x: Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test TEST: Disable soft-deletes in ParentChildTestCase TEST: Disable randomized soft-deletes settings Integrates soft-deletes into Elasticsearch (#33222) drop `index.shard.check_on_startup: fix` (#32279) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi [DOCS] Moves ml folder from x-pack/docs to docs (#33248) TEST: mute more SmokeTestWatcherWithSecurityIT tests [DOCS] Move rollup APIs to docs (#31450) [DOCS] Rename X-Pack Commands section (#33005) Fixes SecurityIntegTestCase so it always adds at least one alias (#33296) TESTS: Fix Random Fail in MockTcpTransportTests (#33061) (#33307) MINOR: Remove Dead Code from PathTrie (#33280) (#33306) Fix pom for build-tools (#33300) Lazy evaluate java9home (#33301) SQL: test coverage for JdbcResultSet (#32813) Work around to be able to generate eclipse projects (#33295) Different handling for security specific errors in the CLI. Fix for #33230 (#33255) [ML] Refactor delimited file structure detection (#33233) SQL: Support multi-index format as table identifier (#33278) Enable forbiddenapis server java9 (#33245) [MUTE] SmokeTestWatcherWithSecurityIT flaky tests Add region ISO code to GeoIP Ingest plugin (#31669) (#33276) Don't be strict for 6.x Update serialization versions for custom IndexMetaData backport Replace IndexMetaData.Custom with Map-based custom metadata (#32749) Painless: Fix Bindings Bug (#33274) SQL: prevent duplicate generation for repeated aggs (#33252) TEST: Mute testMonitorClusterHealth Fix serialization of empty field capabilities response (#33263) Fix nested _source retrieval with includes/excludes (#33180) [DOCS] TLS file resources are reloadable (#33258) Watcher: Ensure TriggerEngine start replaces existing watches (#33157) Ignore module-info in jar hell checks (#33011) Fix docs build after #33241 [DOC] Repository GCS ADC not supported (#33238) Upgrade to latest Gradle 4.10 (#32801) Fix/30904 cluster formation part2 (#32877) Move file-based discovery to core (#33241) HLRC: add client side RefreshPolicy (#33209) [Kerberos] Add unsupported languages for tests (#33253) Watcher: Reload properly on remote shard change (#33167) Fix classpath security checks for external tests. (#33066) [Rollup] Only allow aggregating on multiples of configured interval (#32052) Added deprecation warning for rescore in scroll queries (#33070) Apply settings filter to get cluster settings API (#33247) [Rollup] Re-factor Rollup Indexer into a generic indexer for re-usability (#32743) HLRC: create base timed request class (#33216) HLRC: Use Optional in validation logic (#33104) Painless: Add Bindings (#33042)

* master: (64 commits) HLREST: add update by query API (elastic#32760) TEST: Increase timeout testFollowIndexAndCloseNode (elastic#33333) HLRC: ML Flush job (elastic#33187) HLRC: Adding ML Job stats (elastic#33183) LLREST: Drop deprecated methods (elastic#33223) Mute testSyncerOnClosingShard [DOCS] Moves machine learning APIs to docs folder (elastic#31118) Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test Adjust soft-deletes version after backport into 6.5 completely drop `index.shard.check_on_startup: fix` for 7.0 (elastic#33194) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi drop `index.shard.check_on_startup: fix` (elastic#32279) tracked at [DOCS] Moves ml folder from x-pack/docs to docs (elastic#33248) [DOCS] Move rollup APIs to docs (elastic#31450) [DOCS] Rename X-Pack Commands section (elastic#33005) TEST: Disable soft-deletes in ParentChildTestCase Fixes SecurityIntegTestCase so it always adds at least one alias (elastic#33296) ...

* master: (283 commits) HLREST: add update by query API (elastic#32760) TEST: Increase timeout testFollowIndexAndCloseNode (elastic#33333) HLRC: ML Flush job (elastic#33187) HLRC: Adding ML Job stats (elastic#33183) LLREST: Drop deprecated methods (elastic#33223) Mute testSyncerOnClosingShard [DOCS] Moves machine learning APIs to docs folder (elastic#31118) Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test Adjust soft-deletes version after backport into 6.5 completely drop `index.shard.check_on_startup: fix` for 7.0 (elastic#33194) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi drop `index.shard.check_on_startup: fix` (elastic#32279) tracked at [DOCS] Moves ml folder from x-pack/docs to docs (elastic#33248) [DOCS] Move rollup APIs to docs (elastic#31450) [DOCS] Rename X-Pack Commands section (elastic#33005) TEST: Disable soft-deletes in ParentChildTestCase Fixes SecurityIntegTestCase so it always adds at least one alias (elastic#33296) ...

vladimirdolzhenko added >enhancement :Distributed/Store Issues around managing unopened Lucene indices. If it touches Store.java, this is a likely label. v7.0.0 labels Jul 23, 2018

vladimirdolzhenko requested a review from bleskes July 23, 2018 12:41

vladimirdolzhenko mentioned this pull request Jul 23, 2018

add RemoveCorruptedShardDataCommand #32281

Merged

drop index.shard.check_on_startup: fix

843f977

Relates elastic#31389

vladimirdolzhenko force-pushed the fix/31389_1 branch from f5a6acd to 843f977 Compare July 27, 2018 15:15

Merge remote-tracking branch 'remotes/origin/master' into fix/31389_1

4f01609

vladimirdolzhenko requested review from DaveCTurner and removed request for bleskes August 20, 2018 12:37

DaveCTurner reviewed Aug 20, 2018

View reviewed changes

create corrupted marker on check_on_startup: true; split testIndexC…

153e4f2

…heckChecksum into two tests: testIndexCheckOnStartup, testShardDoesNotStartIfCorruptedMarkerIsPresent; clean up

DaveCTurner requested changes Aug 21, 2018

View reviewed changes

Vladimir Dolzhenko added 3 commits August 21, 2018 16:15

Merge remote-tracking branch 'remotes/origin/master' into fix/31389_1

2964fef

create manually corruption marker (but don't corrupt index files) to …

c71e306

…check shard does not start if corruption marker is there; drop unnecessary new lines

checkstyle fix

a7668d6

vladimirdolzhenko requested a review from DaveCTurner August 23, 2018 11:50

Merge remote-tracking branch 'remotes/origin/master' into fix/31389_1

97fa399

DaveCTurner requested changes Aug 27, 2018

View reviewed changes

addressed unit test comments

c155b36

vladimirdolzhenko added the v6.5.0 label Aug 27, 2018

vladimirdolzhenko requested a review from DaveCTurner August 27, 2018 10:29

DaveCTurner requested changes Aug 28, 2018

View reviewed changes

added fix deprecation log message + test

3231803

DaveCTurner requested changes Aug 28, 2018

View reviewed changes

Vladimir Dolzhenko added 2 commits August 28, 2018 14:31

added fix deprecation log message + test

c2b5b8a

adjusted fix deprecation log message

14e6175

DaveCTurner approved these changes Aug 28, 2018

View reviewed changes

Vladimir Dolzhenko added 2 commits August 28, 2018 15:29

dropped fix to avoid deprecation warnings

fee8a5b

skip files added by Lucene's ExtrasFS

5cee2b9

Vladimir Dolzhenko added 5 commits August 28, 2018 18:17

Merge remote-tracking branch 'remotes/origin/master' into fix/31389_1

ad62da0

Merge remote-tracking branch 'remotes/origin/master' into fix/31389_1

6f6ca5a

Merge remote-tracking branch 'remotes/origin/master' into fix/31389_1

6763cf9

Merge remote-tracking branch 'remotes/origin/master' into fix/31389_1

5083e83

resolved conflicts on Merge remote-tracking branch 'remotes/origin/ma…

2a9dbeb

…ster' into fix/31389_1

Vladimir Dolzhenko added 2 commits August 31, 2018 13:20

Merge remote-tracking branch 'remotes/origin/master' into fix/31389_1

aa16487

Merge remote-tracking branch 'remotes/origin/master' into fix/31389_1

d26fbfb

vladimirdolzhenko merged commit 3d82a30 into elastic:master Aug 31, 2018

vladimirdolzhenko added a commit that referenced this pull request Aug 31, 2018

drop index.shard.check_on_startup: fix (#32279)

6de8c6b

Relates #31389 (cherry picked from commit 3d82a30)

vladimirdolzhenko added a commit that referenced this pull request Aug 31, 2018

completely drop index.shard.check_on_startup: fix for 7.0 (#33194)

d2b2d2f

completely drop `index.shard.check_on_startup: fix` for 7.0 (#33194) Relates to #32279

vladimirdolzhenko pushed a commit that referenced this pull request Aug 31, 2018

completely drop index.shard.check_on_startup: fix for 7.0 (#33194)

00b272a

Relates to #32279

vladimirdolzhenko deleted the fix/31389_1 branch September 22, 2018 10:42

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

jtibshirani mentioned this pull request Jan 21, 2021

Allow index settings behavior to depend on index version #67621

Open


		final Path indexPath = corruptIndexFile(shardPath);

		// check that corrupt marker is NOT there

drop index.shard.check_on_startup: fix #32279

drop index.shard.check_on_startup: fix #32279

Conversation

vladimirdolzhenko commented Jul 23, 2018

elasticmachine commented Jul 23, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirdolzhenko Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirdolzhenko commented Aug 21, 2018

vladimirdolzhenko commented Aug 21, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Aug 21, 2018

vladimirdolzhenko commented Aug 21, 2018 • edited Loading

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Aug 27, 2018

vladimirdolzhenko commented Aug 27, 2018

DaveCTurner commented Aug 27, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

vladimirdolzhenko commented Aug 28, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

vladimirdolzhenko commented Aug 28, 2018

vladimirdolzhenko commented Aug 31, 2018

drop `index.shard.check_on_startup: fix` #32279

drop `index.shard.check_on_startup: fix` #32279

vladimirdolzhenko Aug 21, 2018 •

edited

Loading

vladimirdolzhenko commented Aug 21, 2018 •

edited

Loading