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.

Sign up for GitHub

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

Jump to bottom

Improved build caching for s2i local build #2581

Merged
merged 3 commits into from
Nov 22, 2024
Merged

Improved build caching for s2i local build #2581

merged 3 commits into from
Nov 22, 2024

Conversation

matejvasek
Copy link
Contributor

@matejvasek matejvasek commented Nov 19, 2024 •
edited
Loading

Changes

  • 🎁 Improve caching for local s2i builds, by employing RUN --mount=type=cache....

/kind enhancement

improved caching for local s2i builds

@knative-prow Knative Prow
Copy link

knative-prow bot commented Nov 19, 2024

@matejvasek: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

In response to this:

Changes

/kind

Fixes #

Release Note

Docs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 19, 2024
@matejvasek matejvasek changed the title Enable build caching for s2i local build Improved build caching for s2i local build Nov 19, 2024
@matejvasek matejvasek force-pushed the s2i-caching branch 2 times, most recently from f5d96a7 to 9492f2e Compare November 19, 2024 19:42
@matejvasek
Copy link
Contributor Author

PTAL @lkingland @gauron99 @rhuss

@matejvasek
Improve build caching for s2i local build
6577ffe 
Use 'RUN --mount=type=cache...' to cache build artifacts,
e.g. the local maven repostory.

Signed-off-by: Matej Vašek <[email protected]>
@codecov Codecov
Copy link

codecov bot commented Nov 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.

Project coverage is 65.40%. Comparing base (2fd4982) to head (de99b45).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pkg/builders/s2i/builder.go 68.42% 4 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2581      +/-   ##
==========================================
+ Coverage   64.12%   65.40%   +1.28%     
==========================================
  Files         130      130              
  Lines       15494    15513      +19     
==========================================
+ Hits         9935    10147     +212     
+ Misses       4621     4389     -232     
- Partials      938      977      +39
Flag Coverage Δ
e2e-test 35.83% <0.00%> (-0.05%) ⬇️
e2e-test-oncluster 32.87% <0.00%> (-0.02%) ⬇️
e2e-test-oncluster-runtime 28.65% <0.00%> (?)
e2e-test-runtime-go 26.45% <68.42%> (?)
e2e-test-runtime-node 25.83% <68.42%> (?)
e2e-test-runtime-python 25.86% <68.42%> (?)
e2e-test-runtime-quarkus 26.01% <68.42%> (?)
e2e-test-runtime-rust 24.96% <0.00%> (?)
e2e-test-runtime-springboot 25.00% <0.00%> (?)
e2e-test-runtime-typescript 25.97% <68.42%> (?)
integration-tests 51.94% <68.42%> (+2.28%) ⬆️
unit-tests 50.91% <68.42%> (?)
unit-tests-macos-latest ?
unit-tests-ubuntu-latest ?
unit-tests-windows-latest ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

lkingland
lkingland approved these changes Nov 20, 2024
View reviewed changes
Copy link
Member

@lkingland lkingland left a comment

Choose a reason for hiding this comment

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

Very nice 👍

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024
@knative-prow Knative Prow
Copy link

knative-prow bot commented Nov 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, matejvasek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [lkingland,matejvasek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@matejvasek
Copy link
Contributor Author

@aslom would you try this out? I wonder if it works with docker on mac.

@rhuss
Copy link
Contributor

rhuss commented Nov 20, 2024

Nice finde about RUN --mount ... (didn't know about that). Does it also work with Podman ?

@matejvasek
Copy link
Contributor Author

Nice finde about RUN --mount ... (didn't know about that). Does it also work with Podman ?

@rhuss
I works at least in podman +v4.0, it might not work on older versions.
It works on Docker only if BuildKit is enabled.

@rhuss
Copy link
Contributor

rhuss commented Nov 20, 2024

Looks good to me, however I wonder whether we should hardcode the cache directory to /tmp/artefacts.

  • From the outside there is not indication that it belongs to func.
  • It's shared across multiple function projects, which might or might not be desired
  • Different OSs have different standard location for caches, like on MacOs its ~/Library/Caches. We should respect this.
  • It should be configurable
  • /tmp is often a memory filesystem, so it might be too small for caching large dependencies.

@matejvasek
Copy link
Contributor Author

The /tmp/artefacts dir is s2i specific, I did not made that up.

@matejvasek
Copy link
Contributor Author

matejvasek commented Nov 20, 2024 •
edited
Loading

It's shared across multiple function projects, which might or might not be desired

It is not shared. I am setting cache id to a hash of function path on FS, conflict is unlikely.
https://github.com/knative/func/pull/2581/files#diff-dfe95ab85aa13b847d1b2d57787a6caa4046e108ee77679f58a29ef325121f8bR375-R376

@matejvasek
Copy link
Contributor Author

matejvasek commented Nov 20, 2024 •
edited
Loading

To be clear /tmp/artefacts is path in container not in host. Where it is backed on host FS is handled by daemon (Moby/Podman).

@matejvasek
Copy link
Contributor Author

@rhuss ^^^

@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 21, 2024
@matejvasek
Copy link
Contributor Author

PTAL @gauron99 @lkingland @rhuss

@matejvasek matejvasek requested review from gauron99 and lkingland and removed request for vyasgun and jrangelramos November 21, 2024 15:02
@aslom
Copy link
Member

aslom commented Nov 21, 2024

@matejvasek where I can download func binary build for this PR?

@matejvasek
Copy link
Contributor Author

@aslom you need to build it yourself, or I could send you a binary.

@aslom
Copy link
Member

aslom commented Nov 21, 2024

I thought one of bots was doing tests and created func binaries?
I see it only runs tests and only keep test results? https://github.com/knative/func/actions/runs/11955247512/job/33327188182?pr=2581 ?

@matejvasek
Copy link
Contributor Author

@aslom func.gpg.gz

@matejvasek
Copy link
Contributor Author

This PR should at least fix java maven dep caching @aslom . It probably want change much for Go. I just wonder whether this would work on your setup.

@matejvasek
Copy link
Contributor Author

matejvasek commented Nov 21, 2024 •
edited
Loading

@aslom it looks like we upload artifacts only on push/merge to main, not for each PR.

@matejvasek
Copy link
Contributor Author

matejvasek commented Nov 21, 2024 •
edited
Loading

@aslom There are also some nightly builds in knative infra, but again they are on main not on every PR.

@matejvasek
Copy link
Contributor Author

plz lgtm @gauron99 @lkingland @rhuss @matzew

@gauron99
Copy link
Contributor

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2024
@knative-prow knative-prow bot merged commit 1dd2e43 into knative:main Nov 22, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhuss rhuss Awaiting requested review from rhuss

@gauron99 gauron99 Awaiting requested review from gauron99

@lkingland lkingland Awaiting requested review from lkingland

Assignees

@gauron99 gauron99

@lkingland lkingland

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matejvasek @rhuss @aslom @gauron99 @lkingland