Merge lp:~axwalk/juju-core/lp1271504-ensureavailability-agentalive into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

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://codereview.appspot.com/86490043/

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://codereview.appspot.com/86490043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+215172_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/86490043/

Affected files (+272, -61 lines):
   A [revision details]
   M cmd/juju/ensureavailability_test.go
   M state/addmachine.go
   M state/apiserver/client/client_test.go
   M state/export_test.go
   M state/state_test.go

[Roger Peppe (rogpeppe)]

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/

[Andrew Wilkins (axwalk)]

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.

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

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/lp1271504-ensureavailability-agentalive into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.014s
ok launchpad.net/juju-core/agent 1.205s
ok launchpad.net/juju-core/agent/mongo 0.919s
ok launchpad.net/juju-core/agent/tools 0.174s
ok launchpad.net/juju-core/bzr 5.064s
ok launchpad.net/juju-core/cert 2.691s
ok launchpad.net/juju-core/charm 0.350s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.032s
ok launchpad.net/juju-core/cloudinit/sshinit 0.782s
ok launchpad.net/juju-core/cmd 0.197s
ok launchpad.net/juju-core/cmd/charm-admin 0.759s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/envcmd 0.211s
ok launchpad.net/juju-core/cmd/juju 226.333s
ok launchpad.net/juju-core/cmd/jujud 77.802s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 9.276s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.224s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.029s
ok launchpad.net/juju-core/container 0.039s
ok launchpad.net/juju-core/container/factory 0.054s
ok launchpad.net/juju-core/container/kvm 0.174s
ok launchpad.net/juju-core/container/kvm/mock 0.042s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.261s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.264s
ok launchpad.net/juju-core/environs 2.706s
ok launchpad.net/juju-core/environs/bootstrap 11.064s
ok launchpad.net/juju-core/environs/cloudinit 0.520s
ok launchpad.net/juju-core/environs/config 1.741s
ok launchpad.net/juju-core/environs/configstore 0.028s
ok launchpad.net/juju-core/environs/filestorage 0.027s
ok launchpad.net/juju-core/environs/httpstorage 0.706s
ok launchpad.net/juju-core/environs/imagemetadata 0.421s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.044s
ok launchpad.net/juju-core/environs/jujutest 0.176s
ok launchpad.net/juju-core/environs/manual 11.771s
ok launchpad.net/juju-core/environs/simplestreams 0.257s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.902s
ok launchpad.net/juju-core/environs/storage 0.968s
ok launchpad.net/juju-core/environs/sync 49.180s
ok launchpad.net/juju-core/environs/testing 0.123s
ok launchpad.net/juju-core/environs/tools 4.880s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.017s
ok launchpad.net/juju-core/instance 0.021s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju...

