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

build: add shm-size support #790

Merged
merged 2 commits into from Oct 20, 2021
Merged

build: add shm-size support #790

merged 2 commits into from Oct 20, 2021

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Oct 1, 2021

Follow-up moby/buildkit#2384
Fixes #418

vendor: moby/buildkit@ba673bb...8778943

# Dockerfile
FROM busybox
RUN df -kh /dev/shm
$ docker buildx build --no-cache --shm-size 128m .
#6 [2/2] RUN df -kh /dev/shm
#6 0.170 Filesystem                Size      Used Available Use% Mounted on
#6 0.170 shm                     128.0M         0    128.0M   0% /dev/shm
#6 DONE 0.2s

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

@crazy-max crazy-max marked this pull request as ready for review October 1, 2021 23:29
@tonistiigi
Copy link
Member

I'm not sure about adding this to bake atm. I think we need to think through what to do with this and similar fields like ulimit, memory limits etc and if they should be grouped together under one field in bake. Network=host isn't currently in bake either.

@tonistiigi
Copy link
Member

I'm not sure about adding this to bake atm. I think we need to think through what to do with this and similar fields like ulimit, memory limits etc and if they should be grouped together under one field in bake. Network=host isn't currently in bake either

Also, in a lot of cases, these fields make more sense in a dockerfile, per command.

@crazy-max
Copy link
Member Author

I'm not sure about adding this to bake atm. I think we need to think through what to do with this and similar fields like ulimit, memory limits etc and if they should be grouped together under one field in bake. Network=host isn't currently in bake either.

Yes we can do that in a follow-up for bake sure. Maybe a resources field object in the HCL definition would be ok in this case.

Also, in a lot of cases, these fields make more sense in a dockerfile, per command.

While working on the gateway impl for shm-size, I had started to add RUN --shm=size=128m but it might require some changes related to what you said about resources.

@thaJeztah
Copy link
Member

Also, in a lot of cases, these fields make more sense in a dockerfile, per command.

Yes, was slightly wondering this as well. Perhaps we should look when/why it was added in the classic builder, and what the exact use-case was.

While working on the gateway impl for shm-size, I had started to add RUN --shm=size=128m but it might require some changes related to what you said about resources.

In the end, shm is effectively just a tmpfs. Wondering if adding a size option to --mount would be an alternative (--mount=type=tmpfs.target=/dev/shm,size=xxx)
I recall there were similar discussions about docker service not having a --shm-size option.

@tonistiigi
Copy link
Member

tonistiigi commented Oct 4, 2021

@thaJeztah That's a good point

@crazy-max I wonder if we should revert the llb part in moby/buildkit#2384 and instead add size. They are different in containerd but makes little sense to actually keep them separate here. In runtime specs it is handled as a regular mount as well.

@crazy-max
Copy link
Member Author

I wonder if we should revert the llb part in moby/buildkit#2384 and instead add size.

@tonistiigi Yeah would make sense to change that in llb but keep the frontend attr specific to shm-size for the whole build right?

@tonistiigi
Copy link
Member

but keep the frontend attr specific to shm-size for the whole build right?

yes

@crazy-max
Copy link
Member Author

@tonistiigi Bake impl has been removed.

@thaJeztah
Copy link
Member

(relates to moby/moby#40379)

@crazy-max

This comment has been minimized.

build/build.go Outdated Show resolved Hide resolved
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max merged commit 084b6c0 into docker:master Oct 20, 2021
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.

--shm-size in docker build for Mac is not accepting the values indicated
3 participants