state/api: Client connection health monitoring

state/api: Client connection health monitoring

Added srvState.Ping() and a health checking
goroutine that periodically calls is on any API
client connection. When a connection is broken,
a channel, returned by State.Broken() is closed,
thus notifying of the event.

This will be used in a follow-up to monitor API
client connections vitality, allowing us to detect
quickly when a connection is dead.

https://code.launchpad.net/~dimitern/juju-core/049-apiserver-support-heartbeat-detection/+merge/165900

(do not edit description out of merge proposal)

[dimitern, 2013-05-27 15:32:49]

Please take a look.

[dimitern, 2013-05-27 15:34:53]

Please take a look.

[fwereade, 2013-05-27 21:11:25]

NOT LGTM as is -- I think the Deadline stuff is (1) dangerous and (2) kinda a
red herring at the moment: I'd be fine with something that just took any Ping
error to be a problem (and maybe had a ping-specific deadline that didn't mess
with the conn).

Sorry, I think I may have led you astray in our earlier discussions :(.

https://codereview.appspot.com/9811044/diff/2002/rpc/jsoncodec/codec.go
File rpc/jsoncodec/codec.go (right):

https://codereview.appspot.com/9811044/diff/2002/rpc/jsoncodec/codec.go#newco...
rpc/jsoncodec/codec.go:105: if err, ok := err.(*net.OpError); ok {
Shouldn't this be .(net.Error)?

(and how does it help, anyway?)

https://codereview.appspot.com/9811044/diff/2002/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9811044/diff/2002/state/api/apiclient.go#newco...
state/api/apiclient.go:106: return s.conn.SetDeadline(t)
This looks somewhat goroutine-unsafe.

https://codereview.appspot.com/9811044/diff/2002/state/api/apierror.go
File state/api/apierror.go (right):

https://codereview.appspot.com/9811044/diff/2002/state/api/apierror.go#newcode52
state/api/apierror.go:52: func IsTimeout(err error) bool {
Not sure why we need this either. Surely any error from Ping is good enough to
know we've got a broken connection?

https://codereview.appspot.com/9811044/diff/2002/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/9811044/diff/2002/state/api/state.go#newcode20
state/api/state.go:20: return st.call("State", "", "Ping", nil, nil)
I don't think we should expose this on the client. I was expecting a *State to
expose some method that let us know when the connection was broken, rather than
having to hand-hack everything in the client; thoughts?

https://codereview.appspot.com/9811044/diff/2002/state/apiserver/state.go
File state/apiserver/state.go (right):

https://codereview.appspot.com/9811044/diff/2002/state/apiserver/state.go#new...
state/apiserver/state.go:17: func (r *srvState) Ping() error {
This is fine though.

[dimitern, 2013-05-28 06:32:20]

I'll remove SetDeadline related stuff and add an internal goroutine which pings
and notifies on connection issues.

https://codereview.appspot.com/9811044/diff/2002/rpc/jsoncodec/codec.go
File rpc/jsoncodec/codec.go (right):

https://codereview.appspot.com/9811044/diff/2002/rpc/jsoncodec/codec.go#newco...
rpc/jsoncodec/codec.go:105: if err, ok := err.(*net.OpError); ok {
On 2013/05/27 21:11:25, fwereade wrote:
> Shouldn't this be .(net.Error)?
> 
> (and how does it help, anyway?)

net.OpError is the type that's used to report timeouts and other network errors.

https://codereview.appspot.com/9811044/diff/2002/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9811044/diff/2002/state/api/apiclient.go#newco...
state/api/apiclient.go:106: return s.conn.SetDeadline(t)
On 2013/05/27 21:11:25, fwereade wrote:
> This looks somewhat goroutine-unsafe.

Removed.

https://codereview.appspot.com/9811044/diff/2002/state/api/apierror.go
File state/api/apierror.go (right):

https://codereview.appspot.com/9811044/diff/2002/state/api/apierror.go#newcode52
state/api/apierror.go:52: func IsTimeout(err error) bool {
On 2013/05/27 21:11:25, fwereade wrote:
> Not sure why we need this either. Surely any error from Ping is good enough to
> know we've got a broken connection?

Fair point, the timeout was needed mainly because SetDeadline. Removed.

https://codereview.appspot.com/9811044/diff/2002/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/9811044/diff/2002/state/api/state.go#newcode20
state/api/state.go:20: return st.call("State", "", "Ping", nil, nil)
On 2013/05/27 21:11:25, fwereade wrote:
> I don't think we should expose this on the client. I was expecting a *State to
> expose some method that let us know when the connection was broken, rather
than
> having to hand-hack everything in the client; thoughts?

Well, rpc.Conn has "closing" and "shutdown" fields we could expose. But I was
thinking more of a gouroutine that spuns up internally and calls ping
periodically, closing a channel when the ping fails. The channel will be
available as a method on the client. See the next patch set.

[dimitern, 2013-05-28 07:02:45]

Please take a look.

[fwereade, 2013-05-28 07:11:21]

a few comments still

https://codereview.appspot.com/9811044/diff/12001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9811044/diff/12001/state/api/apiclient.go#newc...
state/api/apiclient.go:20: const PingFrequency = 5 * time.Second
PingPeriod please.

https://codereview.appspot.com/9811044/diff/12001/state/api/apiclient.go#newc...
state/api/apiclient.go:127: // SetDeadlines set the connection's network read
and write deadlines.
I'd prefer something like Broken rather than Closed... maybe that's a bikeshed,
but we can in theory be broken without actually being closed... right?

https://codereview.appspot.com/9811044/diff/12001/state/api/apierror.go
File state/api/apierror.go (right):

https://codereview.appspot.com/9811044/diff/12001/state/api/apierror.go#newcode6
state/api/apierror.go:6: import "launchpad.net/juju-core/rpc"
I'm +-1 on this change, doesn't really feel worth it to me, but up to you.

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test.go
File state/apiserver/state_test.go (right):

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test....
state/apiserver/state_test.go:152: st := s.openAs(c, stm.Tag())
defer st.Close()

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test....
state/apiserver/state_test.go:156: case <-time.After(50 * time.Millisecond):
Maybe best to wait at least one ping period?

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test....
state/apiserver/state_test.go:163: st.Close()
Can we just go st.Close()?

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test....
state/apiserver/state_test.go:167: for {
Do we need to loop here?

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test....
state/apiserver/state_test.go:169: case <-time.After(api.PingFrequency +
time.Second):
Let's make PingFrequency shorter for this whole test.

[dimitern, 2013-05-28 07:22:55]

Please take a look.

https://codereview.appspot.com/9811044/diff/12001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9811044/diff/12001/state/api/apiclient.go#newc...
state/api/apiclient.go:20: const PingFrequency = 5 * time.Second
On 2013/05/28 07:11:21, fwereade wrote:
> PingPeriod please.

Done.

https://codereview.appspot.com/9811044/diff/12001/state/api/apiclient.go#newc...
state/api/apiclient.go:127: // SetDeadlines set the connection's network read
and write deadlines.
On 2013/05/28 07:11:21, fwereade wrote:
> I'd prefer something like Broken rather than Closed... maybe that's a
bikeshed,
> but we can in theory be broken without actually being closed... right?

Done.

https://codereview.appspot.com/9811044/diff/12001/state/api/apierror.go
File state/api/apierror.go (right):

https://codereview.appspot.com/9811044/diff/12001/state/api/apierror.go#newcode6
state/api/apierror.go:6: import "launchpad.net/juju-core/rpc"
On 2013/05/28 07:11:21, fwereade wrote:
> I'm +-1 on this change, doesn't really feel worth it to me, but up to you.

I prefer it - no need for extra lines.

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test.go
File state/apiserver/state_test.go (right):

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test....
state/apiserver/state_test.go:152: st := s.openAs(c, stm.Tag())
On 2013/05/28 07:11:21, fwereade wrote:
> defer st.Close()

Done.

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test....
state/apiserver/state_test.go:156: case <-time.After(50 * time.Millisecond):
On 2013/05/28 07:11:21, fwereade wrote:
> Maybe best to wait at least one ping period?

Done.

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test....
state/apiserver/state_test.go:163: st.Close()
On 2013/05/28 07:11:21, fwereade wrote:
> Can we just go st.Close()?

Done.

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test....
state/apiserver/state_test.go:167: for {
On 2013/05/28 07:11:21, fwereade wrote:
> Do we need to loop here?

Done.

https://codereview.appspot.com/9811044/diff/12001/state/apiserver/state_test....
state/apiserver/state_test.go:169: case <-time.After(api.PingFrequency +
time.Second):
On 2013/05/28 07:11:21, fwereade wrote:
> Let's make PingFrequency shorter for this whole test.

Done.

[fwereade, 2013-05-28 07:25:50]

LGTM, thanks

[mue, 2013-05-28 07:43:04]

LGTM, only one naming hint.

https://codereview.appspot.com/9811044/diff/17001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9811044/diff/17001/state/api/apiclient.go#newc...
state/api/apiclient.go:105: func (s *State) healthMonitor() {
Typically know that as "heartbeat" (well known term in HA environments).

[dimitern, 2013-05-28 07:49:16]

*** Submitted:

state/api: Client connection health monitoring

Added srvState.Ping() and a health checking
goroutine that periodically calls is on any API
client connection. When a connection is broken,
a channel, returned by State.Broken() is closed,
thus notifying of the event.

This will be used in a follow-up to monitor API
client connections vitality, allowing us to detect
quickly when a connection is dead.

R=fwereade, mue
CC=
https://codereview.appspot.com/9811044

https://codereview.appspot.com/9811044/diff/17001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9811044/diff/17001/state/api/apiclient.go#newc...
state/api/apiclient.go:105: func (s *State) healthMonitor() {
On 2013/05/28 07:43:04, mue wrote:
> Typically know that as "heartbeat" (well known term in HA environments).

Didn't sound right, but I guess you're right. Changed.