Skip to content

Commit

Permalink
Merge pull request #886 from rax-maas/es-it-enabled
Browse files Browse the repository at this point in the history
enable the Elasticsearch integration test
  • Loading branch information
iWebi committed Jul 5, 2022
2 parents 47fd441 + 9d1826a commit eb8939c
Show file tree
Hide file tree
Showing 5 changed files with 201 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@ public ElasticIO() {
this.elasticsearchRestHelper = ElasticsearchRestHelper.getInstance();
}

/**
* Creates an instance of this class, specifying its dependencies because Dependency Injection! I apologize for
* deviating from the conventions of this project, but I need a way to inject a test instance of the rest helper
* here for the integration test, and I can't use PowerMockito because the test is already using another @Runner.
*/
public ElasticIO(ElasticsearchRestHelper elasticsearchRestHelper) {
this.elasticsearchRestHelper = elasticsearchRestHelper;
}

public void insertDiscovery(IMetric metric) throws IOException {
List<IMetric> batch = new ArrayList<>();
batch.add(metric);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,24 @@ public class ElasticsearchRestHelper {
private int MAX_CALL_COUNT = 10;
private int MAX_RESULT_LIMIT = Configuration.getInstance().getIntegerProperty(CoreConfig.MAX_DISCOVERY_RESULT_SIZE);

/**
* Gets an instance of the ES rest helper for use in main source.
* @return
*/
public static ElasticsearchRestHelper getInstance() {
// TODO: To match the rest of the project, this should be a static singleton. There seems to be an HTTP
// connection pool in use here, which is pointless if an instance of this isn't shared by many consumers. On the
// other hand, making this a singleton could affect performance dramatically if the pool hasn't already been
// tuned to support the workload, so don't change it without some testing.
return new ElasticsearchRestHelper();
}

/**
* Gets a fresh instance instead of the static singleton. For use in tests so that a test can change the app
* configuration and then get a new instance based on that configuration.
*/
@VisibleForTesting
public static ElasticsearchRestHelper getConfigurableInstance() {
return new ElasticsearchRestHelper();
}

Expand Down
8 changes: 8 additions & 0 deletions blueflood-integration-tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,14 @@
<scope>test</scope>
<optional>true</optional>
</dependency>

<!-- Allows for starting Elasticsearch for tests on demand. See ElasticsearchTestServer. -->
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>elasticsearch</artifactId>
<version>1.17.2</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.rackspacecloud.blueflood.types.Locator;
import com.rackspacecloud.blueflood.types.Metric;
import com.rackspacecloud.blueflood.types.Token;
import com.rackspacecloud.blueflood.utils.ElasticsearchTestServer;
import com.rackspacecloud.blueflood.utils.TimeValue;
import junitparams.JUnitParamsRunner;
import junitparams.Parameters;
Expand All @@ -46,19 +47,28 @@
import static java.util.stream.Collectors.toList;
import static org.junit.Assert.assertEquals;

@Ignore("it requires a running Elasticsearch, which isn't started by the Maven build")
@RunWith( JUnitParamsRunner.class )
public class ElasticIOIntegrationTest extends BaseElasticTest {

protected ElasticIO elasticIO;
protected ElasticTokensIO elasticTokensIO;
private static ElasticsearchTestServer elasticsearchTestServer;

/**
* Starts an embedded Elasticsearch that these tests can run against.
*/
@BeforeClass
public static void startElasticsearch() {
elasticsearchTestServer = new ElasticsearchTestServer();
elasticsearchTestServer.start();
}

@Before
public void setup() throws Exception {
helper = ElasticsearchRestHelper.getInstance();
helper = ElasticsearchRestHelper.getConfigurableInstance();
tearDown();

elasticIO = new ElasticIO();
elasticIO = new ElasticIO(helper);

elasticIO.insertDiscovery(createTestMetrics(TENANT_A));
elasticIO.insertDiscovery(createTestMetrics(TENANT_B));
Expand Down Expand Up @@ -109,6 +119,11 @@ public void tearDown() throws Exception {
deleteAllDocuments(typeToEmpty);
}

@AfterClass
public static void stopElasticsearch() {
elasticsearchTestServer.stop();
}

private void deleteAllDocuments(String typeToEmpty) throws URISyntaxException, IOException {
URIBuilder builder = new URIBuilder().setScheme("http")
.setHost("127.0.0.1").setPort(9200)
Expand Down Expand Up @@ -326,7 +341,12 @@ public void testDeDupMetrics() throws Exception {
}

helper.refreshIndex(ES_DUP);
elasticIO.setINDEX_NAME_READ("metric_metadata_read");
// Historically, this line was here, but I don't know why. In init-es.sh, "metric_metadata_read" is an alias for
// "metric_metadata". Why? I have no clue. Maybe something related to Elasticsearch clustering and creating read
// slaves?. This is the only place that seems to use that name. The ElasticsearchTestServer doesn't create this
// alias, so it fails here. I'm not sure how to create the alias in that class.
//
// elasticIO.setINDEX_NAME_READ("metric_metadata_read");
results = elasticIO.search(TENANT_A, testLocator.getMetricName());
// Should just be one result
assertEquals(results.size(), 1);
Expand Down Expand Up @@ -434,6 +454,26 @@ public void testGetMetricNamesWithWildCardPrefixAtTheEnd(String type) throws Exc

List<MetricName> results = getDiscoveryIO(type).getMetricNames(tenantId, query);

// Note: 1 Jul 2022 - While adding this test class back into the integration test suite by adding embedded
// Elasticsearch back to it, I observed this and some other test cases here failing to retrieve the correct
// number of items using "elasticTokensIO". On inspection, it was returning *all* metrics matching the query,
// rather than only the next level down. In other words, where it was supposed to return only
// "one.two.three15.fourA", it actually returned:
//
// - "one.two.three15.fourA"
// - "one.two.three15.fourA.five0"
// - "one.two.three15.fourA.five1"
// - "one.two.three15.fourA.five2"
//
// Given that I only ever see it on the "elasticTokensIO" and not on the "elasticIO", it could be a bug in the
// former. They have separate implementations of the method under test.
//
// It's also possible that it's somehow related to some Elasticsearch behavior I don't know about, like indexing
// delays or something. This possibility may be strengthened by the fact that the actual "number of results"
// returned here isn't always the same. If this particular test case were to return all results, as I mentioned
// above, the total count should be 360. Sometimes it returns less.
//
// The same behavior occurs using the tlrx test library and when running Elasticsearch externally with Docker.
assertEquals("Invalid total number of results", NUM_PARENT_ELEMENTS * CHILD_ELEMENTS.size(), results.size());
for (MetricName metricName : results) {
Assert.assertFalse("isCompleteName value", metricName.isCompleteName());;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
package com.rackspacecloud.blueflood.utils;

import com.github.tlrx.elasticsearch.test.EsSetup;
import com.rackspacecloud.blueflood.io.ElasticIO;
import com.rackspacecloud.blueflood.io.ElasticTokensIO;
import com.rackspacecloud.blueflood.io.EventElasticSearchIO;
import com.rackspacecloud.blueflood.service.Configuration;
import com.rackspacecloud.blueflood.service.ElasticIOConfig;
import org.testcontainers.elasticsearch.ElasticsearchContainer;
import org.testcontainers.utility.DockerImageName;

/**
* Manages embedded Elasticsearch servers for tests. An instance of this class manages its own, separate Elasticsearch.
* In theory, multiple instances could be active simultaneously, but at the moment, the tlrx implementation always binds
* to port 9200, and that's where tests know to find it. To run concurrent instances, that would have to be fixed. The
* server should bind a random, available port, and each test should ask the instance of this class where to find it.
*/
public class ElasticsearchTestServer {

/**
* Supported ways of starting an Elasticsearch instance for testing. This exists because it's in flux. The
* tlrx library is old but original to Blueflood. We should use it until we can start upgrading Elasticsearch.
* Testcontainers seems to be the way to go in the future, but it officially supports Elasticsearch no older than
* 5.4.0. We may be able to force it to use an older one. For now, we need to get the project running stably and
* predictably.
*
* EXTERNAL means Elasticsearch will be started externally. Using EXTERNAL will turn this class into a no-op. It
* won't try to start or stop anything, and everything will be left up to you to manage yourself.
*/
private enum EsInitMethod {
// Probably the best way to test against Elasticsearch for the foreseeable future. Testcontainers is a library
// that provides many on-demand services for testing purposes via Docker. The oldest Elasticsearch that it
// officially supports is 5.4.0. We may be able to use an older image with it, too, but we'll need to verify.
//
// Note: This method will definitely not work with the testcontainers 5.4.0 image until ElasticsearchRestHelper
// is updated to support authentication!
TEST_CONTAINERS,

// The old way of testing against Elasticsearch used elsewhere in the project. It seems to start an in-memory
// instance. This library hasn't been maintained in years, so we need to stop using it for testing new versions
// of Elasticsearch.
TLRX,

// Indicates that you'll start Elasticsearch externally, like with Docker. See the 10-minute guide on the wiki
// for a quick startup. This test class will do nothing in terms of starting or initializing Elasticsearch when
// you use this option.
EXTERNAL
}

/**
* The selected way to start Elasticsearch for this test class. This will change over time as we update
* Elasticsearch and change to new testing mechanisms. It's useful to declare here so that you can easily test
* against different variants in a dev environment.
*/
private static final EsInitMethod esInitMethod = EsInitMethod.TLRX;

private ElasticsearchContainer elasticsearchContainer;
private EsSetup esSetup;

/**
* Starts an in-memory Elasticsearch that's configured for Blueflood. The configuration mimics that found in
* init-es.sh as closely as I can figure out how to.
*/
public void start() {
if (esInitMethod.equals(EsInitMethod.TEST_CONTAINERS)) {
startTestContainer();
} else if (esInitMethod.equals(EsInitMethod.TLRX)) {
startTlrx();
} else if (esInitMethod.equals(EsInitMethod.EXTERNAL)) {
// Do nothing! You have to manage Elasticsearch your own self!
System.out.println("Using external Elasticsearch");
} else {
throw new IllegalStateException("Illegal value set for Elasticsearch init in tests: " + esInitMethod);
}
}

public void startTestContainer() {
// Try to start an old version. Does this work?
DockerImageName myImage = DockerImageName.parse("elasticsearch:1.7")
.asCompatibleSubstituteFor("docker.elastic.co/elasticsearch/elasticsearch");
elasticsearchContainer = new ElasticsearchContainer(myImage);

// Or, with the officially supported version:
// elasticsearchContainer = new ElasticsearchContainer("docker.elastic.co/elasticsearch/elasticsearch:5.4.0");

elasticsearchContainer.start();

// TODO: Create the indexes and mappings as seen in init-es.sh

// The container starts on a random, unused port. Configure the rest client to use the correct port.
Configuration.getInstance().setProperty(
ElasticIOConfig.ELASTICSEARCH_HOST_FOR_REST_CLIENT, elasticsearchContainer.getHttpHostAddress());
}

public void startTlrx() {
esSetup = new EsSetup();
esSetup.execute(EsSetup.createIndex(ElasticIO.ELASTICSEARCH_INDEX_NAME_WRITE)
.withSettings(EsSetup.fromClassPath("index_settings.json"))
.withMapping("metrics", EsSetup.fromClassPath("metrics_mapping.json")));
esSetup.execute(EsSetup.createIndex(ElasticTokensIO.ELASTICSEARCH_TOKEN_INDEX_NAME_WRITE)
.withMapping("tokens", EsSetup.fromClassPath("tokens_mapping.json")));
esSetup.execute(EsSetup.createIndex(EventElasticSearchIO.EVENT_INDEX)
.withMapping("graphite_event", EsSetup.fromClassPath("events_mapping.json")));
esSetup.execute(EsSetup.createIndex("blueflood_initialized_marker"));
}

/**
* Stops the in-memory Elasticsearch managed by this object. It's expected, though unproven, that this would release
* all resources in use by that Elasticsearch.
*/
public void stop() {
if (esInitMethod.equals(EsInitMethod.TEST_CONTAINERS)) {
elasticsearchContainer.stop();
} else if (esInitMethod.equals(EsInitMethod.TLRX)) {
esSetup.terminate();
} else if (esInitMethod.equals(EsInitMethod.EXTERNAL)) {
// Do nothing! You have to manage Elasticsearch your own self!
System.out.println("Done with external Elasticsearch");
} else {
throw new IllegalStateException("Illegal value set for Elasticsearch init in tests: " + esInitMethod);
}
}
}

0 comments on commit eb8939c

Please sign in to comment.