-
Notifications
You must be signed in to change notification settings - Fork 17
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
generalise the sandbox #58
Conversation
lib/runc_sandbox.ml
Outdated
Arg.value @@ | ||
Arg.opt Arg.bool false @@ | ||
Arg.info | ||
~doc:"Install a seccomp filter that skips allsync syscalls" |
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.
~doc:"Install a seccomp filter that skips allsync syscalls" | |
~doc:"Install a seccomp filter that skips all synchronous syscalls" |
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.
But the new text is wrong! The option skips all sync
and related syscalls, not all synchronous syscalls.
lib/s.ml
Outdated
@@ -64,6 +64,34 @@ end | |||
module type SANDBOX = sig | |||
type t | |||
|
|||
val sandbox_type : string |
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.
can this just be a val pp : t Fmt.t
instead? That would let you pretty print the abstract sandbox state, and it could simply prepend the sandbox type there as well.
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.
Sound very reasonable to me, thanks :)) Will also update the doc comment from above shortly
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.
It looks something like:
runc state:
{runc_state_dir: /hello/world
fast_sync: true
arches: [A,B,C]}
Hopefully that's okay, I'm no Format
wizard 🧙 .
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.
This looks like generally the right direction - sorry for all the nit-picking!
lib/s.ml
Outdated
@@ -64,6 +64,34 @@ end | |||
module type SANDBOX = sig | |||
type t | |||
|
|||
val pp : t Fmt.t | |||
(** A pretty-printer for sandbox environments *) |
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.
Is this used for anything? We didn't need this before.
lib/s.ml
Outdated
(** [cmdliner] is used for command-line interfaces to generate the necessary flags | ||
and parameters to setup a specific sandbox's configuration. *) | ||
|
||
val create : ?state_dir:string -> config -> t Lwt.t |
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.
I don't think state_dir
should be optional. We should pass every sandbox a directory it can use to persist stuff. If a sandbox doesn't need it, it can just ignore it.
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.
Also, not sure that config
and create
should be in SANDBOX
. I see this already caused some trouble for the mock sandbox. This could stay in runc_sandbox.mli
for now, and then when there are multiple implementations, we can have this in a shared sandbox.mli
.
lib/s.ml
Outdated
cancelled:unit Lwt.t -> | ||
log:Build_log.t -> | ||
string -> (unit, [ `Cancelled | `Msg of string ]) result Lwt.t | ||
(** [from t ~log ~from_stage] generates the function to be run as the initial build-step |
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.
Needs to say what the final string
argument is, if we keep this API.
main.ml
Outdated
Obuilder.Store_spec.to_store spec >>= fun (Store ((module Store), store)) -> | ||
let module Builder = Obuilder.Builder(Store)(Sandbox) in | ||
Sandbox.create ~runc_state_dir:(Store.state_dir store / "runc") ?fast_sync () >|= fun sandbox -> | ||
Sandbox.create ~state_dir:(Store.state_dir store / "runc") conf >|= fun sandbox -> |
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.
Since this is now generic, we should rename the directory, e.g.
Sandbox.create ~state_dir:(Store.state_dir store / "runc") conf >|= fun sandbox -> | |
Sandbox.create ~state_dir:(Store.state_dir store / "sandbox") conf >|= fun sandbox -> |
lib/runc_sandbox.ml
Outdated
Arg.value @@ | ||
Arg.opt Arg.bool false @@ | ||
Arg.info | ||
~doc:"Install a seccomp filter that skips all synchronous syscalls" |
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.
The previous text was correct:
~doc:"Install a seccomp filter that skips all synchronous syscalls" | |
~doc:"Ignore sync syscalls (requires runc >= 1.0.0-rc92)" |
test/mock_sandbox.ml
Outdated
@@ -9,8 +13,97 @@ type t = { | |||
(unit, [`Msg of string | `Cancelled]) Lwt_result.t) Queue.t; | |||
} | |||
|
|||
type config = { | |||
dir : string; |
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.
If you move the constructors back out of SANDBOX
, this can all go away.
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.
Yep
test/mock_sandbox.ml
Outdated
let export_env base = | ||
Os.pread ["docker"; "image"; "inspect"; | ||
"--format"; {|{{range .Config.Env}}{{print . "\x00"}}{{end}}|}; | ||
"--"; base] >|= fun env -> | ||
String.split_on_char '\x00' env | ||
|> List.filter_map (function | ||
| "\n" -> None | ||
| kv -> | ||
match Astring.String.cut ~sep:"=" kv with | ||
| None -> Fmt.failwith "Invalid environment in Docker image %S (should be 'K=V')" kv | ||
| Some _ as pair -> pair | ||
) | ||
|
||
let copy_to_log ~src ~dst = | ||
let buf = Bytes.create 4096 in | ||
let rec aux () = | ||
Lwt_unix.read src buf 0 (Bytes.length buf) >>= function | ||
| 0 -> Lwt.return_unit | ||
| n -> Build_log.write dst (Bytes.sub_string buf 0 n) >>= aux | ||
in | ||
aux () | ||
|
||
let with_container ~log base fn = | ||
Os.with_pipe_from_child (fun ~r ~w -> | ||
(* We might need to do a pull here, so log the output to show progress. *) | ||
let copy = copy_to_log ~src:r ~dst:log in | ||
Os.pread ~stderr:(`FD_move_safely w) ["docker"; "create"; "--"; base] >>= fun cid -> | ||
copy >|= fun () -> | ||
String.trim cid | ||
) >>= fun cid -> | ||
Lwt.finalize | ||
(fun () -> fn cid) | ||
(fun () -> Os.exec ~stdout:`Dev_null ["docker"; "rm"; "--"; cid]) | ||
|
||
let from ~log ~from _t = | ||
let ( / ) = Filename.concat in | ||
let base = from in | ||
log `Heading (Fmt.strf "(from %a)" Sexplib.Sexp.pp_hum (Atom base)); | ||
(fun ~cancelled:_ ~log tmp -> | ||
Logs.info (fun f -> f "Base image not present; importing %S...@." base); | ||
let rootfs = tmp / "rootfs" in | ||
Os.sudo ["mkdir"; "--mode=755"; "--"; rootfs] >>= fun () -> | ||
(* Lwt_process.exec ("", [| "docker"; "pull"; "--"; base |]) >>= fun _ -> *) | ||
with_container ~log base (fun cid -> | ||
Os.with_pipe_between_children @@ fun ~r ~w -> | ||
let exporter = Os.exec ~stdout:(`FD_move_safely w) ["docker"; "export"; "--"; cid] in | ||
let tar = Os.sudo ~stdin:(`FD_move_safely r) ["tar"; "-C"; rootfs; "-xf"; "-"] in | ||
exporter >>= fun () -> | ||
tar | ||
) >>= fun () -> | ||
export_env base >>= fun env -> | ||
Os.write_file ~path:(tmp / "env") | ||
(Sexplib.Sexp.to_string_hum Saved_context.(sexp_of_t {env})) >>= fun () -> | ||
Lwt_result.return () | ||
) |
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.
I think we can skip all of this. mock_exec
goes to some trouble to fake out all these docker commands, but since the sandbox now does the fetching we can delete all of that. There's no point testing an old copy of the real code.
It might be worth adding a test to check the real code using the existing mock_exec
on Linux, but we also have integration tests on Travis that will test it anyway.
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.
Okay I think I follow -- would it be enough then for the mock_sandbox
to just be:
let from : Obuilder.Runc_sandbox.from
for now?
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 would be an easy solution for now, yes.
Probably the from
stuff should go in a separate module anyway (so we have three concepts: stores, sandboxes, and image sources). It would be quite possible to use docker pull
even on macos, for example.
lib/build.ml
Outdated
let get_base t ~log base = | ||
log `Heading (Fmt.strf "(from %a)" Sexplib.Sexp.pp_hum (Atom base)); | ||
let id = Sha256.to_hex (Sha256.string base) in | ||
Store.build t.store ~id ~log (fun ~cancelled:_ ~log tmp -> | ||
Log.info (fun f -> f "Base image not present; importing %S..." base); | ||
let rootfs = tmp / "rootfs" in | ||
Os.sudo ["mkdir"; "--mode=755"; "--"; rootfs] >>= fun () -> |
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.
We could just keep this bit of get_base
, I think, instead of making every sandbox duplicate it.
lib/build.ml
Outdated
Os.write_file ~path:(tmp / "env") | ||
(Sexplib.Sexp.to_string_hum Saved_context.(sexp_of_t {env})) >>= fun () -> | ||
Lwt_result.return () | ||
) | ||
>>!= fun id -> | ||
let path = Option.get (Store.result t.store id) in | ||
let { Saved_context.env } = Saved_context.t_of_sexp (Sexplib.Sexp.load_sexp (path / "env")) in | ||
Lwt_result.return (id, env) |
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.
... and keep this bit too.
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.
I think I agree. Certainly the logging of what is going on can stay, so perhaps get_base
can just be passed the Sandbox.from
function to be passed into Store.build
, the only issue I see with that is that we would require Sandbox.from
to persist the environment in a specific location.
Or should Sandbox.from
be changed to something like:
val from :
log:Build_log.t -> base:string -> string -> (Saved_context.env, [ `Cancelled | `Msg of string ]) result Lwt.t
And get base becomes:
let get_base t ~log base =
log `Heading (Fmt.strf "(from %a)" Sexplib.Sexp.pp_hum (Atom base));
let id = Sha256.to_hex (Sha256.string base) in
Store.build t.store ~id ~log (fun ~cancelled:_ ~log tmp ->
Log.info (fun f -> f "Base image not present; importing %S..." base);
Sandbox.from ~log ~base tmp >>!= fun env ->
Os.write_file ~path:(tmp / "env")
(Sexplib.Sexp.to_string_hum Saved_context.(sexp_of_t {env})) >>= fun () ->
Lwt_result.return ()
)
>>!= fun id ->
let path = Option.get (Store.result t.store id) in
let { Saved_context.env } = Saved_context.t_of_sexp (Sexplib.Sexp.load_sexp (path / "env")) in
Lwt_result.return (id, env)
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.
Yes. You could also create rootfs
here too, and pass that to the fetcher, so it doesn't need to know about the layout.
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.
I think I may have been mistaken about the implementation. Am I right in thinking that the runc/zfs implementation builds inside of /tank/result/<hash>/rootfs/
? So that's where I should build the MacOS one too? Up until now I've just been building it inside of /tank/result/<hash>
, to make it uniform I can use rootfs
which as you said reduces the amount of things the fetcher needs to do/know about?
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.
Yes, that's right. It allows us to store extra data about the build, such as the environment variables and the log, plus anything else we think of in the future.
lib/build.ml
Outdated
@@ -307,7 +259,12 @@ module Make (Raw_store : S.STORE) (Sandbox : S.SANDBOX) = struct | |||
(* Get the base image first, before starting the timer. *) | |||
let switch = Lwt_switch.create () in | |||
let context = Context.v ~switch ~log ~src_dir:"/tmp" () in | |||
get_base t ~log healthcheck_base >>= function |
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.
If you keep get_base
, this part doesn't need to be duplicated.
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.
Thanks for the detailed feedback, have responded to various bits but I think these changes clean the code up quite a bit!
lib/build.ml
Outdated
Os.write_file ~path:(tmp / "env") | ||
(Sexplib.Sexp.to_string_hum Saved_context.(sexp_of_t {env})) >>= fun () -> | ||
Lwt_result.return () | ||
) | ||
>>!= fun id -> | ||
let path = Option.get (Store.result t.store id) in | ||
let { Saved_context.env } = Saved_context.t_of_sexp (Sexplib.Sexp.load_sexp (path / "env")) in | ||
Lwt_result.return (id, env) |
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.
I think I agree. Certainly the logging of what is going on can stay, so perhaps get_base
can just be passed the Sandbox.from
function to be passed into Store.build
, the only issue I see with that is that we would require Sandbox.from
to persist the environment in a specific location.
Or should Sandbox.from
be changed to something like:
val from :
log:Build_log.t -> base:string -> string -> (Saved_context.env, [ `Cancelled | `Msg of string ]) result Lwt.t
And get base becomes:
let get_base t ~log base =
log `Heading (Fmt.strf "(from %a)" Sexplib.Sexp.pp_hum (Atom base));
let id = Sha256.to_hex (Sha256.string base) in
Store.build t.store ~id ~log (fun ~cancelled:_ ~log tmp ->
Log.info (fun f -> f "Base image not present; importing %S..." base);
Sandbox.from ~log ~base tmp >>!= fun env ->
Os.write_file ~path:(tmp / "env")
(Sexplib.Sexp.to_string_hum Saved_context.(sexp_of_t {env})) >>= fun () ->
Lwt_result.return ()
)
>>!= fun id ->
let path = Option.get (Store.result t.store id) in
let { Saved_context.env } = Saved_context.t_of_sexp (Sexplib.Sexp.load_sexp (path / "env")) in
Lwt_result.return (id, env)
lib/runc_sandbox.ml
Outdated
module Saved_context = struct | ||
type t = { | ||
env : Os.env; | ||
} [@@deriving sexp] | ||
end |
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.
Agreed -- see comment above about a proposed get_base
+ from
combination that requires from
to return a Saved_context.env
that get_base
can persist.
test/mock_sandbox.ml
Outdated
@@ -9,8 +13,97 @@ type t = { | |||
(unit, [`Msg of string | `Cancelled]) Lwt_result.t) Queue.t; | |||
} | |||
|
|||
type config = { | |||
dir : string; |
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.
Yep
test/mock_sandbox.ml
Outdated
let export_env base = | ||
Os.pread ["docker"; "image"; "inspect"; | ||
"--format"; {|{{range .Config.Env}}{{print . "\x00"}}{{end}}|}; | ||
"--"; base] >|= fun env -> | ||
String.split_on_char '\x00' env | ||
|> List.filter_map (function | ||
| "\n" -> None | ||
| kv -> | ||
match Astring.String.cut ~sep:"=" kv with | ||
| None -> Fmt.failwith "Invalid environment in Docker image %S (should be 'K=V')" kv | ||
| Some _ as pair -> pair | ||
) | ||
|
||
let copy_to_log ~src ~dst = | ||
let buf = Bytes.create 4096 in | ||
let rec aux () = | ||
Lwt_unix.read src buf 0 (Bytes.length buf) >>= function | ||
| 0 -> Lwt.return_unit | ||
| n -> Build_log.write dst (Bytes.sub_string buf 0 n) >>= aux | ||
in | ||
aux () | ||
|
||
let with_container ~log base fn = | ||
Os.with_pipe_from_child (fun ~r ~w -> | ||
(* We might need to do a pull here, so log the output to show progress. *) | ||
let copy = copy_to_log ~src:r ~dst:log in | ||
Os.pread ~stderr:(`FD_move_safely w) ["docker"; "create"; "--"; base] >>= fun cid -> | ||
copy >|= fun () -> | ||
String.trim cid | ||
) >>= fun cid -> | ||
Lwt.finalize | ||
(fun () -> fn cid) | ||
(fun () -> Os.exec ~stdout:`Dev_null ["docker"; "rm"; "--"; cid]) | ||
|
||
let from ~log ~from _t = | ||
let ( / ) = Filename.concat in | ||
let base = from in | ||
log `Heading (Fmt.strf "(from %a)" Sexplib.Sexp.pp_hum (Atom base)); | ||
(fun ~cancelled:_ ~log tmp -> | ||
Logs.info (fun f -> f "Base image not present; importing %S...@." base); | ||
let rootfs = tmp / "rootfs" in | ||
Os.sudo ["mkdir"; "--mode=755"; "--"; rootfs] >>= fun () -> | ||
(* Lwt_process.exec ("", [| "docker"; "pull"; "--"; base |]) >>= fun _ -> *) | ||
with_container ~log base (fun cid -> | ||
Os.with_pipe_between_children @@ fun ~r ~w -> | ||
let exporter = Os.exec ~stdout:(`FD_move_safely w) ["docker"; "export"; "--"; cid] in | ||
let tar = Os.sudo ~stdin:(`FD_move_safely r) ["tar"; "-C"; rootfs; "-xf"; "-"] in | ||
exporter >>= fun () -> | ||
tar | ||
) >>= fun () -> | ||
export_env base >>= fun env -> | ||
Os.write_file ~path:(tmp / "env") | ||
(Sexplib.Sexp.to_string_hum Saved_context.(sexp_of_t {env})) >>= fun () -> | ||
Lwt_result.return () | ||
) |
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.
Okay I think I follow -- would it be enough then for the mock_sandbox
to just be:
let from : Obuilder.Runc_sandbox.from
for now?
What's happening with this? Is there a new version available to review? I'd like to make some improvements to the docs, but I don't want to create conflicts with this work. |
Hi @talex5 :)) I would say conflict away, this is currently somewhat shelved in favour me testing the idea more (e.g. deploying a little version of opam-health-check with my own modified version of OCluster/OBuilder). Sorry for not mentioning that. |
I've updated the docs, so this needs rebasing now. It would be good to get this in soon, because we're going to need it for the Windows support too. |
Okay 👍 -- will work on it this afternoon |
79a871a
to
f6b45d2
Compare
@talex5 this should be ready for a second review, sorry about the weird formatting errors I had to fix. I still have a few questions though. Re:#58 (comment) do you want this in this PR? I took this implementation and used it with ocluster to see what it would look like and because After re-writing everything to go with your suggestions, this comment #58 (comment) really resonated with me. Do you think there should be a Lastly I haven't passed in |
Thanks, this is good progress :-)
Well, the interface should eventually be in
I think that makes sense, especially as the Windows implementation will be using
It's not a good idea to let the sandboxes/fetchers pick a name themselves because:
|
Nothing much really, but you've left trailing whitespace in the obuilder PR. Passing the |
Yeah sorry about the formatting, without ocamlformat I'm at a loss :)) Thanks for looking through this. I didn't change the
See comment #58 (comment) why it was moved out of there. As @talex5 said would this not work eventually with a |
Thanks for the explanation! |
Would be good to get this merged, as it is now blocking @MisterDA. To summarise the changes:
I think that's everything, apart from some formatting what do you think in terms of next steps for this @talex5? @MisterDA too ? Sorry it's taken so long! |
be83721
to
2ae7425
Compare
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.
Thanks! Looks good to merge once CI is done (I just rebased it and reindented the get_base
function).
CHANGES: - Use GNU tar format instead of UStar for `copy` operations (@TheLortex ocurrent/obuilder#82, reviewed @dra27). This enables copying from sources containing long file names (>100 characters). - Add support for secrets (@TheLortex ocurrent/obuilder#63, reviewed by @talex5). The obuilder spec's `run` command supports a new `secrets` fields, which allows to temporarily mount secret files in an user-specified location. The sandbox build context has an additional `secrets` parameter to provide values for the requested keys. - Limit permissions on temporary directories (@talex5 ocurrent/obuilder#67) - Check Linux kernel version support for btrfs (@kit-ty-kate ocurrent/obuilder#68) - Generalise obuilder sandbox, removing runc/linux specifc pieces and making the S.SANDBOX interface more general (@patricoferris ocurrent/obuilder#58, reviewed by @talex5, @avsm, @MisterDA) - Convert --fast-sync back to a flag (@talex5 ocurrent/obuilder#72) - Support Fmt.cli and Logs.cli flags. (@MisterDA ocurrent/obuilder#74, reviewed by @talex5) For Fmt the new options are --color=always|never|auto For Log the new options are: -v, --verbose Increase verbosity --verbosity=LEVEL (absent=warning) Be more or less verbose. LEVEL must be one of quiet, error, warning, info or debug. Takes over -v. - Minor cleanup changes (@talex5 ocurrent/obuilder#76) - Fix deprecations in Fmt 0.8.10 (@tmcgilchrist ocurrent/obuilder#80) - Remove travis-ci and replace with Github Actions (@MisterDA ocurrent/obuilder#84) - Add RSync store backend for obuilder to support macOS builders (@patricoferris ocurrent/obuilder#88, reviewed @talex5) - Fixes for ZFS tests in CI (@patricoferris ocurrent/obuilder#91)
This is in partial fulfilment of #57 -- in particular making the
S.SANDBOX
interface more general by removing runc/linux specific frombuild.ml
.Currently OBuilder supports Linux by using runc and a little bit of docker. The docker component is currently in the more agnostic
lib/build.ml
side of things, and theget_base
function is the linux-y interpretation of(from <hash>)
. For MacOS support (and perhaps other platforms) the interpretation of(from <hash>)
will be different and is therefore dependent on the sandboxing environment -- I think it makes sense to force the sandboxing environment to provide this functionality. To this end, this PR:from
which is used where the originalget_base
function was used -- this is a function that is passed toStore.build
.create
before this was an additional function added torunc_sandbox.mli
but MacOS will also require a create function and since the choice of sandboxing environment will likely be a compile-time choice, it makes sense I think to unify thiscreate
function type by (a) putting it inS.SANDBOX
and (b) extracting aconfig
type that has acmdliner
value that can be used to build a newS.SANDBOX.t
state_dir
parameter -- the reason this isn't baked into theconfig
of therunc
sandbox is that I think it is often provided at runtime, e.g. https://github.com/ocurrent/ocluster/blob/master/worker/obuilder_build.ml#L37