Do not report failure after connections are made

[ywangd]

Today, when the number of attempts is exhausted, ProxyConnectionStrategy checks the number of connections before returns. It reports connection failure if the number of connections is zero at the time of checking. However, this behaviour is incorrect. In rare cases, a connection can be dropped right after it is initially established and before the number checking. From the perspective of the openConnections method, it should not care whether or when opened connections are subsequently closed. As long as connections have been initially established, it should report success instead of failure.

This PR adjusts the code to report success in above situation.

Relates: #94998
Resolves: #99113

[elasticsearchmachine]

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

[ywangd]

@DaveCTurner Could you please help review this PR since you were involved in the PR (#94998) that introduced the bogus assertion? Thanks!

It is a rare failure only happens if connections are dropped quickly enough. For the failed test, it reaches the end of the test and starts to cleanup connections once the addressResolver is called. This can cause connections to be closed when the openConnections is still running. For the two places (1, 2) where we call openConnections, we assume we have at least one connection (connectionManager.size() > 0). This assumption can be false when connections are dropped in between.

I considered whether we could check attempNumber before the call to openConnections in the onResponse callback.

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

Lines 260 to 261 in 16dd1e6

	if (shouldOpenMoreConnections()) {
	openConnections(finished, attemptNumber + 1);

But it does not help the overall situation where number of connections can change right after we check it. Since the else branch already handles the case of zero connection, I think it is reasonable and easier to just remove the overly optimistic assertion.

[ywangd]

@DaveCTurner Ping for awareness. Thank you!

[DaveCTurner]

Ah yes this is not a valid assertion. But why check the number of open connections at all? We may as well just report success at this point, because it makes no difference if the connections have closed before or after we read connectionManager.size().

[ywangd]

Checking the number of open connections is an existing behaviour before the assertion was added. I can see the argument that we don't need to check the connection again in the else branch because we can safely assume that at least one connection has been successful when the code reaches here. Whether the connection subsequently gets disconnected is a separate issue and the disconnetion can happen anytime after the connection is initially established.

That said, can I address it in a separate PR because it requires changing production code and writing tests? For this PR, I'd prefer to resolve the CI failure first. Thanks!

[DaveCTurner]

Can we just mute the test until it's fixed? IMO the behaviour we have today is a genuine bug.

[elasticsearchmachine]

Hi @ywangd, I've created a changelog YAML for you.

[ywangd]

@DaveCTurner I have turned this PR into an actual fix as you suggested. I did not mute the failed test because (1) it is a very rare failure and (2) the failure in theory can happen to any tests that use proxy connection strategy and it is rather inconvenient to mute all of them. So hopefully we can get this PR merged before the next failure happens. Thanks!

[DaveCTurner]

LGTM thanks Yang