Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add helper to get hosts on metricbeat integration tests #11925

Merged
merged 11 commits into from
Apr 29, 2019

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Apr 24, 2019

Add a helper to automatically obtain the host of a service in metricbeat
system tests so tests don't depend on environment variables or
current running docker services. This is needed to allow in the future
to run multiple versions of the same service, is also helpful when
running tests locally.

Environment variables with the format SERVICE_HOST, being SERVICE
the uppercased name of the compose service can be used to override the
automatic value. By now OVERRIDE_HOST needs to be set to for these
variables to work in the python-based tests.

NO_COMPOSE environment variable can be used now to avoid stopping
the running containers too.

Add ports to all services in docker compose files, required for the
helper.

Part of #7957 for python-based integration tests.

@jsoriano jsoriano added module Metricbeat Metricbeat :Testing Team:Integrations Label for the Integrations team labels Apr 24, 2019
@jsoriano jsoriano self-assigned this Apr 24, 2019
Add a helper to automatically obtain the host of a service in metricbeat
integration tests so tests don't depend on environment variables or
current running docker services. This is needed to allow in the future
to run multiple versions of the same service, is also helpful when
running tests locally.

Environment variables with the format SERVICE_HOST, being SERVICE the
uppercased name of the compose service can be used to override the
automatic value.

`NO_COMPOSE` environment variable can be used now to avoid stopping
the running containers too.

Add ports to all services in docker compose files, required for the
helper.

Part of elastic#7957 for python-based integration tests.
@jsoriano jsoriano force-pushed the compose-test-runner-python branch 2 times, most recently from 88cda5b to ede58db Compare April 24, 2019 16:03
@jsoriano
Copy link
Member Author

jenkins, test this again please

@jsoriano jsoriano force-pushed the compose-test-runner-python branch 2 times, most recently from dfcd3e2 to 0071b37 Compare April 25, 2019 09:44
@jsoriano jsoriano marked this pull request as ready for review April 25, 2019 12:19
@jsoriano jsoriano requested review from a team as code owners April 25, 2019 12:19
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change, it should make tests easier!

I wonder if we can detect ports as exposed in the Dockerfile, without explicitly exposing them in docker-compose.yml, or maybe that's too hidden

ENV KAFKA_LOGS_DIR="/kafka-logs"
ENV _JAVA_OPTIONS "-Djava.net.preferIPv4Stack=true"
ENV TERM=linux

RUN apt-get update && apt-get install -y curl openjdk-8-jre-headless netcat dnsutils
RUN apk add -u bash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes to this file and run.sh probably belong to a different PR?

Copy link
Member Author

@jsoriano jsoriano Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes here could probably be reverted, yes, I think they only reduced the size of the image.

Changes in run.sh are needed, kafka announces the addresses clients should use, current container only works if tests can connect to "kafka:9092". After this change services can be listening on random addresses and ports, so we need some plumbing to let kafka know what address it should announce.
But said that, not all changes are needed now, I got all changes done in #7957, that are intended also to be able to have multiple versions of the same service running at the same time, this is not needed till I merge the Go-part of this PR.

I will reduce the changes in these files to the ones really needed by now.

@@ -50,5 +50,4 @@ def test_jmx(self, mbean):
assert evt["jolokia"]["test"]["gc"]["collection_count"] >= 0

def get_hosts(self):
return [os.getenv('JOLOKIA_HOST', 'localhost') + ':' +
os.getenv('JOLOKIA_PORT', '8778')]
return [self.compose_host()]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this pattern a lot, you could also replace get_hosts() call with self.compose_host() and remove get_hosts all together??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, thanks! It should be possible, I will do it and override only when needed.

@jsoriano
Copy link
Member Author

I wonder if we can detect ports as exposed in the Dockerfile, without explicitly exposing them in docker-compose.yml, or maybe that's too hidden

I tried, but I didn't manage to do it, it seems that docker compose only expose ports defined in the docker-compose.yml file. If there is a way to make it expose the ports defined in the Dockerfile we could consider doing it, yes.
In any case it is in my plans to move all the testing code of each module to the module itself, including the docker compose definitions. In many cases we are only using the Dockerfile to define the healthcheck and this can be done in the docker-compose.yml now, so many Dockerfiles might disappear in the future.

@jsoriano
Copy link
Member Author

I have seen Kafka tests to fail a couple of times in travis, I am taking a look to this.

@jsoriano
Copy link
Member Author

Ok, it fails because it is not ready yet to have multiple docker compose files in the same run, I will try to refactor Kafka docker files to don't need to bring here more changes from #7957

I don't know though why it doesn't fail on Jenkins.

@jsoriano jsoriano added [zube]: In Progress in progress Pull request is currently in progress. and removed [zube]: In Review labels Apr 26, 2019
@jsoriano
Copy link
Member Author

Issues fixed by reverting more uneeded changes for kafka, this would be ready for another review.

@jsoriano jsoriano added [zube]: In Review review and removed [zube]: In Progress in progress Pull request is currently in progress. review labels Apr 26, 2019
@zube zube bot added the review label Apr 26, 2019
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat module review Team:Integrations Label for the Integrations team :Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants