Support to keep BuildKit state #211
Conversation
ibalajiarun
force-pushed
the
balaji/retain-cache
branch
from
c54303c to
63216ab
Compare
ibalajiarun
force-pushed
the
balaji/retain-cache
branch
2 times, most recently
from
9ce6fa8 to
3e3b65a
Compare
|
Hey @crazy-max , can I get a review on this one? |
|
/cc @jedevc |
There was a problem hiding this comment.
Sorry for the delay. I left some comments.
Also have you tested this change on your side? Keeping the BuildKit state will only work on persistent runners (self-hosted) and not public ones that are ephemeral. If this is your intent then it should be documented to avoid confusion.
ibalajiarun
force-pushed
the
balaji/retain-cache
branch
from
3e3b65a to
ff70196
Compare
|
Thanks for the review @crazy-max. I've addressed your feedback. We have been using this change at aptos-labs/aptos-core for over a month now with persistent self-hosted runners and have seen speed ups of up to 50% in build times. |
|
@crazy-max can we get this merged please? |
src/context.ts
Outdated
| } | ||
|
|
||
| export async function getInputs(): Promise<Inputs> { | ||
| return { | ||
| version: core.getInput('version'), | ||
| name: await getBuilderName(core.getInput('driver') || 'docker-container'), | ||
| name: getBuilderName(core.getInput('name'), core.getInput('driver') || 'docker-container'), |
There was a problem hiding this comment.
name needs to be defined in https://github.com/docker/setup-buildx-action/blob/master/action.yml. Also needs to update README to add this new input.
There was a problem hiding this comment.
Being able to set a custom name would help with reusing the builder's cache across CI runs (assuming e.g. /var/lib/docker can be restored across runs). Would be even better if it could also check for an env var so that CI platforms (e.g. RunsOn) can automatically set it without user intervention.
|
Any movement on this? |
|
just stumpled upon this, the what is needed to get any movement on this? |
|
@ibalajiarun @crazy-max can we take another look at this? Would be great to have. |
|
This PR might be the missing piece of my ultimate caching pipeline quest. Any chance it will be merged in the near future? |
|
@ibalajiarun @crazy-max is there something still open? happy to contribute |
|
This is a much needed feature: without being able to assign a stable builder name (PR probably needs to be amended so that the name is only used for docker-container driver), we can't easily reuse a previous cache. So being able to set the The PR as it is works very well even on ephemeral runners, when you are able to make block-level snapshots (and restore) e.g. |
ibalajiarun
force-pushed
the
balaji/retain-cache
branch
2 times, most recently
from
ae9b987 to
c94c853
Compare
src/context.ts
Outdated
| }; | ||
| } | ||
|
|
||
| export async function getBuilderName(driver: string): Promise<string> { | ||
| export async function getBuilderName(name: string, driver: string): Promise<string> { | ||
| if (name) { |
There was a problem hiding this comment.
@ibalajiarun I suppose (?) a custom name doesn't make sense if the driver is docker, so maybe do it like this?
| if (name) { | |
| if (driver == 'docker') { | |
| return await Docker.context() | |
| } | |
| return name || `builder-${crypto.randomUUID()}`; |
Signed-off-by: Balaji Arun <[email protected]>
ibalajiarun
force-pushed
the
balaji/retain-cache
branch
from
c94c853 to
703e959
Compare
This PR adds support to keep the cache state when the builder is removed in the post-action step. For certain usecases such as CI builds, it is helpful to keep the cache and reuse it with re-created builders. For this purpose, there is a new
keep-stateoptional actions flag that defaults tofalse.To support cache reuse, this PR also allows specifying a custom name to the builder, since cache reuse is no possible with the exiting random names generated using
uuid.v4(). For this purpose, there is a newnameoptional actions flag.