state: (demote)promote (un)available state servers

state: (demote)promote (un)available state servers

When EnsureAvailability is called, we now check the
availability of existing state servers. If a state server
is unavailable, then we either set WantsVote=false, or
remove it entirely if it doesn't want a vote.

If we need to create new state servers, we will first
promote existing, available, non-voting state servers
before creating new ones.

Availability is currently defined by the state.Machine's
agent presence. We may want to extend this later to also
consider the associated mongo's replicaset member health.

https://code.launchpad.net/~axwalk/juju-core/lp1271504-ensureavailability-agentalive/+merge/215172

(do not edit description out of merge proposal)

[axw, 2014-04-10 13:14:10]

Please take a look.

[rog, 2014-04-11 10:09:17]

LGTM modulo the following.
Very nice.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode531
state/addmachine.go:531: mdocs := make([]*machineDoc,
numStateServers-len(info.VotingMachineIds))
nice clear logic here.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode555
state/addmachine.go:555: if err != nil {
If err==txn.ErrAborted, I think it's only going to be because someone
else has run EnsureAvailability concurrently. Perhaps we should check if the
voting server count has changed - if it has, return an error, otherwise return
nil because our work will already have been done.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode578
state/addmachine.go:578: var newVotingMachineIds, newMachineIds []string
rather than two variables, it might be clearer if we did:

var newInfo StateServerInfo

and then, e.g.
newInfo.MachineIds = append(newInfo.MachineIds, ...)

then it's more obvious when we're just building
up ids in the new info.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode589
state/addmachine.go:589: if !m.WantsVote() {
switch the sense of the condition to avoid the negative?

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode598
state/addmachine.go:598: // If the machine does want to vote, we simply set
novote and
Since the comments are inside the if statement, we can phrase them
unconditionally, which might read slightly more clearly.

// The machine wants to vote, so we simply set novote etc

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode605
state/addmachine.go:605: } else if !m.HasVote() {
perhaps invert the condition here to save negatives?

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode606
state/addmachine.go:606: // If the machine does not want to vote and doesn't
have a vote,
again, we can phrase it unconditionally.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode625
state/addmachine.go:625: Assert: bson.D{{"novote", bson.D{{"$ne", false}}}},
I *think* this should be {{"$eq", true}}, because nil!=false
and legacy machines may have a nil novote field.

similarly below.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode655
state/addmachine.go:655: Update: bson.D{{"$pull", bson.D{{"jobs",
JobManageEnviron}}}},
perhaps set novote=false here too?

[axw, 2014-04-14 03:45:25]

Please take a look.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode555
state/addmachine.go:555: if err != nil {
On 2014/04/11 10:09:17, rog wrote:
> If err==txn.ErrAborted, I think it's only going to be because someone
> else has run EnsureAvailability concurrently. Perhaps we should check if the
> voting server count has changed - if it has, return an error, otherwise return
> nil because our work will already have been done.

Done, I made it a transaction loop and added a few tests.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode578
state/addmachine.go:578: var newVotingMachineIds, newMachineIds []string
On 2014/04/11 10:09:17, rog wrote:
> rather than two variables, it might be clearer if we did:
> 
> var newInfo StateServerInfo
> 
> and then, e.g.
> newInfo.MachineIds = append(newInfo.MachineIds, ...)
> 
> then it's more obvious when we're just building
> up ids in the new info.

Done.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode589
state/addmachine.go:589: if !m.WantsVote() {
On 2014/04/11 10:09:17, rog wrote:
> switch the sense of the condition to avoid the negative?

Done.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode598
state/addmachine.go:598: // If the machine does want to vote, we simply set
novote and
On 2014/04/11 10:09:17, rog wrote:
> Since the comments are inside the if statement, we can phrase them
> unconditionally, which might read slightly more clearly.
> 
> // The machine wants to vote, so we simply set novote etc

Done.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode605
state/addmachine.go:605: } else if !m.HasVote() {
On 2014/04/11 10:09:17, rog wrote:
> perhaps invert the condition here to save negatives?

Done.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode606
state/addmachine.go:606: // If the machine does not want to vote and doesn't
have a vote,
On 2014/04/11 10:09:17, rog wrote:
> again, we can phrase it unconditionally.

Done.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode625
state/addmachine.go:625: Assert: bson.D{{"novote", bson.D{{"$ne", false}}}},
On 2014/04/11 10:09:17, rog wrote:
> I *think* this should be {{"$eq", true}}, because nil!=false
> and legacy machines may have a nil novote field.
> 
> similarly below.

Done.

https://codereview.appspot.com/86490043/diff/1/state/addmachine.go#newcode655
state/addmachine.go:655: Update: bson.D{{"$pull", bson.D{{"jobs",
JobManageEnviron}}}},
On 2014/04/11 10:09:17, rog wrote:
> perhaps set novote=false here too?

Done.