[WIP] Migrate first test_docker_dev_image #1026

bartier · 2020-07-15T23:21:36Z

This PR attempts to start test_docker_dev_image migration.

At the moment, this only contains information about the first test:

Lines 334 to 337 in b59bc09

    
           export TEST_COMMAND="--pipeline=benchmark-only --test-mode --track=geonames --challenge=append-no-conflicts-index-only --target-hosts=es01:9200" 
        
           info "Testing Rally docker image using parameters: ${TEST_COMMAND}" 
        
           docker_compose up 
        
           docker_compose down

I was not able to run the integration tests with all them passing, I guess this may be related with my setup and some tests I guess needs a external ES to send metrics (I'm not sure about that).

Anyway, if the project maintainers like it (and the CI of this PR occurs successfully or it is possible to obtain the root cause) I can expand the migration to the other tests.

danielmitterdorfer

Thanks for your contribution! Your attempt to keep the style of the new test as close as possible to the old test makes sense for the initial port but I left a couple of suggestions and ideas how we can take advantage of features that were previously unavailable (because it was bash code and not Python).

danielmitterdorfer · 2020-07-16T11:38:40Z

it/__init__.py

@@ -205,11 +206,36 @@ def remove_integration_test_config(config_names=None):

 ES_METRICS_STORE = EsMetricsStore()

+def get_license():
+    lines = process.run_subprocess_with_output("awk 'FNR>=2 && FNR<=2' LICENSE", path=ROOT_DIR)


Can we solve this by reading the first line with Python facilities instead of relying on external tools? i.e. something along the lines of:

with open(os.path.join(ROOT_DIR, "LICENSE") as license: ....

danielmitterdorfer · 2020-07-16T11:40:58Z

it/__init__.py

+
+
+def get_version_from_file():
+    lines = process.run_subprocess_with_output("cat version.txt", path=ROOT_DIR)


There is a version helper function in esrally.version that we can use instead.

danielmitterdorfer · 2020-07-16T11:43:29Z

it/__init__.py

+
+def build_docker_image():
+    # First ensure any left overs have been cleaned up
+    if process.run_subprocess_in_path(ROOT_DIR, "docker-compose -f docker/docker-compose-tests.yml down -v") != 0:


Can we use process.run_subprocess_with_logging instead and provide the absolute path to the Docker compose file here instead (os.path.join(ROOT_DIR, "docker", "docker-compose-tests.yml")?

Also, I think we should move this to it/docker_dev_image_test.pyas a teardown function. Unfortunately, the API with docker-compose is a bit odd (we need to provide the environment variable, then start, afterwards stop) so we cannot really use a proper fixture. However, we could implement a helper function to which we provide the actual TEST_COMMAND. I'm thinking of something along the lines of:

def run_docker_compose_test(test_command): try: # pseudo code if run_docker_compose_up(test_command) != 0: raise AssertionError("Indicate here that the test execution has failed...") finally: # always ensure proper cleanup regardless of results run_docker_compose_down()

it/__init__.py

danielmitterdorfer · 2020-07-16T11:45:17Z

it/__init__.py

+    os.environ['RALLY_LICENSE'] = get_license()
+
+    command = f"docker build -t elastic/rally:{rally_version} --build-arg RALLY_VERSION --build-arg RALLY_LICENSE -f docker/Dockerfiles/Dockerfile-dev $PWD"
+    if process.run_subprocess_in_path(ROOT_DIR, command) != 0:


Similarly to the Docker compose invocation this should also be run_subprocess_with_logging.

danielmitterdorfer · 2020-07-16T11:45:55Z

it/docker_dev_image_test.py

+    test_command = "--pipeline=benchmark-only --test-mode --track=geonames --challenge=append-no-conflicts-index-only --target-hosts=es01:9200"
+    os.environ["TEST_COMMAND"] = test_command
+
+    if process.run_subprocess_in_path(ROOT_DIR, DOCKER_COMPOSE_UP) != 0:


Similarly to other invocations, can we use run_subprocess_with_logging here?

danielmitterdorfer · 2020-07-16T11:46:26Z

it/docker_dev_image_test.py

+
+def test_docker_geonames():
+    test_command = "--pipeline=benchmark-only --test-mode --track=geonames --challenge=append-no-conflicts-index-only --target-hosts=es01:9200"
+    os.environ["TEST_COMMAND"] = test_command


Can we pass this via the env parameter to the subprocess invocation?

danielmitterdorfer · 2020-07-16T11:50:24Z

esrally/utils/process.py

@@ -28,11 +28,15 @@ def run_subprocess(command_line):
    return os.system(command_line)


-def run_subprocess_with_output(command_line):
+def run_subprocess_in_path(path, command_line):


I think we don't need changes to this file and can reuse existing functionality (see my comments later on)

bartier · 2020-07-16T22:07:27Z

@danielmitterdorfer Thanks! I made some changes to the code. Could you please check again?

danielmitterdorfer

Thanks, the new iteration looks pretty good already. I left a couple of smaller comments but I think we can merge this soon. Can you also remove the test_docker_dev_image function from integration-test.sh so we don't run the same tests from there as well?

Just as a note: After this PR we will still run test_docker_release_image for the released Docker images but I think we need to tackle this completely separately later on because this is also tied to the release process and will require a bit more discussion how we will handle this.

danielmitterdorfer · 2020-07-17T09:38:06Z

it/__init__.py

@@ -25,10 +25,13 @@

 from esrally import client
 from esrally.utils import process, io
+from esrally.version import version
+from it.docker_dev_image_test import run_docker_compose_down


I think it's better to avoid that the integration test root module depends on a specific integration test module. I think in this case we can even get rid of it as we don't need to call run_docker_compose_down in build_docker_image (see my other comment below)

danielmitterdorfer · 2020-07-17T09:39:13Z

it/__init__.py

+
+def build_docker_image():
+    # First ensure any left overs have been cleaned up
+    run_docker_compose_down()


Now that we run this command in a try ... finally which will shutdown the Docker container cleanly in all circumstances I think we can get rid of this call here.

danielmitterdorfer · 2020-07-17T09:41:43Z

it/__init__.py

+    run_docker_compose_down()
+
+    # We just want the release version without suffix
+    rally_version = version().split(" ")[0]


Good point but in that case I think you could directly use esrally.version.__version__? It's exactly the string that you're after.

danielmitterdorfer · 2020-07-17T09:41:59Z

it/docker_dev_image_test.py

+def run_docker_compose_up(test_command):
+    env_variables = os.environ.copy()
+    env_variables["TEST_COMMAND"] = test_command
+    env_variables['RALLY_VERSION'] = version().split(" ")[0]


Good point but in that case I think you could directly use esrally.version.__version__? It's exactly the string that you're after.

bartier · 2020-07-17T11:08:38Z

@danielmitterdorfer I made some changes, could you please check it again? Indeed the code looks much better now. Thanks for your feedback.

danielmitterdorfer · 2020-07-17T11:12:44Z

@elasticmachine test this please

danielmitterdorfer

Looks great now! Thank you for your change.

Migrate first test_docker_dev_image

9ea8bd1

danielmitterdorfer self-requested a review July 16, 2020 07:03

danielmitterdorfer self-assigned this Jul 16, 2020

danielmitterdorfer added :misc Changes that don't affect users directly: linter fixes, test improvements, etc. enhancement Improves the status quo labels Jul 16, 2020

danielmitterdorfer added this to the 2.0.1 milestone Jul 16, 2020

danielmitterdorfer requested changes Jul 16, 2020

View reviewed changes

danielmitterdorfer mentioned this pull request Jul 16, 2020

Migrate all integration tests to new infrastructure #975

Closed

5 tasks

Refactor code as pointed in code review

72ea989

bartier requested a review from danielmitterdorfer July 16, 2020 22:07

danielmitterdorfer reviewed Jul 17, 2020

View reviewed changes

Code review

4973cb3

bartier requested a review from danielmitterdorfer July 17, 2020 11:08

danielmitterdorfer approved these changes Jul 17, 2020

View reviewed changes

danielmitterdorfer merged commit d298b01 into elastic:master Jul 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Migrate first test_docker_dev_image #1026

[WIP] Migrate first test_docker_dev_image #1026

bartier commented Jul 15, 2020

danielmitterdorfer left a comment

danielmitterdorfer Jul 16, 2020

danielmitterdorfer Jul 16, 2020

danielmitterdorfer Jul 16, 2020

danielmitterdorfer Jul 16, 2020

danielmitterdorfer Jul 16, 2020

danielmitterdorfer Jul 16, 2020

danielmitterdorfer Jul 16, 2020

bartier commented Jul 16, 2020

danielmitterdorfer left a comment

danielmitterdorfer Jul 17, 2020

danielmitterdorfer Jul 17, 2020

danielmitterdorfer Jul 17, 2020

danielmitterdorfer Jul 17, 2020

bartier commented Jul 17, 2020

danielmitterdorfer commented Jul 17, 2020

danielmitterdorfer left a comment

	export TEST_COMMAND="--pipeline=benchmark-only --test-mode --track=geonames --challenge=append-no-conflicts-index-only --target-hosts=es01:9200"
	info "Testing Rally docker image using parameters: ${TEST_COMMAND}"
	docker_compose up
	docker_compose down



		def get_version_from_file():
		lines = process.run_subprocess_with_output("cat version.txt", path=ROOT_DIR)

[WIP] Migrate first test_docker_dev_image #1026

[WIP] Migrate first test_docker_dev_image #1026

Conversation

bartier commented Jul 15, 2020

danielmitterdorfer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartier commented Jul 16, 2020

danielmitterdorfer 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

bartier commented Jul 17, 2020

danielmitterdorfer commented Jul 17, 2020

danielmitterdorfer left a comment

Choose a reason for hiding this comment