Merge into trunk : 021-deployer-lxc-context : Code : juju-core

Status:	Work in progress
Proposed branch:	lp://staging/~themue/juju-core/021-deployer-lxc-context
Merge into:	lp://staging/~juju/juju-core/trunk
Diff against target:	890 lines (+856/-0) 5 files modified worker/deployer/export_test.go (+20/-0) worker/deployer/lxc/context.go (+355/-0) worker/deployer/lxc/export_test.go (+27/-0) worker/deployer/lxc/lxc.go (+213/-0) worker/deployer/lxc/lxc_test.go (+241/-0)
To merge this branch:	bzr merge lp://staging/~themue/juju-core/021-deployer-lxc-context
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju Engineering		2013-05-09	Pending
Review via email: mp+163237@code.staging.launchpad.net

Description of the change

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://codereview.appspot.com/9336045/

Revision history for this message

William Reade (fwereade) wrote on 2013-05-17:

#

Download full text (4.6 KiB)

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#newcode26
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#newcode39
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#newcode45
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#newcode53
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#newcode57
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#newcode67
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#newcode77
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#newcode104
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#newcode127
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#newcode146
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#newcode167
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/work...

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#newcode26
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#newcode39
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#newcode45
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#newcode53
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#newcode57
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#newcode67
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#newcode77
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#newcode104
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#newcode127
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#newcode146
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#newcode167
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#newcode188
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#newcode207
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#newcode246
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#newcode267
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#newcode345
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#newcode352
worker/deployer/lxc/context.go:352: log.Warningf("installer: cannot
remove %q: %v", path, e)
"installer"?

https://codereview.appspot.com/9336045/

Reply

Revision history for this message

Frank Mueller (themue) wrote on 2013-07-08:

#

Download full text (4.5 KiB)

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#newcode26
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#newcode39
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#newcode45
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#newcode53
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#newcode57
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#newcode77
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#newcode104
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#newcode127
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#newcode167
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 age...

Unmerged revisions

972. By Frank Mueller on 2013-05-09

lxc: merged trunk

971. By Frank Mueller on 2013-05-09

lxc: refactored into own package; first network code

970. By Frank Mueller on 2013-05-08

lxc: improved context

969. By Frank Mueller on 2013-03-11

deployer: merged trunk and done review changes

968. By Frank Mueller on 2013-03-08

deployer/lxc: merged after review changes

967. By Frank Mueller on 2013-03-08

deployer/lxc: changes after reviews

966. By Frank Mueller on 2013-03-08

deployer: merged trunk and renamed manager to context

965. By Frank Mueller on 2013-03-08

deployer: added LXC manager

964. By Francesco Banconi on 2013-03-06

Add a CharmInfo API command.

Add a CharmInfo API command to get information
about a charm, given its URL. It is needed by the
Juju GUI, modeled upon the similar one in pyJuju.

R=dimitern, rog
CC=
https://codereview.appspot.com/7444052

963. By Martin Packman on 2013-03-06

Correct url for openstack info on metadata service

The location on the metadata service we were using to retrieve the openstack
specific json file was misnamed and lacked the 'openstack' path component,
which was causing the legacy id to be used in all cases. That results in a
suicidal state server, which also hints we need to worry more about the
other cases when we may run into non-canonical server ids.

R=dfc, jameinel
CC=
https://codereview.appspot.com/7527043

juju-core

Merge lp://staging/~themue/juju-core/021-deployer-lxc-context into lp://staging/~juju/juju-core/trunk

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers