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

docker-container: restart-policy opt #1271

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Aug 14, 2022

Currently we can't set the restart policy for buildkit containers created with docker-container driver. In my use case I have restarted some machines but as the default restart policy is no, the containers were not running. Even if we ensure nodes are booted before building I think it's still useful to have this driver opt.

Current behavior (unless-stopped) is kept.

Needs docs update in follow-up (cc @dvdksn): https://docs.docker.com/build/drivers/docker-container/#synopsis

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@jedevc
Copy link
Collaborator

jedevc commented Aug 16, 2022

LGTM.

Personally, not sure about defaulting to always, I quite like the default lazy behavior with restart-policy=no, since I can sometimes have a lot of builders, which might not all actually be running. But, also, this would then match the kubernetes driver behavior, where the replicas are always running. If we do switch the default, we should default to unless-stopped I think, since buildx stop will stop the container, which then shouldn't automatically restart.

@crazy-max
Copy link
Member Author

If we do switch the default, we should default to unless-stopped I think, since buildx stop will stop the container, which then shouldn't automatically restart.

Good point for unless-stopped, tbf I never used buildx stop and didn't recall it ever exists 😅

@crazy-max
Copy link
Member Author

PTAL @tonistiigi

@AkihiroSuda
Copy link
Collaborator

Needs rebase

@tonistiigi
Copy link
Member

tonistiigi commented Feb 14, 2023

What is the use case for this? In what case would the user need to change this? Running a builder takes resources and memory so if it is not being used it is better if it is not running.

@crazy-max
Copy link
Member Author

crazy-max commented Feb 6, 2024

What is the use case for this? In what case would the user need to change this? Running a builder takes resources and memory so if it is not being used it is better if it is not running.

@tonistiigi Main use case is for the Build UI in Docker Desktop so I can see my build history without needing to start my builders.

Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

I think opt-in this is fine but I would be against making this default behavior.

@crazy-max crazy-max merged commit d0177c6 into docker:master Feb 8, 2024
63 checks passed
@crazy-max crazy-max deleted the ctn-restart branch February 8, 2024 18:32
@crazy-max crazy-max added this to the v0.13.0 milestone Feb 9, 2024
@@ -44,17 +44,18 @@ type Driver struct {

// if you add fields, remember to update docs:
// https://github.com/docker/docs/blob/main/content/build/drivers/docker-container.md
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this PR might've missed updating these docs 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

It's here :-) docker/docs#19355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants