Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NETOBSERV-1564: do not force flushing maps when rb is triggered #348

Merged
merged 1 commit into from
Jun 26, 2024
Merged

NETOBSERV-1564: do not force flushing maps when rb is triggered #348

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions pkg/flow/tracer_ringbuf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,9 @@ func (m *RingBufTracer) listenAndForwardRingBuffer(debugging bool, forwardCh cha
if debugging {
m.stats.logRingBufferFlows(mapFullError)
}
// if the flow was received due to lack of space in the eBPF map
Copy link
Contributor

@msherif1234 msherif1234 Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't removing this logic means we will stay in hmap full condition now for longer and possibly dropping flows ? if the concern here is doing many map flush because system is under stress can we trigger a flush every 1s or so hopping to free up some hmap space ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that having a fixed-rate flush could be a better solution, but actually there's already a flush every 5s (the cacheActiveTimeout setting) ; users can set it to 1s if that works better for them

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or we introduce a new "stress timer" (could be automatically set to cacheActiveTimeout / something) that only starts when stress is detected (ie. maps are full) ? but that's a bit complex, it has to be worth it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by the way: when you said "this logic means we will stay in hmap full condition now for longer and possibly dropping flows" I don't think this is true; when maps are full, flows are not dropped, they're moved to the ring buffer. Dropped flows are only for busy maps when there isn't a new flow creation, so not something that can lead the map being full (we're updating an entry in that case, not adding a new one, so no incidence on hmap size)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use rb for new flows and it send 1 pkt at a time to userspace so it can't handle burst of traffic either

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it still handle that much better than an uncontrolled hot-loop of flush events. Look at the -50% CPU that I'm showing in the PR description

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll run perf-scale tests to check the diff

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/LGTM if perf scale shows improvements

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ingress-perf shows slightly improved stats: -4% memory, -8% CPU
(https://docs.google.com/spreadsheets/d/1EN12dogz-0_H_5tSoV24T4iHa9a3wSgoF396YyOqavA/edit?gid=93930639#gid=93930639 / diff: https://docs.google.com/spreadsheets/d/1q_XCJ48h2Q78JapxgNB37nJe-4lTYVlMx7xVrn7ztck/edit?gid=696044201#gid=696044201)

Now running cluster-density. But I'm not sure to see something very different here, as the RB isn't involved a lot anyway.

Problem is that, none of those test really stress the agents. So I may not see something better than this -8% here. The tests that I did above DO stress the agents much more, and I had -50%

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msherif1234 , @Amoghrd did run cluster-density, with also slightly positive results: https://issues.redhat.com/browse/NETOBSERV-1564?focusedId=24995474&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-24995474

// forces a flow's eviction to leave room for new flows in the ebpf cache
var reason string
if mapFullError {
m.mapFlusher.Flush()
reason = "mapfull"
}
errno := syscall.Errno(readFlow.Metrics.Errno)
// In ringbuffer, a "flow" is a 1-packet flow, it hasn't gone through aggregation yet. So we use the packet counter metric.
m.metrics.EvictedPacketsCounter.WithSourceAndReason("ringbuffer", reason).Inc()
m.metrics.EvictedPacketsCounter.WithSourceAndReason("ringbuffer", errno.Error()).Inc()
// Will need to send it to accounter anyway to account regardless of complete/ongoing flow
forwardCh <- readFlow
return nil
Expand Down
Loading