hubble: Add `--hubble-monitor-events` flag #24828

epk · 2023-04-12T03:46:57Z

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing!

Related #19929

Adds a --hubble-monitor-events flag that let's a user control the event types that get to the hubble subsystem.

Add `--hubble-monitor-events` flag, to control the event types that get to the hubble subsystem.

Why

Nodes with traffic heavy workloads are doing 30k+ flows per second. This causes very high CPU utilization for the cilium-agent container. We'd like to be able to only monitor drop and other events and ignore the normal flows.

Flame graph showing that most of the CPU time is spent in the hubble subsystem decoding the events:

cc @michi-covalent

maintainer-s-little-helper · 2023-04-12T17:28:20Z

Commit 3d92136363dac97bac8e4a67607185225ccf699f does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

pkg/hubble/monitor/filter_test.go

maintainer-s-little-helper · 2023-04-12T20:59:46Z

Commit e4632624f7a096c7fbd1792e5bc7c5a81f8bbee4 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2023-04-12T21:00:39Z

Commit e4632624f7a096c7fbd1792e5bc7c5a81f8bbee4 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

joestringer

Just a minor usability/UX suggestion below.

pkg/hubble/monitor/filter.go

joestringer

LGTM 🎉

joestringer · 2023-04-20T18:26:31Z

It looks like the two test failures are pointing at the same thing: The cmdref docs need to be updated: https://github.com/cilium/cilium/actions/runs/4757401849/jobs/8454194805?pr=24828#step:8:150

michi-covalent · 2023-04-20T20:13:59Z

/test

pippolo84

Thanks!

michi-covalent · 2023-04-21T16:18:29Z

test-1.27-net-next failure looks like #24966. it might have gotten fixed by #24785. i'll rebase and re-run the test

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com> Signed-off-by: Aditya Sharma <aditya.sharma@shopify.com> Co-authored-by: Michi Mutsuzaki <michi@isovalent.com> Co-authored-by: Aditya Sharma <aditya.sharma@shopify.com>

nathanjsweet

LGTM for api changes.

michi-covalent · 2023-04-21T17:53:05Z

/test

michi-covalent · 2023-04-22T19:00:31Z

test-runtime: hit #22373

michi-covalent · 2023-04-22T19:00:43Z

/test-runtime

michi-covalent · 2023-04-22T19:09:30Z

ci-multicluster: filed #25064

michi-covalent · 2023-04-22T19:09:41Z

/ci-multicluster

marqc

I believe you misinterpreted the meaning of return value from the OnMonitorEvent handler. Please check my comments within a PR.

marqc · 2023-04-27T11:04:32Z

pkg/hubble/monitor/filter.go

