Never leave stale delete tombstones in version map #29619

dnhatn · 2018-04-19T16:41:07Z

Today the VersionMap does not clean up a stale delete tombstone if it
does not require safe access. However, in a very rare situation due to
concurrent refreshes, the safe-access flag may be flipped over then an
engine accidentally consult that stale delete tombstone.

This commit ensures to never leave stale delete tombstones in a version
map by always pruning delete tombstones when putting a new index entry
regardless of the value of the safe-access flag.

Today the VersionMap does not clean up a stale delete tombstone if it does not require safe access. However, in a very rare situation due to concurrent refreshes, the safe-access flag may be flipped over then an engine accidentally consult that stale delete tombstone. This commit ensures to never leave stale delete tombstones in a version map by always pruning delete tombstones when putting a new index entry regardless of the value of the safe-access flag.

elasticmachine · 2018-04-19T16:41:09Z

Pinging @elastic/es-distributed

dnhatn · 2018-04-19T16:45:40Z

@DaveCTurner, I have a test simulating the violated case found by the formal model. However, I discussed with @bleskes and decided not to include it because the version-map test may be enough. You can refer this test at 2cff7fb.

DaveCTurner

I think this looks good, but I'm not going to be able to check it in detail before tomorrow now.

bleskes

prod code lgtm. I left a question on the tests.

bleskes · 2018-04-19T17:37:42Z

server/src/main/java/org/elasticsearch/index/engine/LiveVersionMap.java

@@ -312,6 +312,10 @@ void maybePutIndexUnderLock(BytesRef uid, IndexVersionValue version) {
        if (maps.isSafeAccessMode()) {
            putIndexUnderLock(uid, version);
        } else {
+            // The existing delete tombstone is no longer the latest version (.i.e stale)
+            // when a newer version is processed regardless of the safe mode of the version map.
+            // Therefore, it's better to always prune that tombstone to avoid accidental accesses.


How about:
// Even though we don't store a record of the indexing operation (and mark as unsafe) we should still remove any previous delete for this uuid. Note that this is expected to not hurt performance because the tombstone is small (if not empty) when usafe is relevant.

I've updated the comment.

bleskes · 2018-04-19T17:41:43Z

server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java

@@ -396,6 +397,50 @@ public void testPruneTombstonesWhileLocked() throws InterruptedException, IOExce
        assertEquals(0, map.getAllTombstones().size());
    }

+    public void testNeverLeaveStaleDeleteTombstone() throws Exception {


can you explain why we need concurrency here? (i.e., why a single threaded test is not enough)

A single threaded test is good enough ;)

dnhatn · 2018-04-19T18:00:53Z

@bleskes I've addressed your comments. Can you please have another look? Thank you.

bleskes

LGTM. Left some comments. No need for another review.

bleskes · 2018-04-19T19:36:33Z

server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java

+                } else {
+                    versionMap.maybePutIndexUnderLock(uid, new IndexVersionValue(randomTranslogLocation(), version, 1, 1));
+                }
+                VersionValue storedValue = versionMap.getUnderLock(uid);


can we also check that this return value correlates with what we expect? i.e., if the doc was just deleted it should be deleted, if it was indexed it should never be marked as deleted? (and found if we're in safe mode). Also can you check this property holds also not directly after indexing/deleting? (i.e., after refresh, enforceSafe access etc). Said differently - maybe also randomly do nothing under lock?

bleskes · 2018-04-19T19:37:27Z

server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java

@@ -396,6 +397,32 @@ public void testPruneTombstonesWhileLocked() throws InterruptedException, IOExce
        assertEquals(0, map.getAllTombstones().size());
    }

