Skip to content

Commit

Permalink
fix(elasticsearch): Fix race conditions in elasticsearch tests (#4751)
Browse files Browse the repository at this point in the history
The elasticsearch tests are failing intermittently because there
are numerous race conditions between when index refreshes cause
a mutating operation to be visible, and when we assert that the
operation is visible.

Currently the test cluster is set up to reindex every 1s, and we
are using sleep statements and retries to ensure we wait that long
before failing a test. In addition to making these tests slow, there
are intermittent failures (particularly under high load) when we
fail before the re-index has occurred.

Let's make this deterministic as follows:
(1) Manually refresh the index before verifying the status of an
operation.
(2) Set the refresh_interval to -1, which means never automatically
refresh. This ensures that tests will always fail if we forget to
invoke the manual refresh to avoid future intermittent tests.

As an added benefit, this speeds up the tests significantly, as
we're no longer unnecessarily waiting. In my very rough testing,
the time to execute tests in this class is reduced from ~35s to
~7s.
  • Loading branch information
ezimanyi authored Jul 20, 2020
1 parent 6420927 commit 966cf48
Showing 1 changed file with 21 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ import com.netflix.spinnaker.config.ElasticSearchConfig
import com.netflix.spinnaker.config.ElasticSearchConfigProperties
import com.netflix.spinnaker.kork.core.RetrySupport
import io.searchbox.client.JestClient
import io.searchbox.client.JestResult
import io.searchbox.indices.CreateIndex
import io.searchbox.indices.DeleteIndex
import io.searchbox.indices.Refresh
import io.searchbox.indices.template.PutTemplate
import org.springframework.context.ApplicationContext
import org.testcontainers.elasticsearch.ElasticsearchContainer
Expand Down Expand Up @@ -85,7 +87,7 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {
"index": {
"number_of_shards": "1",
"number_of_replicas": "1",
"refresh_interval": "1s"
"refresh_interval": "-1"
}
},
"mappings": {
Expand Down Expand Up @@ -185,6 +187,7 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {
given:
def entityTags = buildEntityTags("aws:cluster:front50-main:myaccount:*", ["tag1": "value1", "tag2": "value2"])
entityTagsProvider.index(entityTags)
refreshIndices()
entityTagsProvider.verifyIndex(entityTags)

expect:
Expand All @@ -202,6 +205,7 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {
entityTags.entityRef.application = application

entityTagsProvider.index(entityTags)
refreshIndices()
entityTagsProvider.verifyIndex(entityTags)

expect:
Expand All @@ -219,10 +223,12 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {
given:
def entityTags = buildEntityTags("aws:cluster:clouddriver-main:myaccount:*", ["tag3": "value3"])
entityTagsProvider.index(entityTags)
refreshIndices()
entityTagsProvider.verifyIndex(entityTags)

def moreEntityTags = buildEntityTags("aws:cluster:front50-main:myaccount:*", ["tag1": "value1"])
entityTagsProvider.index(moreEntityTags)
refreshIndices()
entityTagsProvider.verifyIndex(moreEntityTags)

expect:
Expand Down Expand Up @@ -286,6 +292,7 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {

when:
entityTagsProvider.reindex()
refreshIndices()

then:
1 * front50Service.getAllEntityTags(true) >> { return allEntityTags }
Expand All @@ -304,11 +311,13 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {
]
allEntityTags.each {
entityTagsProvider.index(it)
refreshIndices()
entityTagsProvider.verifyIndex(it)
}
when:
entityTagsProvider.bulkDelete(allEntityTags)
refreshIndices()
then:
verifyNotIndexed(allEntityTags[0])
Expand All @@ -326,6 +335,7 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {
]
allEntityTags.each {
entityTagsProvider.index(it)
refreshIndices()
entityTagsProvider.verifyIndex(it)
}
Expand Down Expand Up @@ -353,7 +363,7 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {
when:
entityTagsProvider.deleteByNamespace("my_namespace", false, false) // remove from elasticsearch (only!)
Thread.sleep(1000)
refreshIndices()
def allIndexedEntityTags = entityTagsProvider.getAll(
null, null, null, null, null, null, null, null, [:], 100
Expand Down Expand Up @@ -383,14 +393,7 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {
}
boolean verifyNotIndexed(EntityTags entityTags) {
return (1..5).any {
if (!entityTagsProvider.get(entityTags.id).isPresent()) {
return true
}
Thread.sleep(500)
return false
}
return !entityTagsProvider.get(entityTags.id).isPresent()
}
private static EntityTags buildEntityTags(String id, Map<String, String> tags, String namespace = "default") {
Expand All @@ -407,4 +410,12 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {
)
)
}
private void refreshIndices() {
JestResult result = jestClient.execute(new Refresh.Builder().build())
if (!result.isSucceeded()) {
throw new ElasticSearchException(
String.format("Failed to refresh index: %s", result.getErrorMessage()))
}
}
}

0 comments on commit 966cf48

Please sign in to comment.