Improve control of outgoing connection lifecycles #77295

DaveCTurner · 2021-09-06T06:45:18Z

Today we open connections to other nodes in various places and largely
assume that they remain open as needed, only closing them when applying
a cluster state that removes the remote node from the cluster. This
isn't ideal: we might preserve unnecessary connections to remote nodes
that aren't in the cluster if they never manage to join the cluster, and
we might also disconnect from a node that left the cluster while it's in
the process of re-joining too (see #67873).

With this commit we move to a model in which each user of a connection
to a remote node acquires a reference to the connection that must be
released once it's no longer needed. Connections remain open while there
are any live references, but are now actively closed when all references
are released.

Fixes #67873

…nectionsService

elasticmachine · 2021-09-06T06:45:21Z

Pinging @elastic/es-distributed (Team:Distributed)

DaveCTurner

Note to reviewers: it will be easier to read this as a sequence of commits rather than looking just at the finished result.

DaveCTurner · 2021-09-06T06:51:42Z

server/src/main/java/org/elasticsearch/cluster/NodeConnectionsService.java

-    /**
-     * {@link ConnectionTarget} ensures that we are never concurrently connecting to and disconnecting from a node, and that we eventually
-     * either connect to or disconnect from it according to whether {@link ConnectionTarget#connect(ActionListener)} or
-     * {@link ConnectionTarget#disconnect()} was called last.
-     * <p>
-     * Each {@link ConnectionTarget} is in one of these states:
-     * <p>
-     * - idle                       ({@link ConnectionTarget#future} has no listeners)
-     * - awaiting connection        ({@link ConnectionTarget#future} may contain listeners awaiting a connection)
-     * - awaiting disconnection     ({@link ConnectionTarget#future} may contain listeners awaiting a disconnection)
-     * <p>
-     * It will be awaiting connection (respectively disconnection) after calling {@code connect()} (respectively {@code disconnect()}). It
-     * will eventually become idle if these methods are not called infinitely often.
-     * <p>
-     * These methods return a {@link Runnable} which starts the connection/disconnection process iff it was idle before the method was
-     * called, and which notifies any failed listeners if the {@code ConnectionTarget} went from {@code CONNECTING} to {@code DISCONNECTING}
-     * or vice versa. The connection/disconnection process continues until all listeners have been removed, at which point it becomes idle
-     * again.
-     * <p>
-     * Additionally if the last step of the process was a disconnection then this target is removed from the current set of targets. Thus
-     * if this {@link ConnectionTarget} is idle and in the current set of targets then it should be connected.
-     * <p>
-     * All of the {@code listeners} are awaiting the completion of the same activity, which is either a connection or a disconnection.  If
-     * we are currently connecting and then {@link ConnectionTarget#disconnect()} is called then all connection listeners are
-     * removed from the list so they can be notified of failure; once the connecting process has finished a disconnection will be started.
-     * Similarly if we are currently disconnecting and then {@link ConnectionTarget#connect(ActionListener)} is called then all
-     * disconnection listeners are immediately removed for failure notification and a connection is started once the disconnection is
-     * complete.
-     */


Treat the changes to NodeConnectionsService as a complete rewrite (with slightly different semantics, see test changes)

DaveCTurner · 2021-09-06T07:21:04Z

server/src/main/java/org/elasticsearch/discovery/PeerFinder.java

+        // it's using and if we're becoming the master then join validation will hold open the connections to the joining peers; this set of
+        // peers is a quorum so that's good enough.
+        //
+        // Note however that this might still close connections to other master-eligible nodes that we discovered but which aren't currently


This is a change in behaviour vs today. As per the comment I think it's ok, but we can discuss alternatives too.

If we disconnect from the other node, do we risk that node dropping its connection to the master too? If this is in the middle of a join I wonder if it could lead to similar issues except AFAICS only once and thus not an issue.

We discussed in another channel and decided that this won't happen: we're only closing connections that we initiated, the changes here won't have any effect on incoming connections.

DaveCTurner · 2021-09-06T07:24:10Z

server/src/main/java/org/elasticsearch/transport/ClusterConnectionManager.java

+                                    if (connectingRefCounter.hasReferences() == false) {
+                                        logger.trace("connection manager shut down, closing transport connection to [{}]", node);
+                                    } else if (conn.hasReferences()) {
+                                        logger.info("transport connection to [{}] closed by remote", node.descriptionWithoutAttributes());


This also emits logs for connections to nodes in remote clusters. I think that's useful to see, we investigate cases that turn out to be flaky cross-cluster connections at a nonzero rate.

henningandersen

I have been through most of this, but not all the tests yet. This looks great and I like how it simplifies NodeConnectionsService, seems more natural now.

libs/core/src/main/java/org/elasticsearch/core/AbstractRefCounted.java

henningandersen · 2021-09-06T08:15:05Z

server/src/main/java/org/elasticsearch/discovery/PeerFinder.java

+        // it's using and if we're becoming the master then join validation will hold open the connections to the joining peers; this set of
+        // peers is a quorum so that's good enough.
+        //
+        // Note however that this might still close connections to other master-eligible nodes that we discovered but which aren't currently


If we disconnect from the other node, do we risk that node dropping its connection to the master too? If this is in the middle of a join I wonder if it could lead to similar issues except AFAICS only once and thus not an issue.

server/src/test/java/org/elasticsearch/transport/ClusterConnectionManagerTests.java

server/src/main/java/org/elasticsearch/cluster/NodeConnectionsService.java

henningandersen

LGTM.

server/src/internalClusterTest/java/org/elasticsearch/discovery/ClusterDisruptionIT.java

Today we open connections to other nodes in various places and largely assume that they remain open as needed, only closing them when applying a cluster state that removes the remote node from the cluster. This isn't ideal: we might preserve unnecessary connections to remote nodes that aren't in the cluster if they never manage to join the cluster, and we might also disconnect from a node that left the cluster while it's in the process of re-joining too (see elastic#67873). With this commit we move to a model in which each user of a connection to a remote node acquires a reference to the connection that must be released once it's no longer needed. Connections remain open while there are any live references, but are now actively closed when all references are released. Fixes elastic#67873 Backport of elastic#77295

Today we open connections to other nodes in various places and largely assume that they remain open as needed, only closing them when applying a cluster state that removes the remote node from the cluster. This isn't ideal: we might preserve unnecessary connections to remote nodes that aren't in the cluster if they never manage to join the cluster, and we might also disconnect from a node that left the cluster while it's in the process of re-joining too (see #67873). With this commit we move to a model in which each user of a connection to a remote node acquires a reference to the connection that must be released once it's no longer needed. Connections remain open while there are any live references, but are now actively closed when all references are released. Fixes #67873 Backport of #77295

Today if the `PeerFinder` experiences a connection failure then it removes the `Peer` with the corresponding transport address. However the removed `Peer` might be a different instance which is actually fine and holds a connection reference which therefore leaks. With this commit we only remove ourselves from the tracked set of peers. Relates elastic#77295 Closes elastic#79550

Today if the `PeerFinder` experiences a connection failure then it removes the `Peer` with the corresponding transport address. However the removed `Peer` might be a different instance which is actually fine and holds a connection reference which therefore leaks. With this commit we only remove ourselves from the tracked set of peers. Relates #77295 Closes #79550

DaveCTurner added 15 commits September 6, 2021 07:44

Add RefCounted#hasReferences utility

36579d5

Add DiscoveryNode#descriptionWithoutAttributes utility

af67f2d

Add test demonstrating the join loop

5e61b43

Remote connections are special, don't use connectToNode directly

b695875

API change: connectToNode yields a Releasable

d80e09c

Add ref-counting to Transport.Connection

cc07e03

Add onRemoved to lifecycle of Transport.Connection

b59ef40

Integrate refcounting into cluster connection manager

3d4c21e

Fake cluster applier service must still pass connections into NodeCon…

5f46f81

…nectionsService

Properly release connections acquired in PeerFinder

098f444

Properly release connections acquired during join validation

2879448

Acquire and release reference during joining

f63ace5

Rework NodeConnectionsService to use refcounting

76c25ae

Assert that connections are not leaked

f051776

Log remotely-closed connections at INFO

44282fa

DaveCTurner added >bug :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. v8.0.0 v7.16.0 labels Sep 6, 2021

DaveCTurner requested a review from henningandersen September 6, 2021 06:45

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Sep 6, 2021

DaveCTurner commented Sep 6, 2021

View reviewed changes

DaveCTurner requested a review from Tim-Brooks September 6, 2021 06:52

DaveCTurner mentioned this pull request Sep 6, 2021

WIP manage lifecycle of outgoing connections #77253

Closed

DaveCTurner commented Sep 6, 2021

View reviewed changes

henningandersen reviewed Sep 6, 2021

View reviewed changes

Review suggestions

4811a35

kkewwei reviewed Sep 6, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/NodeConnectionsService.java Outdated Show resolved Hide resolved

DaveCTurner added 2 commits September 6, 2021 17:42

Collapse duplicated methods

c8a6902

Merge branch 'master' into 2021-09-06-releasable-connections

6a7d381

henningandersen self-requested a review September 13, 2021 16:17

henningandersen approved these changes Sep 13, 2021

View reviewed changes

server/src/internalClusterTest/java/org/elasticsearch/discovery/ClusterDisruptionIT.java Show resolved Hide resolved

DaveCTurner added 4 commits September 13, 2021 19:29

Merge branch 'master' into 2021-09-06-releasable-connections

1712241

Shorter follower check interval

03df026

Maintain reference to request in DeterministicTaskQueue

bbe1683

Merge branch 'master' into 2021-09-06-releasable-connections

fae5a7f

DaveCTurner merged commit a67e07e into elastic:master Sep 14, 2021

DaveCTurner deleted the 2021-09-06-releasable-connections branch September 14, 2021 05:35

DaveCTurner added the backport pending label Sep 14, 2021

DaveCTurner mentioned this pull request Sep 14, 2021

Improve control of outgoing connection lifecycles #77672

Merged

DaveCTurner removed the backport pending label Sep 14, 2021

This was referenced Sep 14, 2021

[CI] ClusterConnectionManagerTests testConcurrentConnectsAndDisconnects failing #77728

Closed

[CI] DiscoveryDisruptionIT testNodeNotReachableFromMaster failing #77751

Closed

DaveCTurner mentioned this pull request Oct 20, 2021

Only remove active peer on connection failure #79557

Merged

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve control of outgoing connection lifecycles #77295

Improve control of outgoing connection lifecycles #77295

DaveCTurner commented Sep 6, 2021

elasticmachine commented Sep 6, 2021

DaveCTurner left a comment

DaveCTurner Sep 6, 2021

DaveCTurner Sep 6, 2021

henningandersen Sep 6, 2021

DaveCTurner Sep 6, 2021

DaveCTurner Sep 6, 2021

henningandersen left a comment

henningandersen Sep 6, 2021

henningandersen left a comment

Improve control of outgoing connection lifecycles #77295

Improve control of outgoing connection lifecycles #77295

Conversation

DaveCTurner commented Sep 6, 2021

elasticmachine commented Sep 6, 2021

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner Sep 6, 2021

Choose a reason for hiding this comment

DaveCTurner Sep 6, 2021

Choose a reason for hiding this comment

henningandersen Sep 6, 2021

Choose a reason for hiding this comment

DaveCTurner Sep 6, 2021

Choose a reason for hiding this comment

DaveCTurner Sep 6, 2021

Choose a reason for hiding this comment

henningandersen left a comment

Choose a reason for hiding this comment

henningandersen Sep 6, 2021

Choose a reason for hiding this comment

henningandersen left a comment

Choose a reason for hiding this comment