Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

run: add support for swappable engines #191

Merged
merged 1 commit into from
Mar 30, 2016

Conversation

cgonyeo
Copy link
Member

@cgonyeo cgonyeo commented Mar 11, 2016

This commit revamps lib/run.go to rely on external binaries to perform
the actual execution of the desired command. Each external binary, or
engine, is expected to either exist on the PATH or in the same directory
as the acbuild binary, and must be named as acbuild-$ENGINENAME. The
default (and currently only) engine is systemd-nspawn.

When the engine is started a JSON message detailing what it is to run
and with what environment variables is written into it on stdin. The
package github.com/appc/acbuild/engine has been added to define the
schema for this (and also holds some helper utilities for engines).

Almost fixes #13.
Kind of a workaround for #162.

@cgonyeo cgonyeo added this to the v0.3.0 milestone Mar 11, 2016
@cgonyeo
Copy link
Member Author

cgonyeo commented Mar 11, 2016

I'd also like to use this opportunity to write some tests for run, and I'd like to make that part of this PR. So don't merge this until that's done, but putting this up for review until I finish that.

@cgonyeo cgonyeo force-pushed the swappable-run-engines branch 3 times, most recently from 526c3ea to 4677463 Compare March 11, 2016 21:02
@cgonyeo
Copy link
Member Author

cgonyeo commented Mar 11, 2016

Also as soon as this gets merged I'll add a chroot-only engine (guess I'll call it fly?) and explore adding a runc engine.

@cgonyeo
Copy link
Member Author

cgonyeo commented Mar 16, 2016

pre-test review ping?

@jonboulle
Copy link
Contributor

I don't really understand the motivation for introducing a new command-line API for this - it seems like an unnecessary layer of indirection - are you anticipating that it's too difficult for people to upstream engine implementations? I can't imagine we'd see more than a handful developed?

@cgonyeo
Copy link
Member Author

cgonyeo commented Mar 17, 2016

I wanted to keep it easy for others to add engines, although if you think this is too much overhead I could just make it an interface that the engine author needs to implement.

On Mar 17, 2016, at 10:31, Jonathan Boulle [email protected] wrote:

I don't really understand the motivation for introducing a new command-line API for this - it seems like another layer of indirection - are you anticipating that it's too difficult for people to upstream engine implementations? I can't imagine we'd see more than a handful developed?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@cgonyeo
Copy link
Member Author

cgonyeo commented Mar 21, 2016

I just changed this to have engines be a golang interface. This made some things in lib/run.go simpler.

- `cp`
- `modprobe`
- `gpg`

Additionally `systemd-nspawn` is required to use the default engine for acbuild
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could inline a link to the engine section here.

@jonboulle
Copy link
Contributor

Thanks, I think we should go with this interface route for now and then later if it makes sense we can explore the exec one

@cgonyeo cgonyeo force-pushed the swappable-run-engines branch 3 times, most recently from da11ac9 to ed87b6c Compare March 23, 2016 23:28
@cgonyeo
Copy link
Member Author

cgonyeo commented Mar 23, 2016

Fixed the issues you found, updated the comment on lib/run.go, and added two tests for acbuild run.

@cgonyeo cgonyeo force-pushed the swappable-run-engines branch 7 times, most recently from 1c9fd2a to b33cc13 Compare March 24, 2016 00:08
@jonboulle
Copy link
Contributor

Waiting on travis fix...

@cgonyeo cgonyeo force-pushed the swappable-run-engines branch 3 times, most recently from 56ea405 to 7dec30b Compare March 24, 2016 21:45
@cgonyeo
Copy link
Member Author

cgonyeo commented Mar 24, 2016

Looks like the OS travis uses doesn't have systemd. I've modified TestRun so it gets skipped if ENABLE_SYSTEMD_TESTS isn't set. The test passes on my laptop, and when I implement other engines I can add in tests for those that'll run on travis.

@cgonyeo
Copy link
Member Author

cgonyeo commented Mar 30, 2016

Travis is fixed. Last call for nits.

"github.com/appc/spec/schema/types"
)

type Engine interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring would be nice.

@jonboulle
Copy link
Contributor

Two nits. LGTM.

This commit revamps `lib/run.go` to rely on an interface for the actual
execution of the desired command, such that alternate ways to run a
command in the container can be easily added. A flag has been added to
the run command to select a non-default engine. The default (and
currently only) engine is systemd-nspawn.
@cgonyeo cgonyeo merged commit ff57a4e into containers:master Mar 30, 2016
@cgonyeo cgonyeo deleted the swappable-run-engines branch March 30, 2016 20:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

run: support different engines
2 participants