state/api/agent: added IsMaster to the API

state/api/agent: added IsMaster to the API

IsMaster provides a way to let an API worker know that it is on the same
machine as the API server that has a mongo peer address that is the primary
mongo server for a given replicaset.

https://code.launchpad.net/~wwitzel3/juju-core/003-ha-mongo-master-api/+merge/213635

(do not edit description out of merge proposal)

[wwitzel, 2014-04-01 11:32:28]

Please take a look.

[wwitzel3, 2014-04-01 11:38:18]

https://codereview.appspot.com/83150043/diff/1/state/api/agent/machine_test.go
File state/api/agent/machine_test.go (right):

https://codereview.appspot.com/83150043/diff/1/state/api/agent/machine_test.g...
state/api/agent/machine_test.go:36: func fakeMasterHostPort(session
*mgo.Session) (string, error) {
This is never used and can be remove.

[hduran, 2014-04-01 11:56:13]

https://codereview.appspot.com/83150043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/83150043/diff/1/agent/mongo/mongo.go#newcode66
agent/mongo/mongo.go:66: var SelectPeerAddress = selectPeerAddress
I find this to be more "generic" (I did not checked the syntax)
But I do not know what is the "go" est way to do, so here is my suggestion

var DefaultPeerAddressSelector = func(adds []instance.Address) string { return
instance.SelectInternalAddress(addrs, false) }

func SelectPeerAddress(addrs []instance.Address) string{
return DefaultPeerAddressSelector(addrs)
}

[rog, 2014-04-01 12:00:14]

Looks great in general, thanks. Some suggestions below.

https://codereview.appspot.com/83150043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/83150043/diff/1/agent/mongo/mongo.go#newcode66
agent/mongo/mongo.go:66: var SelectPeerAddress = selectPeerAddress
I don't think we need to make this into a variable - we can just pass some
addresses in and check that the result matches what we would expect had
SelectPeerAddress been called.

https://codereview.appspot.com/83150043/diff/1/replicaset/replicaset.go
File replicaset/replicaset.go (right):

https://codereview.appspot.com/83150043/diff/1/replicaset/replicaset.go#newco...
replicaset/replicaset.go:247: var MasterHostPort = masterHostPort
again, i'd define this as a normal function.
if we need to mock it, i'd define

var masterHostPort = MasterHostPort

and call that in the place where it needs to be mocked.

That way, the godoc for the package looks normal.

https://codereview.appspot.com/83150043/diff/1/state/api/agent/state.go
File state/api/agent/state.go (right):

https://codereview.appspot.com/83150043/diff/1/state/api/agent/state.go#newco...
state/api/agent/state.go:50: var results params.IsMasterResult
s/results/result/

same above for StateServingInfo, now I see it.

https://codereview.appspot.com/83150043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/83150043/diff/1/state/api/params/params.go#new...
state/api/params/params.go:472: // which represents if a Machines Peer address
is pointing
perhaps:

// IsMasterResult holds the result of an IsMaster call.
type IsMasterResult struct {
    // Master reports whether a Machine's peer address is
    // pointing to the primary mongo server of a replica set.
    Master bool
}

?

[wwitzel, 2014-04-01 13:58:47]

Please take a look.

[wwitzel3, 2014-04-01 14:02:03]

https://codereview.appspot.com/83150043/diff/2/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/83150043/diff/2/agent/mongo/mongo.go#newcode35
agent/mongo/mongo.go:35: type ImplementsAddresses interface {
I wasn't sure what to name this so it was public friendly. I needed it public
for my test so I could PatchValue IsMaster from state/apiserver/agent

https://codereview.appspot.com/83150043/diff/2/state/apiserver/agent/agent.go
File state/apiserver/agent/agent.go (right):

https://codereview.appspot.com/83150043/diff/2/state/apiserver/agent/agent.go...
state/apiserver/agent/agent.go:86: var MongoIsMaster = mongo.IsMaster
I override this in state/api/agent/machine_test.go

[rog, 2014-04-01 17:44:18]

LGTM with the following points addressed. Thanks!

https://codereview.appspot.com/83150043/diff/2/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/83150043/diff/2/agent/mongo/mongo.go#newcode40
agent/mongo/mongo.go:40: // machines peer address is the primary mongo host for
the replicaset
s/machines/machine's/

https://codereview.appspot.com/83150043/diff/2/agent/mongo/mongo.go#newcode54
agent/mongo/mongo.go:54: logger.Infof("masterAddr: %v", masterAddr)
d

https://codereview.appspot.com/83150043/diff/2/agent/mongo/mongo.go#newcode57
agent/mongo/mongo.go:57: logger.Infof("machinePeerAddr: %v", machinePeerAddr)
d

https://codereview.appspot.com/83150043/diff/2/state/api/agent/machine_test.go
File state/api/agent/machine_test.go (right):

https://codereview.appspot.com/83150043/diff/2/state/api/agent/machine_test.g...
state/api/agent/machine_test.go:60: return true, nil
how about setting a variable here, so we can be sure that it has actually been
called?
(true isn't a very unique value)

https://codereview.appspot.com/83150043/diff/2/state/api/agent/state.go
File state/api/agent/state.go (right):

https://codereview.appspot.com/83150043/diff/2/state/api/agent/state.go#newco...
state/api/agent/state.go:49: func (st *State) IsMaster() (params.IsMasterResult,
error) {
The comment on IsMaster in the server should live here instead - this is the
public face of the API.

Also, please make it return bool, not IsMasterResult.

https://codereview.appspot.com/83150043/diff/2/state/apiserver/agent/agent.go
File state/apiserver/agent/agent.go (right):

https://codereview.appspot.com/83150043/diff/2/state/apiserver/agent/agent.go...
state/apiserver/agent/agent.go:85: // MongoIsMaster existis to be overridden in
tests
s/existis/exist/

// MongoIsMaster is called by the IsMaster API call
// instead of mongo.IsMaster. It exists so it can
// be overridden by tests.

?

https://codereview.appspot.com/83150043/diff/2/state/apiserver/agent/agent.go...
state/apiserver/agent/agent.go:88: // IsMaster returns a IsMasterResult, that
result contains
s/, that/. That/

[wwitzel3, 2014-04-01 18:34:45]

https://codereview.appspot.com/83150043/diff/2/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/83150043/diff/2/agent/mongo/mongo.go#newcode35
agent/mongo/mongo.go:35: type ImplementsAddresses interface {
On 2014/04/01 14:02:03, wwitzel3 wrote:
> I wasn't sure what to name this so it was public friendly. I needed it public
> for my test so I could PatchValue IsMaster from state/apiserver/agent

Done.

https://codereview.appspot.com/83150043/diff/2/agent/mongo/mongo.go#newcode40
agent/mongo/mongo.go:40: // machines peer address is the primary mongo host for
the replicaset
On 2014/04/01 17:44:19, rog wrote:
> s/machines/machine's/

Done.

https://codereview.appspot.com/83150043/diff/2/agent/mongo/mongo.go#newcode54
agent/mongo/mongo.go:54: logger.Infof("masterAddr: %v", masterAddr)
On 2014/04/01 17:44:19, rog wrote:
> d

Done.

https://codereview.appspot.com/83150043/diff/2/agent/mongo/mongo.go#newcode57
agent/mongo/mongo.go:57: logger.Infof("machinePeerAddr: %v", machinePeerAddr)
On 2014/04/01 17:44:19, rog wrote:
> d

Done.

https://codereview.appspot.com/83150043/diff/2/state/api/agent/machine_test.go
File state/api/agent/machine_test.go (right):

https://codereview.appspot.com/83150043/diff/2/state/api/agent/machine_test.g...
state/api/agent/machine_test.go:60: return true, nil
On 2014/04/01 17:44:19, rog wrote:
> how about setting a variable here, so we can be sure that it has actually been
> called?
> (true isn't a very unique value)

Done.

https://codereview.appspot.com/83150043/diff/2/state/api/agent/state.go
File state/api/agent/state.go (right):

https://codereview.appspot.com/83150043/diff/2/state/api/agent/state.go#newco...
state/api/agent/state.go:49: func (st *State) IsMaster() (params.IsMasterResult,
error) {
On 2014/04/01 17:44:19, rog wrote:
> The comment on IsMaster in the server should live here instead - this is the
> public face of the API.
> 
> Also, please make it return bool, not IsMasterResult.

Done.

https://codereview.appspot.com/83150043/diff/2/state/apiserver/agent/agent.go
File state/apiserver/agent/agent.go (right):

https://codereview.appspot.com/83150043/diff/2/state/apiserver/agent/agent.go...
state/apiserver/agent/agent.go:85: // MongoIsMaster existis to be overridden in
tests
On 2014/04/01 17:44:19, rog wrote:
> s/existis/exist/
> 
> // MongoIsMaster is called by the IsMaster API call
> // instead of mongo.IsMaster. It exists so it can
> // be overridden by tests.
> 
> ?

Done.

https://codereview.appspot.com/83150043/diff/2/state/apiserver/agent/agent.go...
state/apiserver/agent/agent.go:88: // IsMaster returns a IsMasterResult, that
result contains
On 2014/04/01 17:44:19, rog wrote:
> s/, that/. That/

Done.

[wwitzel, 2014-04-01 18:36:32]

Please take a look.

[rog, 2014-04-02 15:38:51]

LGTM still, with a few doc comment suggestions.

https://codereview.appspot.com/83150043/diff/20001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/83150043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:35: type WithAddresses interface {
// WithAddresses represents an entity that has a set of
// addresses, usually a state Machine object.

?

https://codereview.appspot.com/83150043/diff/20001/state/api/agent/state.go
File state/api/agent/state.go (right):

https://codereview.appspot.com/83150043/diff/20001/state/api/agent/state.go#n...
state/api/agent/state.go:49: // IsMaster returns a boolean which represents if
the current mongo peer
// IsMaster reports whether the connected machine
// agent lives at the same network address as the primary
// mongo server for the replica set.
// This call will return an error if the connected
// agent is not a machine agent with environment-manager
// privileges.

?

https://codereview.appspot.com/83150043/diff/20001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/83150043/diff/20001/state/api/params/params.go...
state/api/params/params.go:474: // pointing to the mongo primary of the replica
set.
// Master reports whether the connected agent
// lives on the same instance as the mongo replica
// set master.

?

[wwitzel3, 2014-04-02 15:45:18]

https://codereview.appspot.com/83150043/diff/20001/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/83150043/diff/20001/agent/mongo/mongo.go#newco...
agent/mongo/mongo.go:35: type WithAddresses interface {
On 2014/04/02 15:38:51, rog wrote:
> // WithAddresses represents an entity that has a set of
> // addresses, usually a state Machine object.
> 
> ?

Done.

https://codereview.appspot.com/83150043/diff/20001/state/api/agent/state.go
File state/api/agent/state.go (right):

https://codereview.appspot.com/83150043/diff/20001/state/api/agent/state.go#n...
state/api/agent/state.go:49: // IsMaster returns a boolean which represents if
the current mongo peer
On 2014/04/02 15:38:51, rog wrote:
> // IsMaster reports whether the connected machine
> // agent lives at the same network address as the primary
> // mongo server for the replica set.
> // This call will return an error if the connected
> // agent is not a machine agent with environment-manager
> // privileges.
> 
> ?

Done.

https://codereview.appspot.com/83150043/diff/20001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/83150043/diff/20001/state/api/params/params.go...
state/api/params/params.go:474: // pointing to the mongo primary of the replica
set.
On 2014/04/02 15:38:51, rog wrote:
> // Master reports whether the connected agent
> // lives on the same instance as the mongo replica
> // set master.
> 
> ?

Done.