TEST: Retry synced-flush if ongoing ops on primary

[dnhatn]

When the last indexing operation is completed, we will fire a global
checkpoint sync. Since a global checkpoint sync request is a
replication request, it will acquire an index shard permit on the
primary when executing. If this happens at the same time while we are
issuing the synced-flush, the synced-flush request will fail as it
thinks there are in-flight operations. We can avoid such situation by
retrying another synced-flush if the current request fails due to
ongoing operations on the primary.

Closes #29392

[elasticmachine]

Pinging @elastic/es-distributed

[dnhatn]

I traced the list of index shard permit acquirers when issuing the synced-flush.

1> [2018-05-30T04:00:48,248][TRACE][o.e.i.f.SyncedFlushService] [node_s1] in flight operations 1, acquirers [GlobalCheckpointSyncAction.Request{shardId=[test][0], timeout=1m, index='test', waitForActiveShards=1}
1> 	at org.elasticsearch.index.shard.IndexShardOperationPermits.acquire(IndexShardOperationPermits.java:229)
1> 	at org.elasticsearch.index.shard.IndexShard.acquirePrimaryOperationPermit(IndexShard.java:2178)
1> 	at org.elasticsearch.action.support.replication.TransportReplicationAction.acquirePrimaryShardReference(TransportReplicationAction.java:968)
1> 	at org.elasticsearch.action.support.replication.TransportReplicationAction.access$500(TransportReplicationAction.java:98)
1> 	at org.elasticsearch.action.support.replication.TransportReplicationAction$AsyncPrimaryAction.doRun(TransportReplicationAction.java:318)
1> 	at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37)
1> 	at org.elasticsearch.action.support.replication.TransportReplicationAction$PrimaryOperationTransportHandler.messageReceived(TransportReplicationAction.java:293)
1> 	at org.elasticsearch.action.support.replication.TransportReplicationAction$PrimaryOperationTransportHandler.messageReceived(TransportReplicationAction.java:280)
1> 	at org.elasticsearch.transport.RequestHandlerRegistry.processMessageReceived(RequestHandlerRegistry.java:66)
1> 	at org.elasticsearch.transport.TransportService$7.doRun(TransportService.java:656)
1> 	at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:724)
1> 	at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37)
1> 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
1> 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
1> 	at java.lang.Thread.run(Thread.java:748)]

1> [2018-05-30T04:00:48,248][INFO ][o.e.i.f.FlushIT          ] Third seal: Total shards: [2], failed: [true], reason: [[1] ongoing operations on primary], detail: []

[dnhatn]

run sample packaging tests

[jasontedor]

LGTM.

[bleskes]

I wonder if we still have a race condition - we periodically acquire a permit to do a background global checkpoint sync . Since synced flush is a best effort anyway (other stuff may fail in the future), maybe a better direction is to repeat it until successful?

[dnhatn]

@bleskes

maybe a better direction is to repeat it until successful?

Unfortunately, this is not the case because we are testing a different kind of results: unchanged, partially successful, and entirely successful in these tests.

we periodically acquire a permit to do a background global checkpoint sync

This is fired for every 30 seconds which I think is long enough for the synced-flush tests. How about disable the background global checkpoint sync in the synced-flush tests?

[dnhatn]

Talked to Boaz, we decided to take a slightly different approach for this. We will retry another synced-flush if the current request fails due to ongoing operations on the primary.

@bleskes and @jasontedor Can you please take another look?

[dnhatn]

I should have provided the reasons for this change. The current approach might not cover these cases:

• Background global checkpoint sync which should be unlikely but is not eliminated entirely in the current solution.
• Global checkpoint sync request is async, thus it's possible that we check an old global checkpoint value instead of the latest one. Here we can not use the max_seqno as we have an artificial out-of-sync replicas test.

[bleskes]

nit: just fold this into an if else?

[bleskes]

LGTM

[bleskes]

I personally favor something like:

if (listener.result != null && listener. result.failureReason != null && 
     listener. result.failureReason.contains("ongoing operations on primary") {
    throw new AsserionError(listener.reasult.failureReason); // cause the assert busy to retry
}

instead of these two ifs.

[dnhatn]

+1

[dnhatn]

Thanks @jasontedor and @bleskes for reviewing!