Querier timeout when reaching ingesters

[cyriltovena]

Currently there is no timeout when doing GRPC between ingesters and querier.

This leads to the querier stacking and starving goroutines until it gets restarted.

It is only happening when ingesters are unresponsive.

We should have a timeout for that connection.

[sh0rez]

Sure?

It looks like the querier configures its IngesterClient using t.cfg.IngesterClient config:

loki/pkg/loki/modules.go

Lines 135 to 136 in 1c95a5b

	func (t *Loki) initQuerier() (err error) {
	t.querier, err = querier.New(t.cfg.Querier, t.cfg.IngesterClient, t.ring, t.store)

loki/pkg/querier/querier.go

Lines 40 to 42 in 1c95a5b

	factory := func(addr string) (grpc_health_v1.HealthClient, error) {
	return client.New(clientCfg, addr)
	}

But this one does include a timeout:

loki/pkg/ingester/client/client.go

Line 22 in 1c95a5b

RemoteTimeout time.Duration `yaml:"remote_timeout,omitempty"`

And this timeout defaults to 5s because of its flag:

loki/pkg/ingester/client/client.go

Line 31 in 1c95a5b

f.DurationVar(&cfg.RemoteTimeout, "ingester.client.timeout", 5*time.Second, "Timeout for ingester client RPCs.")

So did I miss another place where a timeout is required?

[cyriltovena]

I don't think this is used. But what is missing mostly is a query timeout for instance:

clientDeadline := time.Now().Add(time.Duration(*deadlineMs) * time.Millisecond)
ctx, cancel := context.WithDeadline(ctx, clientDeadline)

loki/pkg/querier/querier.go

Line 129 in 1c95a5b

return client.Query(ctx, req)

There is no timeout here but it should be there for most of queries type. (Not sure about tailing)

[pracucci]

@Kuqd I would be glad to pick from here and work on a PR. Few questions, please:

1. Should we allow to set the query timeout via querier config flag, or should be possible to override it via HTTP request too?
2. Should the querier.Label() underlying requests have a different timeout than querier.Query()? If yes, may -querier.logs-query-timeout and -querier.labels-query-timeout work?
3. Any suggestion on how we should unit test it?

[cyriltovena]

Yes please.

1 - Only via a config flag with a sane default.
2 - Label and Query should be the same.
3 - I'm not sure this would be necessary be if you achieve it, I'll take it.

Thanks @pracucci