From e50300ac2084c8ede9b9b41db3cc280aa7b9eb99 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Thu, 19 Apr 2018 11:43:01 -0400 Subject: [PATCH 1/4] Never leave stale delete tombstones in version map 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. --- .../index/engine/LiveVersionMap.java | 4 ++ .../index/engine/InternalEngineTests.java | 17 +++++++ .../index/engine/LiveVersionMapTests.java | 45 +++++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/index/engine/LiveVersionMap.java b/server/src/main/java/org/elasticsearch/index/engine/LiveVersionMap.java index 6d9dc4a38974c..d808a69c30a19 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/LiveVersionMap.java +++ b/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. + removeTombstoneUnderLock(uid); maps.current.markAsUnsafe(); assert putAssertionMap(uid, version); } diff --git a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java index 60913c644eadb..ea4b53f16c003 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java @@ -1576,6 +1576,23 @@ public void testInternalVersioningOnPrimary() throws IOException { assertOpsOnPrimary(ops, Versions.NOT_FOUND, true, engine); } + public void testVersionOnPrimaryWithConcurrentRefresh() throws Exception { + List ops = generateSingleDocHistory(false, VersionType.INTERNAL, false, 2, 10, 100); + CountDownLatch latch = new CountDownLatch(1); + AtomicBoolean running = new AtomicBoolean(true); + Thread refreshThread = new Thread(() -> { + latch.countDown(); + while (running.get()) { + engine.refresh("test"); + } + }); + refreshThread.start(); + latch.await(); + assertOpsOnPrimary(ops, Versions.NOT_FOUND, true, engine); + running.set(false); + refreshThread.join(); + } + private int assertOpsOnPrimary(List ops, long currentOpVersion, boolean docDeleted, InternalEngine engine) throws IOException { String lastFieldValue = null; diff --git a/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java b/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java index e0efcf9f0f73f..b0db498eb51bc 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java @@ -40,6 +40,7 @@ import java.util.concurrent.atomic.AtomicLong; import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.equalTo; public class LiveVersionMapTests extends ESTestCase { @@ -396,6 +397,50 @@ public void testPruneTombstonesWhileLocked() throws InterruptedException, IOExce assertEquals(0, map.getAllTombstones().size()); } + public void testNeverLeaveStaleDeleteTombstone() throws Exception { + LiveVersionMap versionMap = new LiveVersionMap(); + CountDownLatch latch = new CountDownLatch(1); + AtomicBoolean running = new AtomicBoolean(true); + Thread refreshThread = new Thread(() -> { + latch.countDown(); + while (running.get()) { + try { + if (randomBoolean()) { + versionMap.beforeRefresh(); + versionMap.afterRefresh(randomBoolean()); + } + if (rarely()) { + versionMap.enforceSafeAccess(); + } + } catch (Exception ex) { + throw new RuntimeException(ex); + } + } + }); + refreshThread.start(); + BytesRef uid = uid("1"); + latch.await(); + long versions = between(10, 1000); + for (long version = 1; version <= versions; version++) { + try (Releasable ignore = versionMap.acquireLock(uid)) { + if (randomBoolean()) { + versionMap.putDeleteUnderLock(uid, new DeleteVersionValue(version, 1, 1, 1)); + } else { + versionMap.maybePutIndexUnderLock(uid, new IndexVersionValue(randomTranslogLocation(), version, 1, 1)); + } + } + } + VersionValue storedValue = versionMap.getAllCurrent().get(uid); + if (storedValue == null) { + storedValue = versionMap.getAllTombstones().get(uid); + } + running.set(false); + refreshThread.join(); + if (storedValue != null) { + assertThat("Keeping a stale version value", storedValue.version, equalTo(versions)); + } + } + IndexVersionValue randomIndexVersionValue() { return new IndexVersionValue(randomTranslogLocation(), randomNonNegativeLong(), randomNonNegativeLong(), randomNonNegativeLong()); } From e69b6dfdb1dc4eb3a95f5f055029fb4bd67a7a63 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Thu, 19 Apr 2018 13:59:18 -0400 Subject: [PATCH 2/4] =?UTF-8?q?Address=20Boaz=E2=80=99s=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../index/engine/LiveVersionMap.java | 6 +-- .../index/engine/LiveVersionMapTests.java | 40 +++++-------------- 2 files changed, 14 insertions(+), 32 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/engine/LiveVersionMap.java b/server/src/main/java/org/elasticsearch/index/engine/LiveVersionMap.java index d808a69c30a19..18d3cedb37e60 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/LiveVersionMap.java +++ b/server/src/main/java/org/elasticsearch/index/engine/LiveVersionMap.java @@ -312,9 +312,9 @@ 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. + // 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 (avoid accidental accesses). + // Not this should not hurt performance because the tombstone is small (or empty) when unsafe is relevant. removeTombstoneUnderLock(uid); maps.current.markAsUnsafe(); assert putAssertionMap(uid, version); diff --git a/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java b/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java index b0db498eb51bc..bbe3446bd63ee 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java @@ -399,46 +399,28 @@ public void testPruneTombstonesWhileLocked() throws InterruptedException, IOExce public void testNeverLeaveStaleDeleteTombstone() throws Exception { LiveVersionMap versionMap = new LiveVersionMap(); - CountDownLatch latch = new CountDownLatch(1); - AtomicBoolean running = new AtomicBoolean(true); - Thread refreshThread = new Thread(() -> { - latch.countDown(); - while (running.get()) { - try { - if (randomBoolean()) { - versionMap.beforeRefresh(); - versionMap.afterRefresh(randomBoolean()); - } - if (rarely()) { - versionMap.enforceSafeAccess(); - } - } catch (Exception ex) { - throw new RuntimeException(ex); - } - } - }); - refreshThread.start(); BytesRef uid = uid("1"); - latch.await(); long versions = between(10, 1000); for (long version = 1; version <= versions; version++) { + if (randomBoolean()) { + versionMap.beforeRefresh(); + versionMap.afterRefresh(randomBoolean()); + } + if (randomBoolean()) { + versionMap.enforceSafeAccess(); + } try (Releasable ignore = versionMap.acquireLock(uid)) { if (randomBoolean()) { versionMap.putDeleteUnderLock(uid, new DeleteVersionValue(version, 1, 1, 1)); } else { versionMap.maybePutIndexUnderLock(uid, new IndexVersionValue(randomTranslogLocation(), version, 1, 1)); } + VersionValue storedValue = versionMap.getUnderLock(uid); + if (storedValue != null) { + assertThat("Keeping a stale version value", storedValue.version, equalTo(version)); + } } } - VersionValue storedValue = versionMap.getAllCurrent().get(uid); - if (storedValue == null) { - storedValue = versionMap.getAllTombstones().get(uid); - } - running.set(false); - refreshThread.join(); - if (storedValue != null) { - assertThat("Keeping a stale version value", storedValue.version, equalTo(versions)); - } } IndexVersionValue randomIndexVersionValue() { From d869bd0dff55969905444d09c2d2a9bc48f6d7af Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Thu, 19 Apr 2018 16:52:15 -0400 Subject: [PATCH 3/4] stronger test --- .../index/engine/LiveVersionMapTests.java | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java b/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java index bbe3446bd63ee..3797c37267077 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java @@ -41,6 +41,7 @@ import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; public class LiveVersionMapTests extends ESTestCase { @@ -397,11 +398,12 @@ public void testPruneTombstonesWhileLocked() throws InterruptedException, IOExce assertEquals(0, map.getAllTombstones().size()); } - public void testNeverLeaveStaleDeleteTombstone() throws Exception { - LiveVersionMap versionMap = new LiveVersionMap(); - BytesRef uid = uid("1"); - long versions = between(10, 1000); - for (long version = 1; version <= versions; version++) { + public void testRandomlyIndexDeleteAndRefresh() throws Exception { + final LiveVersionMap versionMap = new LiveVersionMap(); + final BytesRef uid = uid("1"); + final long versions = between(10, 1000); + VersionValue latestVersion = null; + for (long i = 0; i <= versions; i++) { if (randomBoolean()) { versionMap.beforeRefresh(); versionMap.afterRefresh(randomBoolean()); @@ -411,13 +413,20 @@ public void testNeverLeaveStaleDeleteTombstone() throws Exception { } try (Releasable ignore = versionMap.acquireLock(uid)) { if (randomBoolean()) { - versionMap.putDeleteUnderLock(uid, new DeleteVersionValue(version, 1, 1, 1)); - } else { - versionMap.maybePutIndexUnderLock(uid, new IndexVersionValue(randomTranslogLocation(), version, 1, 1)); + latestVersion = new DeleteVersionValue(randomNonNegativeLong(), randomLong(), randomLong(), randomLong()); + versionMap.putDeleteUnderLock(uid, (DeleteVersionValue) latestVersion); + assertThat(versionMap.getUnderLock(uid), equalTo(latestVersion)); + } else if (randomBoolean()) { + latestVersion = new IndexVersionValue(randomTranslogLocation(), randomNonNegativeLong(), randomLong(), randomLong()); + versionMap.maybePutIndexUnderLock(uid, (IndexVersionValue) latestVersion); + if (versionMap.isSafeAccessRequired()) { + assertThat(versionMap.getUnderLock(uid), equalTo(latestVersion)); + } else { + assertThat(versionMap.getUnderLock(uid), nullValue()); + } } - VersionValue storedValue = versionMap.getUnderLock(uid); - if (storedValue != null) { - assertThat("Keeping a stale version value", storedValue.version, equalTo(version)); + if (versionMap.getUnderLock(uid) != null) { + assertThat(versionMap.getUnderLock(uid), equalTo(latestVersion)); } } } From 64f415c39c070d078ffea9552c062241db892f85 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Thu, 19 Apr 2018 16:56:48 -0400 Subject: [PATCH 4/4] correct iteration loop --- .../org/elasticsearch/index/engine/LiveVersionMapTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java b/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java index 3797c37267077..286e85cef3fc6 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/LiveVersionMapTests.java @@ -403,7 +403,7 @@ public void testRandomlyIndexDeleteAndRefresh() throws Exception { final BytesRef uid = uid("1"); final long versions = between(10, 1000); VersionValue latestVersion = null; - for (long i = 0; i <= versions; i++) { + for (long i = 0; i < versions; i++) { if (randomBoolean()) { versionMap.beforeRefresh(); versionMap.afterRefresh(randomBoolean());