=== modified file 'cmd/juju/ensureavailability_test.go'
--- cmd/juju/ensureavailability_test.go	2014-03-28 09:55:10 +0000
+++ cmd/juju/ensureavailability_test.go	2014-04-14 03:42:21 +0000
@@ -83,6 +83,19 @@
 func (s *EnsureAvailabilitySuite) TestEnsureAvailabilityMultiple(c *gc.C) {
 	err := runEnsureAvailability(c, "-n", "1")
 	c.Assert(err, gc.IsNil)
+
+	// make sure machine-0 remains alive for the second call to
+	// EnsureAvailability, or machine-0 will get bumped down to
+	// non-voting.
+	m0, err := s.BackingState.Machine("0")
+	c.Assert(err, gc.IsNil)
+	pinger, err := m0.SetAgentAlive()
+	c.Assert(err, gc.IsNil)
+	defer pinger.Kill()
+	s.BackingState.StartSync()
+	err = m0.WaitAgentAlive(testing.LongWait)
+	c.Assert(err, gc.IsNil)
+
 	err = runEnsureAvailability(c, "-n", "3", "--constraints", "mem=4G")
 	c.Assert(err, gc.IsNil)
 

=== modified file 'state/addmachine.go'
--- state/addmachine.go	2014-04-10 14:40:34 +0000
+++ state/addmachine.go	2014-04-14 03:42:21 +0000
@@ -497,14 +497,6 @@
 // EnsureAvailability adds state server machines as necessary to make
 // the number of live state servers equal to numStateServers. The given
 // constraints and series will be attached to any new machines.
-//
-// TODO(rog):
-// If any current state servers are down, they will be
-// removed from the current set of voting replica set
-// peers (although the machines themselves will remain
-// and they will still remain part of the replica set).
-// Once a machine's voting status has been removed,
-// the machine itself may be removed.
 func (st *State) EnsureAvailability(numStateServers int, cons constraints.Value, series string) error {
 	if numStateServers%2 != 1 || numStateServers <= 0 {
 		return fmt.Errorf("number of state servers must be odd and greater than zero")
@@ -512,46 +504,158 @@
 	if numStateServers > replicaset.MaxPeers {
 		return fmt.Errorf("state server count is too large (allowed %d)", replicaset.MaxPeers)
 	}
-	info, err := st.StateServerInfo()
-	if err != nil {
-		return err
-	}
-	if len(info.VotingMachineIds) == numStateServers {
-		// TODO(rog) #1271504 2014-01-22
-		// Find machines which are down, set
-		// their NoVote flag and add new machines to
-		// replace them.
-		return nil
-	}
-	if len(info.VotingMachineIds) > numStateServers {
-		return fmt.Errorf("cannot reduce state server count")
-	}
-	mdocs := make([]*machineDoc, 0, numStateServers-len(info.MachineIds))
-	var ops []txn.Op
-	for i := len(info.MachineIds); i < numStateServers; i++ {
-		template := MachineTemplate{
-			Series: series,
-			Jobs: []MachineJob{
-				JobHostUnits,
-				JobManageEnviron,
-			},
-			Constraints: cons,
-		}
-		mdoc, addOps, err := st.addMachineOps(template)
-		if err != nil {
-			return err
-		}
-		mdocs = append(mdocs, mdoc)
-		ops = append(ops, addOps...)
-	}
-	ssOps, err := st.maintainStateServersOps(mdocs, info)
-	if err != nil {
-		return fmt.Errorf("cannot prepare machine add operations: %v", err)
-	}
-	ops = append(ops, ssOps...)
-	err = st.runTransaction(ops)
-	if err != nil {
-		return fmt.Errorf("failed to create new state server machines: %v", err)
-	}
-	return nil
+	for i := 0; i < 5; i++ {
+		currentInfo, err := st.StateServerInfo()
+		if err != nil {
+			return err
+		}
+		if len(currentInfo.VotingMachineIds) > numStateServers {
+			return fmt.Errorf("cannot reduce state server count")
+		}
+		removeOps, info, promotableMachines, err := st.updateAvailableStateServersOps(currentInfo)
+		if err != nil {
+			return err
+		}
+		if len(info.VotingMachineIds) == numStateServers && len(removeOps) == 0 {
+			return nil
+		}
+		// Promote existing machines first.
+		var ops []txn.Op
+		if n := numStateServers - len(info.VotingMachineIds); n < len(promotableMachines) {
+			promotableMachines = promotableMachines[:n]
+		}
+		for _, m := range promotableMachines {
+			ops = append(ops, promoteStateServerOps(m)...)
+			info.VotingMachineIds = append(info.VotingMachineIds, m.Id())
+		}
+		// Create new machines to make up the shortfall.
+		mdocs := make([]*machineDoc, numStateServers-len(info.VotingMachineIds))
+		for i := range mdocs {
+			template := MachineTemplate{
+				Series: series,
+				Jobs: []MachineJob{
+					JobHostUnits,
+					JobManageEnviron,
+				},
+				Constraints: cons,
+			}
+			mdoc, addOps, err := st.addMachineOps(template)
+			if err != nil {
+				return err
+			}
+			mdocs[i] = mdoc
+			ops = append(ops, addOps...)
+		}
+		ssOps, err := st.maintainStateServersOps(mdocs, currentInfo)
+		if err != nil {
+			return fmt.Errorf("cannot prepare machine add operations: %v", err)
+		}
+		ops = append(ops, removeOps...)
+		ops = append(ops, ssOps...)
+		err = st.runTransaction(ops)
+		if err == nil {
+			return nil
+		} else if err != txn.ErrAborted {
+			return fmt.Errorf("failed to create new state server machines: %v", err)
+		}
+		// The transaction will only be aborted if another call to
+		// EnsureAvailability completed. Loop back around and try again.
+	}
+	return ErrExcessiveContention
+}
+
+// stateServerAvailable returns true if the specified state server machine is
+// available.
+var stateServerAvailable = func(m *Machine) (bool, error) {
+	// TODO(axw) #1271504 2014-01-22
+	// Check the state server's associated mongo health;
+	// requires coordination with worker/peergrouper.
+	return m.AgentAlive()
+}
+
+// updateAvailableStateServersOps checks the availability of state servers,
+//   demoting unavailable, voting machines;
+//   removing unavailable, non-voting, non-vote-holding machines;
+//   gathering available, non-voting machines that may be promoted;
+//   updating StateServerInfo to reflect reality.
+func (st *State) updateAvailableStateServersOps(info *StateServerInfo) (ops []txn.Op, newInfo *StateServerInfo, promotableMachines []*Machine, err error) {
+	newInfo = &StateServerInfo{}
+	for _, mid := range info.MachineIds {
+		m, err := st.Machine(mid)
+		if err != nil {
+			return nil, nil, nil, err
+		}
+		available, err := stateServerAvailable(m)
+		if err != nil {
+			return nil, nil, nil, err
+		}
+		if available {
+			if m.WantsVote() {
+				newInfo.VotingMachineIds = append(newInfo.VotingMachineIds, mid)
+			} else {
+				promotableMachines = append(promotableMachines, m)
+			}
+			newInfo.MachineIds = append(newInfo.MachineIds, mid)
+			continue
+		}
+		if m.WantsVote() {
+			// The machine wants to vote, so we simply set novote and allow it
+			// to run its course to have its vote removed by the worker that
+			// maintains the replicaset. We will replace it with an existing
+			// non-voting state server if there is one, starting a new one if
+			// not.
+			ops = append(ops, demoteStateServerOps(m)...)
+			newInfo.MachineIds = append(newInfo.MachineIds, mid)
+		} else if m.HasVote() {
+			// The machine still has a vote, so keep it in MachineIds for now.
+			newInfo.MachineIds = append(newInfo.MachineIds, mid)
+		} else {
+			// The machine neither wants to nor has a vote, so remove its
+			// JobManageEnviron job immediately.
+			ops = append(ops, removeStateServerOps(m)...)
+		}
+	}
+	return ops, newInfo, promotableMachines, nil
+}
+
+func promoteStateServerOps(m *Machine) []txn.Op {
+	return []txn.Op{{
+		C:      m.st.machines.Name,
+		Id:     m.doc.Id,
+		Assert: bson.D{{"novote", true}},
+		Update: bson.D{{"$set", bson.D{{"novote", false}}}},
+	}, {
+		C:      m.st.stateServers.Name,
+		Id:     environGlobalKey,
+		Update: bson.D{{"$addToSet", bson.D{{"votingmachineids", m.doc.Id}}}},
+	}}
+}
+
+func demoteStateServerOps(m *Machine) []txn.Op {
+	return []txn.Op{{
+		C:      m.st.machines.Name,
+		Id:     m.doc.Id,
+		Assert: bson.D{{"novote", false}},
+		Update: bson.D{{"$set", bson.D{{"novote", true}}}},
+	}, {
+		C:      m.st.stateServers.Name,
+		Id:     environGlobalKey,
+		Update: bson.D{{"$pull", bson.D{{"votingmachineids", m.doc.Id}}}},
+	}}
+}
+
+func removeStateServerOps(m *Machine) []txn.Op {
+	return []txn.Op{{
+		C:      m.st.machines.Name,
+		Id:     m.doc.Id,
+		Assert: bson.D{{"novote", true}, {"hasvote", false}},
+		Update: bson.D{
+			{"$pull", bson.D{{"jobs", JobManageEnviron}}},
+			{"$set", bson.D{{"novote", false}}},
+		},
+	}, {
+		C:      m.st.stateServers.Name,
+		Id:     environGlobalKey,
+		Update: bson.D{{"$pull", bson.D{{"machineids", m.doc.Id}}}},
+	}}
 }

=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2014-04-11 15:16:00 +0000
+++ state/apiserver/client/client_test.go	2014-04-14 03:42:21 +0000
@@ -29,6 +29,7 @@
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/api/usermanager"
 	"launchpad.net/juju-core/state/apiserver/client"
+	"launchpad.net/juju-core/state/presence"
 	"launchpad.net/juju-core/state/statecmd"
 	coretesting "launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/utils"
@@ -2132,17 +2133,24 @@
 	c.Assert(data["transient"], gc.Equals, true)
 }
 
+func (s *clientSuite) setAgentAlive(c *gc.C, machineId string) *presence.Pinger {
+	m, err := s.BackingState.Machine(machineId)
+	c.Assert(err, gc.IsNil)
+	pinger, err := m.SetAgentAlive()
+	c.Assert(err, gc.IsNil)
+	s.BackingState.StartSync()
+	err = m.WaitAgentAlive(coretesting.LongWait)
+	c.Assert(err, gc.IsNil)
+	return pinger
+}
+
 func (s *clientSuite) TestClientEnsureAvailabilitySeries(c *gc.C) {
-	apiParams := []params.EnsureAvailability{{
-		NumStateServers: 1,
-	}, {
-		NumStateServers: 3,
-		Series:          "non-default",
-	}}
-	for _, p := range apiParams {
-		err := s.APIState.Client().EnsureAvailability(p.NumStateServers, p.Constraints, p.Series)
-		c.Assert(err, gc.IsNil)
-	}
+	err := s.APIState.Client().EnsureAvailability(1, constraints.Value{}, "")
+	c.Assert(err, gc.IsNil)
+	pinger := s.setAgentAlive(c, "0")
+	defer pinger.Kill()
+	err = s.APIState.Client().EnsureAvailability(3, constraints.Value{}, "non-default")
+	c.Assert(err, gc.IsNil)
 	machines, err := s.State.AllMachines()
 	c.Assert(err, gc.IsNil)
 	c.Assert(machines, gc.HasLen, 3)
@@ -2152,16 +2160,12 @@
 }
 
 func (s *clientSuite) TestClientEnsureAvailabilityConstraints(c *gc.C) {
-	apiParams := []params.EnsureAvailability{{
-		NumStateServers: 1,
-	}, {
-		NumStateServers: 3,
-		Constraints:     constraints.MustParse("mem=4G"),
-	}}
-	for _, p := range apiParams {
-		err := s.APIState.Client().EnsureAvailability(p.NumStateServers, p.Constraints, p.Series)
-		c.Assert(err, gc.IsNil)
-	}
+	err := s.APIState.Client().EnsureAvailability(1, constraints.Value{}, "")
+	c.Assert(err, gc.IsNil)
+	pinger := s.setAgentAlive(c, "0")
+	defer pinger.Kill()
+	err = s.APIState.Client().EnsureAvailability(3, constraints.MustParse("mem=4G"), "")
+	c.Assert(err, gc.IsNil)
 	machines, err := s.State.AllMachines()
 	c.Assert(err, gc.IsNil)
 	c.Assert(machines, gc.HasLen, 3)

=== modified file 'state/export_test.go'
--- state/export_test.go	2014-03-25 15:06:31 +0000
+++ state/export_test.go	2014-04-14 03:42:21 +0000
@@ -259,3 +259,5 @@
 func CheckUserExists(st *State, name string) (bool, error) {
 	return st.checkUserExists(name)
 }
+
+var StateServerAvailable = &stateServerAvailable

=== modified file 'state/state_test.go'
--- state/state_test.go	2014-04-10 14:40:34 +0000
+++ state/state_test.go	2014-04-14 03:42:21 +0000
@@ -6,7 +6,6 @@
 import (
 	"fmt"
 	"net/url"
-	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -1735,6 +1734,10 @@
 		VotingMachineIds: []string{"0"},
 	})
 
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return true, nil
+	})
+
 	err = s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
 	c.Assert(err, gc.IsNil)
 
