-
Notifications
You must be signed in to change notification settings - Fork 18
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
Allow to bind tmpfs to a custom path bound to each run #62
base: master
Are you sure you want to change the base?
Conversation
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.
Looks good. I suggest extending the Dockerfile unit-test to check the output, however:
https://github.com/ocurrent/obuilder/blob/master/test/test.ml#L462
Fmt.pf f "--mount=type=tmpfs,target=%s" target | ||
|
||
let pp_run ~ctx f { Spec.cache; shell; network = _; tmpfs } = | ||
Fmt.pf f "RUN %a%a%a" Fmt.(list (pp_cache ~ctx ++ const string " ")) cache (Fmt.list pp_tmpfs) tmpfs pp_wrap shell |
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.
Not convinced the spacing is correct here. Probably worth updating the unit-tests to check this.
leaving aside the issues in the docker part, I'm experiencing some issues with the base idea. On ppc64, linux by default [1][2] has a PAGESIZE=64k is only a default, so in theory if we're compiling our own kernel it could be done but at this point maybe it's not worth it. I'm not sure if there is any workaround, I couldn't find any but maybe somebody else has an idea. Maybe simply reducing the capacity from 80 by half and reducing the size allocated for tmpfs to 4Go so that there is still some ram left for the actual programs and some cache. [1] https://github.com/torvalds/linux/blob/master/arch/powerpc/configs/ppc64_defconfig#L54 |
Dropping to a 4k page size on the ppc64 worker's shouldnt be a problem |
It still requires us to build our own kernel. Is that something that we want? |
89e84ac
to
ccd3f2f
Compare
Implement the new
(tmpfs ...)
option suggested in ocaml/opam#4586 (comment)