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

feat: add Oracle support on Metricbeat Docker images #12890

Merged
merged 17 commits into from
Mar 16, 2020

Conversation

kuisathaverat
Copy link
Contributor

@kuisathaverat kuisathaverat commented Jul 12, 2019

  • Add Oracle libraries to the Metricbeat base Docker image

depends on #13764

@kuisathaverat kuisathaverat requested a review from a team as a code owner July 12, 2019 13:06
@kuisathaverat kuisathaverat self-assigned this Jul 12, 2019
.ci/packer_cache.sh Outdated Show resolved Hide resolved
.ci/packer_cache.sh Outdated Show resolved Hide resolved
metricbeat/Dockerfile Outdated Show resolved Hide resolved
@alvarolobato
Copy link

@sayden can you have a look at this? We should merge it.

@kuisathaverat will fix the conflicts today.

@sayden
Copy link
Contributor

sayden commented Sep 20, 2019

@kuisathaverat thanks for working on this, my friend! 🙂 I have taken a look at everything looks ok but we need a green CI before trying to merge. We have this issue

FROM store/oracle/database-instantclient:12.2.0.1 as oracle
ERROR: Service 'beat' failed to build: pull access denied for store/oracle/database-instantclient, repository does not exist or may require 'docker login'

Which is reproducible if you do a docker pull store/oracle/database-instantclient:12.2.0.1 in your local.

@kuisathaverat
Copy link
Contributor Author

I will move the https://github.com/elastic/beats/blob/21302f24583d278312dc05839436b06df7a776ef/.ci/packer_cache.sh file to another PR, it is the script used by Packer to make the pull image on the worker so you do not need to pull the image it is in the Jenkins worker.

@kuisathaverat
Copy link
Contributor Author

I've opened another PR only with the workers Docker cache script #13764, a couple of days after that PR will merge the Docker images will be in the Jenkins workers and this PR should work

@kuisathaverat kuisathaverat deleted the feat/oracle-support branch January 10, 2020 13:39
@kuisathaverat kuisathaverat restored the feat/oracle-support branch February 26, 2020 15:36
@kuisathaverat kuisathaverat reopened this Feb 26, 2020
metricbeat/Dockerfile Outdated Show resolved Hide resolved
@kuisathaverat kuisathaverat force-pushed the feat/oracle-support branch 2 times, most recently from 3a04a0e to b3de9c1 Compare March 9, 2020 17:34
@kuisathaverat
Copy link
Contributor Author

@jsoriano @sayden finally I've nailed it, Could you review it?

Copy link
Contributor

@sayden sayden left a comment

Choose a reason for hiding this comment

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

LGTM

@jsoriano
Copy link
Member

This seems to break travis builds, could we do something to avoid needing the oracle image in travis? Something like this can be also needed to run builds by community contributors.

@kuisathaverat
Copy link
Contributor Author

This seems to break travis builds, could we do something to avoid needing the oracle image in travis? Something like this can be also needed to run builds by community contributors.

I do not think so since you have to sign a before to download the image, also we can not put a user and password in Travis, the only solution would be to disable the Oracle test on Travis

@jsoriano
Copy link
Member

This seems to break travis builds, could we do something to avoid needing the oracle image in travis? Something like this can be also needed to run builds by community contributors.

I do not think so since you have to sign a before to download the image, also we can not put a user and password in Travis, the only solution would be to disable the Oracle test on Travis

I guess this would be fine, we will also have to find a way to have two kind of metricbeat images, one with Oracle and another one without it.

@kuisathaverat
Copy link
Contributor Author

kuisathaverat commented Mar 11, 2020

I think that to have two Dockerfiles and change one for another by setting an environment variable (PROPIETARY_DOCKER_IMAGES=1) could be the solution

@kuisathaverat
Copy link
Contributor Author

kuisathaverat commented Mar 12, 2020

we have changed the approach Oracle instantclient Basic 19 can be download without having to agree on a form, so we download the binaries and build the Docker image, the same for everybody.

@kuisathaverat
Copy link
Contributor Author

I will keep the DOCKER_PULL because I will use in the Pipeline to use the cache to increase speed on CI.

@kuisathaverat
Copy link
Contributor Author

@sayden Could you confirm that the new Docker image works for the test?

@kuisathaverat
Copy link
Contributor Author

@jsoriano @sayden it is ready to merge after your review

@jsoriano
Copy link
Member

jenkins, test this again please (there was an unrelated failure on kubernetes tests but I think it prevented to run the integration tests)

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

This is looking good.

I think we should remove the skips in the oracle tests:

=== RUN   TestData
--- SKIP: TestData (0.00s)
    metricset_test.go:20: Skip until a proper Docker image is setup for Metricbeat
PASS
ok  	github.com/elastic/beats/v7/x-pack/metricbeat/module/oracle/tablespace	0.012s

But this could be also changed in a follow up if @sayden is ok with this image.

metricbeat/Dockerfile Show resolved Hide resolved
dev-tools/mage/integtest.go Show resolved Hide resolved
@sayden
Copy link
Contributor

sayden commented Mar 16, 2020

I think I can remove the skips in this follow up #16833, so we can free @kuisathaverat

I tested the new library, included in this image, and it worked ok (after un-skipping tests, of course 😄 )

@kuisathaverat kuisathaverat merged commit 46a5b8b into elastic:master Mar 16, 2020
@kuisathaverat kuisathaverat deleted the feat/oracle-support branch March 16, 2020 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants