Cache binaries downloaded for packaging locally #9133

swiatekm · 2025-07-25T17:28:34Z

What does this PR do?

It makes it possible to avoid re-downloading binaries on each mage package invocation. It does this by:

Introducing a KEEP_ARCHIVE environment variable, which, when set, prevents deletion of the binary archive in the drop path: 0e63536
Making the binary download code send a If-Modified-Since header based on the modification time of the existing file. If this returns a Not Modified code, we skip the download: a7ec517

The environment variable is introduced to avoid potentially breaking the CI and unified builds. We need to more carefully verify that it won't before enabling it by default.

Why is it important?

Redownloading binaries wastes time and makes agent difficult to work with on slow internet connections.

Checklist

I have read and understood the pull request guidelines of this project.
My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~[ ] 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 ./changelog/fragments using the changelog tool~~
~~[ ] I have added an integration test or an E2E test~~

How to test this PR locally

Run EXTERNAL=true SNAPSHOT=true DEV=true KEEP_ARCHIVE=true mage package twice. The second time should be faster.

Related issues

Relates https://github.com/elastic/observability-dev/issues/4702

elasticmachine · 2025-07-27T12:33:11Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

pkoutsovasilis

I did look at the code and the endgoal makes total sense to me and it is welcome improvement. There is only this part that we rely on the server telling us if something has changed or not (by using "If-Modified-Since" http header) that I am not sure how I feel about it. By leveraging the latter we kinda "expose" our packaging to any bugs of the artifacts server (has caused us issues in the past). Maybe a more controlled by us solution would be to download first the sha512 file compare it with what we have locally and if different download. wdyt?

README.md

magefile.go

swiatekm · 2025-07-31T09:05:56Z

I did look at the code and the endgoal makes total sense to me and it is welcome improvement. There is only this part that we rely on the server telling us if something has changed or not (by using "If-Modified-Since" http header) that I am not sure how I feel about it. By leveraging the latter we kinda "expose" our packaging to any bugs of the artifacts server (has caused us issues in the past). Maybe a more controlled by us solution would be to download first the sha512 file compare it with what we have locally and if different download. wdyt?

For what it's worth, I think our artifacts API just passes that header to the underlying object storage, which should have a correct implementation. I originally thought we always check the downloaded file against the hash, but it seems like we might not? If we did, that would solve the problem, because even if the artifacts api was wrong, we'd get a hash mismatch and at least see the problem.

For the record, I think actual packaging steps in the CI should always redownload the artifacts, ideally by calling mage clean.

pkoutsovasilis · 2025-07-31T09:19:34Z

For what it's worth, I think our artifacts API just passes that header to the underlying object storage, which should have a correct implementation. I originally thought we always check the downloaded file against the hash, but it seems like we might not?

and for this exact reason of having to speculate when we rely on an external server, I would prefer utilising the sha512 files. (PS: if what you say is true and the underlying storage relies on the modified time of the file, is not the most reliable thing out there). Also, as you do mention above, you would solve another problem we currently have of not validating the components we download by hash, win win scenario 🙂

swiatekm · 2025-07-31T09:32:27Z

For what it's worth, I think our artifacts API just passes that header to the underlying object storage, which should have a correct implementation. I originally thought we always check the downloaded file against the hash, but it seems like we might not?

and for this exact reason of having to speculate when we rely on an external server, I would prefer utilising the sha512 files. (PS: if what you say is true and the underlying storage relies on the modified time of the file, is not the most reliable thing out there). Also, as you do mention above, you would solve another problem we currently have of not validating the components we download by hash, win win scenario 🙂

I would consider If-Changed-Since headers on say, S3 or GCS, to be reliable. Surely there's tons of code out there that relies on this working correctly. But the integrity check is useful regardless, because the mod time on the client isn't necessarily reliable, as many processes can easily mess with it, even by accident.

In the meantime, how about I submit a PR with just the download refactor? It has benefits on its own, like not passing the content of downloaded files as strings. 😅

elastic-sonarqube · 2025-07-31T10:03:58Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

michalpristas · 2025-08-27T12:19:27Z

i suspect this is primarily for local usage.
also i would prefer to have other option as well meaning cleanup before build in case i had last build with KEEP and i changed my mind

swiatekm · 2025-08-28T18:58:21Z

i suspect this is primarily for local usage. also i would prefer to have other option as well meaning cleanup before build in case i had last build with KEEP and i changed my mind

Isn't mage clean good enough for that? I'd prefer not to add even more environment variables. We already have a ton.

swiatekm · 2025-09-09T16:19:40Z

I've rebased this change on #9218, making it much simpler. With #9226, we always verify checksums, so we'll immediately know if the artifacts API served us a stale download, or if the file is corrupted for whatever reason. So this change should be quite safe now. It's only really intended for local use, although we can later look into keeping the downloaded binaries on CI runners, if we want.

@pkoutsovasilis @michalpristas can you review again? 🙏

pkoutsovasilis · 2025-09-10T12:34:09Z

@swiatekm please merge the latest main in here; the failure you see is because I merged this one #9832 and I managed to hit your CI run just right in the middle; so the clean up step run with the latest code in main but the pipeline was loaded before the merge so any changes are lost.

PS: we are working on a solution to get away from such failures

Co-authored-by: Panos Koutsovasilis <[email protected]>

elastic-sonarqube · 2025-09-10T14:49:08Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

elasticmachine · 2025-09-10T15:06:58Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: cb04629

Failed CI Steps

arm:sudo: default

History

💔 Build #26515 failed 9a9e956
💔 Build #26472 failed 4680a90
💛 Build #24654 was flaky 1dfd0c7
💔 Build #24426 failed d25fea1
💔 Build #24425 failed e0734b2

cc @swiatekm

github-actions · 2025-09-12T10:59:19Z

@Mergifyio backport 8.18 8.19 9.0 9.1

mergify · 2025-09-12T10:59:27Z

backport 8.18 8.19 9.0 9.1

✅ Backports have been created

#9906 [8.18] (backport #9133) Cache binaries downloaded for packaging locally has been created for branch 8.18
#9907 [8.19] (backport #9133) Cache binaries downloaded for packaging locally has been created for branch 8.19
#9908 [9.0] (backport #9133) Cache binaries downloaded for packaging locally has been created for branch 9.0
#9909 [9.1] (backport #9133) Cache binaries downloaded for packaging locally has been created for branch 9.1

* Add an env variable to keep the download archive * Only download artifacts which aren't present * Document the KEEP_ARCHIVE env variable * Update README.md Co-authored-by: Panos Koutsovasilis <[email protected]> * fixup! Only download artifacts which aren't present --------- Co-authored-by: Panos Koutsovasilis <[email protected]> (cherry picked from commit 6aed8b1)

* Add an env variable to keep the download archive * Only download artifacts which aren't present * Document the KEEP_ARCHIVE env variable * Update README.md * fixup! Only download artifacts which aren't present --------- (cherry picked from commit 6aed8b1) Co-authored-by: Mikołaj Świątek <[email protected]> Co-authored-by: Panos Koutsovasilis <[email protected]>

* upstream: (26 commits) fix: ensure EDOT subprocess shuts down gracefully on agent termination (#9886) [main][Automation] Update versions (#9976) Add Collector reference docs and automation (#9953) [beatreceivers] Integrate beatsauthextension (#9257) [main][Automation] Update versions (#9941) Update OTel components to v0.132.0/v1.38.0 (#9954) Enhancement/5235 wrap errors when marking upgrade (#9366) Mount Go build cache into crossbuild container (#9094) Liveness agent state (#9673) [main][Automation] Bump VM Image version to 1757725254 (#9942) Enhancement/5235 correctly wrap errors from copyActionDir and copyRunDirectory (#9349) [main][Automation] Update elastic/beats to afc53c0479ac (#9874) Add -coverpkg option when running unit test to calculate coverage across packages (#9913) Cache binaries downloaded for packaging locally (#9133) [main][Automation] Update versions (#9897) Disable flaky test TestBeatsReceiverLogs (#9891) Allow overriding AGENT_PACKAGE_VERSION and MANIFEST_URL when USE_PACKAGE_VERSION=true (#9864) add ingest-docs team as CODEOWNERS for release notes and docset.yml (#9865) fix: correct spelling of 'output' in various templates and monitoring code (#9827) k8s: Add comment around hostUsers for Universal Profiling deployments (#9847) ...

* Add an env variable to keep the download archive * Only download artifacts which aren't present * Document the KEEP_ARCHIVE env variable * Update README.md Co-authored-by: Panos Koutsovasilis <[email protected]> * fixup! Only download artifacts which aren't present --------- Co-authored-by: Panos Koutsovasilis <[email protected]>

swiatekm added skip-changelog chore Tasks that just need to be done, they are neither bug, nor enhancements backport-active-all Automated backport with mergify to all the active branches labels Jul 25, 2025

mergify bot assigned swiatekm Jul 25, 2025

swiatekm marked this pull request as ready for review July 26, 2025 00:00

swiatekm requested a review from a team as a code owner July 26, 2025 00:00

swiatekm requested review from michalpristas and straistaru July 26, 2025 00:00

pierrehilbert added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Jul 27, 2025

swiatekm force-pushed the chore/cache-external-binaries branch from be4df5e to fecc75c Compare July 28, 2025 12:56

pkoutsovasilis reviewed Jul 31, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

magefile.go Show resolved Hide resolved

swiatekm mentioned this pull request Jul 31, 2025

Refactor binary downloads in agent packaging targets #9218

Merged

3 tasks

swiatekm force-pushed the chore/cache-external-binaries branch from 1dfd0c7 to f0263b6 Compare September 9, 2025 15:03

swiatekm requested a review from pkoutsovasilis September 9, 2025 16:19

pkoutsovasilis approved these changes Sep 9, 2025

View reviewed changes

swiatekm force-pushed the chore/cache-external-binaries branch from 4680a90 to 9a9e956 Compare September 10, 2025 11:19

Add an env variable to keep the download archive

d37d790

swiatekm and others added 4 commits September 10, 2025 15:00

Only download artifacts which aren't present

e3db457

Document the KEEP_ARCHIVE env variable

6a9f59f

Update README.md

f9f3a1d

Co-authored-by: Panos Koutsovasilis <[email protected]>

fixup! Only download artifacts which aren't present

cb04629

swiatekm force-pushed the chore/cache-external-binaries branch from 9a9e956 to cb04629 Compare September 10, 2025 13:00

pkoutsovasilis approved these changes Sep 10, 2025

View reviewed changes

michalpristas approved these changes Sep 12, 2025

View reviewed changes

swiatekm merged commit 6aed8b1 into main Sep 12, 2025
23 checks passed

swiatekm deleted the chore/cache-external-binaries branch September 12, 2025 10:59

This was referenced Sep 12, 2025

[8.18] (backport #9133) Cache binaries downloaded for packaging locally #9906

Merged

[8.19] (backport #9133) Cache binaries downloaded for packaging locally #9907

Merged

mergify bot mentioned this pull request Sep 12, 2025

[9.0] (backport #9133) Cache binaries downloaded for packaging locally #9908

Merged

3 tasks

mergify bot mentioned this pull request Sep 12, 2025

[9.1] (backport #9133) Cache binaries downloaded for packaging locally #9909

Merged

3 tasks

Cache binaries downloaded for packaging locally #9133

Cache binaries downloaded for packaging locally #9133

Uh oh!

Conversation

swiatekm commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

Uh oh!

elasticmachine commented Jul 27, 2025

Uh oh!

pkoutsovasilis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

swiatekm commented Jul 31, 2025

Uh oh!

pkoutsovasilis commented Jul 31, 2025

Uh oh!

swiatekm commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elastic-sonarqube bot commented Jul 31, 2025

Quality Gate passed

Uh oh!

michalpristas commented Aug 27, 2025

Uh oh!

swiatekm commented Aug 28, 2025

Uh oh!

swiatekm commented Sep 9, 2025

Uh oh!

pkoutsovasilis commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elastic-sonarqube bot commented Sep 10, 2025

Quality Gate passed

Uh oh!

elasticmachine commented Sep 10, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

History

Uh oh!

Uh oh!

github-actions bot commented Sep 12, 2025

Uh oh!

mergify bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Uh oh!

swiatekm commented Jul 25, 2025 •

edited

Loading

swiatekm commented Jul 31, 2025 •

edited

Loading

pkoutsovasilis commented Sep 10, 2025 •

edited

Loading

mergify bot commented Sep 12, 2025 •

edited

Loading