Skip to content
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

Build: Cache Prometheus binary #3261

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

andy-k-improving
Copy link
Contributor

@andy-k-improving andy-k-improving commented Jan 23, 2025

Description

As per the current design of Gradle test workflow, Gradle task startPrometheus is designed to execute upon every integration test and docTest.
However the current implementation of startPrometheus and stopPrometheus will force and trigger the re-download of Prometheus binary, no matter that is a first run or subsequent run. (~100 Mb)

This result in waste of bandwidth and reduce on developer productivity, as integration test will be executed frequently during the typical development process.

Hence this is a PR to introduce FileExistCheck to only download Prometheus binary when it's absent.

Test plan:

  • To execute multiple integTest and make sure Prometheus File Already Exists printed on console and no additional files being downloaded on subsequent IT test execution.

Related Issues

Resolves #3257

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Collaborator

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we should ensure that the Prometheus download is cleared on a clean call - so that the developers don't need to manually delete this to clean up their state.

integ-test/build.gradle Outdated Show resolved Hide resolved
@acarbonetto
Copy link
Collaborator

@dai-chen @penghuo do you remember the reasoning behind wanting to grab a new version of Prometheus each time?

@penghuo
Copy link
Collaborator

penghuo commented Jan 28, 2025

@dai-chen @penghuo do you remember the reasoning behind wanting to grab a new version of Prometheus each time?

I do not remember.
It is ok to add cache latest Prometheus version for now. revisit it If we are facing issue in future.

penghuo
penghuo previously approved these changes Jan 28, 2025
Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!

YANG-DB
YANG-DB previously approved these changes Jan 28, 2025
@andy-k-improving
Copy link
Contributor Author

Thx both, I will update this to cache the last version only.

Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving andy-k-improving dismissed stale reviews from YANG-DB and penghuo via e929ac5 January 29, 2025 00:22
@andy-k-improving
Copy link
Contributor Author

andy-k-improving commented Jan 29, 2025

@YANG-DB @acarbonetto @penghuo I have updated the code to only re-use Prometheus binary when it matches the version.
Can you have another look?
Thanks

@@ -306,7 +311,6 @@ task startPrometheus(type: SpawnProcessTask) {
task stopPrometheus(type: KillProcessTask) {
doLast {
file("$projectDir/bin/prometheus").deleteDir()
file("$projectDir/bin/prometheus.tar.gz").delete()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can delete the tar.gz if it exists:

"prometheus-${prometheus_binary_version}.tar.gz"

Copy link
Contributor Author

@andy-k-improving andy-k-improving Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, I'm thinking this differently, as if we keep this delete call with prometheus-versions.gz.
The module wouldn't be able to achieve the cache behaviour, because prometheus-versions.gz will always be absent during the next run of integ-test.startPrometheus( ).

integ-test/build.gradle Outdated Show resolved Hide resolved
Signed-off-by: Andy Kwok <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] Gradle build clean up
4 participants