+	return &monitorFilter, nil
+}
+
+func (m *monitorFilter) OnMonitorEvent(ctx context.Context, event *observerTypes.MonitorEvent) (bool, error) {


the return bool flag is used as stopPropagation indicator https://github.com/cilium/cilium/blob/main/pkg/hubble/observer/local_observer.go#L119

So if this function returns true the event won't be propagated to hubble observers. If it returns false, the event will be propagated to hubble observers.

Either return value of this function needs to be reverted, or configuration flag should be changed to clearly indicate that this settings is excluding given event types from getting observed by hubble.

Currently when I use --hubble-monitor-events="drop debug capture trace policy-verdict recorder l7 agent" I expect to see everything, but actually there is nothing reaching hubble subsystem.

marqc · 2023-04-27T11:06:10Z

daemon/cmd/daemon_main.go

+	flags.StringSlice(option.HubbleMonitorEvents, []string{},
+		fmt.Sprintf(
+			"Cilium monitor events for Hubble to observe: [%s]. By default, Hubble observes all monitor events.",
+			strings.Join(monitorAPI.AllMessageTypeNames(), " "),


Current implementation is to "exclude", not to "observe"

marqc · 2023-04-27T11:08:51Z

pkg/hubble/monitor/filter.go

+	switch payload := event.Payload.(type) {
+	case *observerTypes.PerfEvent:
+		if len(payload.Data) == 0 {
+			return false, errors.ErrEmptyData


Related to my above comment: return false actually passes this event to the hubble subsystem. If you want to not pass empty events (which makes a lot of sense) this should return true

marqc · 2023-04-27T11:10:31Z

pkg/hubble/monitor/filter_test.go

+
+type testEvent struct {
+	event       *observerTypes.MonitorEvent
+	allowed     bool


to be consistent with https://github.com/cilium/cilium/blob/main/pkg/hubble/observer/local_observer.go#L119 please rename allowed to stop or stopPropagation

marqc · 2023-04-27T11:13:08Z

pkg/hubble/monitor/filter_test.go

+			assert.NoError(t, err)
+
+			for _, event := range tc.events {
+				allowed, err := mf.OnMonitorEvent(context.Background(), event.event)


rename allowed to stop or stopPropagation

wayt · 2023-04-27T12:50:32Z

Can we back-port this to 1.12 / 1.13? that would really help with managing Hubble CPU usage.

Cheers.

epk · 2023-04-27T13:42:32Z

Opened #25167 addressing @marqc's comments.

joestringer · 2023-04-27T17:19:55Z

Can we back-port this to 1.12 / 1.13? that would really help with managing Hubble CPU usage.

In general, only bugfixes are backported, see the backporting policy here. This is considered a new feature. This feature will be made available in an upcoming v1.14 monthly snapshot release which you're welcome to try out and provide feedback. Furthermore, as we can see from recent posts on this PR, there's still active development on it, so it would be premature to release at this stage without further testing and feedback.

wayt · 2023-04-27T17:59:22Z

@joestringer that make sense, thank you for the details.

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 12, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 12, 2023

epk force-pushed the epk/hubble-event-filter branch 3 times, most recently from cce7da8 to d85ad93 Compare April 12, 2023 17:25

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 12, 2023

epk force-pushed the epk/hubble-event-filter branch from 3d92136 to d85ad93 Compare April 12, 2023 17:29

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 12, 2023

epk force-pushed the epk/hubble-event-filter branch from d85ad93 to 2497df3 Compare April 12, 2023 17:30

epk changed the title ~~hubble: Add --hubble-monitor-events flag~~ hubble: Add monitor event filter Apr 12, 2023

epk changed the title ~~hubble: Add monitor event filter~~ hubble: Add --hubble-monitor-events flag Apr 12, 2023

epk marked this pull request as ready for review April 12, 2023 17:47

epk requested review from a team as code owners April 12, 2023 17:47

epk requested review from pippolo84, asauber and chancez April 12, 2023 17:47

epk force-pushed the epk/hubble-event-filter branch from 2497df3 to 9d39387 Compare April 12, 2023 17:54

chancez reviewed Apr 12, 2023

View reviewed changes

pkg/hubble/monitor/filter_test.go Show resolved Hide resolved

michi-covalent added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 12, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 12, 2023

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 12, 2023

epk force-pushed the epk/hubble-event-filter branch from dcfbb3c to 236c8e8 Compare April 12, 2023 21:00

epk force-pushed the epk/hubble-event-filter branch from 236c8e8 to c5a41c8 Compare April 12, 2023 21:03

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 12, 2023

joestringer reviewed Apr 12, 2023

View reviewed changes

pkg/hubble/monitor/filter.go Outdated Show resolved Hide resolved

epk requested review from a team as code owners April 20, 2023 17:59

epk requested a review from nathanjsweet April 20, 2023 17:59

joestringer approved these changes Apr 20, 2023

View reviewed changes

epk force-pushed the epk/hubble-event-filter branch 2 times, most recently from 746a246 to 38484ac Compare April 20, 2023 18:46

pippolo84 approved these changes Apr 21, 2023

View reviewed changes

hubble: Add --hubble-monitor-events flag

f764b10

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com> Signed-off-by: Aditya Sharma <aditya.sharma@shopify.com> Co-authored-by: Michi Mutsuzaki <michi@isovalent.com> Co-authored-by: Aditya Sharma <aditya.sharma@shopify.com>

michi-covalent force-pushed the epk/hubble-event-filter branch from 38484ac to f764b10 Compare April 21, 2023 16:18

nathanjsweet approved these changes Apr 21, 2023

View reviewed changes

michi-covalent merged commit 2834365 into cilium:main Apr 22, 2023
56 checks passed

marqc reviewed Apr 27, 2023

View reviewed changes

epk deleted the epk/hubble-event-filter branch April 27, 2023 13:19

epk mentioned this pull request Apr 27, 2023

Invert --hubble-monitor-events logic to be an allowlist #25167

Merged

8 tasks

marqc mentioned this pull request May 11, 2023

WiP: Add monitor agent perf-event rate limiter. #25385

Closed

8 tasks

marqc mentioned this pull request Sep 1, 2023

WiP: Do not generate events in eBPF if they are going to be dropped in the filter anyway #27885

Closed

5 tasks

kaworu mentioned this pull request Jan 17, 2024

Reduce Hubble CPU usage #30302

Open

marqc mentioned this pull request Jan 23, 2024

agent: add several new flags to control Cilium's datapath events notifications #30063

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hubble: Add `--hubble-monitor-events` flag #24828

hubble: Add `--hubble-monitor-events` flag #24828

epk commented Apr 12, 2023 •

edited

maintainer-s-little-helper bot commented Apr 12, 2023

maintainer-s-little-helper bot commented Apr 12, 2023

maintainer-s-little-helper bot commented Apr 12, 2023

joestringer left a comment

joestringer left a comment

joestringer commented Apr 20, 2023

michi-covalent commented Apr 20, 2023

pippolo84 left a comment

michi-covalent commented Apr 21, 2023

nathanjsweet left a comment

michi-covalent commented Apr 21, 2023

michi-covalent commented Apr 22, 2023

michi-covalent commented Apr 22, 2023

michi-covalent commented Apr 22, 2023

michi-covalent commented Apr 22, 2023

marqc left a comment

marqc Apr 27, 2023

marqc Apr 27, 2023

marqc Apr 27, 2023

marqc Apr 27, 2023

marqc Apr 27, 2023

wayt commented Apr 27, 2023

epk commented Apr 27, 2023

joestringer commented Apr 27, 2023 •

edited

wayt commented Apr 27, 2023

hubble: Add --hubble-monitor-events flag #24828

hubble: Add --hubble-monitor-events flag #24828

Conversation

epk commented Apr 12, 2023 • edited

Why

maintainer-s-little-helper bot commented Apr 12, 2023

maintainer-s-little-helper bot commented Apr 12, 2023

maintainer-s-little-helper bot commented Apr 12, 2023

joestringer left a comment

Choose a reason for hiding this comment

joestringer left a comment

Choose a reason for hiding this comment

joestringer commented Apr 20, 2023

michi-covalent commented Apr 20, 2023

pippolo84 left a comment

Choose a reason for hiding this comment

michi-covalent commented Apr 21, 2023

nathanjsweet left a comment

Choose a reason for hiding this comment

michi-covalent commented Apr 21, 2023

michi-covalent commented Apr 22, 2023

michi-covalent commented Apr 22, 2023

michi-covalent commented Apr 22, 2023

michi-covalent commented Apr 22, 2023

marqc left a comment

Choose a reason for hiding this comment

marqc Apr 27, 2023

Choose a reason for hiding this comment

marqc Apr 27, 2023

Choose a reason for hiding this comment

marqc Apr 27, 2023

Choose a reason for hiding this comment

marqc Apr 27, 2023

Choose a reason for hiding this comment

marqc Apr 27, 2023

Choose a reason for hiding this comment

wayt commented Apr 27, 2023

epk commented Apr 27, 2023

joestringer commented Apr 27, 2023 • edited

wayt commented Apr 27, 2023

hubble: Add `--hubble-monitor-events` flag #24828

hubble: Add `--hubble-monitor-events` flag #24828

epk commented Apr 12, 2023 •

edited

joestringer commented Apr 27, 2023 •

edited