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

Keep BuildKit state in a volume #672

Merged
merged 1 commit into from
Jul 13, 2021

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Jul 13, 2021

Retain BuildKit state inside a named volume if the container is removed for the docker-container driver. --volume flag has also been added to the rm command to be able to force volume removal.

Signed-off-by: CrazyMax [email protected]

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.

But where is the volume created on create?

I wonder if for backward compatibility we should remove volume by default. Or print a warning that volume is still there and data was not released. Maybe there is a good opt-in flag that is opposite of --rm. Most of the time I think users want to remove volume. @thaJeztah

I guess we could revisit the deletion path once upgrade command is added in the future. I guess then rm always deletes volume(or at least by default). And upgrade performs rm+create without touching the volume.

commands/rm.go Outdated Show resolved Hide resolved
@crazy-max
Copy link
Member Author

@tonistiigi

Most of the time I think users want to remove volume.

Agree, just wanted to keep the same UX as other commands of the Docker CLI but maybe it's not legitimate in this case.

@crazy-max crazy-max requested a review from thaJeztah July 13, 2021 09:26
@crazy-max crazy-max force-pushed the keep-buildkit-state branch 2 times, most recently from da26577 to c847455 Compare July 13, 2021 14:15
@thaJeztah
Copy link
Member

Retain BuildKit state inside a named volume if the container is removed for the docker-container driver. --volume flag has also been added to the rm command to be able to force volume removal.

Haven't looked closely at the changes, but be aware that --volume only removes anonymous (unnamed) volumes, and won't remove "named" volumes; from docker rm --help:

-v, --volumes   Remove anonymous volumes associated with the container

@crazy-max
Copy link
Member Author

@thaJeztah

--volume only removes anonymous (unnamed) volumes

In our case it's not so yes that's confusing.

I have changed the behavior to be backward compatible with an opt-in flag called --keep-state instead.

@crazy-max crazy-max marked this pull request as ready for review July 13, 2021 15:01
docs/reference/buildx_rm.md Outdated Show resolved Hide resolved
driver/docker-container/driver.go Outdated Show resolved Hide resolved
commands/rm.go Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member

In our case it's not so yes that's confusing.

Reason for that is that anonymous volumes are commonly used as temporary / ephemeral storage. docker rm -v <some container> is somewhat the equivalent of docker run --rm <some image with a VOLUME>. When naming a volume, in most cases that's for a reason (being able to reference the volume later), and we don't want to inadvertently remove my_important_database_volume.

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.

3 participants