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.