lxc: adding lxc context for local provider

lxc: adding lxc context for local provider

The LxcContext is the second context beside the currently used 
SimpleContext to be used by the Deployer to deploy and recall 
units. It uses a small subset of the LXC commands by calling 
the binaries/scripts. Currently the new context isn't used. This
has to be done by cmd/jujud/agent.go when creating a new Deployer
instance.

https://code.launchpad.net/~themue/juju-core/021-deployer-lxc-context/+merge/163237

(do not edit description out of merge proposal)

[TheMue, 2013-05-09 23:22:54]

Please take a look.

[fwereade, 2013-05-17 06:46:27]

NOT LGTM -- I would be prepared to let the unit-specific stuff slide, but
there's quite a lot to address here.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go
File worker/deployer/lxc/context.go (right):

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:26: environSeries string
An environment does not have a series, other than default-series, which is not
relevant.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:39: // initDir specifies the directory used by
upstart on the local system.
If LXC autoconf works, we don't need upstart on the local system. We *do* need
it relative to a container's root though.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:45: dataDir string
Why not const? We're completely in control here.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:53: logDir string
Similarly, let's just make that const. No need to ever swap it out that I can
see.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:57: lxcContainerDir string
Hmm, I think I saw serge explaining that we can use whatever dir we want. If we
can, I'd really like us to just put them directly inside the deploying agent's
data dir... possible?

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:67: // entity specified in info, that deploys
unit agents as upstart jobs in
FWIW, this can't be unit-specific any more. All containers are "machines" and
get their own machine agents.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:77: hostDataDir = "/var/lib/juju"
I don't think we should default this. We should always have the data dir
available, right?

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:104: if err :=
ctx.createContainer(containerName); err != nil {
Shouldn't we trash it and try again? Failure partway through the first attempt
should not make a container undeployable on subsequent attempts...

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:127: }
If we fail here, the unit'll keep running forever.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:146: if groups[2] != ctx.deployerTag {
This is broken unless we also take into account environ name. But I'm not sure
we should care about environ names...

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:167: svc.InitDir = ctx.initDir
Why are we using the host upstart dir? ISTM that this will cause us to start
containers that don't run agents, and start agents outside containers.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:188: defer removeOnErr(&err, toolsDir)
What's the point of all these? may as well just trash a half-deployed container
entirely, surely? ...in fact, you do, so I think all these defers are pointless.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:207: if _, err =
agent.ChangeAgentTools(ctx.hostDataDir, unitTag, version.Current); err != nil {
We should *not* be using the host data dir here. Was this not a hint that you
were running upstart on the wrong system?

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:246: return uconf.Install()
The new agent needs to run inside the container.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:267: return "juju-" + ctx.environName + ":" +
ctx.deployerTag + ":" + unitTag
What's this for?

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:345: return filepath.Walk(hostToolsDir, copyTool)
This looks like it (1) copies all tools and (2) will probably break upgrades.
Why (1), how (2)?

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:352: log.Warningf("installer: cannot remove %q:
%v", path, e)
"installer"?

[mue, 2013-07-08 08:36:04]

Closed.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go
File worker/deployer/lxc/context.go (right):

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:26: environSeries string
On 2013/05/17 06:46:27, fwereade wrote:
> An environment does not have a series, other than default-series, which is not
> relevant.

Renamed in defaultSeries, it's needed for the lxc container creation. 

PyJuju gets this value by reading $JUJU_ENV, which we don't have so far. But
could also be a solution.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:39: // initDir specifies the directory used by
upstart on the local system.
On 2013/05/17 06:46:27, fwereade wrote:
> If LXC autoconf works, we don't need upstart on the local system. We *do* need
> it relative to a container's root though.

Wrong comment, but removed it due to wrong concept.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:45: dataDir string
On 2013/05/17 06:46:27, fwereade wrote:
> Why not const? We're completely in control here.

Done.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:53: logDir string
On 2013/05/17 06:46:27, fwereade wrote:
> Similarly, let's just make that const. No need to ever swap it out that I can
> see.

Done.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:57: lxcContainerDir string
On 2013/05/17 06:46:27, fwereade wrote:
> Hmm, I think I saw serge explaining that we can use whatever dir we want. If
we
> can, I'd really like us to just put them directly inside the deploying agent's
> data dir... possible?

Seems to be possible by generating the directory in the container configuration.
Still one question left, will check.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:77: hostDataDir = "/var/lib/juju"
On 2013/05/17 06:46:27, fwereade wrote:
> I don't think we should default this. We should always have the data dir
> available, right?

Done.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:104: if err :=
ctx.createContainer(containerName); err != nil {
On 2013/05/17 06:46:27, fwereade wrote:
> Shouldn't we trash it and try again? Failure partway through the first attempt
> should not make a container undeployable on subsequent attempts...

Done.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:127: }
On 2013/05/17 06:46:27, fwereade wrote:
> If we fail here, the unit'll keep running forever.

Done.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:167: svc.InitDir = ctx.initDir
On 2013/05/17 06:46:27, fwereade wrote:
> Why are we using the host upstart dir? ISTM that this will cause us to start
> containers that don't run agents, and start agents outside containers.

Ooops, has been intended to be the init dir in the container. But seen that
upstart.Install() already starts the service, so have to use the LXC cloudinit.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:188: defer removeOnErr(&err, toolsDir)
On 2013/05/17 06:46:27, fwereade wrote:
> What's the point of all these? may as well just trash a half-deployed
container
> entirely, surely? ...in fact, you do, so I think all these defers are
pointless.

Got me. ;)

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:207: if _, err =
agent.ChangeAgentTools(ctx.hostDataDir, unitTag, version.Current); err != nil {
On 2013/05/17 06:46:27, fwereade wrote:
> We should *not* be using the host data dir here. Was this not a hint that you
> were running upstart on the wrong system?

Aaargh, too blindly copied fragments without thinking.

https://codereview.appspot.com/9336045/diff/1/worker/deployer/lxc/context.go#...
worker/deployer/lxc/context.go:246: return uconf.Install()
On 2013/05/17 06:46:27, fwereade wrote:
> The new agent needs to run inside the container.

Yes, intention has been to write the upstart stuff into the container. But
Install() also directly starts the agent. So I'll take a different approach.