Add support for filtering events and minor related changes #673

aalemayhu · 2017-05-09T14:12:06Z

The new filtering support consists of --type, --to and --from.
The cilium help monitor output now looks like this:

The monitor displays notifications and events omitted by the BPF
programs attached to endpoints and devices. This includes:
  * Dropped packet notifications
  * Captured packet traces
  * Debugging information

Usage:
  cilium monitor [flags]

Flags:
  -d, --dissect             Dissect packet data
      --from uint16         filter by source endpoint id
  -c, --num-cpus int        Number of CPUs (default 2)
  -n, --num-pages int       Number of pages for ring buffer (default 64)
      --related-to uint32   filter by either source or destination endpoint id
      --to uint32           filter by destination endpoint id
  -t, --type string         Filter by event types [drop debug capture]

Global Flags:
      --config string   config file (default is $HOME/.cilium.yaml)
  -D, --debug           Enable debug messages
  -H, --host string     URI to server-side API

Some example usage:

cilium monitor --type drop will only show you messages for packets being
dropped.
cilium monitor --type drop --from 173 is same as above but will only match
when the source has the provided id.
cilium monitor --type drop --to 123 should match when we get a drop event
and the destination has the provided id.
cilium monitor --type drop --related-to 749 shows drop notifications where
the source or destination has the provided id.

Closes: #473 (Feature: Add filtering to cilium monitor)

tgraf

Overall this is great. It would be even better if we can support a --related-to ID which takes an endpoint ID and then prints everything which has the endpoint ID in either the from to to. This would show everything in and out of a particular endpoint.

tgraf · 2017-05-09T16:16:12Z

cilium/cmd/monitor.go

