Respect ARCH env var when downloading JDK via gradle#18733
Respect ARCH env var when downloading JDK via gradle#18733donoghuc merged 5 commits intoelastic:mainfrom
Conversation
With the refactor of gradle to be the root of every orchestration task https://github.com/elastic/logstash/pull/18471/changes the artifactAll task depends on copyJdk, which calls downloadJdk. downloadJdk uses selectArch() to determine which JDK to download which results in the ARCH env var to be ignored. Given we build aarch64 packages on x86_64 in CI we always end up with the x86_64 jdk. This commit updates selectArch() to respect the env var
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label. Could you fix it @donoghuc? 🙏
|
donoghuc
added
the
backport-active-all
|
run exhaustive tests |
|
Kicked off a DRA snapshot build and checked correct version: |
| if (envArch) { | ||
| return (envArch == "x86_64") ? "x86_64" : "arm64" | ||
| } | ||
| // 3. Fall back to host architecture |
There was a problem hiding this comment.
I still think this is a dangerous assumption. Do we really have cases fall back to CI host architecture?
If not or not intention, I would support failing if neither ARCH or jdk_arch provided.
There was a problem hiding this comment.
I'm hesitant to agree. If this were in isolation maybe, but pretty much every other packaging bit assumes its being compiled on the host arch (for example if you tried to build for windows on linux it would break many other assumptions like selectOsType). I think there is just a single case of "optimization" where we cross compile/build for linux x86/arm. I think that the "pattern" for building is "assume you are building for the OS you are on" otherwise you have to configure flags/env vars to do otherwise.
There was a problem hiding this comment.
actually, that is not accurate. I need to think about this more... Something seems off.
There was a problem hiding this comment.
So this actually exposes a bit of an extension of the bug. Previously we would use rake to call back in to gradle to do multiple invocations of downloading a specific jdk then delete it. This means that tarballs for windows/mac would be getting the wrong JDK as well. Looking at options for fixing this.
|
run exhaustive test |
|
Put this back in to WIP... The extent of the bug is that previously rake was calling gradle multiple times during build to download different JDKs based on the artifact being prepared. This PR now downloads all required JDK (based on ARCH) to mirror the needs of building in CI (it always be split by ARCH). I am still vetting this change (need to check out remote VMs due to missing build tools on mac for building rpms), but I wanted an early look at CI. |
|
run exhaustive test |
|
CI showing some holes... The rough idea here was to download all the required JDKs and then pick which one depending on the task. I added a very rough outline of an attempt at doing that. Obviously it needs to be tweeked (it has some failures in container builds and is untested on mac/windows). The place I was going to pick up tomorrow is checking out a builder VM (linux x86) and target VM (a windows or mac one). Build on the builder and test on the target. This would let me validate things are working as expected. We could also look at downloading the necesary JDK at the time it is needed. Currently the logic is in gradle which makes me want to leave it there. This is probably not a hard requirement though if there is a case to move it to rake. |
|
Updated as more of a complete thought and did a bunch of testing (detaiiled in PR description). Marking this as ready for review. DRA snapshot: https://buildkite.com/elastic/logstash-dra-snapshot-pipeline/builds/4620 |
This commit extends the aarch64 pipeline to build a `.deb` for `aarch64` on an x86 VM then run the exhaustive tests on `aarch64` VM with the `.deb`. This replicates how artifacts are prepared in DRA on x86. This is net new in that it adds exhaustive tests for an `aarch` platform. This would have caught a recent packaging bug. More thourough tests will be added in the future, this is just a first step to get toward that goal. actually build arm
💚 Build Succeeded
History
|
|
Reviewer note : I applied d46e0cb to show this fixes the bug in an automated test: https://buildkite.com/elastic/logstash-aarch64-pipeline/builds/624#_ You can see that without this fix, it fails #18741 (comment) I can rever that and merge it separately in the other PR, but i wanted confirmation that this will pass :) |
jsvd
left a comment
jsvd
left a comment
There was a problem hiding this comment.
LGTM, tested locally and observed the difference in picking up the ARCH during assemble tasks like assembleTarDistribution
|
@Mergifyio backport 8.19 9.2 9.3 |
✅ Backports have been createdDetails
Cherry-pick of 7f1ca8e has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
|
* Respect ARCH env var when downloading JDK via gradle With the refactor of gradle to be the root of every orchestration task https://github.com/elastic/logstash/pull/18471/changes the artifactAll task depends on copyJdk, which calls downloadJdk. downloadJdk uses selectArch() to determine which JDK to download which results in the ARCH env var to be ignored. Given we build aarch64 packages on x86_64 in CI we always end up with the x86_64 jdk. This commit updates selectArch() to respect the env var * actually set the ARCH env var * WIP: downlaod all JDK for a specific ARCH * fix container builds and remove dead code * Add exhaustive tests for linux (deb) aarch64 artifacts This commit extends the aarch64 pipeline to build a `.deb` for `aarch64` on an x86 VM then run the exhaustive tests on `aarch64` VM with the `.deb`. This replicates how artifacts are prepared in DRA on x86. This is net new in that it adds exhaustive tests for an `aarch` platform. This would have caught a recent packaging bug. More thourough tests will be added in the future, this is just a first step to get toward that goal. actually build arm (cherry picked from commit 7f1ca8e) # Conflicts: # build.gradle
* Respect ARCH env var when downloading JDK via gradle With the refactor of gradle to be the root of every orchestration task https://github.com/elastic/logstash/pull/18471/changes the artifactAll task depends on copyJdk, which calls downloadJdk. downloadJdk uses selectArch() to determine which JDK to download which results in the ARCH env var to be ignored. Given we build aarch64 packages on x86_64 in CI we always end up with the x86_64 jdk. This commit updates selectArch() to respect the env var * actually set the ARCH env var * WIP: downlaod all JDK for a specific ARCH * fix container builds and remove dead code * Add exhaustive tests for linux (deb) aarch64 artifacts This commit extends the aarch64 pipeline to build a `.deb` for `aarch64` on an x86 VM then run the exhaustive tests on `aarch64` VM with the `.deb`. This replicates how artifacts are prepared in DRA on x86. This is net new in that it adds exhaustive tests for an `aarch` platform. This would have caught a recent packaging bug. More thourough tests will be added in the future, this is just a first step to get toward that goal. actually build arm (cherry picked from commit 7f1ca8e)
* Respect ARCH env var when downloading JDK via gradle With the refactor of gradle to be the root of every orchestration task https://github.com/elastic/logstash/pull/18471/changes the artifactAll task depends on copyJdk, which calls downloadJdk. downloadJdk uses selectArch() to determine which JDK to download which results in the ARCH env var to be ignored. Given we build aarch64 packages on x86_64 in CI we always end up with the x86_64 jdk. This commit updates selectArch() to respect the env var * actually set the ARCH env var * WIP: downlaod all JDK for a specific ARCH * fix container builds and remove dead code * Add exhaustive tests for linux (deb) aarch64 artifacts This commit extends the aarch64 pipeline to build a `.deb` for `aarch64` on an x86 VM then run the exhaustive tests on `aarch64` VM with the `.deb`. This replicates how artifacts are prepared in DRA on x86. This is net new in that it adds exhaustive tests for an `aarch` platform. This would have caught a recent packaging bug. More thourough tests will be added in the future, this is just a first step to get toward that goal. actually build arm (cherry picked from commit 7f1ca8e)
* Respect ARCH env var when downloading JDK via gradle With the refactor of gradle to be the root of every orchestration task https://github.com/elastic/logstash/pull/18471/changes the artifactAll task depends on copyJdk, which calls downloadJdk. downloadJdk uses selectArch() to determine which JDK to download which results in the ARCH env var to be ignored. Given we build aarch64 packages on x86_64 in CI we always end up with the x86_64 jdk. This commit updates selectArch() to respect the env var * actually set the ARCH env var * WIP: downlaod all JDK for a specific ARCH * fix container builds and remove dead code * Add exhaustive tests for linux (deb) aarch64 artifacts This commit extends the aarch64 pipeline to build a `.deb` for `aarch64` on an x86 VM then run the exhaustive tests on `aarch64` VM with the `.deb`. This replicates how artifacts are prepared in DRA on x86. This is net new in that it adds exhaustive tests for an `aarch` platform. This would have caught a recent packaging bug. More thourough tests will be added in the future, this is just a first step to get toward that goal. actually build arm (cherry picked from commit 7f1ca8e) Co-authored-by: Cas Donoghue <cas.donoghue@gmail.com>
* Respect ARCH env var when downloading JDK via gradle With the refactor of gradle to be the root of every orchestration task https://github.com/elastic/logstash/pull/18471/changes the artifactAll task depends on copyJdk, which calls downloadJdk. downloadJdk uses selectArch() to determine which JDK to download which results in the ARCH env var to be ignored. Given we build aarch64 packages on x86_64 in CI we always end up with the x86_64 jdk. This commit updates selectArch() to respect the env var * actually set the ARCH env var * WIP: downlaod all JDK for a specific ARCH * fix container builds and remove dead code * Add exhaustive tests for linux (deb) aarch64 artifacts This commit extends the aarch64 pipeline to build a `.deb` for `aarch64` on an x86 VM then run the exhaustive tests on `aarch64` VM with the `.deb`. This replicates how artifacts are prepared in DRA on x86. This is net new in that it adds exhaustive tests for an `aarch` platform. This would have caught a recent packaging bug. More thourough tests will be added in the future, this is just a first step to get toward that goal. actually build arm (cherry picked from commit 7f1ca8e) Co-authored-by: Cas Donoghue <cas.donoghue@gmail.com>
…a gradle (#18748) * Respect ARCH env var when downloading JDK via gradle (#18733) * Respect ARCH env var when downloading JDK via gradle With the refactor of gradle to be the root of every orchestration task https://github.com/elastic/logstash/pull/18471/changes the artifactAll task depends on copyJdk, which calls downloadJdk. downloadJdk uses selectArch() to determine which JDK to download which results in the ARCH env var to be ignored. Given we build aarch64 packages on x86_64 in CI we always end up with the x86_64 jdk. This commit updates selectArch() to respect the env var * actually set the ARCH env var * WIP: downlaod all JDK for a specific ARCH * fix container builds and remove dead code * Add exhaustive tests for linux (deb) aarch64 artifacts This commit extends the aarch64 pipeline to build a `.deb` for `aarch64` on an x86 VM then run the exhaustive tests on `aarch64` VM with the `.deb`. This replicates how artifacts are prepared in DRA on x86. This is net new in that it adds exhaustive tests for an `aarch` platform. This would have caught a recent packaging bug. More thourough tests will be added in the future, this is just a first step to get toward that goal. actually build arm (cherry picked from commit 7f1ca8e) # Conflicts: # build.gradle * fix merge conflicts --------- Co-authored-by: Cas Donoghue <cas.donoghue@gmail.com>
4 participants
Release notes
Ship the correct bundled JDK for all logstash artifacts.
What does this PR do?
With the refactor to make gradle the entry point to every CI task #18471 (and to not have any cyclical dependencies) the
copyJdktask ended up satisfying the dependency for all tasks for which it was needed. Previously, rake would call back in to gradle and continuously download and delete JDKs. This resulted in a bug where a single JDK was shipped for ALL artifacts.In practice, CI for building artifacts is split across:
The combination of selecting the architecture and operating system were overly complex and made assumptions about the host os/arch.
New approach
Now we just download all the JDKs for a specific arch up front and then package the correct one based on the target artifact (NOT the host). The
copyAllJdkstask usesgetTargetPlatforms()which reads the ARCH environment variable:ARCH=x86_64downloads linux-x64, windows-x64, darwin-x64 JDKsARCH=aarch64downloads linux-aarch64, darwin-aarch64 JDKsEach platform gets its own download task (downloadJdk_linux_aarch64) and output folder (jdk-linux-aarch64/). The rake packaging scripts select the correct JDK folder based on the artifact being built.
Why is it important/What is the impact to the user?
Arm/aarch64/windows/mac users can now run logstash installed from
.rpm,.tar,.debartifacts.Validation steps:
I checked out a VM we use as a builder and build artifacts for all platforms for both architecture sets:
The following script is used to check JDKs
For x86_64
ARCH=x86_64 ./gradlew artifactAll:The build dir contains
The validate script results:
For arm
ARCH=aarch64 ./gradlew artifactAll:The build dir contains
The validate results:
Similarly I build container artifacts (which we have better test coverage for) and validated those worked for arm (we have a ton of test coverage in CI for x86).
Closes #18728