Merge lp:~jameinel/juju-core/ensure-availability-default-3-1311083 into lp:~go-bot/juju-core/trunk

[John A Meinel]

cmd/juju/ensureavailability: default to 3 servers

I filed bug #1311083 because we currently have a default of '-1' and we
immediately fail when running the command. I don't think this is very
good UI for a user. I think we can make an opinionated statement that
n=3 is going to be the best for someone who wants to run this command.
And that is what this patch does.

The new default is not strictly '3' but to use 3 if you only have 1, and otherwise preserve whatever number of servers you already have. (which will start new servers to bring you back up to your requested availability.)

https://codereview.appspot.com/90160044/

cmd/juju/ensureavailability: default to 3 servers

I filed bug #1311083 because we currently have a default of '-1' and we
immediately fail when running the command. I don't think this is very
good UI for a user. I think we can make an opinionated statement that
n=3 is going to be the best for someone who wants to run this command.
And that is what this patch does.

https://codereview.appspot.com/90160044/

[John A Meinel (jameinel)]

Reviewers: mp+216706_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju/ensureavailability: default to 3 servers

I filed bug #1311083 because we currently have a default of '-1' and we
immediately fail when running the command. I don't think this is very
good UI for a user. I think we can make an opinionated statement that
n=3 is going to be the best for someone who wants to run this command.
And that is what this patch does.

https://code.launchpad.net/~jameinel/juju-core/ensure-availability-default-3-1311083/+merge/216706

(do not edit description out of merge proposal)

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

Affected files (+13, -5 lines):
   A [revision details]
   M cmd/juju/ensureavailability.go
   M cmd/juju/ensureavailability_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140422051657-1svpzqvmysbn4tqm
+New revision: <email address hidden>

Index: cmd/juju/ensureavailability.go
=== modified file 'cmd/juju/ensureavailability.go'
--- cmd/juju/ensureavailability.go 2014-04-01 04:53:43 +0000
+++ cmd/juju/ensureavailability.go 2014-04-22 12:28:07 +0000
@@ -33,7 +33,7 @@
  An odd number of state servers is required.

  Examples:
- juju ensure-availability -n 3
+ juju ensure-availability
       Ensure that 3 state servers are available,
       with newly created state server machines
       having the default series and constraints.