@@ -2889,6 +2892,11 @@
 }
 
 func (s *StateSuite) TestEnsureAvailabilityAddsNewMachines(c *gc.C) {
+	// Don't use agent presence to decide on machine availability.
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return true, nil
+	})
+
 	ids := make([]string, 3)
 	m0, err := s.State.AddMachine("quantal", state.JobHostUnits, state.JobManageEnviron)
 	c.Assert(err, gc.IsNil)
@@ -2898,12 +2906,7 @@
 	_, err = s.State.AddMachine("quantal", state.JobHostUnits)
 	c.Assert(err, gc.IsNil)
 
-	info, err := s.State.StateServerInfo()
-	c.Assert(err, gc.IsNil)
-	c.Assert(info, gc.DeepEquals, &state.StateServerInfo{
-		MachineIds:       []string{m0.Id()},
-		VotingMachineIds: []string{m0.Id()},
-	})
+	s.assertStateServerInfo(c, []string{"0"}, []string{"0"})
 
 	cons := constraints.Value{
 		Mem: newUint64(100),
@@ -2924,22 +2927,182 @@
 		c.Assert(m.WantsVote(), jc.IsTrue)
 		ids[i] = m.Id()
 	}
-	sort.Strings(ids)
-
-	info, err = s.State.StateServerInfo()
-	c.Assert(err, gc.IsNil)
-	sort.Strings(info.MachineIds)
-	sort.Strings(info.VotingMachineIds)
-	c.Assert(info, gc.DeepEquals, &state.StateServerInfo{
-		MachineIds:       ids,
-		VotingMachineIds: ids,
-	})
+	s.assertStateServerInfo(c, ids, ids)
 }
 
 func newUint64(i uint64) *uint64 {
 	return &i
 }
 
