Hono: Conceptual problem in connection handling

[ctron]

To me it looks like we have a conceptual problem in the handling of connections to other services.

Every now and then, I recognized cases, where the adapters just don't become ready. And so I was digging into the issue. In the log you can then find something like:

~
2019-09-02T17:11:38.515Z INFO [HonoConnectionImpl] stopping connection attempt to server [host: iot-device-registry.enmasse-infra.svc, port: 5671] due to terminal error
javax.security.sasl.AuthenticationException: Failed to authenticate
~

This "terminal error" will stop the client from trying to re-connect, and it seems like it never will re-try again. For as long as process is alive. So effectively, the process would be dead now.

My assumption was, that the pod would get cleaned up at some point, due to failing health checks. However that also doesn't seem to be the case. Taking a look at org.eclipse.hono.service.AbstractProtocolAdapterBase, you can see that the connections are checked in registerReadinessChecks, but not in the registerLivenessChecks method.

In that case, the "liveness check" would still always succeed.

Taking a look at the Kubernetes documentation (https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/):

The kubelet uses liveness probes to know when to restart a Container. For example, liveness probes could catch a deadlock, where an application is running, but unable to make progress. Restarting a Container in such a state can help to make the application more available despite bugs.
The kubelet uses readiness probes to know when a Container is ready to start accepting traffic. A Pod is considered ready when all of its Containers are ready. One use of this signal is to control which Pods are used as backends for Services. When a Pod is not ready, it is removed from Service load balancers.

It looks to me, as if the correct way to signal the "terminal error", is not through the "readiness" check, but through the "liveness" check instead.

Additionally, I would question the idea of a terminal error altogether. From what I see in our case, it has to do with an invalid service credential. So that problem will go away in a minute.

I think we should:

1. Trigger a liveness failure in case of a terminal error.
2. Event for a terminal error, allow to try again.

Fixing the first point is required to solve the issue. The second point would only allow the pod to recover itself, before it get killed by Kubernetes, and might be more of an improvement to the situation.

I also think we should fix this before 1.0.0!

[ctron]

Digging a bit more into the code, I think we could also fix the method org.eclipse.hono.service.AbstractProtocolAdapterBase.startInternal() in a way that the call to connectToService actually makes use of the returned future, and reports its failure to the startup chain. Failing the startup of the application.

[sophokles73]

This "terminal error" will stop the client from trying to re-connect, and it seems like it never will re-try again. For as long as process is alive. So effectively, the process would be dead now.

The behavior of the protocol adapters has been implemented this way deliberately in order to prevent useless connection attempts to services with which no connection can be established due to misconfiguration (on either the client or server side).

It looks to me, as if the correct way to signal the "terminal error", is not through the "readiness" check, but through the "liveness" check instead.

I do not think that there is a right or wrong way here. We need to address two different scenarios here:

1. The service we try to connect to is implemented/configured correctly but is not (yet) ready for operation, i.e. it is in the process of starting up or we have temporarily lost network connection to it. In any case, once we are able to reach the service, a connection will be established.
2. The service is wrongly implemented or there is a configuration mismatch between the adapter and the service. This means that no matter how often we try to connect to the adapter, the connection attempt will always fail unless the configuration/implementation is fixed.

The first case is already covered by the current re-connect behavior.

Additionally, I would question the idea of a terminal error altogether. From what I see in our case, it has to do with an invalid service credential. So that problem will go away in a minute.

While the problem might be easy to fix (in most cases), whether it happens or not depends on an operator to become aware of it. I agree that with the current implementation this will be hard to detect as the adapter process seems to be running happily while in fact it doesn't try to (re-)establish a connection anymore.

I think we should:
Trigger a liveness failure in case of a terminal error.
Event for a terminal error, allow to try again.

I am not sure what the second item is supposed to mean. But I would rather simply allow the adapter to try to re-connect in case of a terminal error instead of restarting the whole pod based on a failed liveness check, which is much slower and more costly.

FMPOV the readiness check is still the right way to make sure that

1. operators become aware of a problem with the adapter/service
2. allow the adapter to auto recover from the existing problem

The liveness check does not add much value here FMPOV because we would use it basically do the same as the HonoClient already does, i.e. trying to establish the connection once more.

However, the startup of the adapter does not need to depend on the successful establishment of the connections to the services because with the re-try behavior of the HonoClient, we already make sure that the connections get established eventually and the readiness check already signals to Kubernetes when this is the case.

[ctron]

But I would rather simply allow the adapter to try to re-connect in case of a terminal error instead of restarting the whole pod based on a failed liveness check, which is much slower and more costly.

That is exactly my second point.

So reading through your explanations, I fail to see a reason for having a "terminal error" in the first place:

Additionally, I would question the idea of a terminal error altogether.

Overall, still fail to see your intent here:

FMPOV the readiness check is still the right way to make sure that

1. operators become aware of a problem with the adapter/service
2. allow the adapter to auto recover from the existing problem

When there is a way to "auto recover" from a "terminal error"? Then I fail to see the concept of a "terminal error".

Anyway, if your proposal is to simply ignore the "terminal" part, and continue re-connecting, then I am fine with that.

[ctron]

@sophokles73 Do you think #1473 is a better fix for this issue?

[sophokles73]

Anyway, if your proposal is to simply ignore the "terminal" part, and continue re-connecting, then I am fine with that.

That is exactly the proposal. We should no longer distinguish between terminal and non-terminal errors. With the backoff algorithm that @calohmn implemented recently, we should be safe to always try to re-connect, regardless of the underlying cause of the error.

@dejanb WDYT?

[dejanb]

@sophokles73 I think this is a move in the right direction, as there's no point in leaving pod running with the failed connection.

As for the liveness check, I'd still prefer to restart a pod if the reconnection logic is not successful in certain period of time. It might help with situations where process can pick up updated env and resolve the issue.

[ctron]

@sophokles73 So, I will move on with #1473 and simply drop the "terminal" stuff

[sophokles73]

@ctron can this be closed?