conntrack: prevent potential memory leak #1058
Conversation
4a0682e to
e97da78
Compare
e97da78 to
f875a5d
Compare
Currently, the ConntrackDeleteFilters captures all flow entries it fails to delete and reports them as errors. This behavior can potentially lead to memory leaks in high-traffic systems, where thousands of conntrack flow entries are cleared in a single batch. With this commit, instead of returning all the un-deleted flow entries, we now return a single error message for all of them. Signed-off-by: Daman Arora <aroradaman@gmail.com>
f875a5d to
f87286a
Compare
| @@ -178,19 +179,20 @@ func (h *Handle) ConntrackDeleteFilters(table ConntrackTableType, family InetFam | |||
| req2 := h.newConntrackRequest(table, family, nl.IPCTNL_MSG_CT_DELETE, unix.NLM_F_ACK) | |||
| // skip the first 4 byte that are the netfilter header, the newConntrackRequest is adding it already | |||
| req2.AddRawData(dataRaw[4:]) | |||
| if _, err = req2.Execute(unix.NETLINK_NETFILTER, 0); err == nil { | |||
| if _, err = req2.Execute(unix.NETLINK_NETFILTER, 0); err == nil || errors.Is(err, fs.ErrNotExist) { | |||
There was a problem hiding this comment.
fore reference, this is how golang std lib recommends to do this check https://pkg.go.dev/os#IsNotExist
There was a problem hiding this comment.
I compared the err with syscall.ENONET, fs.ErrNotExist and unix.ENONET. Only got a match on fs.ErrNotExist .
https://github.com/vishvananda/netlink/compare/main...aroradaman:netlink:debug?expand=1
####################################################### Netlink ConntrackDeleteFilters
error occurred no such file or directory false true false
length 1808537
####################################################### Netlink ConntrackDeleteFilters
There was a problem hiding this comment.
is how golang exposes them ... I found it out recently too aojea/kindnet#158
| if len(errMsgs) > 0 { | ||
| return matched, fmt.Errorf(strings.Join(errMsgs, "; ")) | ||
| if totalFilterErrors > 0 { | ||
| finalErr = errors.Join(finalErr, fmt.Errorf("failed to delete %d conntrack flows with %d filters", totalFilterErrors, len(filters))) |
There was a problem hiding this comment.
this was added in 1.20 https://pkg.go.dev/errors#Join
There was a problem hiding this comment.
Yes, this breaks the module;
Line 3 in 655392b
For this one we may be able to just use the old approach (at least, I think it would give mostly the same for now)
(there was another change also requiring newer go versions)
There was a problem hiding this comment.
yeah, but 1.20 was EOL one year ago devcontainers/images#944 ,
|
/lgtm /assign @aboch PTAL this is causing disruption and a big impact on performance and scalability kubernetes/kubernetes#129982 |
|
LGTM |
|
Thanks 🙇 |
Currently, the ConntrackDeleteFilters captures all flow entries it fails to delete and reports them as errors. This behavior can potentially lead to memory leaks in high-traffic systems, where thousands of conntrack flow entries are cleared in a single batch. With this commit, instead of returning all the un-deleted flow entries, we now return a single error message for all of them.
ref: kubernetes/kubernetes#129982