-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Upgrade test Docker images to latest versions #9198
Conversation
I think the CI test failure is legitimate and should be fixed once #9179 is merged. |
157c65c
to
fb38725
Compare
metricbeat/module/elasticsearch/elasticsearch_integration_test.go
Outdated
Show resolved
Hide resolved
jenkins, test this |
1 similar comment
jenkins, test this |
@ruflin @jsoriano As you can see from the evolution of this PR, upgrading the ES docker image to 6.5.1 caused the Would you like me to pull out the CCR test-related fixes into their own PR(s) and keep this PR just for the actual version upgrade changes? |
I am fine with leaving the upgrade of the image in the same PR, this way we know what was needed to support it. I'd add some explanation about why CCR tests existed for previous versions but weren't failing. Also I don't like so much to have the implementation of CCR setup in both python and Go, but I guess we'd have to live with that while we have system testing in python. |
Thanks @jsoriano. Added the note about why these tests were not failing before this PR in the PR description. Agreed about the duplication of code. |
Could you update the branch after #9179 to see if tests pass in CI? |
Hmmmm, now the ES module system tests are all failing because the However, when I run the exact same Docker image and make the same API call against it locally, it works:
I'm investigating further but if something jumps out to you, @jsoriano or @ruflin, please let me know. Thanks! |
2cbbdb7
to
b263c16
Compare
Did the code work for the ml test only with 6.5.1? Or did it also break? The reason I'm asking is because it seems the trial activation breaks the tests. So if the trial activation works with the old code for ML, then the problem is likely in the code, if it didn't work, my assumption is that something changed in 6.5.1 docker containers? |
Good thinking, @ruflin. Let me put up a "test" PR (so CI can run on it) with just the 6.5.1 version upgrade and no changes to the tests code. Let's see what happens to that one. |
37ca1ab
to
0d54c05
Compare
@jsoriano @ruflin This PR is ready for review again. I did a bunch of testing and eventually found the fix to be changing this line: https://github.com/elastic/beats/pull/9198/files#diff-5a5de1a556d9b66b8140e73286c014aeR2. Before this change I believe we were hitting a race condition between the new code in this PR calling the |
FROM docker.elastic.co/elasticsearch/elasticsearch:6.4.3 | ||
HEALTHCHECK --interval=1s --retries=300 CMD curl -f http://localhost:9200 | ||
FROM docker.elastic.co/elasticsearch/elasticsearch:6.5.1 | ||
HEALTHCHECK --interval=1s --retries=300 CMD curl -f http://localhost:9200/_xpack/license |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it also work with a more generic health check like /_cluster/health
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try this and see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it works reliably with _cluster/health
. Jenkins CI passed but Travis CI failed with the same 404 error on GET _xpack/license
that we were seeing earlier.
I think it's okay to base the health check on the URL we expect to call, i.e. GET _xpack/license
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's fine, lets leave it with _xpack/license
then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But lets take this into account elastic/elasticsearch#35959 :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to fix all deprecated _xpack
endpoints across the codebase in #9656.
* Upgrade to latest versions * Fixing integration test by setting up CCR stats * Adding test fixtures * Renaming per hound suggestion * Start trial for all metricsets * Create CCR stats first! * Fix formatting issues * Refactoring to reuse ES client * [WIP] Testing if this is a timing issue * Fixing formatting * WIP: Make container health check use GET /_xpack/license * Removing test sleep * Trying a more generic health check (cherry picked from commit ecc09cc)
…ns (#9734) * Upgrade test Docker images to latest versions (#9198) * Upgrade to latest versions * Fixing integration test by setting up CCR stats * Adding test fixtures * Renaming per hound suggestion * Start trial for all metricsets * Create CCR stats first! * Fix formatting issues * Refactoring to reuse ES client * [WIP] Testing if this is a timing issue * Fixing formatting * WIP: Make container health check use GET /_xpack/license * Removing test sleep * Trying a more generic health check (cherry picked from commit ecc09cc) * Updating healthcheck (#9749)
This PR upgrades the Elasticsearch and Kibana Docker images to their latest versions in their respective Metricbeat module tests.
In addition to the actual version upgrade changes, this PR also contains several changes necessary to get the integration and system tests for the
elasticsearch/ccr
metricset passing. These tests existed before this PR but were skipped because: