hubble: Add `--hubble-monitor-events` flag by epk · Pull Request #24828 · cilium/cilium

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

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.

Hubble Monitor Filter: invert logic Cherry-picks: * cilium#24828 * cilium#25167 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> Change-Id: Iaa292209dce920c19b70dd8c62db2334e3c8b514 Reviewed-on: https://gke-internal-review.googlesource.com/c/third_party/cilium/+/694178 Unit-Verified: Prow_Bot_V2 <425329972751-compute@developer.gserviceaccount.com> Reviewed-by: Alan Kutniewski <kutniewski@google.com> Upstream-PR: cilium#24828 Upstream-PR: cilium#25167 Reviewed-on: https://gke-internal-review.googlesource.com/c/third_party/cilium/+/782721 Reviewed-by: Akhil Velagapudi <avelagap@google.com>

This commit introduce a new Hubble cell. It is the first of a series of refactoring patches gradually transitioning Hubble configuration and startup code away from the Cilium daemon. Currently, all the Hubble configuration and initialization is implemented in the Cilium daemon. Hubble related flags (e.g. --enable-hubble) are member of the DaemonConfig struct, and the Hubble startup code is under daemon/cmd/hubble.go. Most notably the launchHubble() func which handle all the Hubble component startup is called from cmd.newDaemon(). Over time, launchHubble() has become increasingly complex, initializing more and more Hubble subsystems while bailing out if any of them encounter a configuration or startup error. This has lead to a situation where a misconfiguration of one part of Hubble (e.g. metrics) could effectively disable other Hubble components without crashing the Cilium daemon. The recent addition of many new Hubble feature / subsystems, for example monitor event filtering[1], L7 data redaction[2], and k8s Events integration on drop notifications[3] introducing new Hubble related flags and logic both increase the potential of startup failure and the surface of affected features. Furthermore, most of the new feature increase Hubble's reliance on the Cilium daemon for either flag setup/parsing and/or dependencies injection, making it harder to decouple Hubble from the Daemon. Fortunately, many part of Cilium already use the hive/cell framework for configuration, startup, and dependency injection. Most notably, all Hubble dependencies are now available as cell, which open the possibility to convert Hubble components as cell themselves. The goal of this patch series is to refactor Hubble into its own new cell and decouple it from the Cilium daemon. Concretely, all the code under daemon/cmd/hubble.go will be moved away into the new cell, along with the flags and Hubble startup, until the point where it would be possible to create a hive with the Hubble cell but without the Cilium daemon. Splitting the Hubble components into smaller cells is left for a later time. [1]: cilium#24828 [2]: cilium#23887 [3]: cilium#29565 Signed-off-by: Alexandre Perrin <alex@isovalent.com>

This commit introduce a new Hubble cell. It is the first of a series of refactoring patches gradually transitioning Hubble configuration and startup code away from the Cilium daemon. Currently, all the Hubble configuration and initialization is implemented in the Cilium daemon. Hubble related flags (e.g. --enable-hubble) are member of the DaemonConfig struct, and the Hubble startup code is under daemon/cmd/hubble.go. Most notably the launchHubble() func which handle all the Hubble component startup is called from cmd.newDaemon(). Over time, launchHubble() has become increasingly complex, initializing more and more Hubble subsystems while bailing out if any of them encounter a configuration or startup error. This has lead to a situation where a misconfiguration of one part of Hubble (e.g. metrics) could effectively disable other Hubble components without crashing the Cilium daemon. The recent addition of many new Hubble feature / subsystems, for example monitor event filtering[1], L7 data redaction[2], and k8s Events integration on drop notifications[3] introducing new Hubble related flags and logic both increase the potential of startup failure and the surface of affected features. Furthermore, most of the new feature increase Hubble's reliance on the Cilium daemon for either flag setup/parsing and/or dependencies injection, making it harder to decouple Hubble from the Daemon. Fortunately, many part of Cilium already use the hive/cell framework for configuration, startup, and dependency injection. Most notably, all Hubble dependencies are now available as cell, which open the possibility to convert Hubble components as cell themselves. The goal of this patch series is to refactor Hubble into its own new cell and decouple it from the Cilium daemon. Concretely, all the code under daemon/cmd/hubble.go will be moved away into the new cell, along with the flags and Hubble startup, until the point where it would be possible to create a hive with the Hubble cell but without the Cilium daemon. Splitting the Hubble components into smaller cells is left for a later time. [1]: #24828 [2]: #23887 [3]: #29565 Signed-off-by: Alexandre Perrin <alex@isovalent.com>

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 asauber, chancez and pippolo84 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 Outdated 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

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

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

kaworu mentioned this pull request Oct 3, 2024

Refactor Hubble as a cell #35206

Merged

Conversation

epk commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

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

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michi-covalent commented Apr 21, 2023

Uh oh!

nathanjsweet left a comment

Choose a reason for hiding this comment

Uh oh!

michi-covalent commented Apr 21, 2023

Uh oh!

michi-covalent commented Apr 22, 2023

Uh oh!

michi-covalent commented Apr 22, 2023

Uh oh!

michi-covalent commented Apr 22, 2023

Uh oh!

michi-covalent commented Apr 22, 2023

Uh oh!

marqc left a comment

Choose a reason for hiding this comment

Uh oh!

marqc Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

marqc Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

marqc Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

marqc Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

marqc Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

wayt commented Apr 27, 2023

Uh oh!

epk commented Apr 27, 2023

Uh oh!

joestringer commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wayt commented Apr 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

epk commented Apr 12, 2023 •

edited

Loading

joestringer commented Apr 27, 2023 •

edited

Loading