+func (s *StateSuite) assertStateServerInfo(c *gc.C, machineIds []string, votingMachineIds []string) {
+	info, err := s.State.StateServerInfo()
+	c.Assert(err, gc.IsNil)
+	c.Assert(info.MachineIds, jc.SameContents, machineIds)
+	c.Assert(info.VotingMachineIds, jc.SameContents, votingMachineIds)
+}
+
+func (s *StateSuite) TestEnsureAvailabilityDemotesUnavailableMachines(c *gc.C) {
+	err := s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	s.assertStateServerInfo(c, []string{"0", "1", "2"}, []string{"0", "1", "2"})
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return m.Id() != "0", nil
+	})
+	err = s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+
+	// New state server machine "3" is created; "0" still exists in MachineIds,
+	// but no longer in VotingMachineIds.
+	s.assertStateServerInfo(c, []string{"0", "1", "2", "3"}, []string{"1", "2", "3"})
+	m0, err := s.State.Machine("0")
+	c.Assert(err, gc.IsNil)
+	c.Assert(m0.WantsVote(), jc.IsFalse)
+	c.Assert(m0.IsManager(), jc.IsTrue) // job still intact for now
+	m3, err := s.State.Machine("3")
+	c.Assert(err, gc.IsNil)
+	c.Assert(m3.WantsVote(), jc.IsTrue)
+	c.Assert(m3.IsManager(), jc.IsTrue)
+}
+
+func (s *StateSuite) TestEnsureAvailabilityPromotesAvailableMachines(c *gc.C) {
+	err := s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	s.assertStateServerInfo(c, []string{"0", "1", "2"}, []string{"0", "1", "2"})
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return m.Id() != "0", nil
+	})
+	err = s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+
+	// New state server machine "3" is created; "0" still exists in MachineIds,
+	// but no longer in VotingMachineIds.
+	s.assertStateServerInfo(c, []string{"0", "1", "2", "3"}, []string{"1", "2", "3"})
+	m0, err := s.State.Machine("0")
+	c.Assert(err, gc.IsNil)
+	c.Assert(m0.WantsVote(), jc.IsFalse)
+
+	// Mark machine 0 as having a vote, so it doesn't get removed, and make it
+	// available once more.
+	err = m0.SetHasVote(true)
+	c.Assert(err, gc.IsNil)
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return true, nil
+	})
+	err = s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	// No change; we've got as many voting machines as we need.
+	s.assertStateServerInfo(c, []string{"0", "1", "2", "3"}, []string{"1", "2", "3"})
+
+	// Make machine 3 unavailable; machine 0 should be promoted, and two new
+	// machines created.
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return m.Id() != "3", nil
+	})
+	err = s.State.EnsureAvailability(5, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	s.assertStateServerInfo(c, []string{"0", "1", "2", "3", "4", "5"}, []string{"0", "1", "2", "4", "5"})
+	err = m0.Refresh()
+	c.Assert(err, gc.IsNil)
+	c.Assert(m0.WantsVote(), jc.IsTrue)
+}
+
+func (s *StateSuite) TestEnsureAvailabilityRemovesUnavailableMachines(c *gc.C) {
+	err := s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	s.assertStateServerInfo(c, []string{"0", "1", "2"}, []string{"0", "1", "2"})
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return m.Id() != "0", nil
+	})
+	err = s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	s.assertStateServerInfo(c, []string{"0", "1", "2", "3"}, []string{"1", "2", "3"})
+	// machine 0 does not have a vote, so another call to EnsureAvailability
+	// will remove machine 0's JobEnvironManager job.
+	err = s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	s.assertStateServerInfo(c, []string{"1", "2", "3"}, []string{"1", "2", "3"})
+	m0, err := s.State.Machine("0")
+	c.Assert(err, gc.IsNil)
+	c.Assert(m0.IsManager(), jc.IsFalse)
+}
+
+func (s *StateSuite) TestEnsureAvailabilityConcurrentSame(c *gc.C) {
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return true, nil
+	})
+
+	defer state.SetBeforeHooks(c, s.State, func() {
+		err := s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+		c.Assert(err, gc.IsNil)
+		// The outer EnsureAvailability call will allocate IDs 0..2,
+		// and the inner one 3..5.
+		expected := []string{"3", "4", "5"}
+		s.assertStateServerInfo(c, expected, expected)
+	}).Check()
+
+	err := s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	s.assertStateServerInfo(c, []string{"3", "4", "5"}, []string{"3", "4", "5"})
+
+	// Machine 0 should never have been created.
+	_, err = s.State.Machine("0")
+	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+}
+
+func (s *StateSuite) TestEnsureAvailabilityConcurrentLess(c *gc.C) {
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return true, nil
+	})
+
+	defer state.SetBeforeHooks(c, s.State, func() {
+		err := s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+		c.Assert(err, gc.IsNil)
+		// The outer EnsureAvailability call will initially allocate IDs 0..4,
+		// and the inner one 5..7.
+		expected := []string{"5", "6", "7"}
+		s.assertStateServerInfo(c, expected, expected)
+	}).Check()
+
+	// This call to EnsureAvailability will initially attempt to allocate
+	// machines 0..4, and fail due to the concurrent change. It will then
+	// allocate machines 8..9 to make up the difference from the concurrent
+	// EnsureAvailability call.
+	err := s.State.EnsureAvailability(5, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	expected := []string{"5", "6", "7", "8", "9"}
+	s.assertStateServerInfo(c, expected, expected)
+
+	// Machine 0 should never have been created.
+	_, err = s.State.Machine("0")
+	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+}
+
+func (s *StateSuite) TestEnsureAvailabilityConcurrentMore(c *gc.C) {
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return true, nil
+	})
+
+	defer state.SetBeforeHooks(c, s.State, func() {
+		err := s.State.EnsureAvailability(5, constraints.Value{}, "quantal")
+		c.Assert(err, gc.IsNil)
+		// The outer EnsureAvailability call will allocate IDs 0..2,
+		// and the inner one 3..7.
+		expected := []string{"3", "4", "5", "6", "7"}
+		s.assertStateServerInfo(c, expected, expected)
+	}).Check()
+
+	// This call to EnsureAvailability will initially attempt to allocate
+	// machines 0..2, and fail due to the concurrent change. It will then
+	// find that the number of voting machines in state is greater than
+	// what we're attempting to ensure, and fail.
+	err := s.State.EnsureAvailability(3, constraints.Value{}, "quantal")
+	c.Assert(err, gc.ErrorMatches, "cannot reduce state server count")
+
+	// Machine 0 should never have been created.
+	_, err = s.State.Machine("0")
+	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+}
+
 func (s *StateSuite) TestStateServingInfo(c *gc.C) {
 	info, err := s.State.StateServingInfo()
 	c.Assert(info, jc.DeepEquals, params.StateServingInfo{})