generalise the sandbox - Githubissues

patricoferris commented 3 years ago

This is in partial fulfilment of #57 -- in particular making the S.SANDBOX interface more general by removing runc/linux specific from build.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 the get_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:

Extended the interface for sandboxes by providing:
- from which is used where the original get_base function was used -- this is a function that is passed to Store.build.
- create before this was an additional function added to runc_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 this create function type by (a) putting it in S.SANDBOX and (b) extracting a config type that has a cmdliner value that can be used to build a new S.SANDBOX.t
- The create function has an optional state_dir parameter -- the reason this isn't baked into the config of the runc 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

talex5 commented 3 years ago

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.

patricoferris commented 3 years ago

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.

talex5 commented 3 years ago

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.

patricoferris commented 3 years ago

Okay 👍 -- will work on it this afternoon

patricoferris commented 3 years ago

@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:https://github.com/ocurrent/obuilder/pull/58#discussion_r554558472 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 cmdliner/config is in Runc_sandbox.mli everything is... not very generalised 😅 ?

After re-writing everything to go with your suggestions, this comment https://github.com/ocurrent/obuilder/pull/58#discussion_r555014886 really resonated with me. Do you think there should be a S.FETCH with a docker.ml implementation or something like that (if so in this PR or a different one?).

Lastly I haven't passed in tmp / "rootfs" -- I wasn't 100% sure all implementations would use that directory so just passing tmp and forcing the implementation to decide seemed safest, but let me know if that's not the case :)

talex5 commented 3 years ago

Thanks, this is good progress :-)

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 cmdliner/config is in Runc_sandbox.mli everything is... not very generalised?

Well, the interface should eventually be in sandbox.mli, not runc_sandbox.mli, and then it should look OK. If you prefer, I don't mind creating another signature for this (e.g. STORE_MAKER or something), as long as it's separate from STORE.

Do you think there should be a S.FETCH with a docker.ml implementation or something like that (if so in this PR or a different one?).

I think that makes sense, especially as the Windows implementation will be using docker pull too. For now, I'd suggest just putting the docker-pull stuff in its own module (since you're moving it anyway).

Lastly I haven't passed in tmp / "rootfs" -- I wasn't 100% sure all implementations would use that directory so just passing tmp and forcing the implementation to decide seemed safest, but let me know if that's not the case :)

It's not a good idea to let the sandboxes/fetchers pick a name themselves because:

It has to not conflict with the names used in the main library (e.g. log), and we may add new ones later.
It has to be the same for fetchers and sandboxes.

MisterDA commented 3 years ago

Nothing much really, but you've left trailing whitespace in the obuilder PR. Could you do a little run of something akin to this? sed -i'' 's/[[:space:]]*$//' **/*.ml{i,}

Passing the conf parameter doesn't play nicely with multiple sandboxes implementation: I have the Runc_sandbox.config type and the Docker_sandbox.config that conflict. I'm thinking of moving it to the S.SANDBOX module, a bit like the BUILDER.context type.

patricoferris commented 3 years ago

Yeah sorry about the formatting, without ocamlformat I'm at a loss :)) Thanks for looking through this.

I didn't change the config type problem just yet because I wasn't adding anymore sandboxes I can definitely extract this out now to a sandbox.mli and a dune rule for always choosing the runc implementation.

I'm thinking of moving it to the S.SANDBOX module

See comment https://github.com/ocurrent/obuilder/pull/58#discussion_r554558472 why it was moved out of there. As @talex5 said would this not work eventually with a sandbox.mli interface? The eventual solution for macOS was something like this https://github.com/patricoferris/obuilder/blob/macos-sandbox/lib/dune#L1-L13 so the sandbox became a build-time choice, does this work for windows?

MisterDA commented 3 years ago

Thanks for the explanation! The sandbox doesn't have to become a build choice, it can be selected according to the chosen storage. In the case of the Docker sandbox, it makes sense to decide to use it depending on the storage: if the Docker storage is used, then the Docker sandbox should be used too, otherwise (btrfs/zfs) use runc. On Windows, if (when) we'll use a snapshotting filesystem, we'll also switch to the port of runc.

patricoferris commented 3 years ago

Would be good to get this merged, as it is now blocking @MisterDA.

To summarise the changes:

There is now a notion of FETCHER to perform the initialisation of the rootfs directory. For now there is one implementation, Docker, but macOS currently does something completely different so it is nice to have this flexibility.
Sandboxes have been generalised to be passed a config argument which is implementation specific and a config Cmdliner.Term.t value is provided so the CLI(s) can expose the configuration parameters, in runc's case this is just the fast_sync functionality. At the moment I don't think we're using OBuilder anywhere where we can't use a CLI so the fact that the config is abstract and can only be built via the CLI is ok, but I'm not 100% sure on this.
The build log copy function which is used in multiple places now lives in build_log.ml

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!

ocurrent / obuilder

generalise the sandbox #58