Please take a look.


https://codereview.appspot.com/10489043/diff/7001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/10489043/diff/7001/cmd/jujud/machine.go#newcode172
cmd/jujud/machine.go:172: runner.StartWorker(workerName, func()
(worker.Worker, error) {
On 2013/06/27 00:16:56, wallyworld wrote:
> I'm confused by this. I don't understand why an lxc provisoner is used
for
> non-lxc containers. I know the comment says something, but it's still
not clear
> to me. How would an lxc provisioner task provision a kvm container?
What does
> not embedding lxc containers have to do with it? Should the worker
name contain
> the actual container? eg "LXC-provisioner-for-KVM"?

Extended the comment somewhat to help with that confusion.

https://codereview.appspot.com/10489043/diff/7001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/10489043/diff/7001/juju/testing/conn.go#newcode225
juju/testing/conn.go:225: c.Assert(s.APIConn.Close(), IsNil)
On 2013/06/26 23:34:47, fwereade wrote:
> Thanks for that.

> /me wonders whether this might have to do with the races davecheney
was looking
> into... ping him maybe?

Done.

https://codereview.appspot.com/10489043/diff/7001/worker/provisioner/lxc-broker.go
File worker/provisioner/lxc-broker.go (right):

https://codereview.appspot.com/10489043/diff/7001/worker/provisioner/lxc-broker.go#newcode45
worker/provisioner/lxc-broker.go:45: lxcLogger.Infof("started lxc
container for machineId: %s, %s", machineId, inst.Id())
On 2013/06/27 00:16:56, wallyworld wrote:
> On 2013/06/26 23:34:47, fwereade wrote:
> > I'd be fine with package vars called "log" by convention.

> +1
> "logger" is the colour of my bikeshed


logger is what I've used, however that one already exists at package
level, which is why I needed another.  I was torn about what to do with
it, but for now, I'm going to leave this.

Perhaps we should move the lxc broker into an lxc package under this
one.

https://codereview.appspot.com/10489043/diff/7001/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/10489043/diff/7001/worker/provisioner/provisioner.go#newcode112
worker/provisioner/provisioner.go:112: machineBroker,
On 2013/06/26 23:34:47, fwereade wrote:
> instance broker? even, perhaps, instance.Broker? just a thought...

I would love to move Broker into the instance package, but right now,
the instance packages doesn't have dependencies, and the Broker would
bring in state, api and constraints.

I will change the name though to instanceBroker.

https://codereview.appspot.com/10489043/diff/7001/worker/provisioner/provisioner.go#newcode137
worker/provisioner/provisioner.go:137: if p.machine == nil {
On 2013/06/27 00:16:56, wallyworld wrote:
> This is not thread safe. Is this being used in a single threaded
context? I
> guess it doesn't matter so much since we are just reading some info
from state.

Yes, getMachine is used just during the start-up process in the loop()
call.  It doesn't need to be thread-safe.

https://codereview.appspot.com/10489043/diff/7001/worker/provisioner/provisioner_task.go
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/10489043/diff/7001/worker/provisioner/provisioner_task.go#newcode252
worker/provisioner/provisioner_task.go:252: // have been removed from
the task.machines map.
On 2013/06/26 23:34:47, fwereade wrote:
> In a spirit purely of philosophical enquiry: would it have been
equivalent to
> have just removed the machines from the map a bit later?

I did originally do that.  You had me move it in an earlier branch. I
remember saying at the time "there is a reason for this but I don't
remember what it is", so I moved it.  The reason things kept working is
that the provisioner would die and get restarted.

I think this is cleaner now.

https://codereview.appspot.com/10489043/diff/7001/worker/provisioner/provisioner_task.go#newcode338
worker/provisioner/provisioner_task.go:338: logger.Errorf("cannot start
the machine as provisioned %q: %v", machine, err)
On 2013/06/26 23:34:47, fwereade wrote:
> maybe "cannot register instance for machine %v: %v"?

> fwiw, machine ids have hitherto *not* been reported with quotes. This
no longer
> really makes sense, but it'd be nice to maintain consistency (and
maybe do a
> big-bang fix in our copious free time).

Done.

https://codereview.appspot.com/10489043/