Code review comment for lp:~axwalk/juju-core/lp1271504-ensureavailability-agentalive

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?

https://codereview.appspot.com/86490043/