Panic in tailing code

[cyriltovena]

Stumble on this today

/go/src/github.com/grafana/loki/pkg/querier/tail.go:193 +0x295
created by github.com/grafana/loki/pkg/querier.(*Tailer).checkIngesterConnections	
/go/src/github.com/grafana/loki/pkg/querier/tail.go:219 +0xa8
github.com/grafana/loki/pkg/querier.(*Tailer).readTailClient(0xc020d72000, 0xc000504840, 0x10, 0x0, 0x0)
goroutine 95605 [running]:
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x105e078]
panic: runtime error: invalid memory address or nil pointer dereference

We should make sure this doesn't happen. Might be already fixed.

/cc @sandlis

[sandeepsukhani]

I was suspecting this might be happening due to server stopping before closing websockets, it seems it is not the case. If you see it again please let me know, will check it out.

[pracucci]

@cyriltovena An hypothesis is the tailClients map - returned by Querier.tailDisconnectedIngesters() - contains a nil value (if you manually add it, the stack trace you get is the same of you, obviously with different memory addresses).

Looking at the tailDisconnectedIngesters() implementation, isn't the case possible if q.ring.GetAll() omits an ingester which is listed in tailDisconnectedIngesters input argument? I'm not much familiar with the ring implementation yet, but isn't possible if an ingester left the ring in the meanwhile?

[sandeepsukhani]

I doubt that would be the case, otherwise code at other places which also use ring for querying ingesters would also have panicked. Would still check for surety.

[pracucci]

I doubt that would be the case, otherwise code at other places which also use ring for querying ingesters would also have panicked.

@sandlis Can you elaborate on this, please? Do you mean it's not possible an ingester leave the ring?

[sandeepsukhani]

Ring would most likely just not return ingester which is leaving or left.

[pracucci]

Ring would most likely just not return ingester which is leaving or left.

Ok, that was my initial hypothesis. If so, then look at this

func (q *Querier) tailDisconnectedIngesters(ctx context.Context, req *logproto.TailRequest, connectedIngestersAddr []string) (map[string]logproto.Querier_TailClient, error) {
	tailClients := make(map[string]logproto.Querier_TailClient)
	for i := range connectedIngestersAddr {
		tailClients[connectedIngestersAddr[i]] = nil
	}

	disconnectedIngesters := []ring.IngesterDesc{}
	replicationSet, err := q.ring.GetAll()
	if err != nil {
		return nil, err
	}

	for _, ingester := range replicationSet.Ingesters {
		if _, isOk := tailClients[ingester.Addr]; isOk {
			delete(tailClients, ingester.Addr)
		} else {
			disconnectedIngesters = append(disconnectedIngesters, ingester)
		}
	}

	clients, err := q.forGivenIngesters(ring.ReplicationSet{Ingesters: disconnectedIngesters}, func(client logproto.QuerierClient) (interface{}, error) {
		return client.Tail(ctx, req)
	})
	if err != nil {
		return nil, err
	}

	for i := range clients {
		tailClients[clients[i].addr] = clients[i].response.(logproto.Querier_TailClient)
	}

	return tailClients, nil
}

If an ingester which was previously in the ring (passed as connectedIngestersAddr) now is no more returned by q.ring.GetAll(), then tailClients (which is returned by this function) will contain nil for that ingester address, causing the panic.

Am I missing anything?

[sandeepsukhani]

This code only conn to disconnected ingesters. The second for loop removes those nil entries.

[pracucci]

The second for loop removes those nil entries.

The second for loop iterates over the output of q.ring.GetAll(), so if a previous ingester is not returned anymore from q.ring.GetAll(), that ingester address will be kept in tailClients with nil value.

[sandeepsukhani]

Ohh, you are right. Sorry, was reading code on my phone so missed that bit. Will fix it.

[pracucci]

@sandlis I'm already working on a PR to fix it (while validating the idea with you). I will submit the PR in the next few hours 🙏

[sandeepsukhani]

@pracucci Thank you so much for taking care of it!