Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deleted docs disregarded for if_seq_no check #50526

Merged
merged 3 commits into from
Jan 6, 2020
Merged

Deleted docs disregarded for if_seq_no check #50526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fadc627
Deleted docs disregarded for if_seq_no check
henningandersen Dec 30, 2019
df0f2cc
Fix tests
henningandersen Dec 30, 2019
e34e62c
Fix tests
henningandersen Jan 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,7 +1032,7 @@ private IndexingStrategy planIndexingAsPrimary(Index index) throws IOException {
currentVersion = versionValue.version;
currentNotFoundOrDeleted = versionValue.isDelete();
}
if (index.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && versionValue == null) {
if (index.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && currentNotFoundOrDeleted) {
dnhatn marked this conversation as resolved.
Show resolved Hide resolved
final VersionConflictEngineException e = new VersionConflictEngineException(shardId, index.id(),
index.getIfSeqNo(), index.getIfPrimaryTerm(), SequenceNumbers.UNASSIGNED_SEQ_NO,
SequenceNumbers.UNASSIGNED_PRIMARY_TERM);
Expand Down Expand Up @@ -1361,7 +1361,7 @@ private DeletionStrategy planDeletionAsPrimary(Delete delete) throws IOException
currentlyDeleted = versionValue.isDelete();
}
final DeletionStrategy plan;
if (delete.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && versionValue == null) {
if (delete.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && currentlyDeleted) {
final VersionConflictEngineException e = new VersionConflictEngineException(shardId, delete.id(),
delete.getIfSeqNo(), delete.getIfPrimaryTerm(), SequenceNumbers.UNASSIGNED_SEQ_NO, SequenceNumbers.UNASSIGNED_PRIMARY_TERM);
plan = DeletionStrategy.skipDueToVersionConflict(e, currentVersion, true);
dnhatn marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
38 changes: 35 additions & 3 deletions server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@
import org.elasticsearch.test.VersionUtils;
import org.elasticsearch.threadpool.ThreadPool;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;

import java.io.Closeable;
import java.io.IOException;
Expand Down Expand Up @@ -1719,7 +1720,7 @@ private int assertOpsOnPrimary(List<Engine.Operation> ops, long currentOpVersion
currentTerm.set(currentTerm.get() + 1L);
engine.rollTranslogGeneration();
}
final long correctVersion = docDeleted && randomBoolean() ? Versions.MATCH_DELETED : lastOpVersion;
final long correctVersion = docDeleted ? Versions.MATCH_DELETED : lastOpVersion;
logger.info("performing [{}]{}{}",
op.operationType().name().charAt(0),
versionConflict ? " (conflict " + conflictingVersion + ")" : "",
Expand All @@ -1742,7 +1743,7 @@ private int assertOpsOnPrimary(List<Engine.Operation> ops, long currentOpVersion
final Engine.IndexResult result;
if (versionedOp) {
// TODO: add support for non-existing docs
if (randomBoolean() && lastOpSeqNo != SequenceNumbers.UNASSIGNED_SEQ_NO) {
if (randomBoolean() && lastOpSeqNo != SequenceNumbers.UNASSIGNED_SEQ_NO && docDeleted == false) {
result = engine.index(indexWithSeq.apply(lastOpSeqNo, lastOpTerm, index));
} else {
result = engine.index(indexWithVersion.apply(correctVersion, index));
Expand Down Expand Up @@ -1777,8 +1778,9 @@ private int assertOpsOnPrimary(List<Engine.Operation> ops, long currentOpVersion
assertThat(result.getFailure(), instanceOf(VersionConflictEngineException.class));
} else {
final Engine.DeleteResult result;
long correctSeqNo = docDeleted ? UNASSIGNED_SEQ_NO : lastOpSeqNo;
if (versionedOp && lastOpSeqNo != UNASSIGNED_SEQ_NO && randomBoolean()) {
result = engine.delete(delWithSeq.apply(lastOpSeqNo, lastOpTerm, delete));
result = engine.delete(delWithSeq.apply(correctSeqNo, lastOpTerm, delete));
} else if (versionedOp) {
result = engine.delete(delWithVersion.apply(correctVersion, delete));
} else {
Expand Down Expand Up @@ -4031,6 +4033,36 @@ public void testOutOfOrderSequenceNumbersWithVersionConflict() throws IOExceptio
}
}

/**
* Test that we do not leak out information on a deleted doc due to it existing in version map. There are at least 2 cases:
* <ul>
* <li>Guessing the deleted seqNo makes the operation succeed</li>
* <li>Providing any other seqNo leaks info that the doc was deleted (and its SeqNo)</li>
* </ul>
*/
public void testVersionConflictIgnoreDeletedDoc() throws IOException {
ParsedDocument doc = testParsedDocument("1", null, testDocument(),
new BytesArray("{}".getBytes(Charset.defaultCharset())), null);
engine.delete(new Engine.Delete("1", newUid("1"), 1));
for (long seqNo : new long[]{0, 1, randomNonNegativeLong()}) {
assertDeletedVersionConflict(engine.index(new Engine.Index(newUid("1"), doc, UNASSIGNED_SEQ_NO, 1,
Versions.MATCH_ANY, VersionType.INTERNAL,
PRIMARY, randomNonNegativeLong(), IndexRequest.UNSET_AUTO_GENERATED_TIMESTAMP, false, seqNo, 1)),
"update: " + seqNo);

assertDeletedVersionConflict(engine.delete(new Engine.Delete("1", newUid("1"), UNASSIGNED_SEQ_NO, 1,
Versions.MATCH_ANY, VersionType.INTERNAL, PRIMARY, randomNonNegativeLong(), seqNo, 1)),
"delete: " + seqNo);
}
}

private void assertDeletedVersionConflict(Engine.Result result, String operation) {
assertNotNull("Must have failure for " + operation, result.getFailure());
assertThat(operation, result.getFailure(), Matchers.instanceOf(VersionConflictEngineException.class));
VersionConflictEngineException exception = (VersionConflictEngineException) result.getFailure();
assertThat(operation, exception.getMessage(), containsString("but no document was found"));
}

/*
* This test tests that a no-op does not generate a new sequence number, that no-ops can advance the local checkpoint, and that no-ops
* are correctly added to the translog.
Expand Down
22 changes: 10 additions & 12 deletions server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@

import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse;
import org.elasticsearch.action.admin.indices.settings.get.GetSettingsResponse;
import org.elasticsearch.action.delete.DeleteResponse;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.Priority;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.IndexModule;
import org.elasticsearch.index.IndexService;
import org.elasticsearch.index.VersionType;
import org.elasticsearch.index.engine.VersionConflictEngineException;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.plugins.Plugin;
Expand Down Expand Up @@ -449,26 +449,24 @@ public void testOpenCloseUpdateSettings() throws Exception {

public void testEngineGCDeletesSetting() throws Exception {
createIndex("test");
client().prepareIndex("test").setId("1").setSource("f", 1).get();
DeleteResponse response = client().prepareDelete("test", "1").get();
long seqNo = response.getSeqNo();
long primaryTerm = response.getPrimaryTerm();
// delete is still in cache this should work
client().prepareIndex("test").setId("1").setSource("f", 2).setIfSeqNo(seqNo).setIfPrimaryTerm(primaryTerm).get();
client().prepareIndex("test").setId("1").setSource("f", 1).setVersionType(VersionType.EXTERNAL).setVersion(1).get();
client().prepareDelete("test", "1").setVersionType(VersionType.EXTERNAL).setVersion(2).get();
// delete is still in cache this should fail
assertThrows(client().prepareIndex("test").setId("1").setSource("f", 3).setVersionType(VersionType.EXTERNAL).setVersion(1),
VersionConflictEngineException.class);

assertAcked(client().admin().indices().prepareUpdateSettings("test").setSettings(Settings.builder().put("index.gc_deletes", 0)));

response = client().prepareDelete("test", "1").get();
seqNo = response.getSeqNo();
client().prepareDelete("test", "1").setVersionType(VersionType.EXTERNAL).setVersion(4).get();

// Make sure the time has advanced for InternalEngine#resolveDocVersion()
for (ThreadPool threadPool : internalCluster().getInstances(ThreadPool.class)) {
long startTime = threadPool.relativeTimeInMillis();
assertBusy(() -> assertThat(threadPool.relativeTimeInMillis(), greaterThan(startTime)));
}

// delete is should not be in cache
assertThrows(client().prepareIndex("test").setId("1").setSource("f", 3).setIfSeqNo(seqNo).setIfPrimaryTerm(primaryTerm),
VersionConflictEngineException.class);
// delete should not be in cache
client().prepareIndex("test").setId("1").setSource("f", 2).setVersionType(VersionType.EXTERNAL).setVersion(1);
}

public void testUpdateSettingsWithBlocks() {
Expand Down
9 changes: 2 additions & 7 deletions server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,13 +283,8 @@ public void testCompareAndSet() {
assertThrows(client().prepareDelete("test", "1").setIfSeqNo(3).setIfPrimaryTerm(12), VersionConflictEngineException.class);
assertThrows(client().prepareDelete("test", "1").setIfSeqNo(1).setIfPrimaryTerm(2), VersionConflictEngineException.class);


// This is intricate - the object was deleted but a delete transaction was with the right version. We add another one
// and thus the transaction is increased.
deleteResponse = client().prepareDelete("test", "1").setIfSeqNo(2).setIfPrimaryTerm(1).get();
assertEquals(DocWriteResponse.Result.NOT_FOUND, deleteResponse.getResult());
assertThat(deleteResponse.getSeqNo(), equalTo(3L));
assertThat(deleteResponse.getPrimaryTerm(), equalTo(1L));
// the doc is deleted. Even when we hit the deleted seqNo, a conditional delete should fail.
assertThrows(client().prepareDelete("test", "1").setIfSeqNo(2).setIfPrimaryTerm(1), VersionConflictEngineException.class);
}

public void testSimpleVersioningWithFlush() throws Exception {
Expand Down