state: implement EnsureAvailability

state: implement EnsureAvailability

This also changes the semantics of AddMachine
so that it can only be used to add a state server
machine if it's adding the bootstrap machine.

After than, only EnsureAvailability can be used
to add state servers.

https://code.launchpad.net/~rogpeppe/juju-core/475-ensure-availability-2/+merge/202518

(do not edit description out of merge proposal)

[rog, 2014-01-21 19:02:11]

Please take a look.

[dimitern, 2014-01-22 09:29:06]

Looks great in general, just a few minor suggestions and questions below.

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:464: // (although the machines themselves will remain).
And if the come back up won't that be a problem?

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:477: // TODO find machines which are down, clear
Please follow the agreed upon format (and file a tech-debt bug):

// TODO(rog) bug #... 2014-01-22
// Find machines which are down, clear their
// WantVote flag and add new machines to replace
// them.

https://codereview.appspot.com/55150043/diff/20001/state/api/deployer/deploye...
File state/api/deployer/deployer_test.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/api/deployer/deploye...
state/api/deployer/deployer_test.go:235: addrs := []instance.Address{
Isn't this what AddStateServerMachine actually does (SetAddresses etc.) ?

https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes...
File state/api/uniter/state_test.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes...
state/api/uniter/state_test.go:25: 
d

https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des...
File state/apiserver/client/destroy_test.go (left):

https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des...
state/apiserver/client/destroy_test.go:56: s.setUpScenario(c)
Why remove this?

https://codereview.appspot.com/55150043/diff/20001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/open.go#newcode312
state/open.go:312: // We remove the document before inserting it because
It doesn't seem you're removing the document, just updating some fields and then
inserting the whole document again. Is the update necessary to handle newer
documents with votingmachineids?

https://codereview.appspot.com/55150043/diff/20001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/state.go#newcode1091
state/state.go:1091: // StateServerInfo returns returns information about
s/StateServerInfo/stateServerInfo/

https://codereview.appspot.com/55150043/diff/20001/state/state.go#newcode1093
state/state.go:1093: // TODO(rog) export this method when the stateServers
Is this TODO relevant anymore?

https://codereview.appspot.com/55150043/diff/20001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/state_test.go#newcod...
state/state_test.go:2497: m, err := s.State.Machine(fmt.Sprint(i + 1))
Cool trick - instead of fmt.Sprintf("%d", i+1) or strconv.Itoa() :)

[fwereade, 2014-01-22 10:31:57]

Just a couple of quick comments, might be able to circle back and look more
closely in a bit.

https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes...
File state/api/uniter/state_test.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes...
state/api/uniter/state_test.go:38: err := s.stateServerMachine.Destroy()
I don't think this should be possible, should it?

https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des...
File state/apiserver/client/destroy_test.go (left):

https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des...
state/apiserver/client/destroy_test.go:56: s.setUpScenario(c)
On 2014/01/22 09:29:07, dimitern wrote:
> Why remove this?

Why have it if it's not needed? It's not very cheap.

https://codereview.appspot.com/55150043/diff/20001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/open.go#newcode312
state/open.go:312: // We remove the document before inserting it because
On 2014/01/22 09:29:07, dimitern wrote:
> It doesn't seem you're removing the document, just updating some fields and
then
> inserting the whole document again. Is the update necessary to handle newer
> documents with votingmachineids?

Yeah, I think the technique is sane but the comment is wrong. If it exists,
update it; if not, insert it. (if it doesn't exist, an assertion-free up date is
a no-op; if it does, the insert is)

[natefinch, 2014-01-22 12:20:51]

