Shuffle-sharding of queriers may not work as intended

[pstibrany]

Shuffle sharding is intended to protect the system in a case, where user is sending queries to the system that can crash the querier.

However, if this happens and querier crashes, from query-frontend (or query-scheduler) point of view, querier is simply disconnected, and query-frontend will recompute the shard for user and simply use different queriers. If user sends more crashing queries at this moment, he can eventually crash all the queriers.

In practice, crashed queriers should restart quickly, which will make them part of users shard again, but the potential problem still exists.

[tomwilkie]

How about we only "remove" queriers from the list that have not connected to the frontend for ~1 minute (tuneable) - then we can trade off availability in an outage vs the potential for a client to kill all the queriers.

[pracucci]

Agree. We have to pay attention to trigger this logic only when a querier does not gracefully shutdown (eg. shouldn't be applied when we roll out queriers).

[pracucci]

We need to detect in the query-frontend/scheduler if a querier has crashed (eg. bug triggered by the query, OOMKilled, ...). I did several experiments and investigated gRPC internals.

Unfortunately, when such condition happen while the query-frontend/scheduler is waiting on .Recv() they get a context canceled error. The same error is received on a querier graceful shutdown, because we cancel on-going queries, so currently there's no way to distinguish the two cases.

Why?

When the query-frontend calls Recv() on the stream, under the hood the reader blocks on ReadFrame():
https://github.com/grpc/grpc-go/blob/9dfe677337d567bdea82c6c11603127a7d83cdac/internal/transport/http2_server.go#L455-L459

The ReadFrame() return io.EOF if the connection is closed. This triggers http2Server.Close():
https://github.com/grpc/grpc-go/blob/9dfe677337d567bdea82c6c11603127a7d83cdac/internal/transport/http2_server.go#L481-L483

The http2Server.Close(), among other things, cancels the context on all active streams:
https://github.com/grpc/grpc-go/blob/9dfe677337d567bdea82c6c11603127a7d83cdac/internal/transport/http2_server.go#L1068-L1071

Having cancelled the contexts, the Recv() (which was waiting for a frame) returns context.Canceled.

Proposal

Since we have no way to distinguish between a querier graceful shutdown (eg. during a rolling update) and an abruptly termination (eg. crash due to a bug triggered by the query or OOMKilled), I would suggest to handle the graceful shutdown in the querier.

When the querier receives a SIGTERM and begins the shutdown procedure, in-flight queries will be completed instead of cancelling them, so that the only case when the query-frontend receives a context canceled error should be when a abruptly termination happens.

[tomwilkie]

Since we have no way to distinguish between a querier graceful shutdown (eg. during a rolling update) and an abruptly termination (eg. crash due to a bug triggered by the query or OOMKilled), I would suggest to handle the graceful shutdown in the querier.

+1, in general there is no way to tell a failure from a timeout, so handling graceful shutdown as a special case sounds reasonable.

[pstibrany]

You describe one part of the problem, and I agree with your proposed approach. The other part of the problem is how to make querier to communicate graceful shutdown to scheduler or frontend. I don't think it can be done with current "processing" loop, as all communication there is initiated by opposite side, and not the querier. However querier can shutdown any time.

I think we need to introduce new call from querier to scheduler/frontend that would inform scheduler/frontend that querier with given ID is being shut down. This allows scheduler/frontend to remove it from the list of queriers immediately. If on the other hand querier just closes connection without doing this call first, scheduler/frontend will use "delay" mechanism before removing querier.

Reason for introducing new call is to make this work even in when rate of queries is low. Alternative would be to make regular "ping" request from frontend/scheduler to querier that would allow querier to inform about shutdown.

[tomwilkie]

as all communication there is initiated by opposite side, and not the querier.

By this you mean the scheduler / frontend only listens for responses to messages it sends, right? I suspect we could work around this by making them always listen in a separate goroutine, and if its not a shutdown message, stick it on a channel for the normal request-response flow to pick up.

[pstibrany]

as all communication there is initiated by opposite side, and not the querier.

By this you mean the scheduler / frontend only listens for responses to messages it sends, right? I suspect we could work around this by making them always listen in a separate goroutine, and if its not a shutdown message, stick it on a channel for the normal request-response flow to pick up.

Yes, that is what I meant. Your solution would work too.

[alvinlin123]

I am just curious, does the behaviour that @pracucci described above applies to when frontend/scheduler calles server.Send() too?

[pracucci]

I am just curious, does the behaviour that @pracucci described above applies to when frontend/scheduler calles server.Send() too?

I haven't checked the Send() behaviour in details but don't think we care here because the proposed logic should trigger only on the Recv(). If the  Send() fail it's not that query to have crashed the querier.

[alvinlin123]

If the Send() fail it's not that query to have crashed the querier.

I think there might be one case where we might care. Let's assume we have 2 frontends FE1 and FE2 and a single querier Q1. FE1 sent a query that can cause the querier to OOM to Q1. At the same time FE2 is pushing bits and bytes to Q1 through Send(), in the middle of the Send() Q1 OOM'ed. In this case, because FE2 have not completely finish sending bits and bytes, then I am wondering if Send() would return context.Canceled too, and if FE2 should kick in the "delay remove" too.