Modularize eventsmap and monitor.Agent #25197

bimmlerd · 2023-04-28T14:25:51Z

Modularizes both the eventsmap and the monitor agent in the simplest way possible.

Would be happy to get some input on:

There's still pkg/option/monitor{_test}.go which are at least somewhat related. It's unclear to me whether there's a benefit in pulling all of that into pkg/monitor, though.
I've done a little copying instead of adding a little dependency (see https://github.com/cilium/cilium/pull/25197/files#r1192527976) , is that acceptable?

bimmlerd · 2023-05-10T08:16:32Z

Huh, the integration test failure is an ipcache/identity cache panic, seemingly unrelated to my PR:

 panic: send on closed channel

goroutine 778 [running]:
github.com/cilium/cilium/pkg/identity/cache.(*localIdentityCache).lookupOrCreate(0xc001d3bb40, 0xc001a2e360, 0x0?)
	/home/runner/work/cilium/cilium/pkg/identity/cache/local.go:109 +0x55d
github.com/cilium/cilium/pkg/identity/cache.(*CachingIdentityAllocator).AllocateIdentity(0xc002828300, {0x3b9b180, 0xc000776280}, 0xc001a2e360, 0x0?, 0xc0?)
	/home/runner/work/cilium/cilium/pkg/identity/cache/allocator.go:349 +0x76c
github.com/cilium/cilium/pkg/ipcache.(*IPCache).resolveIdentity(0xc00010fd00, {0x3b9b180, 0xc000776280}, {{{0x0?, 0x0?}, 0xc000010048?}, 0x0?}, 0x0?, 0x2fd8340?)
	/home/runner/work/cilium/cilium/pkg/ipcache/metadata.go:436 +0xcdd
github.com/cilium/cilium/pkg/ipcache.(*IPCache).InjectLabels(0xc00010fd00, {0x3b9b180, 0xc000776280}, {0xc002958000, 0x3, 0x3})
	/home/runner/work/cilium/cilium/pkg/ipcache/metadata.go:197 +0x68f
github.com/cilium/cilium/pkg/ipcache.(*IPCache).TriggerLabelInjection.func1({0x3b9b180, 0xc000776280})
	/home/runner/work/cilium/cilium/pkg/ipcache/metadata.go:547 +0x54
github.com/cilium/cilium/pkg/controller.(*Controller).runController(0xc0012e0240)
	/home/runner/work/cilium/cilium/pkg/controller/controller.go:216 +0x217
created by github.com/cilium/cilium/pkg/controller.(*Manager).updateController
	/home/runner/work/cilium/cilium/pkg/controller/manager.go:111 +0xb51
FAIL	github.com/cilium/cilium/daemon/cmd	12.826s

Adressed in #25353, rerunning.

bimmlerd · 2023-05-11T11:31:04Z

/test

tklauser · 2023-05-12T15:22:18Z

I've done a little copying instead of adding a little dependency in the determination of the default queue size: here, is that acceptable?

That link unfortunately doesn't show commit contents anymore. Presumably because you've updated/rebased the commits on the branch?

daemon/cmd/daemon_main.go

daemon/cmd/daemon_test.go

pkg/datapath/cells.go

pkg/monitor/agent/cell.go

jrajahalme

Some nits only, take them as coming from from a complete novice in hive/cell :-)

jrajahalme · 2023-05-16T09:39:17Z

pkg/monitor/agent/cell.go

