Merge lp:~rogpeppe/juju-core/479-desired-peer-group into lp:~go-bot/juju-core/trunk

[Roger Peppe]

worker/peergrouper: new package

This will be a worker that maintains the replica set
with respect to the state.

The function in this CL implements the core functionality - it's
a stateless function that looks at a representation of the
current state of affairs and decides what the replicaset
member list should look like.

It may well change when faced with reality (although I've been
trying to sanity check with experimental code),
but I think it should be reasonable to check in now
as a staging post.

There are current some extraneous changes in this
branch (everything outside worker/peergrouper)
which are made redundant by other MPs. Please
ignore for the purposes of this review; I'll remove
before submitting.

https://codereview.appspot.com/49920045/

worker/peergrouper: new package

This will be a worker that maintains the replica set
with respect to the state.

The function in this CL implements the core functionality - it's
a stateless function that looks at a representation of the
current state of affairs and decides what the replicaset
member list should look like.

It may well change when faced with reality (although I've been
trying to sanity check with experimental code),
but I think it should be reasonable to check in now
as a staging post.

There are current some extraneous changes in this
branch (everything outside worker/peergrouper)
which are made redundant by other MPs. Please
ignore for the purposes of this review; I'll remove
before submitting.

https://codereview.appspot.com/49920045/

[Roger Peppe (rogpeppe)]

Reviewers: mp+201245_code.launchpad.net,

Message:
Please take a look.

Description:
worker/peergrouper: new package

This will be a worker that maintains the replica set
with respect to the state.

The function in this CL implements the core functionality - it's
a stateless function that looks at a representation of the
current state of affairs and decides what the replicaset
member list should look like.

It may well change when faced with reality (although I've been
trying to sanity check with experimental code),
but I think it should be reasonable to check in now
as a staging post.

There are current some extraneous changes in this
branch (everything outside worker/peergrouper)
which are made redundant by other MPs. Please
ignore for the purposes of this review; I'll remove
before submitting.

https://code.launchpad.net/~rogpeppe/juju-core/479-desired-peer-group/+merge/201245

(do not edit description out of merge proposal)

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

Affected files (+684, -38 lines):
   A [revision details]
   M replicaset/replicaset.go
   M replicaset/replicaset_test.go
   M testing/mgo.go
   A worker/peergrouper/desired.go
   A worker/peergrouper/desired_test.go
   M worker/uniter/uniter_test.go

[John A Meinel (jameinel)]