@@ -58,7 +58,7 @@

  func (c *EnsureAvailabilityCommand) SetFlags(f *gnuflag.FlagSet) {
   c.EnvCommandBase.SetFlags(f)
- f.IntVar(&c.NumStateServers, "n", -1, "number of state servers to make
available")
+ f.IntVar(&c.NumStateServers, "n", 3, "number of state servers to make
available")
   f.StringVar(&c.Series, "series", "", "the charm series")

f.Var(constraints.ConstraintsValue{&c.Constraints}, "constraints", "additional
machine constraints")
  }

Index: cmd/juju/ensureavailability_test.go
=== modified file 'cmd/juju/ensureavailability_test.go'
--- cmd/juju/ensureavailability_test.go 2014-04-10 13:00:21 +0000
+++ cmd/juju/ensureavailability_test.go 2014-04-22 12:28:07 +0000
@@ -114,14 +114,20 @@
  }

  func (s *EnsureAvailabilitySuite) TestEnsureAvailabilityErrors(c *gc.C) {
- err := runEnsureAvailability(c)
- c.Assert(err, gc.ErrorMatches, "must specify a number of state servers
odd and greater than zero")
   for _, n := range []int{-1, 0, 2} {
    err := runEnsureAvailability(c, "-n", fmt.Sprint(n))
    c.Assert(err, gc.ErrorMatches, "must specify a number of state servers
odd and greater than zero")
   }
- err = runEnsureAvailability(c, "-n", "3")
+ err := runEnsureAvailability(c, "-n", "3")
   c.Assert(err, gc.IsNil)
   err = runEnsureAvailability(c, "-n", "1")
   c.Assert(err, gc.ErrorMatches, "cannot reduce state server count")
  }
+
+func (s *EnsureAvailabilitySuite) TestEnsureAvailabilityDefaultsTo3(c
*gc.C) {
+ err := runEnsureAvailability(c)
+ c.Assert(err, gc.IsNil)
+ machines, err := s.State.AllMachines()
+ c.Assert(err, gc.IsNil)
+ c.Assert(...

[Andrew Wilkins (axwalk)]

On 2014/04/22 12:32:29, jameinel wrote:
> Please take a look.

I originally just defaulted it to 1, but rog asked me to take out the
default so that we can, in the future, change the command to maintain
the current availability. That would enable a simple cron job to
periodically "juju ensure-availability", replacing unavailable state
servers and reinstating available ones.

https://codereview.appspot.com/90160044/

[John A Meinel (jameinel)]

On 2014/04/22 12:42:23, axw wrote:
> On 2014/04/22 12:32:29, jameinel wrote:
> > Please take a look.

> I originally just defaulted it to 1, but rog asked me to take out the
default so
> that we can, in the future, change the command to maintain the current
> availability. That would enable a simple cron job to periodically
"juju
> ensure-availability", replacing unavailable state servers and
reinstating
> available ones.

ensure-availability 1 can't ever do anything.

Because if 1 machine is dead, there is nobody to contact to set up
availability.
And if you pass 1 to a system that has 3, you get a message about "you
cannot reduce the number", because we would be trying to decrease from 3
to 1.

There is room to say that "ensure-availability" with nothing specified
is actually -1, which means "3 if you are currently running with only 1,
and N if you are currently running with N".

I would be happy to make that change, so that we allow -1 or 0 to be
passed up, and then interpret it according to the above rules.

But *quite honestly* I don't expect anyone to ever run with anything but
1 or 3, so making it overly flexible doesn't actually gain us much. (It
is possible that in a year or more we'll have found ways to scale beyond
3 productively, I don't think we're anywhere close to that yet).

https://codereview.appspot.com/90160044/

[Roger Peppe (rogpeppe)]

On 2014/04/22 14:11:08, jameinel wrote:
> On 2014/04/22 12:42:23, axw wrote:
> > On 2014/04/22 12:32:29, jameinel wrote:
> > > Please take a look.
> >
> > I originally just defaulted it to 1, but rog asked me to take out
the default
> so
> > that we can, in the future, change the command to maintain the
current
> > availability. That would enable a simple cron job to periodically
"juju
> > ensure-availability", replacing unavailable state servers and
reinstating
> > available ones.

> ensure-availability 1 can't ever do anything.

> Because if 1 machine is dead, there is nobody to contact to set up
availability.
> And if you pass 1 to a system that has 3, you get a message about "you
cannot
> reduce the number", because we would be trying to decrease from 3 to
1.

That should be fixed eventually - we should allow people to reduce their
state server count.

> There is room to say that "ensure-availability" with nothing specified
is
> actually -1, which means "3 if you are currently running with only 1,
and N if
> you are currently running with N".

That was the plan.

> I would be happy to make that change, so that we allow -1 or 0 to be
passed up,
> and then interpret it according to the above rules.

SGTM

https://codereview.appspot.com/90160044/

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/90160044/diff/1/cmd/juju/ensureavailability.go
File cmd/juju/ensureavailability.go (right):

https://codereview.appspot.com/90160044/diff/1/cmd/juju/ensureavailability.go#newcode36
cmd/juju/ensureavailability.go:36: juju ensure-availability
nitpick - add another example showing use of n?, eg
juju ensure-availability -n 5
Ensure that 5 state servers ....

https://codereview.appspot.com/90160044/

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/90160044/

[Andrew Wilkins (axwalk)]

On 2014/04/23 06:55:47, jameinel wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/90160044/

=== modified file 'cmd/juju/ensureavailability.go'
--- cmd/juju/ensureavailability.go	2014-04-01 04:53:43 +0000
+++ cmd/juju/ensureavailability.go	2014-04-23 06:53:26 +0000
@@ -33,18 +33,17 @@
 An odd number of state servers is required.
 
 Examples:
- juju ensure-availability -n 3
-     Ensure that 3 state servers are available,
-     with newly created state server machines
-     having the default series and constraints.
+ juju ensure-availability
+     Ensure that the system is still in highly available mode. If
+     there is only 1 state server running, this will ensure there
+     are 3 running. If you have previously requested more than 3,
+     then that number will be ensured.
  juju ensure-availability -n 5 --series=trusty
-     Ensure that 5 state servers are available,
-     with newly created state server machines
-     having the "trusty" series.
+     Ensure that 5 state servers are available, with newly created
+     state server machines having the "trusty" series.
  juju ensure-availability -n 7 --constraints mem=8G
-     Ensure that 7 state servers are available,
-     with newly created state server machines
-     having the default series, and at least
+     Ensure that 7 state servers are available, with newly created
+     state server machines having the default series, and at least
      8GB RAM.
 `
 
@@ -58,7 +57,7 @@
 
 func (c *EnsureAvailabilityCommand) SetFlags(f *gnuflag.FlagSet) {
 	c.EnvCommandBase.SetFlags(f)
-	f.IntVar(&c.NumStateServers, "n", -1, "number of state servers to make available")
+	f.IntVar(&c.NumStateServers, "n", 0, "number of state servers to make available")
 	f.StringVar(&c.Series, "series", "", "the charm series")
 	f.Var(constraints.ConstraintsValue{&c.Constraints}, "constraints", "additional machine constraints")
 }
@@ -68,8 +67,8 @@
 	if err != nil {
 		return err
 	}
-	if c.NumStateServers%2 != 1 || c.NumStateServers <= 0 {
-		return fmt.Errorf("must specify a number of state servers odd and greater than zero")
+	if c.NumStateServers < 0 || (c.NumStateServers%2 != 1 && c.NumStateServers != 0) {
+		return fmt.Errorf("must specify a number of state servers odd and non-negative")
 	}
 	return cmd.CheckEmpty(args)
 }

=== modified file 'cmd/juju/ensureavailability_test.go'
--- cmd/juju/ensureavailability_test.go	2014-04-10 13:00:21 +0000
+++ cmd/juju/ensureavailability_test.go	2014-04-23 06:53:26 +0000
@@ -12,17 +12,44 @@
 	"launchpad.net/juju-core/constraints"
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
-	"launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/state/presence"
+	coretesting "launchpad.net/juju-core/testing"
 )
 
 type EnsureAvailabilitySuite struct {
 	jujutesting.RepoSuite
+	machine0Pinger *presence.Pinger
 }
 
 var _ = gc.Suite(&EnsureAvailabilitySuite{})
 
+func (s *EnsureAvailabilitySuite) SetUpTest(c *gc.C) {
+	s.RepoSuite.SetUpTest(c)
+	// Add a state server to the environment, and ensure that it is
+	// considered 'alive' so that calls don't spawn new instances
+	_, err := s.State.AddMachine("precise", state.JobManageEnviron)
+	c.Assert(err, gc.IsNil)
+	m, err := s.BackingState.Machine("0")
+	c.Assert(err, gc.IsNil)
+	s.machine0Pinger, err = m.SetAgentAlive()
+	c.Assert(err, gc.IsNil)
+	s.BackingState.StartSync()
+	err = m.WaitAgentAlive(coretesting.LongWait)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *EnsureAvailabilitySuite) TearDownTest(c *gc.C) {
+	// We have to Kill the Pinger before TearDownTest, otherwise the State
+	// connection is already closed.
+	if s.machine0Pinger != nil {
+		s.machine0Pinger.Kill()
+		s.machine0Pinger = nil
+	}
+	s.RepoSuite.TearDownTest(c)
+}
+
 func runEnsureAvailability(c *gc.C, args ...string) error {
-	_, err := testing.RunCommand(c, &EnsureAvailabilityCommand{}, args)
+	_, err := coretesting.RunCommand(c, &EnsureAvailabilityCommand{}, args)
 	return err
 }
 
@@ -39,17 +66,17 @@
 }
 
 func (s *EnsureAvailabilitySuite) TestEnsureAvailabilityWithSeries(c *gc.C) {
-	err := runEnsureAvailability(c, "--series", "series", "-n", "1")
+	err := runEnsureAvailability(c, "--series", "series", "-n", "3")
 	c.Assert(err, gc.IsNil)
-	m, err := s.State.Machine("0")
+	m, err := s.State.Machine("1")
 	c.Assert(err, gc.IsNil)
 	c.Assert(m.Series(), gc.DeepEquals, "series")
 }
 
 func (s *EnsureAvailabilitySuite) TestEnsureAvailabilityWithConstraints(c *gc.C) {
-	err := runEnsureAvailability(c, "--constraints", "mem=4G", "-n", "1")
+	err := runEnsureAvailability(c, "--constraints", "mem=4G", "-n", "3")
 	c.Assert(err, gc.IsNil)
-	m, err := s.State.Machine("0")
+	m, err := s.State.Machine("1")
 	c.Assert(err, gc.IsNil)
 	mcons, err := m.Constraints()
 	c.Assert(err, gc.IsNil)
@@ -62,6 +89,9 @@
 		err := runEnsureAvailability(c, "-n", "1")
 		c.Assert(err, gc.IsNil)
 	}
+	machines, err := s.State.AllMachines()
+	c.Assert(err, gc.IsNil)
+	c.Assert(machines, gc.HasLen, 1)
 	m, err := s.State.Machine("0")
 	c.Assert(err, gc.IsNil)
 	mcons, err := m.Constraints()
@@ -81,22 +111,7 @@
 }
 
 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")
+	err := runEnsureAvailability(c, "-n", "3", "--constraints", "mem=4G")
 	c.Assert(err, gc.IsNil)
 
 	machines, err := s.State.AllMachines()
@@ -114,14 +129,28 @@
 }
 
 func (s *EnsureAvailabilitySuite) TestEnsureAvailabilityErrors(c *gc.C) {
-	err := runEnsureAvailability(c)
-	c.Assert(err, gc.ErrorMatches, "must specify a number of state servers odd and greater than zero")
-	for _, n := range []int{-1, 0, 2} {
+	for _, n := range []int{-1, 2} {
 		err := runEnsureAvailability(c, "-n", fmt.Sprint(n))
-		c.Assert(err, gc.ErrorMatches, "must specify a number of state servers odd and greater than zero")
+		c.Assert(err, gc.ErrorMatches, "must specify a number of state servers odd and non-negative")
 	}
-	err = runEnsureAvailability(c, "-n", "3")
+	err := runEnsureAvailability(c, "-n", "3")
 	c.Assert(err, gc.IsNil)
 	err = runEnsureAvailability(c, "-n", "1")
 	c.Assert(err, gc.ErrorMatches, "cannot reduce state server count")
 }
+
+func (s *EnsureAvailabilitySuite) TestEnsureAvailabilityAllows0(c *gc.C) {
+	err := runEnsureAvailability(c, "-n", "0")
+	c.Assert(err, gc.IsNil)
+	machines, err := s.State.AllMachines()
+	c.Assert(err, gc.IsNil)
+	c.Assert(machines, gc.HasLen, 3)
+}
+
+func (s *EnsureAvailabilitySuite) TestEnsureAvailabilityDefaultsTo3(c *gc.C) {
+	err := runEnsureAvailability(c)
+	c.Assert(err, gc.IsNil)
+	machines, err := s.State.AllMachines()
+	c.Assert(err, gc.IsNil)
+	c.Assert(machines, gc.HasLen, 3)
+}

=== modified file 'state/addmachine.go'
--- state/addmachine.go	2014-04-21 23:10:05 +0000
+++ state/addmachine.go	2014-04-23 06:53:26 +0000
@@ -497,8 +497,8 @@
 // the number of live state servers equal to numStateServers. The given
 // constraints and series will be attached to any new machines.
 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")
+	if numStateServers < 0 || (numStateServers != 0 && numStateServers%2 != 1) {
+		return fmt.Errorf("number of state servers must be odd and non-negative")
 	}
 	if numStateServers > replicaset.MaxPeers {
 		return fmt.Errorf("state server count is too large (allowed %d)", replicaset.MaxPeers)
@@ -508,7 +508,14 @@
 		if err != nil {
 			return err
 		}
-		if len(currentInfo.VotingMachineIds) > numStateServers {
+		desiredStateServerCount := numStateServers
+		if desiredStateServerCount == 0 {
+			desiredStateServerCount = len(currentInfo.VotingMachineIds)
+			if desiredStateServerCount <= 1 {
+				desiredStateServerCount = 3
+			}
+		}
+		if len(currentInfo.VotingMachineIds) > desiredStateServerCount {
 			return fmt.Errorf("cannot reduce state server count")
 		}
 
@@ -522,15 +529,15 @@
 				voteCount++
 			}
 		}
-		if voteCount == numStateServers && len(intent.remove) == 0 {
+		if voteCount == desiredStateServerCount && len(intent.remove) == 0 {
 			return nil
 		}
 		// Promote as many machines as we can to fulfil the shortfall.
-		if n := numStateServers - voteCount; n < len(intent.promote) {
+		if n := desiredStateServerCount - voteCount; n < len(intent.promote) {
 			intent.promote = intent.promote[:n]
 		}
 		voteCount += len(intent.promote)
-		intent.newCount = numStateServers - voteCount
+		intent.newCount = desiredStateServerCount - voteCount
 		logger.Infof("%d new machines; promoting %v", intent.newCount, intent.promote)
 		ops, err := st.ensureAvailabilityIntentionOps(intent, currentInfo, cons, series)
 		if err != nil {

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2014-04-17 12:53:23 +0000
+++ state/apiserver/client/client.go	2014-04-23 06:53:26 +0000
@@ -1044,11 +1044,23 @@
 func (c *Client) EnsureAvailability(args params.EnsureAvailability) error {
 	series := args.Series
 	if series == "" {
-		cfg, err := c.api.state.EnvironConfig()
-		if err != nil {
-			return err
-		}
-		series = config.PreferredSeries(cfg)
+		ssi, err := c.api.state.StateServerInfo()
+		if err != nil {
+			return err
+		}
+		// We should always have at least one voting machine
+		// If we *really* wanted we could just pick whatever series is
+		// in the majority, but really, if we always copy the value of
+		// the first one, then they'll stay in sync.
+		if len(ssi.VotingMachineIds) == 0 {
+			// Better than a panic()?
+			return fmt.Errorf("internal error, failed to find any voting machines")
+		}
+		templateMachine, err := c.api.state.Machine(ssi.VotingMachineIds[0])
+		if err != nil {
+			return err
+		}
+		series = templateMachine.Series()
 	}
 	return c.api.state.EnsureAvailability(args.NumStateServers, args.Constraints, series)
 }

=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2014-04-17 12:53:23 +0000
+++ state/apiserver/client/client_test.go	2014-04-23 06:53:26 +0000
@@ -2143,27 +2143,51 @@
 	return pinger
 }
 
+var (
+	emptyCons     = constraints.Value{}
+	defaultSeries = ""
+)
+
 func (s *clientSuite) TestClientEnsureAvailabilitySeries(c *gc.C) {
-	err := s.APIState.Client().EnsureAvailability(1, constraints.Value{}, "")
+	_, err := s.State.AddMachine("quantal", state.JobManageEnviron)
 	c.Assert(err, gc.IsNil)
+	// We have to ensure the agents are alive, or EnsureAvailability will
+	// create more to replace them.
 	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, 1)
+	c.Assert(machines[0].Series(), gc.Equals, "quantal")
+	err = s.APIState.Client().EnsureAvailability(3, emptyCons, defaultSeries)
+	c.Assert(err, gc.IsNil)
+	machines, err = s.State.AllMachines()
+	c.Assert(err, gc.IsNil)
 	c.Assert(machines, gc.HasLen, 3)
-	c.Assert(machines[0].Series(), gc.Equals, "precise")
-	c.Assert(machines[1].Series(), gc.Equals, "non-default")
-	c.Assert(machines[2].Series(), gc.Equals, "non-default")
+	c.Assert(machines[0].Series(), gc.Equals, "quantal")
+	c.Assert(machines[1].Series(), gc.Equals, "quantal")
+	c.Assert(machines[2].Series(), gc.Equals, "quantal")
+	defer s.setAgentAlive(c, "1").Kill()
+	defer s.setAgentAlive(c, "2").Kill()
+	err = s.APIState.Client().EnsureAvailability(5, emptyCons, "non-default")
+	c.Assert(err, gc.IsNil)
+	machines, err = s.State.AllMachines()
+	c.Assert(err, gc.IsNil)
+	c.Assert(machines, gc.HasLen, 5)
+	c.Assert(machines[0].Series(), gc.Equals, "quantal")
+	c.Assert(machines[1].Series(), gc.Equals, "quantal")
+	c.Assert(machines[2].Series(), gc.Equals, "quantal")
+	c.Assert(machines[3].Series(), gc.Equals, "non-default")
+	c.Assert(machines[4].Series(), gc.Equals, "non-default")
 }
 
 func (s *clientSuite) TestClientEnsureAvailabilityConstraints(c *gc.C) {
-	err := s.APIState.Client().EnsureAvailability(1, constraints.Value{}, "")
+	_, err := s.State.AddMachine("quantal", state.JobManageEnviron)
 	c.Assert(err, gc.IsNil)
 	pinger := s.setAgentAlive(c, "0")
 	defer pinger.Kill()
-	err = s.APIState.Client().EnsureAvailability(3, constraints.MustParse("mem=4G"), "")
+	err = s.APIState.Client().EnsureAvailability(
+		3, constraints.MustParse("mem=4G"), defaultSeries)
 	c.Assert(err, gc.IsNil)
 	machines, err := s.State.AllMachines()
 	c.Assert(err, gc.IsNil)
@@ -2180,11 +2204,53 @@
 	}
 }
 
+func (s *clientSuite) TestClientEnsureAvailability0Preserves(c *gc.C) {
+	_, err := s.State.AddMachine("quantal", state.JobManageEnviron)
+	c.Assert(err, gc.IsNil)
+	pinger := s.setAgentAlive(c, "0")
+	defer pinger.Kill()
+	// A value of 0 says either "if I'm not HA, make me HA" or "preserve my
+	// current HA settings".
+	err = s.APIState.Client().EnsureAvailability(0, emptyCons, defaultSeries)
+	c.Assert(err, gc.IsNil)
+	machines, err := s.State.AllMachines()
+	c.Assert(machines, gc.HasLen, 3)
+	defer s.setAgentAlive(c, "1").Kill()
+	// Now, we keep agent 1 alive, but not agent 2, calling
+	// EnsureAvailability(0) again will cause us to start another machine
+	err = s.APIState.Client().EnsureAvailability(0, emptyCons, defaultSeries)
+	c.Assert(err, gc.IsNil)
+	machines, err = s.State.AllMachines()
+	c.Assert(machines, gc.HasLen, 4)
+}
+
+func (s *clientSuite) TestClientEnsureAvailability0Preserves5(c *gc.C) {
+	_, err := s.State.AddMachine("quantal", state.JobManageEnviron)
+	c.Assert(err, gc.IsNil)
+	pinger := s.setAgentAlive(c, "0")
+	defer pinger.Kill()
+	// Start off with 5 servers
+	err = s.APIState.Client().EnsureAvailability(5, emptyCons, defaultSeries)
+	c.Assert(err, gc.IsNil)
+	machines, err := s.State.AllMachines()
+	c.Assert(machines, gc.HasLen, 5)
+	defer s.setAgentAlive(c, "1").Kill()
+	defer s.setAgentAlive(c, "2").Kill()
+	defer s.setAgentAlive(c, "3").Kill()
+	// Keeping all alive but one, will bring up 1 more server to preserve 5
+	err = s.APIState.Client().EnsureAvailability(0, emptyCons, defaultSeries)
+	c.Assert(err, gc.IsNil)
+	machines, err = s.State.AllMachines()
+	c.Assert(machines, gc.HasLen, 6)
+}
+
 func (s *clientSuite) TestClientEnsureAvailabilityErrors(c *gc.C) {
-	var emptyCons constraints.Value
-	defaultSeries := ""
-	err := s.APIState.Client().EnsureAvailability(0, emptyCons, defaultSeries)
-	c.Assert(err, gc.ErrorMatches, "number of state servers must be odd and greater than zero")
+	_, err := s.State.AddMachine("quantal", state.JobManageEnviron)
+	c.Assert(err, gc.IsNil)
+	pinger := s.setAgentAlive(c, "0")
+	defer pinger.Kill()
+	err = s.APIState.Client().EnsureAvailability(-1, emptyCons, defaultSeries)
+	c.Assert(err, gc.ErrorMatches, "number of state servers must be odd and non-negative")
 	err = s.APIState.Client().EnsureAvailability(3, emptyCons, defaultSeries)
 	c.Assert(err, gc.IsNil)
 	err = s.APIState.Client().EnsureAvailability(1, emptyCons, defaultSeries)

=== modified file 'state/state_test.go'
--- state/state_test.go	2014-04-21 23:10:05 +0000
+++ state/state_test.go	2014-04-23 06:53:26 +0000
@@ -2926,9 +2926,9 @@
 }
 
 func (s *StateSuite) TestEnsureAvailabilityFailsWithBadCount(c *gc.C) {
-	for _, n := range []int{-1, 0, 2, 6} {
+	for _, n := range []int{-1, 2, 6} {
 		err := s.State.EnsureAvailability(n, constraints.Value{}, "")
-		c.Assert(err, gc.ErrorMatches, "number of state servers must be odd and greater than zero")
+		c.Assert(err, gc.ErrorMatches, "number of state servers must be odd and non-negative")
 	}
 	err := s.State.EnsureAvailability(replicaset.MaxPeers+2, constraints.Value{}, "")
 	c.Assert(err, gc.ErrorMatches, `state server count is too large \(allowed \d+\)`)
@@ -3069,6 +3069,60 @@
 	c.Assert(m0.IsManager(), jc.IsFalse)
 }
 
+func (s *StateSuite) TestEnsureAvailabilityMaintainsVoteList(c *gc.C) {
+	err := s.State.EnsureAvailability(5, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	s.assertStateServerInfo(c,
+		[]string{"0", "1", "2", "3", "4"},
+		[]string{"0", "1", "2", "3", "4"})
+	// Mark machine-0 as dead, so we'll want to create another one again
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return m.Id() != "0", nil
+	})
+	err = s.State.EnsureAvailability(0, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+
+	// New state server machine "5" is created; "0" still exists in MachineIds,
+	// but no longer in VotingMachineIds.
+	s.assertStateServerInfo(c,
+		[]string{"0", "1", "2", "3", "4", "5"},
+		[]string{"1", "2", "3", "4", "5"})
+	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("5")
+	c.Assert(err, gc.IsNil)
+	c.Assert(m3.WantsVote(), jc.IsTrue)
+	c.Assert(m3.IsManager(), jc.IsTrue)
+}
+
+func (s *StateSuite) TestEnsureAvailabilityDefaultsTo3(c *gc.C) {
+	err := s.State.EnsureAvailability(0, constraints.Value{}, "quantal")
+	c.Assert(err, gc.IsNil)
+	s.assertStateServerInfo(c, []string{"0", "1", "2"}, []string{"0", "1", "2"})
+	// Mark machine-0 as dead, so we'll want to create it again
+	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
+		return m.Id() != "0", nil
+	})
+	err = s.State.EnsureAvailability(0, 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) TestEnsureAvailabilityConcurrentSame(c *gc.C) {
 	s.PatchValue(state.StateServerAvailable, func(m *state.Machine) (bool, error) {
 		return true, nil