-
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
[Heartbeat] Build elastic agent with synthetics support / offline mode #27052
Conversation
Pinging @elastic/uptime (Team:Uptime) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
Linking to #26398 |
I see a checkbox for documentation. We documented the docker container here elastic/observability-docs#622. Can you add content for the offline mode or at least create an issue to track adding the docs? |
As a first step I think the most important part is that it does not affect the current build we have. Would be good to validate this with a package build. I did not test it locally or go into the detail about overall LGTM. @andrewkroh @blakerouse Could you take a closer look at this as you are more familiar with the agent docker configs and the build system overall and might spot some side effects of this? |
dev-tools/mage/dockerbuilder.go
Outdated
@@ -58,6 +59,11 @@ func newDockerBuilder(spec PackageSpec) (*dockerBuilder, error) { | |||
} | |||
|
|||
func (b *dockerBuilder) Build() error { | |||
variants := []string{""} | |||
if os.Getenv("OFFLINE_DOCKER_IMAGE") == "true" || b.ServiceName == "elastic-agent" { |
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.
We should document this somewhere in the Beats repo?
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.
Is there a place we already document similar things? It's my understanding that we don't have such a place. I'd be glad to document it, my only question is where?
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 started to put things into elastic agent readme: https://github.com/elastic/beats/tree/master/x-pack/elastic-agent Lets put it there so we have it and can figure out a better place later.
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've since removed this and replaced it in the package spec, where it has an explanatory comment. I've also added a note to the README you mentioned about the behavior of the package spec.
@mostlyjason we have this captured in elastic/observability-docs#663 (I've added a reference to the existing docs page you mentioned) |
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.
If there is an "offline" image available I would expect it to include other artifacts that are not currently bundled with Agent like osquerybeat. Do others have this expectation? Should we add that as well?
dev-tools/mage/dockerbuilder.go
Outdated
@@ -58,6 +59,11 @@ func newDockerBuilder(spec PackageSpec) (*dockerBuilder, error) { | |||
} | |||
|
|||
func (b *dockerBuilder) Build() error { |
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.
This doesn't seem like the right place for any project specific build options. IIRC this code path doesn't have knowledge of elastic-agent at all.
If there are new artifacts that the Elastic Agent build should produce those should be defined in a package spec YAML (elastic-agent appears to be adding artifacts to the https://github.com/elastic/beats/blob/master/dev-tools/packaging/packages.yml#L938-L939).
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.
Fixed!
@andrewkroh ++ on your proposal that the "offline" image contains "all" of it. |
Co-authored-by: Manuel de la Peña <[email protected]>
My latest push removes the assigning of |
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.
LGTM 👍
@@ -6,6 +6,10 @@ | |||
# the final image because of permission changes. | |||
FROM {{ .buildFrom }} AS home | |||
|
|||
# Contains the elastic agent image variant, an empty string for the standard variant | |||
# or "offline" for the offline one. | |||
ENV ELASTIC_AGENT_IMAGE_VARIANT={{.Variant}} |
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.
What do you think about omitting this from the environment when .Variant
is empty?
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 neutral on it. I do kind of like having it empty because it makes it maybe more explicit that this is the "" variant.
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.
IIUC, this particular variable does nothing but reporting the docker image is a variant? If so, maybe the docker metadata attributes could be a better approach?
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.
Code change LGTM. Only one part I'm not sure how it is related.
I assume you did a full package run to see if things still work as expected? Lets make sure as soon as we get this in we keep an eye on the builds to make sure these still pass.
For the publishing of the images, I assume some additional work in the release manager will be needed?
@@ -122,9 +122,9 @@ func (je *journeyEnricher) enrichSynthEvent(event *beat.Event, se *SynthEvent) e | |||
case "step/screenshot_ref": | |||
fallthrough | |||
case "screenshot/block": | |||
add_data_stream_index.SetEventDataset(event, "browser_screenshot") | |||
add_data_stream_index.SetEventDataset(event, "browser.screenshot") |
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.
How is this related?
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.
The main purpose of this PR, really, is to enable the integration, which breaks without this. I agree it's a bit unrelated, but I think it makes sense to include because this PR isn't useful without it.
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.
LGTM
{{- if (and (eq .Variant "offline") (not (contains .from "ubi-minimal"))) }} | ||
# Setup synthetics env vars | ||
ENV ELASTIC_SYNTHETICS_CAPABLE=true | ||
ENV SUITES_DIR={{ $beatHome }}/suites |
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.
Are we using a dedicated folder instead of /tmp
for copying suites? I forgot what wat was the real use of this flag.
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.
This can probably be nixed. It's not really used for anything.
case "journey/network_info": | ||
add_data_stream_index.SetEventDataset(event, "browser_network") | ||
add_data_stream_index.SetEventDataset(event, "browser.network") |
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.
wouldn't it break for existing users?
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.
No, existing users don't use datasets, only people using the integration do. For existing users it's all under heartbeat-*
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.
Gotcha 👍🏽
@Mergifyio backport 7.x |
#27052) Creates a new offline tagged docker image that includes the dependencies required for synthetics. Building elastic-agent docker images will now create additional -offline tagged images with these extras. We may need to do additional work to ensure that these additional images are incorporated into the release process and onto the public website. These changes also set the ELASTIC_SYNTHETICS_CAPABLE env flag, as done in heartbeat, to enable synthetics features in the docker environment. This dovetails with the work @dominiqueclarke is doing in elastic/integrations#1064 and elsewhere Fixes #22932 (cherry picked from commit 4727470)
Command
|
#27052) (#27324) Creates a new offline tagged docker image that includes the dependencies required for synthetics. Building elastic-agent docker images will now create additional -offline tagged images with these extras. We may need to do additional work to ensure that these additional images are incorporated into the release process and onto the public website. These changes also set the ELASTIC_SYNTHETICS_CAPABLE env flag, as done in heartbeat, to enable synthetics features in the docker environment. This dovetails with the work @dominiqueclarke is doing in elastic/integrations#1064 and elsewhere Fixes #22932 (cherry picked from commit 4727470) Co-authored-by: Andrew Cholakian <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@Mergifyio backport 7.15 |
#27052) Creates a new offline tagged docker image that includes the dependencies required for synthetics. Building elastic-agent docker images will now create additional -offline tagged images with these extras. We may need to do additional work to ensure that these additional images are incorporated into the release process and onto the public website. These changes also set the ELASTIC_SYNTHETICS_CAPABLE env flag, as done in heartbeat, to enable synthetics features in the docker environment. This dovetails with the work @dominiqueclarke is doing in elastic/integrations#1064 and elsewhere Fixes #22932 (cherry picked from commit 4727470) # Conflicts: # dev-tools/mage/dockerbuilder.go
Command
|
#27052) (#27324) Creates a new offline tagged docker image that includes the dependencies required for synthetics. Building elastic-agent docker images will now create additional -offline tagged images with these extras. We may need to do additional work to ensure that these additional images are incorporated into the release process and onto the public website. These changes also set the ELASTIC_SYNTHETICS_CAPABLE env flag, as done in heartbeat, to enable synthetics features in the docker environment. This dovetails with the work @dominiqueclarke is doing in elastic/integrations#1064 and elsewhere Fixes #22932 (cherry picked from commit 4727470) Co-authored-by: Andrew Cholakian <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit f9b2610) # Conflicts: # dev-tools/mage/dockerbuilder.go
This pull request does not have a backport label. Could you fix it @andrewvc? 🙏
NOTE: |
Creates a new
offline
tagged docker image that includes the dependencies required for synthetics. Building elastic-agent docker images will now create additional-offline
tagged images with these extras. We may need to do additional work to ensure that these additional images are incorporated into the release process and onto the public website.These changes also set the
ELASTIC_SYNTHETICS_CAPABLE
env flag, as done in heartbeat, to enable synthetics features in the docker environment. This dovetails with the work @dominiqueclarke is doing in elastic/integrations#1064 and elsewhereFixes #22932
CC @ruflin @v1v @mostlyjason @drewpost @vigneshshanmugam @dominiqueclarke
Checklist
- [ ] I have made corresponding change to the default configuration files- [ ] I have added tests that prove my fix is effective or that my feature worksCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.How to test this PR locally
TODO: Test this with the upcoming synthetics support in integrations (see elastic/integrations#1064)