Go Docker API #21
Go Docker API #21
Conversation
Notes: * create godoc during this refactor and remove `docs.md`
Major Changes: * connect to docker client in #newSandbox constructor * break up #prepare into #PrepareTmpDir and #PrepareContainer * implement #PrepareContainer for setting up sandbox docker container * refactor #execute to start docker container and wait for completion * return errors from all sandbox methods rather than logging errors (this way, calling function can decide how to proceed on error) * assign sandbox ID corresponding to container id * create tmpDirPrefix constant * add docstrign comments Todos: * must include test coverage.
…nRemoved Notes: for some reason, WaitConditionStoped is insufficient and we must use WaitconditionRemoved. The prior resulted in a race condition in which the stdout/err was not finished writing to the files (errors/completed) specified in the `script.sh` bash script used to compile/run the user code.
|
Nice stuff. I'll check this out in detail later this week/weekend. re: vendoring, the upcoming release includes an experimental new dependency resolution tool that should eliminate the need to do so (instead we'd have roughly a |
|
@frenata, awesome! thanks! ah interesting, i wasn't aware of go 1.11-- looks like there are some good changes here. i think experimenting with the new dependency management tool would be a great idea. |
| tmpFolder, err := ioutil.TempDir(s.options.folder, "xaqt-") | ||
| // prepares the execution environment and the sandbox docker container. | ||
| // | ||
| func (s *sandbox) prepare() error { |
There was a problem hiding this comment.
Some important cleanup such that we deal with errors more robustly.
| } | ||
|
|
||
| //log.Println(s.code) | ||
| // write source file into tmp dir | ||
| // TODO (cw|4.29.2018) we should be able to write an arbitrary number of files |
There was a problem hiding this comment.
Makes sense! iow, consume a list of source files and map WriteFile over it?
There was a problem hiding this comment.
oops, i meant to take this comment out-- it was meant to say "write an arbitrary number of resource files" (i.e. files which the user submitted source can read and write to if it needs to load, say parameters or some data for the code to work). this has been addressed in the PR so i can take out this TODO.
| dockerCommand := s.options.path + "/DockerTimeout.sh" | ||
| // setup stdout stream from container | ||
| // TODO (cw|8.21.2018) do we need this? | ||
| hijackedResp, err := s.docker.ContainerAttach( |
| optionalExecutable := s.language.OptionalExecutable | ||
| flags := s.language.CompilerFlags | ||
| // create docker container for executing user code | ||
| _, err = s.docker.ContainerCreate( |
There was a problem hiding this comment.
Do we need to close/destroy/cleanup this container in some fashion within a defer block?
There was a problem hiding this comment.
when the container is configured it has been set to be auto removed once the process in the container completes (see this part of the container config)
| return string(outputBytes), err | ||
| return string(outputBytes), nil | ||
| case err = <-s.errChan: | ||
| // the damn container errored |
| *TODO (cw|4.28.2018) create godocs instead of this file...these are mostly notes for myself during development.* | ||
|
|
||
| # Public Types | ||
| * **`xaqt.Context`**: entrypoint into all functionality. TODO Propose renaming? |
There was a problem hiding this comment.
Yeah despite naming it that, I don't love it... especially now that we're using the Context library.
There was a problem hiding this comment.
yeah, i feel you, naming is hard and often doesn't feel like it holds up over time as the code progresses-- luckily this is a painless refactor at this point, so if we decide on a good rename we can change it. i won't do it in this PR since its not super relevant...but we can think it over :)
Overview
instead of using
os/exec.Commandto start docker containers, we use the official Docker Go API. This arguably makes the code more readable/maintainable and is possibly more platform independent (we don't have to rely on the assumption that we are running in a UNIX-like shell like we do when we make call outs to a shell usingos/exec/Command).this PR resolves issue #6
Major Changes
DockerClientand result/error channels as fields insandboxstructsandboxconstructorsandboxconstructor (TODO breadcrumb)PrepareContainersubmethod of(*sandbox).prepare()PrepareContainer(*sandbox).execute()selectstatementspawnDockermethod (replaced by Docker API methods).sandboxmethodsscript.shgovendorNotes
it is probably a good idea to start vendoring our dependencies. i chose the
govendortool just because i am most familiar with it, though it would be very easy to swap this out for another tool if anyone has a strong preference. On that note, there are a bunch of vendored files in this PR (sorry), but Github is smart enough to collapse them so you don't have to look at them.