I didn't finish the review before I'm on the next call, but some
thoughts I didn't want to lose.

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired.go
File worker/peergrouper/desired.go (right):

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired.go#newcode32
worker/peergrouper/desired.go:32: //func getPeerGroupInfo(st
*state.State, ms []*state.Machine) (*peerGroupInfo, error) {
this function is commented out. Generally either we should delete it, or
have it. I'm guessing this is just part of the process as you're getting
there.

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired.go#newcode58
worker/peergrouper/desired.go:58: return j
Is there really nothing like min that we can just use?

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired.go#newcode66
worker/peergrouper/desired.go:66: func desiredPeerGroup(info
*peerGroupInfo) ([]replicaset.Member, error) {
Your description makes it sound like this function mixes a query (what
peer group do you want?) with an action (set the voting status of all
the machines).

It would be best to split informational queries from side effects if
possible.

https://codereview.appspot.com/49920045/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired.go
File worker/peergrouper/desired.go (right):

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired.go#newcode32
worker/peergrouper/desired.go:32: //func getPeerGroupInfo(st
*state.State, ms []*state.Machine) (*peerGroupInfo, error) {
On 2014/01/13 11:37:12, jameinel wrote:
> this function is commented out. Generally either we should delete it,
or have
> it. I'm guessing this is just part of the process as you're getting
there.

It is. I suppose I should probably delete it for this CL and bring it
back in the next one.

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired.go#newcode58
worker/peergrouper/desired.go:58: return j
On 2014/01/13 11:37:12, jameinel wrote:
> Is there really nothing like min that we can just use?

nope. it's not considered worth adding to the standard library.
(well, it wouldn't be one - there would be 12 of them).

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired.go#newcode66
worker/peergrouper/desired.go:66: func desiredPeerGroup(info
*peerGroupInfo) ([]replicaset.Member, error) {
On 2014/01/13 11:37:12, jameinel wrote:
> Your description makes it sound like this function mixes a query (what
peer
> group do you want?) with an action (set the voting status of all the
machines).

> It would be best to split informational queries from side effects if
possible.

I've changed it so that it returns a map of values rather than setting
the machine fields in place. We want the caller to set the votes of the
machines after calling this function, and it's much easier to work out
that information here, rather than have the caller try to work out the
info from the replicaset members, which may not include all the machines
passed in.

https://codereview.appspot.com/49920045/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/49920045/

[Nate Finch (natefinch)]

LGTM overall. I'd like to see comments on the functions even if they're
not exported... makes it a little clearer what each one is doing.

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired.go
File worker/peergrouper/desired.go (right):

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired.go#newcode38
worker/peergrouper/desired.go:38: func desiredPeerGroup(info
*peerGroupInfo) ([]replicaset.Member, map[*machine]bool, error) {
This function is pretty long, would be a little easier to read if it
were broken up more, I think.

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired.go#newcode199
worker/peergrouper/desired.go:199: found = m
break here?

https://codereview.appspot.com/49920045/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired.go
File worker/peergrouper/desired.go (right):

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired.go#newcode38
worker/peergrouper/desired.go:38: func desiredPeerGroup(info
*peerGroupInfo) ([]replicaset.Member, map[*machine]bool, error) {
On 2014/01/13 18:47:15, nate.finch wrote:
> This function is pretty long, would be a little easier to read if it
were broken
> up more, I think.

Yeah, I'd toyed with that idea but not mustered up the necessary energy.

Done; definitely a good idea, thanks.

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired.go#newcode199
worker/peergrouper/desired.go:199: found = m
On 2014/01/13 18:47:15, nate.finch wrote:
> break here?

Done.

https://codereview.appspot.com/49920045/

[Nate Finch (natefinch)]

LGTM The refactor makes a big difference.

https://codereview.appspot.com/49920045/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/49920045/

[Go Bot (go-bot)]

The attempt to merge lp:~rogpeppe/juju-core/479-desired-peer-group into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.262s
ok launchpad.net/juju-core/agent/tools 0.279s
ok launchpad.net/juju-core/bzr 8.061s
ok launchpad.net/juju-core/cert 3.420s
ok launchpad.net/juju-core/charm 0.609s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.033s
ok launchpad.net/juju-core/cloudinit/sshinit 1.067s
ok launchpad.net/juju-core/cmd 0.224s
ok launchpad.net/juju-core/cmd/charm-admin 0.848s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 174.789s

----------------------------------------------------------------------
FAIL: machine_test.go:273: MachineSuite.TestManageEnviron

[LOG] 30.61946 DEBUG juju.environs.configstore Making /tmp/gocheck-6129484611666145821/45/home/ubuntu/.juju/environments
[LOG] 30.73054 DEBUG juju.environs.tools reading v1.* tools
[LOG] 30.73059 INFO juju environs/testing: uploading FAKE tools 1.17.1-precise-amd64
[LOG] 30.73153 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 30.73155 DEBUG juju.environs.tools no series specified when finding tools, looking for any
[LOG] 30.73162 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:52068/dummyenv/private/tools/streams/v1/index.sjson": file "tools/streams/v1/index.sjson" not found not found
[LOG] 30.73165 DEBUG juju.environs.simplestreams cannot load index "http://127.0.0.1:52068/dummyenv/private/tools/streams/v1/index.sjson": invalid URL "http://127.0.0.1:52068/dummyenv/private/tools/streams/v1/index.sjson" not found
[LOG] 30.73169 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:52068/dummyenv/private/tools/streams/v1/index.json": file "tools/streams/v1/index.json" not found not found
[LOG] 30.73171 DEBUG juju.environs.simplestreams cannot load index "http://127.0.0.1:52068/dummyenv/private/tools/streams/v1/index.json": invalid URL "http://127.0.0.1:52068/dummyenv/private/tools/streams/v1/index.json" not found
[LOG] 30.73198 INFO juju.environs.tools Writing tools/streams/v1/index.json
[LOG] 30.73202 INFO juju.environs.tools Writing tools/streams/v1/com.ubuntu.juju:released:tools.json
[LOG] 30.73218 INFO juju.environs.bootstrap bootstrapping environment "dummyenv"
[LOG] 30.73221 DEBUG juju.environs.bootstrap looking for bootstrap tools: series="precise", arch=<nil>, version=1.17.1
[LOG] 30.73223 INFO juju.environs.tools reading tools with major.minor version 1.17
[LOG] 30.73224 INFO juju.environs.tools filtering tools by version: 1.17.1
[LOG] 30.73225 INFO juju.environs.tools filtering tools by series: precise
[LOG] 30.73227 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 30.73231 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:52068/dummyenv/private/tools/streams/v1/index.sjson": file "tools/streams/v1/index.sjson" not found not found
[LOG] 30.73233 DEBUG juju.environs.simplestreams cannot load index "http:/...

=== added directory 'worker/peergrouper'
=== added file 'worker/peergrouper/desired.go'
--- worker/peergrouper/desired.go	1970-01-01 00:00:00 +0000
+++ worker/peergrouper/desired.go	2014-02-14 12:14:58 +0000
@@ -0,0 +1,290 @@
+package peergrouper
+
+import (
+	"fmt"
+	"sort"
+
+	"launchpad.net/juju-core/replicaset"
+	"launchpad.net/loggo"
+)
+
+var logger = loggo.GetLogger("juju.worker.peergrouper")
+
+// peerGroupInfo holds information that may contribute to
+// a peer group.
+type peerGroupInfo struct {
+	machines []*machine // possibly map[id] *machine
+	statuses []replicaset.MemberStatus
+	members  []replicaset.Member
+}
+
+// machine represents a machine in State.
+type machine struct {
+	id        string
+	wantsVote bool
+	hostPort  string
+}
+
+// desiredPeerGroup returns the mongo peer group according to the given
+// servers and a map with an element for each machine in info.machines
+// specifying whether that machine has been configured as voting. It may
+// return (nil, nil, nil) if the current group is already correct.
+func desiredPeerGroup(info *peerGroupInfo) ([]replicaset.Member, map[*machine]bool, error) {
+	changed := false
+	members, extra, maxId := info.membersMap()
+
+	// We may find extra peer group members if the machines
+	// have been removed or their state server status removed.
+	// This should only happen if they had been set to non-voting
+	// before removal, in which case we want to remove it
+	// from the members list. If we find a member that's still configured
+	// to vote, it's an error.
+	// TODO There are some other possibilities
+	// for what to do in that case.
+	// 1) leave them untouched, but deal
+	// with others as usual "i didn't see that bit"
+	// 2) leave them untouched, deal with others,
+	// but make sure the extras aren't eligible to
+	// be primary.
+	// 3) remove them "get rid of bad rubbish"
+	// 4) bomb out "run in circles, scream and shout"
+	// 5) do nothing "nothing to see here"
+	for _, member := range extra {
+		if member.Votes == nil || *member.Votes > 0 {
+			return nil, nil, fmt.Errorf("voting non-machine member found in peer group")
+		}
+		changed = true
+	}
+
+	toRemoveVote, toAddVote, toKeep := possiblePeerGroupChanges(info, members)
+
+	// Set up initial record of machine votes. Any changes after
+	// this will trigger a peer group election.
+	machineVoting := make(map[*machine]bool)
+	for _, m := range info.machines {
+		if member := members[m]; member != nil && isVotingMember(member) {
+			machineVoting[m] = true
+		}
+	}
+	setVoting := func(m *machine, voting bool) {
+		setMemberVoting(members[m], voting)
+		machineVoting[m] = voting
+		changed = true
+	}
+	adjustVotes(toRemoveVote, toAddVote, setVoting)
+
+	addNewMembers(members, toKeep, maxId, setVoting)
+	if updateAddresses(members, info.machines) {
+		changed = true
+	}
+	if !changed {
+		return nil, nil, nil
+	}
+	var memberSet []replicaset.Member
+	for _, member := range members {
+		memberSet = append(memberSet, *member)
+	}
+	return memberSet, machineVoting, nil
+}
+
+func isVotingMember(member *replicaset.Member) bool {
+	return member.Votes == nil || *member.Votes > 0
+}
+
+// possiblePeerGroupChanges returns a set of slices
+// classifying all the existing machines according to
+// how their vote might move.
+// toRemoveVote holds machines whose vote should
+// be removed; toAddVote holds machines which are
+// ready to vote; toKeep holds machines with no desired
+// change to their voting status (this includes machines
+// that are not yet represented in the peer group).
+func possiblePeerGroupChanges(
+	info *peerGroupInfo,
+	members map[*machine]*replicaset.Member,
+) (toRemoveVote, toAddVote, toKeep []*machine) {
+	statuses := info.statusesMap(members)
+
+	for _, m := range info.machines {
+		member := members[m]
+		isVoting := member != nil && isVotingMember(member)
+		switch {
+		case m.wantsVote && isVoting:
+			toKeep = append(toKeep, m)
+		case m.wantsVote && !isVoting:
+			if status, ok := statuses[m]; ok && isReady(status) {
+				toAddVote = append(toAddVote, m)
+			} else {
+				toKeep = append(toKeep, m)
+			}
+		case !m.wantsVote && isVoting:
+			toRemoveVote = append(toRemoveVote, m)
+		case !m.wantsVote && !isVoting:
+			toKeep = append(toKeep, m)
+		}
+	}
+	// sort machines to be added and removed so that we
+	// get deterministic behaviour when testing. Earlier
+	// entries will be dealt with preferentially, so we could
+	// potentially sort by some other metric in each case.
+	sort.Sort(byId(toRemoveVote))
+	sort.Sort(byId(toAddVote))
+	sort.Sort(byId(toKeep))
+	return toRemoveVote, toAddVote, toKeep
+}
+
+// updateAddresses updates the members' addresses from the machines' addresses.
+// It reports whether any changes have been made.
+func updateAddresses(members map[*machine]*replicaset.Member, machines []*machine) bool {
+	changed := false
+	// Make sure all members' machine addresses are up to date.
+	for _, m := range machines {
+		if m.hostPort == "" {
+			continue
+		}
+		// TODO ensure that replicaset works correctly with IPv6 [host]:port addresses.
+		if m.hostPort != members[m].Address {
+			members[m].Address = m.hostPort
+			changed = true
+		}
+	}
+	return changed
+}
+
+// adjustVotes adjusts the votes of the given machines, taking
+// care not to let the total number of votes become even at
+// any time. It calls setVoting to change the voting status
+// of a machine.
+func adjustVotes(toRemoveVote, toAddVote []*machine, setVoting func(*machine, bool)) {
+	// Remove voting members if they can be replaced by
+	// candidates that are ready. This does not affect
+	// the total number of votes.
+	nreplace := min(len(toRemoveVote), len(toAddVote))
+	for i := 0; i < nreplace; i++ {
+		from := toRemoveVote[i]
+		to := toAddVote[i]
+		setVoting(from, false)
+		setVoting(to, true)
+	}
+	toAddVote = toAddVote[nreplace:]
+	toRemoveVote = toRemoveVote[nreplace:]
+
+	// At this point, one or both of toAdd or toRemove is empty, so
+	// we can adjust the voting-member count by an even delta,
+	// maintaining the invariant that the total vote count is odd.
+	if len(toAddVote) > 0 {
+		toAddVote = toAddVote[0 : len(toAddVote)-len(toAddVote)%2]
+		for _, m := range toAddVote {
+			setVoting(m, true)
+		}
+	} else {
+		toRemoveVote = toRemoveVote[0 : len(toRemoveVote)-len(toRemoveVote)%2]
+		for _, m := range toRemoveVote {
+			setVoting(m, false)
+		}
+	}
+}
+
+// addNewMembers adds new members from toKeep
+// to the given set of members, allocating ids from
+// maxId upwards. It calls setVoting to set the voting
+// status of each new member.
+func addNewMembers(
+	members map[*machine]*replicaset.Member,
+	toKeep []*machine,
+	maxId int,
+	setVoting func(*machine, bool),
+) {
+	for _, m := range toKeep {
+		if members[m] == nil && m.hostPort != "" {
+			// This machine was not previously in the members list,
+			// so add it (as non-voting). We maintain the
+			// id manually to make it easier for tests.
+			maxId++
+			member := &replicaset.Member{
+				Tags: map[string]string{
+					"juju-machine-id": m.id,
+				},
+				Id: maxId,
+			}
+			members[m] = member
+			setVoting(m, false)
+		}
+	}
+}
+
+func isReady(status replicaset.MemberStatus) bool {
+	return status.Healthy && (status.State == replicaset.PrimaryState ||
+		status.State == replicaset.SecondaryState)
+}
+
+func setMemberVoting(member *replicaset.Member, voting bool) {
+	if voting {
+		member.Votes = nil
+		member.Priority = nil
+	} else {
+		votes := 0
+		member.Votes = &votes
+		priority := 0.0
+		member.Priority = &priority
+	}
+}
+
+type byId []*machine
+
+func (l byId) Len() int           { return len(l) }
+func (l byId) Swap(i, j int)      { l[i], l[j] = l[j], l[i] }
+func (l byId) Less(i, j int) bool { return l[i].id < l[j].id }
+
+// membersMap returns the replica-set members inside info keyed
+// by machine. Any members that do not have a corresponding
+// machine are returned in extra.
+// The maximum replica-set id is returned in maxId.
+func (info *peerGroupInfo) membersMap() (members map[*machine]*replicaset.Member, extra []replicaset.Member, maxId int) {
+	maxId = -1
+	members = make(map[*machine]*replicaset.Member)
+	for _, member := range info.members {
+		member := member
+		var found *machine
+		if mid, ok := member.Tags["juju-machine-id"]; ok {
+			for _, m := range info.machines {
+				if m.id == mid {
+					found = m
+					break
+				}
+			}
+		}
+		if found != nil {
+			members[found] = &member
+		} else {
+			extra = append(extra, member)
+		}
+		if member.Id > maxId {
+			maxId = member.Id
+		}
+	}
+	return members, extra, maxId
+}
+
+// statusesMap returns the statuses inside info keyed by machine.
+// The provided members map holds the members keyed by machine,
+// as returned by membersMap.
+func (info *peerGroupInfo) statusesMap(members map[*machine]*replicaset.Member) map[*machine]replicaset.MemberStatus {
+	statuses := make(map[*machine]replicaset.MemberStatus)
+	for _, status := range info.statuses {
+		for m, member := range members {
+			if member.Id == status.Id {
+				statuses[m] = status
+				break
+			}
+		}
+	}
+	return statuses
+}
+
+func min(i, j int) int {
+	if i < j {
+		return i
+	}
+	return j
+}

=== added file 'worker/peergrouper/desired_test.go'
--- worker/peergrouper/desired_test.go	1970-01-01 00:00:00 +0000
+++ worker/peergrouper/desired_test.go	2014-02-14 12:14:58 +0000
@@ -0,0 +1,358 @@
+package peergrouper
+
+import (
+	"fmt"
+	"sort"
+	"strconv"
+	"strings"
+	stdtesting "testing"
+
+	gc "launchpad.net/gocheck"
+	"launchpad.net/juju-core/replicaset"
+	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/testing/testbase"
+)
+
+func TestPackage(t *stdtesting.T) {
+	gc.TestingT(t)
+}
+
+type desiredPeerGroupSuite struct {
+	testbase.LoggingSuite
+}
+
+var _ = gc.Suite(&desiredPeerGroupSuite{})
+
+const mongoPort = 1234
+
+var desiredPeerGroupTests = []struct {
+	about    string
+	machines []*machine
+	statuses []replicaset.MemberStatus
+	members  []replicaset.Member
+
+	expectMembers []replicaset.Member
+	expectVoting  []bool
+	expectErr     string
+}{{
+	about:         "single machine, no change",
+	machines:      mkMachines("11v"),
+	members:       mkMembers("1v"),
+	statuses:      mkStatuses("1p"),
+	expectVoting:  []bool{true},
+	expectMembers: nil,
+}, {
+	about:        "extra member with nil Vote",
+	machines:     mkMachines("11v"),
+	members:      mkMembers("1v 2vT"),
+	statuses:     mkStatuses("1p 2s"),
+	expectVoting: []bool{true},
+	expectErr:    "voting non-machine member found in peer group",
+}, {
+	about:    "extra member with >1 votes",
+	machines: mkMachines("11v"),
+	members: append(mkMembers("1v"), replicaset.Member{
+		Id:      2,
+		Votes:   newInt(2),
+		Address: "0.1.2.12:1234",
+	}),
+	statuses:     mkStatuses("1p 2s"),
+	expectVoting: []bool{true},
+	expectErr:    "voting non-machine member found in peer group",
+}, {
+	about:         "new machine with no associated member",
+	machines:      mkMachines("11v 12v"),
+	members:       mkMembers("1v"),
+	statuses:      mkStatuses("1p"),
+	expectVoting:  []bool{true, false},
+	expectMembers: mkMembers("1v 2"),
+}, {
+	about:         "one machine has become ready to vote  (-> no change)",
+	machines:      mkMachines("11v 12v"),
+	members:       mkMembers("1v 2"),
+	statuses:      mkStatuses("1p 2s"),
+	expectVoting:  []bool{true, false},
+	expectMembers: nil,
+}, {
+	about:         "two machines have become ready to vote (-> added)",
+	machines:      mkMachines("11v 12v 13v"),
+	members:       mkMembers("1v 2 3"),
+	statuses:      mkStatuses("1p 2s 3s"),
+	expectVoting:  []bool{true, true, true},
+	expectMembers: mkMembers("1v 2v 3v"),
+}, {
+	about:         "two machines have become ready to vote but one is not healthy (-> no change)",
+	machines:      mkMachines("11v 12v 13v"),
+	members:       mkMembers("1v 2 3"),
+	statuses:      mkStatuses("1p 2s 3sH"),
+	expectVoting:  []bool{true, false, false},
+	expectMembers: nil,
+}, {
+	about:         "three machines have become ready to vote (-> 2 added)",
+	machines:      mkMachines("11v 12v 13v 14v"),
+	members:       mkMembers("1v 2 3 4"),
+	statuses:      mkStatuses("1p 2s 3s 4s"),
+	expectVoting:  []bool{true, true, true, false},
+	expectMembers: mkMembers("1v 2v 3v 4"),
+}, {
+	about:         "one machine ready to lose vote with no others -> no change",
+	machines:      mkMachines("11"),
+	members:       mkMembers("1v"),
+	statuses:      mkStatuses("1p"),
+	expectVoting:  []bool{true},
+	expectMembers: nil,
+}, {
+	about:         "two machines ready to lose vote -> votes removed",
+	machines:      mkMachines("11 12v 13"),
+	members:       mkMembers("1v 2v 3v"),
+	statuses:      mkStatuses("1p 2p 3p"),
+	expectVoting:  []bool{false, true, false},
+	expectMembers: mkMembers("1 2v 3"),
+}, {
+	about:         "machines removed as state server -> removed from members",
+	machines:      mkMachines("11v"),
+	members:       mkMembers("1v 2 3"),
+	statuses:      mkStatuses("1p 2s 3s"),
+	expectVoting:  []bool{true},
+	expectMembers: mkMembers("1v"),
+}, {
+	about:         "a candidate can take the vote of a non-candidate when they're ready",
+	machines:      mkMachines("11v 12v 13 14v"),
+	members:       mkMembers("1v 2v 3v 4"),
+	statuses:      mkStatuses("1p 2s 3s 4s"),
+	expectVoting:  []bool{true, true, false, true},
+	expectMembers: mkMembers("1v 2v 3 4v"),
+}, {
+	about:         "several candidates can take non-candidates' votes",
+	machines:      mkMachines("11v 12v 13 14 15 16v 17v 18v"),
+	members:       mkMembers("1v 2v 3v 4v 5v 6 7 8"),
+	statuses:      mkStatuses("1p 2s 3s 4s 5s 6s 7s 8s"),
+	expectVoting:  []bool{true, true, false, false, false, true, true, true},
+	expectMembers: mkMembers("1v 2v 3 4 5 6v 7v 8v"),
+}, {
+	about: "a changed machine address should propagate to the members",
+	machines: append(mkMachines("11v 12v"), &machine{
+		id:        "13",
+		wantsVote: true,
+		hostPort:  "0.1.99.13:1234",
+	}),
+	statuses:     mkStatuses("1s 2p 3p"),
+	members:      mkMembers("1v 2v 3v"),
+	expectVoting: []bool{true, true, true},
+	expectMembers: append(mkMembers("1v 2v"), replicaset.Member{
+		Id:      3,
+		Address: "0.1.99.13:1234",
+		Tags:    memberTag("13"),
+	}),
+}}
+
+func (*desiredPeerGroupSuite) TestDesiredPeerGroup(c *gc.C) {
+	for i, test := range desiredPeerGroupTests {
+		c.Logf("\ntest %d: %s", i, test.about)
+		info := &peerGroupInfo{
+			machines: test.machines,
+			statuses: test.statuses,
+			members:  test.members,
+		}
+		members, voting, err := desiredPeerGroup(info)
+		if test.expectErr != "" {
+			c.Assert(err, gc.ErrorMatches, test.expectErr)
+			c.Assert(members, gc.IsNil)
+			continue
+		}
+		sort.Sort(membersById(members))
+		c.Assert(members, jc.DeepEquals, test.expectMembers)
+		if len(members) == 0 {
+			continue
+		}
+		for i, m := range info.machines {
+			c.Assert(voting[m], gc.Equals, test.expectVoting[i], gc.Commentf("machine %s", m.id))
+		}
+		// Assure ourselves that the total number of desired votes is odd in
+		// all circumstances.
+		c.Assert(countVotes(members)%2, gc.Equals, 1)
+
+		// Make sure that when the members are set as
+		// required, that there's no further change
+		// if desiredPeerGroup is called again.
+		info.members = members
+		members, voting, err = desiredPeerGroup(info)
+		c.Assert(members, gc.IsNil)
+		c.Assert(voting, gc.IsNil)
+		c.Assert(err, gc.IsNil)
+	}
+}
+
+func countVotes(members []replicaset.Member) int {
+	tot := 0
+	for _, m := range members {
+		v := 1
+		if m.Votes != nil {
+			v = *m.Votes
+		}
+		tot += v
+	}
+	return tot
+}
+
+func newInt(i int) *int {
+	return &i
+}
+
+func newFloat64(f float64) *float64 {
+	return &f
+}
+
+// mkMachines returns a slice of *machine based on
+// the given description.
+// Each machine in the description is white-space separated
+// and holds the decimal machine id followed by an optional
+// "v" if the machine wants a vote.
+func mkMachines(description string) []*machine {
+	descrs := parseDescr(description)
+	ms := make([]*machine, len(descrs))
+	for i, d := range descrs {
+		ms[i] = &machine{
+			id:        fmt.Sprint(d.id),
+			hostPort:  fmt.Sprintf("0.1.2.%d:%d", d.id, mongoPort),
+			wantsVote: strings.Contains(d.flags, "v"),
+		}
+	}
+	return ms
+}
+
+func memberTag(id string) map[string]string {
+	return map[string]string{"juju-machine-id": id}
+}
+
+// mkMembers returns a slice of *replicaset.Member
+// based on the given description.
+// Each member in the description is white-space separated
+// and holds the decimal replica-set id optionally followed by the characters:
+//	- 'v' if the member is voting.
+// 	- 'T' if the member has no associated machine tags.
+// Unless the T flag is specified, the machine tag
+// will be the replica-set id + 10.
+func mkMembers(description string) []replicaset.Member {
+	descrs := parseDescr(description)
+	ms := make([]replicaset.Member, len(descrs))
+	for i, d := range descrs {
+		machineId := d.id + 10
+		m := replicaset.Member{
+			Id:      d.id,
+			Address: fmt.Sprintf("0.1.2.%d:%d", machineId, mongoPort),
+			Tags:    memberTag(fmt.Sprint(machineId)),
+		}
+		if !strings.Contains(d.flags, "v") {
+			m.Priority = newFloat64(0)
+			m.Votes = newInt(0)
+		}
+		if strings.Contains(d.flags, "T") {
+			m.Tags = nil
+		}
+		ms[i] = m
+	}
+	return ms
+}
+
+var stateFlags = map[rune]replicaset.MemberState{
+	'p': replicaset.PrimaryState,
+	's': replicaset.SecondaryState,
+}
+
+// mkStatuses returns a slice of *replicaset.Member
+// based on the given description.
+// Each member in the description is white-space separated
+// and holds the decimal replica-set id optionally followed by the
+// characters:
+// 	- 'H' if the instance is not healthy.
+//	- 'p' if the instance is in PrimaryState
+//	- 's' if the instance is in SecondaryState
+func mkStatuses(description string) []replicaset.MemberStatus {
+	descrs := parseDescr(description)
+	ss := make([]replicaset.MemberStatus, len(descrs))
+	for i, d := range descrs {
+		machineId := d.id + 10
+		s := replicaset.MemberStatus{
+			Id:      d.id,
+			Address: fmt.Sprintf("0.1.2.%d:%d", machineId, mongoPort),
+			Healthy: !strings.Contains(d.flags, "H"),
+			State:   replicaset.UnknownState,
+		}
+		for _, r := range d.flags {
+			if state, ok := stateFlags[r]; ok {
+				s.State = state
+			}
+		}
+		ss[i] = s
+	}
+	return ss
+}
+
+type descr struct {
+	id    int
+	flags string
+}
+
+func isNotDigit(r rune) bool {
+	return r < '0' || r > '9'
+}
+
+var parseDescrTests = []struct {
+	descr  string
+	expect []descr
+}{{
+	descr:  "",
+	expect: []descr{},
+}, {
+	descr:  "0",
+	expect: []descr{{id: 0}},
+}, {
+	descr:  "1foo",
+	expect: []descr{{id: 1, flags: "foo"}},
+}, {
+	descr: "10c  5 6443arble ",
+	expect: []descr{{
+		id:    10,
+		flags: "c",
+	}, {
+		id: 5,
+	}, {
+		id:    6443,
+		flags: "arble",
+	}},
+}}
+
+func (*desiredPeerGroupSuite) TestParseDescr(c *gc.C) {
+	for i, test := range parseDescrTests {
+		c.Logf("test %d. %q", i, test.descr)
+		c.Assert(parseDescr(test.descr), jc.DeepEquals, test.expect)
+	}
+}
+
+// parseDescr parses white-space separated fields of the form
+// <id><flags> into descr structures.
+func parseDescr(s string) []descr {
+	fields := strings.Fields(s)
+	descrs := make([]descr, len(fields))
+	for i, field := range fields {
+		d := &descrs[i]
+		i := strings.IndexFunc(field, isNotDigit)
+		if i == -1 {
+			i = len(field)
+		}
+		id, err := strconv.Atoi(field[0:i])
+		if err != nil {
+			panic(fmt.Errorf("bad field %q", field))
+		}
+		d.id = id
+		d.flags = field[i:]
+	}
+	return descrs
+}
+
+type membersById []replicaset.Member
+
+func (l membersById) Len() int           { return len(l) }
+func (l membersById) Swap(i, j int)      { l[i], l[j] = l[j], l[i] }
+func (l membersById) Less(i, j int) bool { return l[i].Id < l[j].Id }