Rename tests, parallel tests, one elasticsearch replica, readiness probe #50

pavolloffay · 2017-10-11T12:18:11Z

related to #44

rename tests
use travis stages for parallel builds
use one ES replica no two. It seems that data between ES nodes were not synchronized properly, therefore reverting the change.
update nsenster

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2017-10-11T13:47:44Z

ES tests are passing when cassandra tests are disabled.

I have introduced travis stages to run tests in parallel. Now it seems that it works fine.

pavolloffay · 2017-10-11T13:49:54Z

I am restarting the build to be sure :)

jpkrohling

Change to the README requires a clarification. Other than that, this can be merged once Travis approves this PR.

jpkrohling · 2017-10-11T14:11:20Z

README.md

@@ -109,7 +109,7 @@ kubernetes cluster (via `kubectl`). When executing tests from IDE make sure that

 ```bash
 minikube start
-./mvnw clean verify -Pe2e
+./mvnw clean verify -Pcassandra,elasticsearch,all-in-one


ES tests are passing when cassandra tests are disabled.

Should one expect this command to finish successfully, or will this fail as per your comment on the PR ?

locally it passes fine, the problem is with travis.

This is a one liner to run everything

Signed-off-by: Pavol Loffay <[email protected]>

jpkrohling · 2017-10-12T10:27:40Z

production-elasticsearch/src/test/java/io/jaegertracing/kubernetes/ElasticearchETest.java

@@ -35,7 +36,9 @@
   * 2. jaeger-query returns 500 is ES storage is empty (without indices) https://github.com/jaegertracing/jaeger/issues/464
   */
  @Before
-  public void before() {
+  public void before() throws InterruptedException {
+    TimeUnit.SECONDS.sleep(8);


This PR is not ready for review, right?

not ready, it is still failing

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2017-10-12T15:35:46Z

I think the main here is actually two replicas. What happens is that one server has data and the other no

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2017-10-12T15:53:04Z

@jpkrohling this is ready to review. See the PR description for changes.

jpkrohling · 2017-10-13T09:29:16Z

production-elasticsearch/src/test/java/io/jaegertracing/kubernetes/ElasticearchETest.java


  /**
   * We need to initialize ES storage, before we proceed to tests for two reasons:
   * 1. sometimes first span is not stored
   * 2. jaeger-query returns 500 is ES storage is empty (without indices) https://github.com/jaegertracing/jaeger/issues/464
   */
  @Before
-  public void before() {
+  public void before() throws InterruptedException {
+    TimeUnit.SECONDS.sleep(15);


Nope :) It's better to ping for something from time to time than just sleeping for a long time and hoping it will be enough. Besides, waiting 15 seconds won't scale: if you have 10 tests, that's already two and a half minutes of just waiting...

As the only test on this class is being ignored, can't you just remove this @Before entirely? Or even the whole test?

Wow I forgot to remove this sleep statement.

As the only test on this class is being ignored, can't you just remove this @before entirely? Or even the whole test?

The class extends different test class. Before is still needed.

jpkrohling · 2017-10-13T09:30:30Z

production/src/test/java/io/jaegertracing/kubernetes/CassandraETest.java


-  @Ignore("dependency links returns 404 because of old Cassandra image")
+  @Ignore("It requires spark job")


Same as above: if the only test is being ignored, can't you just remove the whole test class?

No, test class extends base test class with other tests.

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2017-10-17T07:23:36Z

Readiness probe for ES has been added. Tests still sometimes fail on travis. Maybe it's just unstable travis environment or something with arquillian-cube. However now with travis stages can clearly see which template failed and restart only that.

jpkrohling · 2017-10-17T08:01:57Z

Tests still sometimes fail on travis.

I'd rather than just remove this and have a proper fix (proper environment, more resilient test... ) . Unstable tests are a "broken window"[1] and a distraction. We'll eventually just ignore the failure and not know when something wrong really happens.

1 - Broken windows theory

pavolloffay · 2017-10-17T08:06:26Z

I'd rather than just remove this and have a proper fix (proper environment, more resilient test... )

I am not sure what you suggest can be achieved in reasonable time. Can you setup a proper k8s cluster for tests? Because I can't. Let's keep the issue about tests open and move this forward? This is improvement against current master.

pavolloffay · 2017-10-17T08:07:16Z

I would rather restart tests if necessary than have no tests or disable them.

jpkrohling · 2017-10-17T08:45:33Z

I don't have time, but introducing a test with intermittent failures is really a bad idea. Merge if you will, I won't block this merge, but I still think it's a terrible idea :)

pavolloffay · 2017-10-17T10:20:40Z

The problem is that travis environment is not stable. It's not failing only here but also in other Jaeger repositories (e.g. Jaeger main repo xdock tests, all-in-one).

I am fine with restarting the build rather than not having tests at all. I will merge and keep the issue for tests open.

Rename tests, minor refactor

ed8c948

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay force-pushed the fix-tests branch from 07bfd6d to ed8c948 Compare October 11, 2017 12:19

pavolloffay added 3 commits October 11, 2017 14:21

Remove unused import

a37154b

Signed-off-by: Pavol Loffay <[email protected]>

Add docker service to travis

a7b0d6d

Signed-off-by: Pavol Loffay <[email protected]>

Bumb nsenter

7888085

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay force-pushed the fix-tests branch from 0a56d5d to 7888085 Compare October 11, 2017 13:09

pavolloffay added 2 commits October 11, 2017 15:22

Ignore cassandra test

5e10cae

Signed-off-by: Pavol Loffay <[email protected]>

Run tests in parallel

44bc9b3

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay requested a review from jpkrohling October 11, 2017 13:47

jpkrohling reviewed Oct 11, 2017

View reviewed changes

pavolloffay added 2 commits October 12, 2017 09:32

wait fixed time before ES tests

8f4986d

Signed-off-by: Pavol Loffay <[email protected]>

Increase wait time

517f0e8

Signed-off-by: Pavol Loffay <[email protected]>

jpkrohling reviewed Oct 12, 2017

View reviewed changes

pavolloffay added 3 commits October 12, 2017 15:14

increase memory and CPU for minikube

1ad7292

Signed-off-by: Pavol Loffay <[email protected]>

restrict ES heap

7773667

Signed-off-by: Pavol Loffay <[email protected]>

increase minimum mem and use newer ES image

46cf417

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay added 2 commits October 12, 2017 17:37

Use one elastic replica

ade5365

Signed-off-by: Pavol Loffay <[email protected]>

revert minikube changes

0863a74

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay mentioned this pull request Oct 12, 2017

Fixed: set --es.num-shards to 1. By default jaeger st this value to 5… #42

Closed

pavolloffay changed the title ~~Rename tests and minor refactor~~ Rename tests, parallel tests, one elasticsearch replica Oct 12, 2017

jpkrohling reviewed Oct 13, 2017

View reviewed changes

pavolloffay added 2 commits October 16, 2017 10:18

Remove sleep

5327cc7

Signed-off-by: Pavol Loffay <[email protected]>

Readiness probe for ES

3af67a6

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay merged commit c586dba into jaegertracing:master Oct 17, 2017

pavolloffay changed the title ~~Rename tests, parallel tests, one elasticsearch replica~~ Rename tests, parallel tests, one elasticsearch replica, readiness probe Oct 17, 2017

pavolloffay mentioned this pull request Oct 17, 2017

Investigate and fix Elasticsearch test failures #44

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename tests, parallel tests, one elasticsearch replica, readiness probe #50

Rename tests, parallel tests, one elasticsearch replica, readiness probe #50

pavolloffay commented Oct 11, 2017 •

edited

Loading

pavolloffay commented Oct 11, 2017

pavolloffay commented Oct 11, 2017

jpkrohling left a comment

jpkrohling Oct 11, 2017

pavolloffay Oct 11, 2017

jpkrohling Oct 12, 2017

pavolloffay Oct 12, 2017

pavolloffay commented Oct 12, 2017

pavolloffay commented Oct 12, 2017

jpkrohling Oct 13, 2017

pavolloffay Oct 16, 2017

jpkrohling Oct 13, 2017

pavolloffay Oct 16, 2017

pavolloffay commented Oct 17, 2017

jpkrohling commented Oct 17, 2017

pavolloffay commented Oct 17, 2017

pavolloffay commented Oct 17, 2017

jpkrohling commented Oct 17, 2017

pavolloffay commented Oct 17, 2017


		@Ignore("dependency links returns 404 because of old Cassandra image")
		@Ignore("It requires spark job")

Rename tests, parallel tests, one elasticsearch replica, readiness probe #50

Rename tests, parallel tests, one elasticsearch replica, readiness probe #50

Conversation

pavolloffay commented Oct 11, 2017 • edited Loading

pavolloffay commented Oct 11, 2017

pavolloffay commented Oct 11, 2017

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Oct 12, 2017

pavolloffay commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Oct 17, 2017

jpkrohling commented Oct 17, 2017

pavolloffay commented Oct 17, 2017

pavolloffay commented Oct 17, 2017

jpkrohling commented Oct 17, 2017

pavolloffay commented Oct 17, 2017

pavolloffay commented Oct 11, 2017 •

edited

Loading