cmd/jujud: provisioner shuts down unknown instance

cmd/jujud: provisioner shuts down unknown instance

This proposal adds the facility for the PA to detect lost or
unknown instances running in the environment that are not 
associated with a machine in the state and shut them down.

https://code.launchpad.net/~dave-cheney/juju-core/go-cmd-jujud-provisioner-unknown-machines/+merge/110777

(do not edit description out of merge proposal)

[niemeyer, 2012-06-19 01:53:28]

LGTM, with a couple of minors.

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:110: p.tomb.Kill(err)
return?

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:197: machines, err := p.st.AllMachines()
These All* methods aren't a great API. We can do that for now, but we'll need
something better soon. Think about 100k machines running. This is fetching 200k
items from the DB regularly, without much benefit in most cases.

Can you please add a note here and file a bug in Launchpad against juju-core?

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:206: if id == "" {
Please add the same TODO we have elsewhere. InstanceId should really not be
returning an empty id.

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning_test.go
File cmd/jujud/provisioning_test.go (right):

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:392: // where the final machine has been removed
from the state which the PA was
"removed from the state which the PA was down"?

[dave_cheney.net, 2012-06-19 05:19:17]

Please take a look.

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:197: machines, err := p.st.AllMachines()
On 2012/06/19 01:53:28, niemeyer wrote:
> These All* methods aren't a great API. We can do that for now, but we'll need
> something better soon. Think about 100k machines running. This is fetching
200k
> items from the DB regularly, without much benefit in most cases.
> 
> Can you please add a note here and file a bug in Launchpad against juju-core?

Will do. The idea of the local caches in the provisioner was to try to limit the
damage of this style of call to on average once per provisioner process. I'm not
sure if they will be as effective as hoped.

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:206: if id == "" {
On 2012/06/19 01:53:28, niemeyer wrote:
> Please add the same TODO we have elsewhere. InstanceId should really not be
> returning an empty id.

Done.

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning_test.go
File cmd/jujud/provisioning_test.go (right):

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:392: // where the final machine has been removed
from the state which the PA was
On 2012/06/19 01:53:28, niemeyer wrote:
> "removed from the state which the PA was down"?

Done.

[niemeyer, 2012-06-19 10:30:01]

LGTM, with a couple of trivials.

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:206: if id == "" {
On 2012/06/19 05:19:17, dfc wrote:
> On 2012/06/19 01:53:28, niemeyer wrote:
> > Please add the same TODO we have elsewhere. InstanceId should really not be
> > returning an empty id.
> 
> Done.

Not done?

https://codereview.appspot.com/6315043/diff/10001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6315043/diff/10001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:104: p.tomb.Kill(err)
return

[dave_cheney.net, 2012-06-19 10:41:16]

*** Submitted:

cmd/jujud: provisioner shuts down unknown instances

This proposal adds the facility for the PA to detect lost or
unknown instances running in the environment that are not 
associated with a machine in the state and shut them down.

R=
CC=
https://codereview.appspot.com/6315043

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:206: if id == "" {
On 2012/06/19 10:30:01, niemeyer wrote:
> On 2012/06/19 05:19:17, dfc wrote:
> > On 2012/06/19 01:53:28, niemeyer wrote:
> > > Please add the same TODO we have elsewhere. InstanceId should really not
be
> > > returning an empty id.
> > 
> > Done.
> 
> Not done?

Sorry, I opened a branch but didn't add the TODO

https://codereview.appspot.com/6315043/diff/10001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6315043/diff/10001/cmd/jujud/provisioning.go#n...
cmd/jujud/provisioning.go:104: p.tomb.Kill(err)
On 2012/06/19 10:30:01, niemeyer wrote:
> return

done

[dave_cheney.net, 2012-06-20 12:25:53]

Please take a look.

[niemeyer, 2012-06-20 20:09:47]

On 2012/06/20 12:25:53, dfc wrote:
> Please take a look.

The fix looks good, Dave, but this will need a different branch and a different
CL. This one was already submitted.

Feel free to propose+submit it directly with the specific fix.

[dave_cheney.net, 2012-06-20 23:27:04]

On 2012/06/20 20:09:47, niemeyer wrote:
> On 2012/06/20 12:25:53, dfc wrote:
> > Please take a look.
> 
> The fix looks good, Dave, but this will need a different branch and a
different
> CL. This one was already submitted.
> 
> Feel free to propose+submit it directly with the specific fix.

Will do