+    public void testNeverLeaveStaleDeleteTombstone() throws Exception {


Maybe name the test: "testRandomlyDeletingAndIndexingDoc"? it tests much more, which is good.

dnhatn · 2018-04-20T00:49:37Z

Thanks @bleskes and @DaveCTurner for reviewing.

@DaveCTurner I am merging this to catch the release train but feel free to leave your feedbacks.

Previously we did not put an indexing to a version map if that map does not require safe access but removed the existing delete tombstone only if assertion enabled. In #29585, we removed the side-effect caused by assertion then this test started failing. This failure can be explained as follows: - Step 1: Index a doc then delete that doc - Step 2: The version map can switch to unsafe mode because of concurrent refreshes (implicitly called by flushes) - Step 3: Index a document - the version map won't add this version value and won't prune the tombstone (previously it did) - Step 4: Delete a document - this will return NOT_FOUND instead of DELETED because of the stale delete tombstone This failure is actually fixed by #29619 in which we never leave stale delete tombstones Closes #29626

Today the VersionMap does not clean up a stale delete tombstone if it does not require safe access. However, in a very rare situation due to concurrent refreshes, the safe-access flag may be flipped over then an engine accidentally consult that stale delete tombstone. This commit ensures to never leave stale delete tombstones in a version map by always pruning delete tombstones when putting a new index entry regardless of the value of the safe-access flag.

Previously we did not put an indexing to a version map if that map does not require safe access but removed the existing delete tombstone only if assertion enabled. In #29585, we removed the side-effect caused by assertion then this test started failing. This failure can be explained as follows: - Step 1: Index a doc then delete that doc - Step 2: The version map can switch to unsafe mode because of concurrent refreshes (implicitly called by flushes) - Step 3: Index a document - the version map won't add this version value and won't prune the tombstone (previously it did) - Step 4: Delete a document - this will return NOT_FOUND instead of DELETED because of the stale delete tombstone This failure is actually fixed by #29619 in which we never leave stale delete tombstones Closes #29626

* es/master: (32 commits) TEST: Unmute testPrimaryRelocationWhileIndexing Remove remaining tribe node references (#29574) Never leave stale delete tombstones in version map (#29619) Do not serialize common stats flags using ordinal (#29600) Remove stale comment from JVM stats (#29625) TEST: Mute testPrimaryRelocationWhileIndexing Remove bulk fallback for write thread pool (#29609) Fix an incorrect reference to 'zero_terms_docs' in match_phrase queries. Update the version compatibility for zero_terms_query in match_phrase. Account translog location to ram usage in version map Remove extra spaces from changelog Add support to match_phrase query for zero_terms_query. (#29598) Fix incorrect references to 'zero_terms_docs' in query parsing error messages. (#29599) Build: Move java home checks to pre-execution phase (#29548) Avoid side-effect in VersionMap when assertion enabled (#29585) [Tests] Remove accidental logger usage Add tests for ranking evaluation with aliases (#29452) Deprecate use of `htmlStrip` as name for HtmlStripCharFilter (#27429) Update plan for the removal of mapping types. (#29586) [Docs] Add rankEval method for Jva HL client ...

* es/6.x: (28 commits) TEST: Unmute testPrimaryRelocationWhileIndexing Never leave stale delete tombstones in version map (#29619) Do not serialize common stats flags using ordinal (#29600) Remove stale comment from JVM stats (#29625) TEST: Mute testPrimaryRelocationWhileIndexing Remove 7.0.0 from 6.x changelog (#29621) Add support to match_phrase query for zero_terms_query. (#29598) Account translog location to ram usage in version map Avoid side-effect in VersionMap when assertion enabled (#29585) Build: Move java home checks to pre-execution phase (#29548) Add tests for ranking evaluation with aliases (#29452) [Test] Fix assertion in SearchDocumentationIT Deprecate use of `htmlStrip` as name for HtmlStripCharFilter (#27429) test: Assert deprecated http.enebled setting warning Update plan for the removal of mapping types. (#29586) [Docs] Add rankEval method for Jva HL client Make ranking evaluation details accessible for client Rename the bulk thread pool to write thread pool (#29593) [Test] Minor changes to rank_eval tests (#29577) test: Assert deprecated http.enebled setting warning ...

dnhatn added >enhancement v7.0.0 v6.3.0 :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. labels Apr 19, 2018

dnhatn requested review from s1monw, bleskes and DaveCTurner April 19, 2018 16:41

DaveCTurner reviewed Apr 19, 2018

View reviewed changes

bleskes reviewed Apr 19, 2018

View reviewed changes

Address Boaz’s comments

e69b6df

dnhatn requested a review from bleskes April 19, 2018 18:01

bleskes approved these changes Apr 19, 2018

View reviewed changes

dnhatn added 3 commits April 19, 2018 16:55

stronger test

d869bd0

correct iteration loop

64f415c

Merge branch 'master' into never-leave-stale-tombstone

75595f2

dnhatn merged commit 9cf8b01 into elastic:master Apr 20, 2018

dnhatn deleted the never-leave-stale-tombstone branch April 20, 2018 00:50

dnhatn added the backport pending label Apr 20, 2018

dnhatn mentioned this pull request Apr 20, 2018

[CI] testPrimaryRelocationWhileIndexing fails on master branch #29626

Closed

dnhatn removed the backport pending label Apr 20, 2018

dnhatn mentioned this pull request Jul 13, 2018

Possible to index duplicate documents with same id and routing id. #31976

Closed

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

seut mentioned this pull request Feb 25, 2019

Duplicate results to query because of replication crate/crate#8277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Never leave stale delete tombstones in version map #29619

Never leave stale delete tombstones in version map #29619

dnhatn commented Apr 19, 2018

elasticmachine commented Apr 19, 2018

dnhatn commented Apr 19, 2018

DaveCTurner left a comment

bleskes left a comment

bleskes Apr 19, 2018

dnhatn Apr 19, 2018

bleskes Apr 19, 2018

dnhatn Apr 19, 2018

dnhatn commented Apr 19, 2018

bleskes left a comment

bleskes Apr 19, 2018

bleskes Apr 19, 2018

dnhatn commented Apr 20, 2018

Never leave stale delete tombstones in version map #29619

Never leave stale delete tombstones in version map #29619

Conversation

dnhatn commented Apr 19, 2018

elasticmachine commented Apr 19, 2018

dnhatn commented Apr 19, 2018

DaveCTurner left a comment

Choose a reason for hiding this comment

bleskes left a comment

Choose a reason for hiding this comment

bleskes Apr 19, 2018

Choose a reason for hiding this comment

dnhatn Apr 19, 2018

Choose a reason for hiding this comment

bleskes Apr 19, 2018

Choose a reason for hiding this comment

dnhatn Apr 19, 2018

Choose a reason for hiding this comment

dnhatn commented Apr 19, 2018

bleskes left a comment

Choose a reason for hiding this comment

bleskes Apr 19, 2018

Choose a reason for hiding this comment

bleskes Apr 19, 2018

Choose a reason for hiding this comment

dnhatn commented Apr 20, 2018