LGTM with a couple small items.

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:205: if jset[JobManageState] {
I'm not really a fan of where this method is placed, and the fact that now
everywhere we call this method, we have to pass in true or false.  I'd rather
just call hasJob(p.Jobs, MachineJob) and put the call at the beginning of the
couple of functions where we need it.  I think it would be cleaner that way.

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:428: if len(mdocs) != 1 || mdocs[0].Id != "0" {
Can you comment this so it's clear why we return the error?  I know it's in the
function comment, but it would be useful to put it here, too.

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:436: if len(currentInfo.MachineIds) != 0 ||
len(currentInfo.VotingMachineIds) != 0 {
Can you make these >0 ?  I know len can't return negative numbers, but it makes
my spidey sense twitch.

[fwereade, 2014-01-24 16:56:25]

there's a couple of other comments I made earlier I don't think you spotted

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:205: if jset[JobManageState] {
On 2014/01/22 12:20:52, nate.finch wrote:
> I'm not really a fan of where this method is placed, and the fact that now
> everywhere we call this method, we have to pass in true or false.  I'd rather
> just call hasJob(p.Jobs, MachineJob) and put the call at the beginning of the
> couple of functions where we need it.  I think it would be cleaner that way.

Yeah, those bools do smell a little.

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:464: // (although the machines themselves will remain).
On 2014/01/22 09:29:07, dimitern wrote:
> And if the come back up won't that be a problem?

So long as there's a bug tracking this I'm ok making progress with this
potential problem in play

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:477: // TODO find machines which are down, clear
On 2014/01/22 09:29:07, dimitern wrote:
> Please follow the agreed upon format (and file a tech-debt bug):
> 
> // TODO(rog) bug #... 2014-01-22
> // Find machines which are down, clear their
> // WantVote flag and add new machines to replace
> // them.

+1

https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes...
File state/api/uniter/state_test.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes...
state/api/uniter/state_test.go:38: err := s.stateServerMachine.Destroy()
On 2014/01/22 10:31:57, fwereade wrote:
> I don't think this should be possible, should it?

poke

[rog, 2014-01-24 17:01:31]

I forgot to publish these.

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:205: if jset[JobManageState] {
On 2014/01/22 12:20:52, nate.finch wrote:
> I'm not really a fan of where this method is placed, and the fact that now
> everywhere we call this method, we have to pass in true or false.  I'd rather
> just call hasJob(p.Jobs, MachineJob) and put the call at the beginning of the
> couple of functions where we need it.  I think it would be cleaner that way.

I'm not sure I agree. effectiveMachineTemplate is already a guardian of sanity
in several ways, and it's called in 4 places, three of which need
this check. Adding the extra argument means that I can be very certain that
state servers are not added unwittingly (and if someone adds a call to
effectiveMachineTemplate in the future, they will be less likely to make
that non-obvious mistake).

There's probably an argument for making it a non-bool value, say
allowStateServer, but it's a private method so I don't think
that's worth the hassle.

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:428: if len(mdocs) != 1 || mdocs[0].Id != "0" {
On 2014/01/22 12:20:52, nate.finch wrote:
> Can you comment this so it's clear why we return the error?  I know it's in
the
> function comment, but it would be useful to put it here, too.

Done.

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:436: if len(currentInfo.MachineIds) != 0 ||
len(currentInfo.VotingMachineIds) != 0 {
On 2014/01/22 12:20:52, nate.finch wrote:
> Can you make these >0 ?  I know len can't return negative numbers, but it
makes
> my spidey sense twitch.

Done.

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:464: // (although the machines themselves will remain).
On 2014/01/22 09:29:07, dimitern wrote:
> And if the come back up won't that be a problem?

Actually, the comment isn't quite accurate. They won't
be removed from the current set of state servers - they'll
just have NoVote set. That way if they come back up, they're
still part of the peer set and can potentially resynchronise
and be quickly added back to the voting set.

I've fixed the comment.

https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod...
state/addmachine.go:477: // TODO find machines which are down, clear
On 2014/01/22 09:29:07, dimitern wrote:
> Please follow the agreed upon format (and file a tech-debt bug):
> 
> // TODO(rog) bug #... 2014-01-22
> // Find machines which are down, clear their
> // WantVote flag and add new machines to replace
> // them.

Done.

https://codereview.appspot.com/55150043/diff/20001/state/api/deployer/deploye...
File state/api/deployer/deployer_test.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/api/deployer/deploye...
state/api/deployer/deployer_test.go:235: addrs := []instance.Address{
On 2014/01/22 09:29:07, dimitern wrote:
> Isn't this what AddStateServerMachine actually does (SetAddresses etc.) ?

We can't use AddStateServerMachine here because it adds a machine, but the test
suite has already created a machine and we're only allowed to use AddMachine to
create machine 0 as a state server.

https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes...
File state/api/uniter/state_test.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes...
state/api/uniter/state_test.go:25: 
On 2014/01/22 09:29:07, dimitern wrote:
> d

Done.

https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes...
state/api/uniter/state_test.go:38: err := s.stateServerMachine.Destroy()
On 2014/01/22 10:31:57, fwereade wrote:
> I don't think this should be possible, should it?

you're right - i was surprised when this worked.

it's a bug in Machine.Destroy - it doesn't fail
when the machine only has JobManageState.

https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des...
File state/apiserver/client/destroy_test.go (left):

https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des...
state/apiserver/client/destroy_test.go:56: s.setUpScenario(c)
On 2014/01/22 09:29:07, dimitern wrote:
> Why remove this?

Because it's actually not relevant to the test, and because it
adds machines, it prevents the rest of this test working
(it relies on add a state-server bootstrap machine)

https://codereview.appspot.com/55150043/diff/20001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/open.go#newcode312
state/open.go:312: // We remove the document before inserting it because
On 2014/01/22 09:29:07, dimitern wrote:
> It doesn't seem you're removing the document, just updating some fields and
then
> inserting the whole document again. Is the update necessary to handle newer
> documents with votingmachineids?

Ah, good catch. I *did* remove then add the document, but it didn't work (not
sure why actually). The update is necessary to handle 1.17 upgrades which have
the stateServers doc but not the votingmachineids field or a correctly
up-to-date machineids field.

https://codereview.appspot.com/55150043/diff/20001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/state.go#newcode1093
state/state.go:1093: // TODO(rog) export this method when the stateServers
On 2014/01/22 09:29:07, dimitern wrote:
> Is this TODO relevant anymore?

ha, no, good catch; i can export it now.

[rog, 2014-01-25 11:59:47]

Please take a look.

https://codereview.appspot.com/55150043/diff/20001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/55150043/diff/20001/state/state.go#newcode1093
state/state.go:1093: // TODO(rog) export this method when the stateServers
On 2014/01/24 17:01:31, rog wrote:
> On 2014/01/22 09:29:07, dimitern wrote:
> > Is this TODO relevant anymore?
> 
> ha, no, good catch; i can export it now.

Done.