Repository navigation
Propagate networking connection error handling into NodeConnection error handling #224
Description
Specification
When errors are encountered in the network domain for a particular connection, we expect these errors to propagate up to the higher-level NodeConnection. In this process, an active NodeConnection object that experiences a lower-level failure from the network domain must also be destroyed and garbage collected, as the underlying connection is no longer valid.
As per Roger's comment here #224 (comment), the network domain can be seen to be operating in a separate space to the more procedural elements of the source code. The network components are singleton instances that operate and handle errors in isolation. So, how do we propagate these errors to the higher level NodeConnection?
So the problem here is that the
NodeManagerorNodeConnectionusesGRPCClientAgentwhich extends fromGRPCClient.The
GRPCClientis what encapsulates the underlyingClientobject provided bygrpc-js.At this current point in time, in the source code for
GRPCClient, there's no provision for asynchronous handlers in the case where the GRPC connection breaks.The GRPC connection can break when the networking domain has an error, or this may also happen in #276.
But there would no way of handling this error event automatically. Instead I believe an exception would only occur at the point of calling another client method. This is obviously not suitable for our
NodeManageras we need to handle when the connections are dead and garbage collect them.We need to investigate the source code, issues and docs for
grpc-jshttps://github.com/grpc/grpc-node/blob/master/packages/grpc-js/src/client.ts to see where we can add a handler for these GRPC client problems.If we cannot do this, the alternative is to do interval pings, which are unary calls and this should trigger an exception if the connection has been broken. We already do this inside the
networkdomain at a low level. But this would a GRPC-level ping, which uses a full GRPC message. This would trigger exceptions to occur as well.Also in reference to the #225, we have to make a choice of whether
NodeManageris the primary way to access, orNodeConnection. If users are usingNodeConnection, then it would be expected that they callNodeManagerto reconnect if there are problems. It seemsNodeManageris the better choice, but this makes it a different architecture from how we are doing it inVaultManager. Which is fine, because vaults do not represent objects over a network. Networks are always more flakier. So we don't expect one to lose a connection to a vault. But we do expect losing connections to nodes. Which is why it may be better to haveNodeManagerhandle all the complexity.Check this https://github.com/grpc/grpc-node/search?q=connection+drop+handle&type=issues
Additional context
- Refactoring Nodes Domain: NodeConnection & NodeConnectionManager and Fixing Bugs #225 (comment) original comment about potentially implementing error handlers at the
NodeConnectionlevel
Tasks
- Investigate the current network connection errors that arise in the lower-level
networkdomain: how do these arise? Where do they currently bubble up to? - Investigate existing code, issues, and docs for
grpc-jsfor how to handle connection errors. Are there existing functions that could provide error handlers? - If no handlers exist at the GRPC level, look into how to perform internal pings (very similar to the keep-alive packets in the lower level
networkdomain). We expect that these exceptions may only arise after a call is made, so these internal pings will be a means of assessing the connection status (i.e. no GRPC call error, connection is still active; GRPC call error, connection is no longer active, so destroyNodeConnectionobject)
Activity
[-]Handle errors for agent connection (losing connection, failing to connect)[/-][+]Test error handling at the GRPC client/server sites for connection problems in the networking domain (losing connection, failing to connect)[/+]CMCDragonkai commentedon Aug 30, 2021
We must add tests that break the
networkdomain connections while a unary calls and streaming calls are taking place and observe and apply tests on that behaviour. I'm expecting some sort of exception to take place on the nextawait/promise at the GRPC client stub and service handler simultaneously. But this should be tested and properly speced out.CMCDragonkai commentedon Oct 26, 2021
Issue template is a bug here, but should this be a development spec?
[-]Test error handling at the GRPC client/server sites for connection problems in the networking domain (losing connection, failing to connect)[/-][+]Propagate networking connection error handling into NodeConnection error handling[/+]joshuakarp commentedon Oct 27, 2021
As I've started to spec this issue out a bit more, I remembered that I'd actually already made a comment about error handling at the
NodeConnectionlevel on our issue on restructuring the nodes domain, where I talk about incorporating handlers at thenodeslevel: #225 (comment)joshuakarp commentedon Oct 27, 2021
@CMCDragonkai I initially created this issue in August with respect to this comment from you: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/198#note_644652543
Is propagating the errors to a generic
AgentConnectionErroractually what this issue should be doing?CMCDragonkai commentedon Oct 27, 2021
You should be able to catch all networking exceptions in the nodes domain.
CMCDragonkai commentedon Oct 27, 2021
Regarding
AgentConnectionError, in that comment, I meant that there should be a general error for losing connection when calling from the PK Client CLI or GUI to the PK Agent. So it's about losing connection to the client service.Whereas this issue is about losing connection to the PK agent between node to node. And this requires integrating with the networking domain.
Losing connection to client service and client server does not use the networking domain. It's just straightforward from a GRPC error that we need catch and convert into a domain exception in the
src/client/errors.ts. That's a separate problem. This should be a separate issue. Note that this may already be covered bysrc/grpc/errors.ts:The PK CLI commands are going to be using the
PolykeyClientwhich usessrc/client. Losing connection to the agent should have a domain exception insrc/client/errors. So the above GRPC exceptions should be converted.There should be a good human readable message for this kind of exception when
pkcommands are executed on the terminal.New issue in #276.
CMCDragonkai commentedon Oct 27, 2021
So the problem here is that the
NodeManagerorNodeConnectionusesGRPCClientAgentwhich extends fromGRPCClient.The
GRPCClientis what encapsulates the underlyingClientobject provided bygrpc-js.At this current point in time, in the source code for
GRPCClient, there's no provision for asynchronous handlers in the case where the GRPC connection breaks.The GRPC connection can break when the networking domain has an error, or this may also happen in #276.
But there would no way of handling this error event automatically. Instead I believe an exception would only occur at the point of calling another client method. This is obviously not suitable for our
NodeManageras we need to handle when the connections are dead and garbage collect them.We need to investigate the source code, issues and docs for
grpc-jshttps://github.com/grpc/grpc-node/blob/master/packages/grpc-js/src/client.ts to see where we can add a handler for these GRPC client problems.If we cannot do this, the alternative is to do interval pings, which are unary calls and this should trigger an exception if the connection has been broken. We already do this inside the
networkdomain at a low level. But this would a GRPC-level ping, which uses a full GRPC message. This would trigger exceptions to occur as well.Also in reference to the #225, we have to make a choice of whether
NodeManageris the primary way to access, orNodeConnection. If users are usingNodeConnection, then it would be expected that they callNodeManagerto reconnect if there are problems. It seemsNodeManageris the better choice, but this makes it a different architecture from how we are doing it inVaultManager. Which is fine, because vaults do not represent objects over a network. Networks are always more flakier. So we don't expect one to lose a connection to a vault. But we do expect losing connections to nodes. Which is why it may be better to haveNodeManagerhandle all the complexity.Check this https://github.com/grpc/grpc-node/search?q=connection+drop+handle&type=issues
joshuakarp commentedon Nov 11, 2021
Recall our TCP connection between the
ForwardProxyandGRPCClient, as per:Network diagram
We'd discussed earlier that the networking components of Polykey can be treated as a completely separate component of the system. They have no knowledge of GRPC, etc.
However, the
GRPCClientandForwardProxyare connected through a "a legitimate fat free 100% natural TCP connection" (Roger's words). This means that an explicit close throughFIN/ACKis propagated from theForwardProxyto theGRPCClient, and theGRPCClientcan close the connection. This is different from an exception arising (e.g. a dropped connection), which cannot be propagated.This also means that the
NodeConnectionneeds to be garbage collected when this explicit close occurs. Roger mentioned that there should be a callback thatgrpc-jsexposes in order to handle closing.waitForReadypasses BUT no calls are made and GRPC server side closes? Do the keepAlives kick in? What if the server does an explicit close?ForwardProxyandReverseProxy-ReverseProxy.closeConnectionperforms a force close, but this is not from GRPC, but from the networking connection. Upon closing it here, both the GRPC client and GRPC server (on client side) see that the other side has closed.GRPCClient? If so, we can add a callback there: https://grpc.github.io/grpc/node/grpc.html#~getClientChannel__anchor and https://grpc.github.io/grpc/node/grpc.Client.html#getChannel (client.getChannel())ChannelOptionsobject, with these possible options https://grpc.github.io/grpc/core/group__grpc__arg__keys.html8 remaining items
joshuakarp commentedon Feb 1, 2022
I've made a primitive start to mapping the various transitions for
NodeConnectionstate. Still unfinished, but gives an idea of the structure of the diagram, as per discussion from #310 (comment). I'll keep comments concerning its progress in this issue, to keep #310 as clear as possible.The more I create this, the more I'm inclined to giving the states a proper title too (as opposed to numbering them). Obviously though, this is still a WIP.
Details
joshuakarp commentedon Feb 1, 2022
Will be able to use the completed diagram to also check that we have adequate testing for each path of a
NodeConnectionlifecycle. The logging outputs inNodeConnection.test.tsare especially useful to check the transitions between client state here,joshuakarp commentedon Feb 7, 2022
In the process of writing up a state diagram in #224, there's an interesting difference in behaviour with connection shutdown and corresponding client status that I'm trying to make sense of and elucidate through the diagram. This follows on from the following comments:
#310 (comment)
and #310 (comment)
The above 2 comments refer to the transitions when we move to
IDLEafter a connection shutdown. However, there's also a transition where we move toSHUTDOWN. I'm wanting to make this process clear. We have 2 different tests that demonstrate these 2 scenarios:connects to its target (via direct connection)Log output:
Details
Important part:
Full log:
should call 'killSelf' - if the Agent is stoppedLog output:
Details
Important part:
Full log:
So as a result of this:
IDLESHUTDOWNjoshuakarp commentedon Feb 8, 2022
From discussions this afternoon, there's a few extra states and transitions that need to be verified.
Some first remarks:
NodeConnectionManagerlevel (specified byconnTimeoutTime)The states and transitions to explore:
IDLE, but nothing else should happen.IDLEclient (i.e. an expired TTL), we expect the client to transition toREADYafter a GRPC call is madeREADYclient (seeNodeConnectionManager.test.tsandshould time out a connection)IDLE. The following:ConnectionReverseis closed (i.e. called when the server agent shuts down), and therefore is verified by theshould call 'killSelf' - if the Agent is stoppedtest (see above comment)end()anddestroy()on the socket, give a timeout for 1 second. We expect the client to goIDLEas well. The following:joshuakarp commentedon Feb 8, 2022
@tegefaulkes mentioned that he previously experimented with the TTL at the GRPC level, but it seemed to take more than 6 minutes for the client's state to transition to
IDLEon its own. He believes there is a way to set the timeout manually, but would need some further investigation.joshuakarp commentedon Feb 8, 2022
Got a final first draft of the connection state diagram:
Details
Some notes:
IDLEorSHUTDOWN- see Propagate networking connection error handling into NodeConnection error handling #224 (comment)). Can potentially figure out a neater way of representing this in the diagram, but we can consider this as the first version of the connection state diagramjoshuakarp commentedon Feb 9, 2022
@tegefaulkes made the comment that for the bottom left
Destroyed connection, the channel watcher shuts down before we show the actualSHUTDOWNof the client. So in actual fact, it's likely that we do go fromIDLEtoSHUTDOWNafter the connection is closed from the server-side. As a result, we can possibly synthesis the twoDestroyed connectionstates in this way.Another comment was whether the
statusfield in each of the states was valid at all.NodeConnectionis just a container class for the underlying client, so it doesn't make a huge amount of sense to consider whether theNodeConnectionis "starting" or "destroying".joshuakarp commentedon Feb 9, 2022
So in actual fact, if we assume that we have already seen the
CONNECTINGandREADYclient states, then at the moment, any later transition toIDLEwill kill off the connection. This is how it's currently implemented with the channel watcher.At the moment anyway, our internal TTL in
NodeConnectionManageris longer than the GRPC TTL (@tegefaulkes played with this and said it's something like 6 minutes), so it's not possible to reach the GRPC TTL and move fromREADYtoIDLE. But, if we were to see that transition fromREADYtoIDLE, it would kill the connection.joshuakarp commentedon Feb 9, 2022
Based on feedback, a somewhat simpler version:
Details
CMCDragonkai commentedon Feb 11, 2022
So
NodeConnectionManager.test.stcan be separated into smaller test modules. But forNodeConnection.test.ts, the naming of the test functions should be related to the above transition and state names, so it's easy for one to follow from the model to the actual tests to know if a transition is being tested.CMCDragonkai commentedon Feb 11, 2022
@tegefaulkes please try splitting the test modules atm, especially the nested describe. Do some basic renaming, and further renaming can be done later after the merge.
CMCDragonkai commentedon Feb 14, 2022
#332 has been created as a continuation of verifying the
READYtoIDLEtransition.