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.
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.
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 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.
> 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).
Please take a look.
https:/ /codereview. appspot. com/10489043/ diff/7001/ cmd/jujud/ machine. go machine. go (right):
File cmd/jujud/
https:/ /codereview. appspot. com/10489043/ diff/7001/ cmd/jujud/ machine. go#newcode172 machine. go:172: runner. StartWorker( workerName, func() r-for-KVM" ?
cmd/jujud/
(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-provisione
Extended the comment somewhat to help with that confusion.
https:/ /codereview. appspot. com/10489043/ diff/7001/ juju/testing/ conn.go conn.go (right):
File juju/testing/
https:/ /codereview. appspot. com/10489043/ diff/7001/ juju/testing/ conn.go# newcode225 conn.go: 225: c.Assert( s.APIConn. Close() , IsNil)
juju/testing/
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 provisioner/ lxc-broker. go (right):
File worker/
https:/ /codereview. appspot. com/10489043/ diff/7001/ worker/ provisioner/ lxc-broker. go#newcode45 provisioner/ lxc-broker. go:45: lxcLogger. Infof(" started lxc
worker/
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 provisioner/ provisioner. go (right):
File worker/
https:/ /codereview. appspot. com/10489043/ diff/7001/ worker/ provisioner/ provisioner. go#newcode112 provisioner/ provisioner. go:112: machineBroker,
worker/
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 provisioner/ provisioner. go:137: if p.machine == nil {
worker/
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 provisioner/ provisioner_ task.go (right):
File worker/
https:/ /codereview. appspot. com/10489043/ diff/7001/ worker/ provisioner/ provisioner_ task.go# newcode252 provisioner/ provisioner_ task.go: 252: // have been removed from
worker/
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 provisioner/ provisioner_ task.go: 338: logger. Errorf( "cannot start
worker/
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/