sonic-net / sonic-gnmi

SONiC gNMI server and gNOI repo
Other
22 stars 52 forks source link

Memory grows until OOM with slow telemetry collector and lots of data. #26

Open seiferteric opened 3 years ago

seiferteric commented 3 years ago

I am opening this issue to get an opinion on how to handle an issue I have observed. We had a telemetry collector (telegraf with custom gNMI plugin) that was running with limited CPU quota (I think limited to ~20% CPU time). The collector was using SAMPLE mode to get a large BGP table. Over time we noticed that the memory of the telemetry process was increasing until we hit OOM and the process was killed.

I identified the issue in that when in client_subscribe.go send() function we call err = stream.Send(resp). This call will actually block in the case I described above when the collector is not processing data quickly enough. The problem then is that the telemetry process will keep adding data to the PriorityQueue which causes the memory to grow. To rectify this issue, I introduced a new "LimitedQueue" instead of the current PriorityQueue in our (Dell) sonic-telemetry. The LimitedQueue will check the size of the Queue and reject adding newitems if the size is greater than the predefined maximum size (default I set to 100MB).

This is working, however it means that the collector will start to miss telemetry updates. Recently Broadcom recommended instead I close the connection with gRCP code RESOURCE_EXHAUSTED instead of silently dropping updates.

Wanting to know what is the community preferred way to do this before opening a PR.

lguohan commented 3 years ago

in case of closing the connection, will all memory allocated be released?

when will the telemety reconnect?

seiferteric commented 3 years ago

Yes the queue will be disposed and so the memory will be freed. It would be up to the collector to reconnect after receiving the RESOURCE_EXHAUSTED error since this is for dial-in telemetry.

macikgozwa commented 3 years ago

I think sending RESOURCE_EXHAUSTED and terminating the subscription is a good option. That would be analogous to HTTP 429 (throttling) response from a Rest API.

One potential concern would be a client which mixes a high volume data and low volume data in the same SubscriptionList request. Since the queue is per SubscriptionList request, the telemetry service would end up canceling both high volume and low volume paths. The collector side should be aware of this case and not mix them in the same SubsciptionList request.

qiluo-msft commented 2 years ago

@seiferteric Could you help connect to right person in Broadcom

Recently Broadcom recommended instead I close the connection with gRCP code RESOURCE_EXHAUSTED instead of silently dropping updates

I think the disconnecting gRPC client is a better choice.

sneelam20 commented 3 months ago

Assigning this @anand-kumar-subramanian based on the last comment.

anand-kumar-subramanian commented 3 months ago

This issue was already fixed by Dell. Assigning this to @kwangsuk to make sure this issue is fixed.

kwangsuk commented 3 months ago

This issue was already fixed by Dell. Assigning this to @kwangsuk to make sure this issue is fixed.

The fix is not yet made in the community. We will plan to replicate the fix.