Code review comment for lp:~themue/juju-core/014-deployer-lxc-context

I may be confused, but I don't think this works how it should... it
seems to be running the units on the host system.

https://codereview.appspot.com/7637045/diff/15001/worker/deployer/lxc.go
File worker/deployer/lxc.go (right):

https://codereview.appspot.com/7637045/diff/15001/worker/deployer/lxc.go#newcode184
worker/deployer/lxc.go:184: defer removeOnErr(&err, toolsDir)
Why are we bothering to remove directories inside the container? Can't
we just treat a half-inited container as useless crack and trash the
whole thing?

https://codereview.appspot.com/7637045/diff/15001/worker/deployer/lxc.go#newcode191
worker/deployer/lxc.go:191: defer removeOnErr(&err, agentsDir)
Ditto.

https://codereview.appspot.com/7637045/diff/15001/worker/deployer/lxc.go#newcode198
worker/deployer/lxc.go:198: defer removeOnErr(&err, logDir)
Ditto.

https://codereview.appspot.com/7637045/diff/15001/worker/deployer/lxc.go#newcode203
worker/deployer/lxc.go:203: if _, err =
agent.ChangeAgentTools(ctx.hostDataDir, entityName, version.Current);
err != nil {
Why are we doing this in the host data dir instead of the container one?

https://codereview.appspot.com/7637045/diff/15001/worker/deployer/lxc.go#newcode224
worker/deployer/lxc.go:224: defer removeOnErr(&err, conf.Dir())
Why remove this on failure?

https://codereview.appspot.com/7637045/diff/15001/worker/deployer/lxc.go#newcode242
worker/deployer/lxc.go:242: return uconf.Install()
Doesn't this run the upstart job on the host system? AFAICT the initDir
used is always /etc/init; and if it weren't, woudln't this be guaranteed
to fail (because we'd be trying to start a job that doesn't exist?)

Note that, as part of fixing this, you'll want to tweak the upstart
package so that Install doesn't also Start.

https://codereview.appspot.com/7637045/diff/15001/worker/deployer/lxc.go#newcode263
worker/deployer/lxc.go:263: // script based on the unit name.
These aren't the same. There's no reason to give the upstart script a
name based on the environment -- it should go inside its container
(which *should* indeed have an env-differentiated name).

https://codereview.appspot.com/7637045/diff/15001/worker/deployer/lxc.go#newcode337
worker/deployer/lxc.go:337: // Copy file.
I think we should probably handle symlinks here -- they do sometimes
live in the tools directory, even if the situations are *currently*
non-overlapping.

https://codereview.appspot.com/7637045/