+// checkFilter does some input validation to give the user feedback if they
+// wrote something close that did not match for example 'srop' instead of
+// 'drop'.
+func checkFilter() {


I would rename this to validateEventTypeFilter

tgraf · 2017-05-09T16:20:09Z

cilium/cmd/monitor.go

@@ -59,40 +63,97 @@ var (
 		SampleType:   bpf.PERF_SAMPLE_RAW,
 		WakeupEvents: 1,
 	}
+	eventTypeIdx = -1 // for integer comparison
+	eventType    = ""
+	eventTypes   = []string{"drop", "debug", "capture"}


Consider writing this as a map instead:

eventTypes := map[string]int { "debug": bpfdebug.MessageTypeDebug, "drop": bpfdebug.MessageTypeDrop, "capture": bpfdebug.MessageTypeCapture, }

This also makes it clear which names corresponds to which message type without depending on order

tgraf · 2017-05-09T16:21:30Z

cilium/cmd/monitor.go

+func checkFilter() {
+	for i, e := range eventTypes {
+		if eventType == e {
+			eventTypeIdx = i + 1 // 0 == .MessageTypeUnspec, but we only want valid ones.


You can then just check the type against the map:

if eventTypeIdx, ok = eventTypes[eventType]; !ok { err := "Unknown type (%s). Please use one of the following ones %s\n" [...] }

tgraf · 2017-05-09T16:23:30Z

cilium/cmd/monitor.go

+// are supplied. If either one of them is less than zero, then it is assumed user did
+// use them.
+func match(messageType int, src uint16, dst uint32) bool {
+	if eventTypeIdx > bpfdebug.MessageTypeUnspec && messageType != eventTypeIdx {


The > is misleading, write this as:

if eventTypeIdx != bpfdebug.MessageTypeUnspec && messageType != eventTypeIdx {

@scanf was this addressed?

@aanm meant my comment above which is still not addressed. It's a cosmetic thing though.

@aanm ah, must have left this on a different machine. It was marked as outdated so I just assumed it was addressed since Github automatically collapses these. Will follow up. Thanks!

tgraf · 2017-05-09T16:24:10Z

cilium/cmd/monitor.go

+	eventTypeIdx = -1 // for integer comparison
+	eventType    = ""
+	eventTypes   = []string{"drop", "debug", "capture"}
+	fromSource   = 0


Initialize them to uint16 so you don't have to cast later on:

fromSource = uint16(0)

tgraf · 2017-05-09T21:02:40Z

cilium/cmd/monitor.go

@@ -43,11 +43,19 @@ programs attached to endpoints and devices. This includes:
 	},
 }

+func listEventTypes() string {
+	return "[ drop, debug, capture ]"


This is fine, if you want to generate it:

types := []string{} for _, v := range eventTypes { types = append(types, v) } return types

aanm

Great additions. I've one question. Is it possible do multiple filters at the same time?

aanm · 2017-05-09T21:54:27Z

cilium/cmd/monitor.go

 func init() {
 	RootCmd.AddCommand(monitorCmd)
 	monitorCmd.Flags().IntVarP(&eventConfig.NumCpus, "num-cpus", "c", runtime.NumCPU(), "Number of CPUs")
 	monitorCmd.Flags().IntVarP(&eventConfig.NumPages, "num-pages", "n", 64, "Number of pages for ring buffer")
 	monitorCmd.Flags().BoolVarP(&dissect, "dissect", "d", false, "Dissect packet data")
+	monitorCmd.Flags().StringVarP(&eventType, "type", "t", "", fmt.Sprintf("Filter by event types %v", listEventTypes()))
+	monitorCmd.Flags().Uint16Var(&fromSource, "from", 0, "filter by source endpoint id")


filter - > Filter

The same goes with the next 2 liness

aanm · 2017-05-09T21:54:48Z

cilium/cmd/monitor.go

 func init() {
 	RootCmd.AddCommand(monitorCmd)
 	monitorCmd.Flags().IntVarP(&eventConfig.NumCpus, "num-cpus", "c", runtime.NumCPU(), "Number of CPUs")
 	monitorCmd.Flags().IntVarP(&eventConfig.NumPages, "num-pages", "n", 64, "Number of pages for ring buffer")
 	monitorCmd.Flags().BoolVarP(&dissect, "dissect", "d", false, "Dissect packet data")
+	monitorCmd.Flags().StringVarP(&eventType, "type", "t", "", fmt.Sprintf("Filter by event types %v", listEventTypes()))
+	monitorCmd.Flags().Uint16Var(&fromSource, "from", 0, "filter by source endpoint id")


Why Uint16Var and the others have Uint32Var?

To match the existing message format. Using other types would generate compile
time errors. The source fields from the message types define the
.Source format as uint16.

See
https://github.com/cilium/cilium/blob/master/pkg/bpfdebug/drop.go#L30
https://github.com/cilium/cilium/blob/master/pkg/bpfdebug/debug.go#L129
https://github.com/cilium/cilium/blob/master/pkg/bpfdebug/debug.go#L229

also for DstID or Arg1 (uint32) see

https://github.com/cilium/cilium/blob/master/pkg/bpfdebug/drop.go#L36
https://github.com/cilium/cilium/blob/master/pkg/bpfdebug/debug.go#L131
https://github.com/cilium/cilium/blob/master/pkg/bpfdebug/debug.go#L233

The ctypes are in

https://github.com/cilium/cilium/blob/master/bpf/lib/common.h
https://github.com/cilium/cilium/blob/master/bpf/lib/dbg.h

but @borkmann and @tgraf can probably give a much better explanation on why
those fields are uint16 and uint32.

aanm · 2017-05-09T21:57:36Z

cilium/cmd/monitor.go

+		return false
+	} else if toDst > 0 && toDst != dst {
+		return false
+	} else if uint16(related) != src && related > 0 && related != dst {


Shouldn't this be related > 0 && uint16(related) != src && related != dst?

aalemayhu · 2017-05-09T23:25:18Z

@aanm yes combining options should work fine, but please try out. If you discover something doesn't work. Let me know, so I can fix and add a test for it. Thanks.

aanm · 2017-05-10T07:40:08Z

@scanf How does the to flag works? I've been using it but I don't see any packets showing up, only if I use the from flag.

aalemayhu · 2017-05-10T08:08:03Z

@aanm --to option uses the .Arg1 or .DstID which is what I thought
destination is supposed to be. Can you show an example of how you are using it?
Did you enable the relevant configuration you are interested in cilium config Debug=true DropNotification=true Policy=true?

aanm · 2017-05-10T08:21:40Z

@scanf why Policy needs to be set to true?

tgraf · 2017-05-10T13:15:54Z

cilium/cmd/monitor.go

+	if err := binary.Read(bytes.NewReader(data), binary.LittleEndian, &dm); err != nil {
+		fmt.Printf("Error while parsing debug message: %s\n", err)
+	}
+	if match(bpfdebug.MessageTypeDebug, dm.Source, dm.Arg1) {


Passing dm.Arg1 as destination ID is wrong. In most cases, that is not the destination. We can't support --to for debug messages. Let's just pass in0 for now.

tgraf · 2017-05-10T13:17:33Z

cilium/cmd/monitor.go

+	if err := binary.Read(bytes.NewReader(data), binary.LittleEndian, &dc); err != nil {
+		fmt.Printf("Error while parsing debug capture message: %s\n", err)
+	}
+	if match(bpfdebug.MessageTypeCapture, dc.Source, dc.Arg1) {


Same here, we would need to extend the BPF progs to always specify the destination ID before we can pass that in. dc.Arg1 will mean something different depending on the actual message being sent.

This will be used for testing the new filtering arguments in a follow up commit. Related-to: #473 (Feature: Add filtering to cilium monitor) Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>

monitor_resume is useful because it behaves the same as monitor_start() but instead does not overwrite DUMP_FILE. This will be used in follow up to test multiple runs of the monitor. Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>

ianvernon · 2017-05-10T18:15:07Z

cilium/cmd/monitor.go

 )

 func lostEvent(lost *bpf.PerfEventLost, cpu int) {
-	fmt.Printf("Lost %d events\n", lost.Lost)
+	fmt.Printf("CPU %02d: Lost %d events\n", lost.Lost, cpu)


The ordering seems incorrect here - it should be:
fmt.Printf("CPU: %02d: Lost %d events\n", cpu, lost.Lost)

ianvernon · 2017-05-10T18:16:27Z

cilium/cmd/monitor.go

+	return true
+}
+
+func dropEvents(prefix string, data []byte) {


Documentation needed for this function.

ianvernon · 2017-05-10T18:16:56Z

cilium/cmd/monitor.go

+	}
+}
+
+func debugEvents(prefix string, data []byte) {


Documentation needed for this function.

ianvernon · 2017-05-10T18:18:11Z

cilium/cmd/monitor.go

+	}
+}
+
+func captureEvents(prefix string, data []byte) {


Documentation needed for this function.

ianvernon · 2017-05-10T18:18:47Z

cilium/cmd/monitor.go

+	}
+}
+
+func receiveEvent(msg *bpf.PerfEventSample, cpu int) {


Documentation needed for this function.

ianvernon

Mainly nits on my part, but please add the requested documentation.

We are already printing it in func receiveEvent(...). To be consistent print it here as well, and maybe this will be useful for future debugging. Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>

The new filtering support consists of `--type`, `--to` and `--from`. The `cilium help monitor` output now looks like this: The monitor displays notifications and events omitted by the BPF programs attached to endpoints and devices. This includes: * Dropped packet notifications * Captured packet traces * Debugging information Usage: cilium monitor [flags] Flags: -d, --dissect Dissect packet data --from uint16 filter by source endpoint id -c, --num-cpus int Number of CPUs (default 2) -n, --num-pages int Number of pages for ring buffer (default 64) --related-to uint32 filter by either source or destination endpoint id --to uint32 filter by destination endpoint id -t, --type string Filter by event types [drop debug capture] Global Flags: --config string config file (default is $HOME/.cilium.yaml) -D, --debug Enable debug messages -H, --host string URI to server-side API Some example usage: - `cilium monitor --type drop` will only show you messages for packets being dropped. - `cilium monitor --type drop --from 173` is same as above but will only match when the source has the provided id. - `cilium monitor --type drop --to 123` should match when we get a drop event and the destination has the provided id. - `cilium monitor --type drop --related-to 749` shows drop notifications where the source or destination has the provided id. Closes: #473 (Feature: Add filtering to cilium monitor) Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>

aalemayhu · 2017-05-10T19:20:17Z

@aanm sorry, the policy part can be ignored. Please try again to see if you still have issues.

aalemayhu · 2017-05-10T19:20:38Z

@ianvernon fixed, let me know what to reword. Thanks!

aanm · 2017-05-12T20:54:27Z

cilium/cmd/monitor.go

+// are supplied. If either one of them is less than zero, then it is assumed user did
+// use them.
+func match(messageType int, src uint16, dst uint32) bool {
+	if eventTypeIdx > bpfdebug.MessageTypeUnspec && messageType != eventTypeIdx {


@scanf was this addressed?

aalemayhu · 2017-05-13T06:05:50Z

@aanm I am not sure what you mean in the last comment. Could you elaborate on what should have been addressed? Thanks.

Let's merge as-is, we can fix stlye afterwards.

aanm · 2017-05-13T11:41:28Z

@scanf I was referring this comment #673 (comment)

Thomas says: >The > is misleading Related-to: #673 (Add support for filtering events and minor related changes) Suggested-by: Thomas Graf <thomas@cilium.io> Suggested-by: André Martins <andre@cilium.io> Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>

aalemayhu requested a review from tgraf May 9, 2017 14:12

tgraf requested changes May 9, 2017

View reviewed changes

tgraf reviewed May 9, 2017

View reviewed changes

aanm requested changes May 9, 2017

View reviewed changes

tgraf requested changes May 10, 2017

View reviewed changes

aalemayhu added 2 commits May 10, 2017 16:19

tests: use arguments if passed in to monitor_start

f529680

This will be used for testing the new filtering arguments in a follow up commit. Related-to: #473 (Feature: Add filtering to cilium monitor) Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>

tests: new function monitor_resume

c55aa99

monitor_resume is useful because it behaves the same as monitor_start() but instead does not overwrite DUMP_FILE. This will be used in follow up to test multiple runs of the monitor. Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>

ianvernon reviewed May 10, 2017

View reviewed changes

ianvernon requested changes May 10, 2017

View reviewed changes

aalemayhu added 2 commits May 10, 2017 21:01

cmd/monitor: print CPU when losing events

5001e1c

We are already printing it in func receiveEvent(...). To be consistent print it here as well, and maybe this will be useful for future debugging. Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>

aalemayhu self-assigned this May 11, 2017

tgraf approved these changes May 12, 2017

View reviewed changes

tgraf requested review from aanm and ianvernon May 12, 2017 20:25

aanm previously requested changes May 12, 2017

View reviewed changes

ianvernon approved these changes May 12, 2017

View reviewed changes

tgraf merged commit bc0d0e5 into cilium:master May 13, 2017

aalemayhu deleted the filter-473 branch May 13, 2017 11:44

aalemayhu mentioned this pull request May 13, 2017

cmd/monitor: be more specific with eventType match #700

Merged

aalemayhu removed their assignment Jan 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for filtering events and minor related changes #673

Add support for filtering events and minor related changes #673

aalemayhu commented May 9, 2017 •

edited

tgraf left a comment

tgraf May 9, 2017

tgraf May 9, 2017

tgraf May 9, 2017

tgraf May 9, 2017

tgraf May 9, 2017

aanm May 12, 2017

tgraf May 13, 2017

aalemayhu May 13, 2017

tgraf May 9, 2017

tgraf May 9, 2017

aanm left a comment

aanm May 9, 2017

aanm May 9, 2017

aalemayhu May 9, 2017

aanm May 9, 2017

aalemayhu commented May 9, 2017

aanm commented May 10, 2017

aalemayhu commented May 10, 2017

aanm commented May 10, 2017

tgraf May 10, 2017

tgraf May 10, 2017

ianvernon May 10, 2017

ianvernon May 10, 2017

ianvernon May 10, 2017

ianvernon May 10, 2017

ianvernon May 10, 2017

ianvernon left a comment

aalemayhu commented May 10, 2017

aalemayhu commented May 10, 2017

aanm May 12, 2017

aalemayhu commented May 13, 2017

aanm commented May 13, 2017

Add support for filtering events and minor related changes #673

Add support for filtering events and minor related changes #673

Conversation

aalemayhu commented May 9, 2017 • edited

tgraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aanm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalemayhu commented May 9, 2017

aanm commented May 10, 2017

aalemayhu commented May 10, 2017

aanm commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianvernon left a comment

Choose a reason for hiding this comment

aalemayhu commented May 10, 2017

aalemayhu commented May 10, 2017

Choose a reason for hiding this comment

aalemayhu commented May 13, 2017

aanm commented May 13, 2017

aalemayhu commented May 9, 2017 •

edited