Add elastic-agent-service container to packaging#5349
Add elastic-agent-service container to packaging#5349blakerouse merged 17 commits intoelastic:mainfrom
Conversation
seanstory
left a comment
There was a problem hiding this comment.
Few nits but this is super exciting. Thank you so much for your help, it would have taken us ages to figure out how to stitch into this system. 🙏
dev-tools/packaging/packages.yml
Outdated
| source: '{{.AgentDropPath}}/archives/{{.GOOS}}-{{.AgentArchName}}.tar.gz/connectors-{{ beat_version }}{{if .Snapshot}}-SNAPSHOT{{end}}.zip' | ||
| mode: 0755 | ||
| 'data/{{.BeatName}}-{{ commit_short }}/components/connectors': | ||
| source: '{{ elastic_beats_dir }}/dev-tools/packaging/files/linux/connectors.py' |
There was a problem hiding this comment.
I expect this will end up being a bash script rather than python, but I don't think that really matters right now.
There was a problem hiding this comment.
Yeah it doesn't matter. Just need to know what the contents should be.
|
|
||
| {{- if (and (contains .image_name "-complete") (contains .from "ubuntu")) }} | ||
| {{- if eq .Variant "service" }} | ||
| RUN apk add --no-cache python-3.12 py3.12-pip && \ |
There was a problem hiding this comment.
We don't support python 3.12 yet :( It's on our roadmap, but 3.11 is probably better for now. See elastic/connectors#2734 for details
There was a problem hiding this comment.
I can lower the version, once we know whats in the execution script. By the time its ready you might support 3.12 :-)
There was a problem hiding this comment.
I don't expect 3.12 support to make it that fast. :) We were on 3.10 for about 2 years before we just added 3.11 a few weeks ago. Urgency is not there.
| {{- if (and (contains .image_name "-complete") (contains .from "ubuntu")) }} | ||
| {{- if eq .Variant "service" }} | ||
| RUN apk add --no-cache python-3.12 py3.12-pip && \ | ||
| pip install {{ $beatHome }}/data/service_downloads/connectors-*.zip && \ |
There was a problem hiding this comment.
Would be safer to unzip and make install, as that'll also give it a python virtual environment, and keep it resileint to any other system python deps that might end up in this image one day.
There was a problem hiding this comment.
I can do that, seems a little un-needed being this is the only python used in the image. The simple pip install without requiring unzip to be installed in the image seems better to me.
There was a problem hiding this comment.
it's best practice (AFAIU) to not use system python if you can help it. Also by using a make target, it allows us to hook into other setup bits. For example, our make isntall insures that bin/pip install --upgrade pip and bin/pip install --upgrade setuptools are both run, and also ensure that the right requirements/$ARCH.txt is selected, depending upon the OS that it's being run on.
|
Just note I am blocked on making any more progress on this PR, under we know what the contents of the bash script should be and I believe we are waiting for the python version of the elastic-agent-client to be included in the DRA or possibly in a pypi dist (some way of getting it). |
Can we not merge it as is, and follow up later with edits to make the binary more "real"? Is there some sort of testing bar that happens for each included binary that we need to to be able to meet?
@artem-shelkovnikov is currently working on that, but is needing to do some repo restructuring so that the private-repo py-elastic-agent-client lib can be installed in this docker image, but won't prevent community members from cloning and building self-managed connectors from source. |
|
This pull request is now in conflicts. Could you fix it? 🙏 |
|
👋 We should be unblocked on making progress on this PR. We've recently merged elastic/connectors#2787 that has a component ready to be ran. To run the component you'd need:
I tested this code locally by having an agent downloaded and adding component and component spec into the components with following code (would need to be adapted to run in the docker image): |
|
Woo!! Thanks, Artem! |
|
@artem-shelkovnikov I am unable to get this to actually build, I am getting the following error calling Let me know how to solve this issue: |
|
@seanstory I can add an integration test using this container, once I have it building and working. |
|
@blakerouse the problem comes from the fact that connectors dependency is a private Elastic repository. We had to work around this with this change: https://github.com/elastic/ci/pull/3327 Additionally, we're gonna be opening the private repo ASAP. I don't think it would work even if we provided a |
|
@artem-shelkovnikov Opening it to be public would make this much easier and allow developers to build this locally with our simple |
|
ETA unknown - I've submitted a request to #open-source here: https://github.com/elastic/open-source/issues/443 Open source team will look at it today and best case will allow to open it up, I'll follow up here after I get a response |
|
We just got the approval, and https://github.com/elastic/python-elastic-agent-client now has public/open visibility 🎉 |
|
@seanstory @artem-shelkovnikov Nice! I did get it to build. With it built I ran the container and tried to execute the Check its using python from venv: |
|
Fixed by elastic/connectors#2811. Sorry about that. New DRAs should be available momentarily. 🤞 |
|
I don't think the DRA has that change or even the change that was merged last week, or something is wrong The contents of the DRA only show the following which doesn't have the |
|
For those watching, we solved the missing dir issue with a three-part combo of:
and now we're discussing further hiccups in this slack thread. |
|
|
|
|
|
This pull request is now in conflicts. Could you fix it? 🙏 |
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
|
@seanstory I finally have a passing integration test for this new container. Its all working now. Just need a review and it can be merged. |
|
hey @blakerouse 👋 I left only a minor comment that I think you need to address. |
|
@pkoutsovasilis Fixed |
| // never flatten any python wheels, the packages.yml and docker should handle | ||
| // those specifically so that the python wheels are installed into the container | ||
| matches = removePythonWheels(matches, packageVersion) |
There was a problem hiding this comment.
Is there an alternative to adding the PythonWheel binary specs to the ExpectedBinaries just to remove them from the flattened directory? The connectors are added to ExpectedBinaries just to download the archive ?
If the handling of connectors is different from the other components' binaries we package with agent maybe we can separate the handling with a collectAdditionalDependencies target after flattening or something similar ?
My understanding is also that these dependencies are not needed to create an agent archive but get installed when we run docker build, maybe we can move the processing at that stage ?
|
buildkite test this |
1 similar comment
|
buildkite test this |
|
Adds the elastic-agent-service container with the py-connectors component enabled. (cherry picked from commit 701f8b9) # Conflicts: # dev-tools/mage/manifest/manifest.go
#5634) * Add elastic-agent-service container to packaging (#5349) Adds the elastic-agent-service container with the py-connectors component enabled. (cherry picked from commit 701f8b9) # Conflicts: # dev-tools/mage/manifest/manifest.go * Update manifest.go Fix conflicts caused by mergify * Include python wheel packages when moving downloaded dependencies archives (#5647) This commit modifies movePackagesToArchive() function to include python wheel packages introduced with commit 701f8b9 . This solves an issue when packaging docker images using a DROP_PATH env var: prior to this change, the python wheel archives present in DROP_PATH were not moved to `<DROP PATH>/archive/<platform>` failing to create an image for which the Dockerfile expects to install the python wheel package * Fix docker image build when multiple platforms are specified (#5658) Force merging to unblock the unified release process --------- Co-authored-by: Blake Rouse <blake.rouse@elastic.co> Co-authored-by: Julien Lind <julien.lind@elastic.co> Co-authored-by: Paolo Chilà <paolo.chila@elastic.co>





What does this PR do?
Adds a new docker variant of
elastic-agent-servicethat includes the python connectors.Currently this PR includes the following PR until that PR is merged - #5338
Why is it important?
Used by the Elastic Agent service.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files[ ] I have added tests that prove my fix is effective or that my feature works[ ] I have added an entry in(not really user facing at this level)./changelog/fragmentsusing the changelog tool[ ] I have added an integration test or an E2E testDisruptive User Impact
None.
How to test this PR locally