Skip to content

Adds TRACE_TO_NETWORK obs label and trace pkts in to-netdev prog.#12245

Merged
qmonnet merged 1 commit intocilium:masterfrom
Weil0ng:trace
Jul 14, 2020
Merged

Adds TRACE_TO_NETWORK obs label and trace pkts in to-netdev prog.#12245
qmonnet merged 1 commit intocilium:masterfrom
Weil0ng:trace

Conversation

@Weil0ng
Copy link
Copy Markdown
Contributor

@Weil0ng Weil0ng commented Jun 23, 2020 •
edited
Loading

This adds TRACE_TO_NETWORK label in the datapath and starts tracing pkts going out in to-netdev prog.

Fixes: #12084

Signed-off-by: Weilong Cui cuiwl@google.com

@Weil0ng Weil0ng requested a review from a team as a code owner June 23, 2020 21:24
@Weil0ng Weil0ng requested review from a team June 23, 2020 21:24
@Weil0ng Weil0ng requested a review from a team as a code owner June 23, 2020 21:24
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper Bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 23, 2020
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@Weil0ng Weil0ng added the wip label Jun 23, 2020
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@Weil0ng
Copy link
Copy Markdown
Contributor Author

Weil0ng commented Jun 24, 2020

test-me-please

gandro
gandro requested changes Jun 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I do have some concerns about API compatibility, both with regards to Hubble (where this is a breaking API change which is not allowed) and with regards to the cilium monitor socket (where I do not know our compatibility policy).

Comment thread api/v1/flow/flow.proto Outdated
Comment thread bpf/lib/trace.h Outdated
@aanm aanm marked this pull request as draft June 24, 2020 10:03
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
vadorovsky
vadorovsky suggested changes Jun 27, 2020
View reviewed changes
Comment thread bpf/lib/trace.h Outdated
@maintainer-s-little-helper maintainer-s-little-helper Bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels Jul 6, 2020
pchaigno
pchaigno requested changes Jul 7, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Thanks @Weil0ng for tackling this!

Comment thread bpf/bpf_host.c Outdated
@Weil0ng Weil0ng force-pushed the trace branch 2 times, most recently from ddb8e21 to ab1b75a Compare July 9, 2020 18:20
pchaigno
pchaigno reviewed Jul 9, 2020
View reviewed changes
Comment thread bpf/bpf_host.c Outdated
@pchaigno pchaigno added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jul 9, 2020
@maintainer-s-little-helper 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 Jul 9, 2020
@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented Jul 9, 2020

@Weil0ng FYI, the BPF checkpatch GitHub Action is failing here, but I don't think you need to fix it. It is not required, would require a lot more line changes than you PR currently has, and you didn't introduce it.

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 9, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.01%) to 36.97% when pulling d214019 on Weil0ng:trace into fb82325 on cilium:master.

@Weil0ng
Adds TRACE_TO_NETWORK obs label and trace pkts in to-netdev prog.
d214019 
Signed-off-by: Weilong Cui <cuiwl@google.com>
@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented Jul 9, 2020

test-me-please

@pchaigno pchaigno requested review from gandro and vadorovsky July 9, 2020 20:35
@Weil0ng
Copy link
Copy Markdown
Contributor Author

Weil0ng commented Jul 10, 2020

K8s-1.11-Kernel-netnext seems to be failing for reasons irrelevant to this PR?

@Weil0ng Weil0ng removed the wip label Jul 10, 2020
@pchaigno pchaigno marked this pull request as ready for review July 10, 2020 02:26
@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented Jul 10, 2020

K8s-1.11-Kernel-netnext seems to be failing for reasons irrelevant to this PR?

Looks like it. I checked the logs for that failure and couldn't find anything wierd or indicate this PR was to blame, so I opened #12491 with all necessary information to track it. I'll now restart just the net-next build to double check it's just a flake.

@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented Jul 10, 2020

retest-net-next

@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented Jul 14, 2020

There are several reviews on all touched code areas and the required tests are passing (+GKE only has the usual failures). I'm marking as ready to merge.

@pchaigno pchaigno added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. labels Jul 14, 2020
@qmonnet qmonnet merged commit c470e28 into cilium:master Jul 14, 2020
@brb brb mentioned this pull request Jul 15, 2020
Merged
@joestringer joestringer mentioned this pull request Jul 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glibsm glibsm glibsm left review comments

@gandro gandro gandro approved these changes

@joestringer joestringer joestringer approved these changes

@pchaigno pchaigno pchaigno approved these changes

@vadorovsky vadorovsky Awaiting requested review from vadorovsky

Assignees

No one assigned

Labels

area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bpf: Fix packet tracing for TO/FROM_NETWORK

10 participants

@Weil0ng @pchaigno @coveralls @gandro @glibsm @joestringer @vadorovsky @brb @christarazi @qmonnet