-
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
[Beats] Build darwin/arm64 and universal binary #29585
Conversation
This pull request does not have a backport label. Could you fix it @AndersonQ? 🙏
NOTE: |
572f1be
to
bcd1029
Compare
e00ff3c
to
3453f2c
Compare
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
This feature will be enabled no just in ElasticAgent but in all the other Beats, is that correct? |
@AndersonQ I've tried to build all the artifacts with
I also try to use the |
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.
Should have used request changes before for my last comment.
Perhaps, I'll double check with the data plane |
That's probably because not all beats have the
Probably, that's the catch of the approach I took.
|
I think |
This pull request is now in conflicts. Could you fix it? 🙏
|
I've got some questions:
Then, it's worth to involve the CI System and Release teams as they are the ones who provide:
On the other hand, I tried to solve -> #29585 (comment) in #30505 but I'm now facing issues with the docker-machine not being available at runtime. But before doing anything else, we might need to get some clarity about the above questions. |
Hi @AndersonQ
Please find below logs for the same using diagnostics command : Please let us know if anything else is required from our end. |
@amolnater-qasource @dikshachauhan-qasource it's ready for another QA round, the new packages are on the same Google drive as before. |
Hi @AndersonQ We have revalidated all the artifacts shared with us on 8.1 kibana build and found that:
logs for M1 agent with aarch artifact : elastic-agent-diagnostics-2022-03-10T10-40-48Z-00.zip Build details: Thanks |
This pull request is now in conflicts. Could you fix it? 🙏
|
f918b77
to
2001ccf
Compare
2001ccf
to
4fe0196
Compare
@AndersonQ I see that @elastic/infra-monitoring-ui is added a codeowner reviewer here, but I don't see any files owned by us there. Can you help me understand why that might be the case? (Did it at one point include changes to the kibana, elasticsearch, logstash, or beats modules, for example?) |
Hi @jasonrhodes, you're correct, there is nothing you folks would need to review. I don't know why you were asked to review. |
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, but I have a couple of questions.
// TODO(AndersonQ): comment in after the tests pass | ||
// 'darwin/arm64' |
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 is the plan here? Comment it in once CI is green and merge or do it in another PR?
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 ended up testing it on the elastic-agent repo, didn't do what I needed. Besides in order to unblock the release team, better to merge it now and adjust the CI later. But good catch anyway :)
// AssembleDarwinUniversal merges the darwin/amd64 and darwin/arm64 into a single | ||
// universal binary using `lipo`. It assumes the darwin/amd64 and darwin/arm64 | ||
// were built and only performs the merge. |
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.
Why not use mage
dependencies to ensure the binaries are built instead of failing?
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.
Mage dependencies will ensure a function was called, here I need more than just that. I need the build to have been invoked with the right environment variables, it has happened 2 times (once for each architecture). Thus it isn't really a solution here.
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 think @belimawr comment is good concerning the mage dependencies to ensure it work.
Other than that, I've tested this the associated beats branch and I was able to get the artifacts built. Correctly
This reverts commit f2ed3ab.
* build and packages beats for darwin/arm64. The tar.gz package is called darwin-aarch64 * Osquerybeat: Add darwin/arm64 packaging support (elastic#30935) Co-authored-by: Aleksandr Maus <[email protected]>
This reverts commit 33f918c.
* build and packages beats for darwin/arm64. The tar.gz package is called darwin-aarch64 * Osquerybeat: Add darwin/arm64 packaging support (#30935) Co-authored-by: Aleksandr Maus <[email protected]>
What does this PR do?
It enables the Elastic-Agent and Beats to be built for
darwin/arm64
and generates a universal binary (darwin/universal
).The universal binary is the merge of the
darwin/arm64
withdarwin/amd64
. Therefore it requires to first build thedarwin/arm64
anddarwin/amd64
and them "merge" them together into thedarwin/universal
. Alsodarwin/universal
isn't a valid platform or OS/ARCH pair, therefore it isn't recognised by the Go compiler. Given that, the choice was to adddarwin/arm64
as a new platform for the Elastic-Agent and when ever both platforms (darwin/arm64
anddarwin/amd64
) are built, also generate and package the universal binary.Even though it's
quitean implicit behaviour, the alternative would also require a lot of changes and exceptions to have our build process recognisingdarwin/universal
, but compilingdarwin/arm64
anddarwin/amd64
and merging them. Also the current process makes available all three of them, allowing the user to choose which fits better their needs. The drawback of the universal binary is its size, the double of the single binaries.A new target,
BuildDarwinUniversal
was added on thex-pack
smagefile
s required by the Elastic-Agent.Also fixes some small typos, redundant information on the help command and remove the use of some deprecated functions.
Why is it important?
We need to support Mac with M1 chips
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Build the elastic-agent for the new platforms:
PLATFORMS="darwin/arm64" PACKAGES="tar.gz" mage package
PLATFORMS="darwin/arm64,darwin/amd64" PACKAGES="tar.gz" mage package
Run the elastic-agent
####Watch out:
The elastic-agent tries to download any binary it might need to comply with the assigned policy. However none of the new binaries from this PR are present on the artifact API and other applications (e.g. fleet-server and elastic-endpoint) also aren't publishing
darwin/arm64
versions. Therefore if the elastic-agent tries to download them, it'll fail. This is the expected behavior and this PR does not address it, see Out of Scope.Out of scope:
darwin-universal
artifacts on the artifact APIdarwin/amd64
agent running under Rosetta2 on a Mac with M1 chip. This will be addressed on another PR.Related issues