-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make provider reconnectable #4591
Conversation
3fffdb3
to
8a55228
Compare
I think this is going to be leaking connections for long running instance (serve mode). If we always reconnect all connections, they won't be closed properly. The scanner will only close connections that were just scanned. I think this may be solved easily. If you remove the |
aa0e94b
to
6be847a
Compare
I've updated things so that we keep track of which connections rely on which other connections. This information is used to reconnect. If we disconnect a connection and something else relies on it, we keep around the reconnection information until the thing that relies on it is disconnected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One general question that I have is what happens in the following case:
- I connect to the provider with a root asset
- I connect to the provider with child asset of that root
- I scan the root
- Root connection is closed
- Provider crashes
- We trigger a restart + reconnect
After step 6 is done what connection will be open for the restarted provider?
} | ||
|
||
type connectionGraph struct { | ||
nodes map[uint32]connectionGraphNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add some more comments what we store in these properties? It's not immediately clear for me when I see it, so it may help us in case we need to change this in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would also help to describe in text how this is intended to work and put it in a comment:
- when do we add connections to the graph?
- when do we remove connections from the graph?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will add comments here
So assuming there is another asset that needs to be scanned, and it has a connection that depends on the root, first the root connection is recreated, and then the next assets connection. mondoohq/cnspec#1412 has the change to try to make sure the providers are up before starting an asset scan. The root connection is then disconnected automatically once all the connections that depend on it are disconnected |
157a06e
to
29fa5a8
Compare
Makes providers reconnectable. Keeps track of all connect requests to a provider. If it crashes, and is restarted, all the connect requests are replayed. Disconnected connections are also recreated because they can be a parent connect, and its non-existence can cause a child to fail to connect