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

Use the Go API to spawn the docker image? #6

Open
frenata opened this issue Mar 8, 2018 · 4 comments
Open

Use the Go API to spawn the docker image? #6

frenata opened this issue Mar 8, 2018 · 4 comments
Labels
question Further information is requested

Comments

@frenata
Copy link
Owner

frenata commented Mar 8, 2018

As it stands, the code is a pretty ugly hack translated near directly from the old node implementation:

	dockerCommand := s.options.path + "/DockerTimeout.sh"
	args := []string{fmt.Sprintf("%s", s.options.timeout), "-u", "mysql", "-i", "-t", "--volume=" + s.options.folder + ":/usercode", s.options.vm_name, "/usercode/script.sh", compiler, filename, optionalExecutable, flags}

(and later)

func spawnDocker(dockerCommand string, args []string, done chan error) {
	cmd := exec.Command(dockerCommand, args...)
	bytes, err := cmd.CombinedOutput()
	_ = bytes
	//log.Printf("Docker stdout: %v", string(bytes))
	done <- err
}

It seems strange to fall back to the OS and a bash script to communicate to another process written in the same language, which seems to provide a healthy API.

On the other hand, interacting directly with the docker API might constrain the environments in which compilebox can run, requiring some amount of coupling between the host's version of docker and the API target of compilebox.

https://docs.docker.com/develop/sdk/examples/ may be useful reading as well.

@frenata frenata added the question Further information is requested label Mar 8, 2018
@asmr-hex
Copy link
Collaborator

asmr-hex commented Mar 9, 2018

This is a great consideration!

We are already loosely coupling our code with the docker CLI API, so the question is do you think the golang docker API is more likely to change with higher frequency than the docker CLI API? My unsubstantiated guess is that the CLI and golang APIs will likely change in tandem (approximately at least) so it might be more efficient and clean to just choose the golang API.

@frenata
Copy link
Owner Author

frenata commented Mar 9, 2018

That's a good point that the various APIs probably move somewhat in tandem.

Maybe? encouraging info from the docs:

The Docker daemon and client do not necessarily need to be the same version at all times. However, keep the following in mind.

  • If the daemon is newer than the client, the client does not know about new features or deprecated API endpoints in the daemon.
  • If the client is newer than the daemon, the client can request API endpoints that the daemon does not know about.

@asmr-hex
Copy link
Collaborator

asmr-hex commented Apr 2, 2018

@frenata i've been working with the Docker golang API in another project and I'm p familiar with who it works etc. now, I totally wouldn't mind taking on a small refactor to leverage this API instead of using os.Exec!

If this sounds alright, I could make a feature branch and get to work on this :)

@frenata
Copy link
Owner Author

frenata commented Apr 5, 2018

Sounds great, I think this will make the code "more complicated" but also better. Calling out to exec definitely feels like cheating to me.

asmr-hex pushed a commit that referenced this issue Apr 28, 2018
Notes:
* create godoc during this refactor and remove `docs.md`
asmr-hex pushed a commit that referenced this issue Apr 28, 2018
Notes:
* create godoc during this refactor and remove `docs.md`
asmr-hex pushed a commit that referenced this issue Apr 29, 2018
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.
@asmr-hex asmr-hex mentioned this issue Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants