-
Notifications
You must be signed in to change notification settings - Fork 195
Support to keep BuildKit state #211
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
base: master
Are you sure you want to change the base?
Conversation
c54303c
to
63216ab
Compare
9ce6fa8
to
3e3b65a
Compare
Hey @crazy-max , can I get a review on this one? |
/cc @jedevc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry got pending reviews I forgot to submit 😣
It would be better if we have another PR for the name
input being added.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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()}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Updated.
Signed-off-by: Balaji Arun <[email protected]>
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-state
optional 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 newname
optional actions flag.