containerd / containerd/ttrpc

Occasional streamHandler map memory leak

Open
#236 0 comments 1 reaction 1 assignee Claimed by @fuweid View on GitHub
Dominant language
Go
Stars
664
Forks
86
PR merge metrics
No merged PRs in 30d

Description

### Summary
I'm chasing a slow unbounded memory growth in the `containerd-shim-runc-v2`. After profiling and reviewing the code my conclusion is that there is an occasional memory leak in the ttrpc server where keys in the `streams sync.Map` are sometimes removed before the corresponding stream id key gets added (in a separate goroutine) causing the key to remain forever until connection is closed.

### Background
I'm running `containerd` on an older ARMv7 32-bit IoT device, we are currently running older 1.7 containerd branch, but the ttrpc library is fairly recent at `v1.2.7`. Our 2 core CPU is a bit on the slower side so that could perhaps explain why this leak has not been noticed before. In our setup we use containerd directly without any kubernetes/kubelet.

### Symptoms
Over a 7 days period the `containerd-shim-runc-v2` PSS_Dirty (also confirmed with Golang's `HeapAlloc`) increases from 4.2 MB to 4.6 MB.
Golangs heap pprof initially didn't initially show any leaks, but after reducing the `runtime.MemProfileRate = 1` an 8 hour diff between two heap profiles showed following:
Image

i.e. this `streams.Store` line https://github.com/containerd/ttrpc/blob/v1.2.7/server.go#L489

### Analysis
After adding some logs to the `streams.Store` and `streams.Delete` I could see that when the `Delete` method is called it sometimes tries to remove a key that doesn't exist, when this happened then usually the deletion was also logged before the insertion.

Problems was also a bit easier to reproduce when adding an extra sleep before storing the streamHandler:
```go
diff --git a/vendor/github.com/containerd/ttrpc/server.go b/vendor/github.com/containerd/ttrpc/server.go
index bb71de677..35cfa1723 100644
--- a/vendor/github.com/containerd/ttrpc/server.go
+++ b/vendor/github.com/containerd/ttrpc/server.go
@@ -486,6 +486,7 @@ func (c *serverConn) run(sctx context.Context) {
continue
}

+ time.Sleep(1 * time.Millisecond)
streams.Store(id, sh)
atomic.AddInt32(&active, 1)
}
```

My takeaway is that there is a race where two different goroutines tries to modify the `streams` map with the same key. If the service handler runs to completion ([line 481](https://github.com/containerd/ttrpc/blob/main/server.go#L481)) before the `streams.Store` is called the key will remain forever.

### Possible solution
For the containerd use case I've only seen this race on non-streaming messages. So a potential fix is to simply skip storing the streamHandler when it is `nil`:

```go
diff --git a/vendor/github.com/containerd/ttrpc/server.go b/vendor/github.com/containerd/ttrpc/server.go
index bb71de677..9b9c0bdc7 100644
--- a/vendor/github.com/containerd/ttrpc/server.go
+++ b/vendor/github.com/containerd/ttrpc/server.go
@@ -486,7 +486,9 @@ func (c *serverConn) run(sctx context.Context) {
continue
}

- streams.Store(id, sh)
+ if sh != nil {
+ streams.Store(id, sh)
+ }
atomic.AddInt32(&active, 1)
}
// TODO: else we must ignore this for future compat. log this?
```

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.