+	flags.Int("monitor-queue-size", 0, "Size of the event queue when reading monitor events")
+}
+
+func new(lc hive.Lifecycle, log logrus.FieldLogger, cfg AgentConfig, _ eventsmap.Map) Agent {


I'd prefer naming this to newMonitorAgent for the sake of "greppability".

Since it's not exported the naming doesn't matter much. For exported constructors I'd still recommend just using New as per usual Go guidelines. You can always grep for foo.New.

jrajahalme · 2023-05-16T09:51:24Z

test/controlplane/suite/agent.go

@@ -20,6 +20,8 @@ import (
 	"github.com/cilium/cilium/pkg/maps/authmap"
 	fakeauthmap "github.com/cilium/cilium/pkg/maps/authmap/fake"
 	"github.com/cilium/cilium/pkg/maps/egressmap"
+	"github.com/cilium/cilium/pkg/monitor/agent"
+	fakeagent "github.com/cilium/cilium/pkg/monitor/agent/fake"


I'd call this fakemonitoragent to disambiguate with any other (future) "agents"

jrajahalme · 2023-05-16T09:52:03Z

pkg/datapath/cells.go

+	eventsmap.Cell,
+
+	// The monitor agent, which multicasts cilium and agent events to its subscribers.
+	monitorAgent.Cell,


Maybe monitorAgent would be better placed in daemon/cmd/cells.go?

The thinking was that anything which interacts with bpf Maps directly is datapath; but I agree that this is sort of on the fence. Let's leave it here for now

I'd argue it belongs to pkg/datapath since it provides a datapath-specific service.

bimmlerd · 2023-05-23T14:57:03Z

Addressed most comments, as well as made the dependency on the eventsmap optional as suggested by Jussi out of band - this way, no config overrides are necessary.

I haven't removed --enable-monitor as it was present before -- but I guess we could talk about deprecating it; disabling doesn't make that much sense as it breaks hubble, and if it's not used, it also doesn't consume CPU cycles. Questions for another PR, though.

bimmlerd · 2023-05-25T11:52:42Z

CI triage:

runtime: jenkins 404, from the time we lost jenkins runs - no choice but to rerun 😕
ci-e2e: filed CI: Setup & Test (14, 6.0-main, none, strict, vxlan, wireguard, true, snat, true) #25669, rerunning.

bimmlerd · 2023-05-25T11:52:54Z

/ci-e2e

bimmlerd · 2023-05-25T11:53:02Z

/test-runtime

bimmlerd · 2023-05-25T13:37:28Z

rebased to address conflicts

bimmlerd · 2023-05-25T13:38:53Z

/test

bimmlerd · 2023-05-26T08:01:51Z

Rebased to get #25681

bimmlerd · 2023-05-26T08:32:20Z

/test

squeed · 2023-05-26T11:19:11Z

needs rebase :-(

The cilium events map is used by the monitor.Agent. In preparation of modularization of the agent, move the initialization of the eventsmap into the hive. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>

The monitor agent consumes the eventsmap to multicast its events, as well as other agent events, to subscribers. Pull it's initialization logic into a cell, to reduce the amount of logic needed in the daemon startup sequence. If there's no eventsmap in the hive graph, the agent functions solely for passing along agent events. Dropping RunMonitor flag: Except for testing, there isn't really a use case for not attaching to the eventsmap (The attachment alone doesn't cause performance overhead yet, only once there is a consumer/listener we start reading.) Putting RunMonitor into AgentConfig would mean exposing it as a flag, which is not desirable. Instead, make the eventsmap dependency optional so that non-privileged tests/benchmarks can still use the monitor agent for its userspace capabilities. Proxy Logger changes: To improve on the isolation of components, make NewAgent private. The only consumer other than the cell is the proxy logger benchmark, which can create a hive instead. Unfortunately, this makes the benchmarks require privileges. Dropping Context() from the API: There isn't a good reason to export the context, and it doesn't belong in the monitor's public API. Rework the coordination between the monitor API server and the agent slightly to make the Context() method unnecessary. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>

bimmlerd · 2023-05-26T12:40:55Z

/test

bimmlerd · 2023-05-30T11:07:06Z

/ci-eks

bimmlerd · 2023-05-30T11:07:31Z

CI triage:

CI EKS failed with connectivity test failed: failed to fetch cilium runtime config: error dialing backend: read tcp 192.168.171.74:55806->192.168.107.35:10250: read: connection reset by peer which sounds like a network blip.

bimmlerd added kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. sig/agent Cilium agent related. area/modularization labels Apr 28, 2023

bimmlerd changed the title ~~Modularize eventsmap and monito.Agent~~ Modularize eventsmap and monitor.Agent Apr 28, 2023

bimmlerd force-pushed the pr/bimmlerd/monitor-agent-cell branch 6 times, most recently from 4cca0b3 to ae82183 Compare May 9, 2023 15:31

bimmlerd mentioned this pull request May 10, 2023

CI: daemon/cmd test panic'ed in TravisCI #25235

Closed

bimmlerd mentioned this pull request May 12, 2023

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

Closed

8 tasks

bimmlerd force-pushed the pr/bimmlerd/monitor-agent-cell branch from ae82183 to ae9a453 Compare May 12, 2023 08:59

bimmlerd marked this pull request as ready for review May 12, 2023 10:56

bimmlerd requested review from a team as code owners May 12, 2023 10:56

bimmlerd requested review from tklauser, jrajahalme and YutaroHayakawa May 12, 2023 10:56

tklauser reviewed May 12, 2023

View reviewed changes

bimmlerd force-pushed the pr/bimmlerd/monitor-agent-cell branch from ae9a453 to afa6252 Compare May 16, 2023 08:32

jrajahalme approved these changes May 16, 2023

View reviewed changes

bimmlerd requested a review from joamaki May 23, 2023 14:57

bimmlerd force-pushed the pr/bimmlerd/monitor-agent-cell branch from b2ff005 to dab15a4 Compare May 25, 2023 13:35

bimmlerd force-pushed the pr/bimmlerd/monitor-agent-cell branch from dab15a4 to a92188d Compare May 26, 2023 08:01

joamaki approved these changes May 26, 2023

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 26, 2023

squeed added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels May 26, 2023

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 26, 2023

squeed added dont-merge/bad-bot To prevent MLH from marking ready-to-merge. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels May 26, 2023

bimmlerd added 2 commits May 26, 2023 14:21

eventsmap: encapsulate in cell

957f601

The cilium events map is used by the monitor.Agent. In preparation of modularization of the agent, move the initialization of the eventsmap into the hive. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>

bimmlerd force-pushed the pr/bimmlerd/monitor-agent-cell branch from a92188d to f594833 Compare May 26, 2023 12:40

bimmlerd removed dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. dont-merge/bad-bot To prevent MLH from marking ready-to-merge. labels May 26, 2023

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 30, 2023

julianwiedmann merged commit d496777 into cilium:main May 31, 2023
58 checks passed

bimmlerd deleted the pr/bimmlerd/monitor-agent-cell branch May 31, 2023 06:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularize eventsmap and monitor.Agent #25197

Modularize eventsmap and monitor.Agent #25197

bimmlerd commented Apr 28, 2023 •

edited

bimmlerd commented May 10, 2023 •

edited

bimmlerd commented May 11, 2023

tklauser commented May 12, 2023

jrajahalme left a comment

jrajahalme May 16, 2023

joamaki May 26, 2023

jrajahalme May 16, 2023

jrajahalme May 16, 2023

bimmlerd May 22, 2023

joamaki May 22, 2023

bimmlerd commented May 23, 2023

bimmlerd commented May 25, 2023

bimmlerd commented May 25, 2023

bimmlerd commented May 25, 2023

bimmlerd commented May 25, 2023

bimmlerd commented May 25, 2023

bimmlerd commented May 26, 2023

bimmlerd commented May 26, 2023

squeed commented May 26, 2023

bimmlerd commented May 26, 2023

bimmlerd commented May 30, 2023

bimmlerd commented May 30, 2023

Modularize eventsmap and monitor.Agent #25197

Modularize eventsmap and monitor.Agent #25197

Conversation

bimmlerd commented Apr 28, 2023 • edited

bimmlerd commented May 10, 2023 • edited

bimmlerd commented May 11, 2023

tklauser commented May 12, 2023

jrajahalme left a comment

Choose a reason for hiding this comment

jrajahalme May 16, 2023

Choose a reason for hiding this comment

joamaki May 26, 2023

Choose a reason for hiding this comment

jrajahalme May 16, 2023

Choose a reason for hiding this comment

jrajahalme May 16, 2023

Choose a reason for hiding this comment

bimmlerd May 22, 2023

Choose a reason for hiding this comment

joamaki May 22, 2023

Choose a reason for hiding this comment

bimmlerd commented May 23, 2023

bimmlerd commented May 25, 2023

bimmlerd commented May 25, 2023

bimmlerd commented May 25, 2023

bimmlerd commented May 25, 2023

bimmlerd commented May 25, 2023

bimmlerd commented May 26, 2023

bimmlerd commented May 26, 2023

squeed commented May 26, 2023

bimmlerd commented May 26, 2023

bimmlerd commented May 30, 2023

bimmlerd commented May 30, 2023

bimmlerd commented Apr 28, 2023 •

edited

bimmlerd commented May 10, 2023 •

edited