daemon: Add "datapath" opt to --debug-verbose flag by brb · Pull Request #12568 · cilium/cilium

brb · 2020-07-17T07:28:39Z

This PR adds a support for the "datapath" option to --debug-verbose

Adding the "datapath" to --debug-verbose enables debug messages produced by the datapath.

Previously, it could have been enabled for new endpoints by invoking cilium config Debug{,LB}=Enabled on each node, and for existing - by cilium config $EID Debug{,LB}=Enabled.

Added needs-backport/1.8, as it will ease debugging of 1.8 user datapath issues.

daemon: Add "datapath" opt to --debug-verbose flag to enable datapath debug messages

brb · 2020-07-17T07:30:02Z

test-me-please

pchaigno

global.debugDatapath made more sense to me, but I'm not sure it matters that much either way 🤷

joestringer · 2020-07-17T17:19:07Z

Minor nit, I wondered if you considered extending --debug-verbose instead for something like --debug-verbose=bpf. One less flag and would mean more of the verbose debug functionality can be found from the same commandline argument. If you think this would be better, feel free to adjust the PR for this approach but also feel free to disagree, I'm not adamant that it must be done this way :-)

joestringer · 2020-07-17T17:22:32Z

+{{- if .Values.global.debug.datapath }}
+  debug-datapath: "{{ .Values.global.debug.datapath }}"
+{{- end }}


Shouldn't this be part of config rather than global?

I'm not very familiar with those groupings. What's a logical difference between config and global?

config is part of the settings specific to the helm chart that deploys the Cilium configmap; global applies settings to the entire set of helm charts we generate. Recent discussions with @cilium/helm particularly around the time of v1.8 release established that our use of Helm is a bit unusual to the detriment of operators, and one of the areas we can improve is in using more chart-local variables rather than global variables.

borkmann · 2020-07-17T17:22:54Z

Minor nit, I wondered if you considered extending --debug-verbose instead for something like --debug-verbose=bpf. One less flag and would mean more of the verbose debug functionality can be found from the same commandline argument. If you think this would be better, feel free to adjust the PR for this approach but also feel free to disagree, I'm not adamant that it must be done this way :-)

Or even reuse the existing flag instead of a new one. --debug is as-is for agent debug, but it could take a string: --debug={agent,bpf,all} to select in a more fine-grained manner. Noone aside from developers would be using --debug anyway, so I think it would be better to reuse than to add yet more agent flags.

christarazi

Change looks good to me. I'd also like to +1 @borkmann's idea of reusing the same agent flags.

joestringer · 2020-07-17T20:54:35Z

Just to be clear, debug-verbose isn't new, it already exists and takes a stringslice argument for extra verbosity arguments like @borkmann 's suggestion, the most common usage so far would be for --debug-verbose=flow to get some additional L7 configuration information.

borkmann · 2020-07-20T08:08:09Z

Just to be clear, debug-verbose isn't new, it already exists and takes a stringslice argument for extra verbosity arguments like @borkmann 's suggestion, the most common usage so far would be for --debug-verbose=flow to get some additional L7 configuration information.

Good to know, wasn't aware of that. That sounds reasonable to me.

brb · 2020-07-20T09:11:28Z

I cannot reuse --debug as it accepts bool, and changing its expected values would break existing installations. --debug-verbose SGTM.

Adding the "datapath" to --debug-verbose enables debug messages produced by the datapath. Previously, it could have been enabled for new endpoints by invoking "cilium config Debug{,LB}=Enabled" on each node, and for existing - by "cilium config $EID Debug{,LB}=Enabled". Signed-off-by: Martynas Pumputis <m@lambda.lt>

brb · 2020-07-20T10:15:45Z

test-me-please

brb · 2020-07-20T10:15:50Z

PTAL.

coveralls · 2020-07-20T10:51:18Z

Coverage increased (+0.03%) to 37.028% when pulling 9230ce5 on pr/brb/debug-datapath-flag into 2aae988 on master.

aanm

Please add a note in the upgrade guide as this is a breaking change. Previous environments running with debug: true will no longer see any debug message when running with cilium monitor -v

aanm · 2020-07-20T14:14:44Z


-	option.Config.Opts.SetBool(option.Debug, false)
-	option.Config.Opts.SetBool(option.DebugLB, false)
+	option.Config.Opts.SetBool(option.Debug, debugDatapath)


Also, perhaps it would be a good idea to add a note in the flag description of the debug flag that it no longer enables debug messages in the datapath and it should users should use debug-verbose=datapath instead.

^ Probably needs backport right?

@joestringer this PR is already marked for backport, was that your concern?

aanm

Also, please add --debug-verbose=datapath in the contrib/vagrant/start.sh where we have --debug

brb · 2020-07-20T15:14:32Z

@aanm My PR doesn't change the --debug behavior. It was changed quite some time ago by 583e67c. Do you think that we should still update the upgrade guide? Also IMHO, cilium monitor -v is used only by developers.

aanm

I wasn't aware that ee5e396 introduced this breaking change. Given that it did, yes we should describe it in the upgrade guide. If we start helping users where we ask them to run cilium with debug: true so that we can see debug messages in Cilium monitor we won't see debug messages and would waste more time until we could realize this was introduced by us.

christarazi

LGTM assuming Andre's comments are addressed

joestringer · 2020-07-20T21:40:46Z


-	option.Config.Opts.SetBool(option.Debug, false)
-	option.Config.Opts.SetBool(option.DebugLB, false)
+	option.Config.Opts.SetBool(option.Debug, debugDatapath)


^ Probably needs backport right?

Add a note to the upgrade guide how to enable datapath debug messages which were disabled by [1] (released in v1.8.0). [1]: 583e67c Signed-off-by: Martynas Pumputis <m@lambda.lt>

brb · 2020-07-21T11:38:02Z

@aanm PTAL - I've updated the upgrade guide. No integration tests (except for docs) need to be rerun, as the new commit touches only the docs (previously all the tests passed).

brb added pending-review area/daemon Impacts operation of the Cilium daemon. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jul 17, 2020

brb requested a review from a team July 17, 2020 07:28

brb requested review from a team as code owners July 17, 2020 07:28

brb requested a review from a team July 17, 2020 07:28

pchaigno approved these changes Jul 17, 2020

View reviewed changes

joestringer requested changes Jul 17, 2020

View reviewed changes

christarazi reviewed Jul 17, 2020

View reviewed changes

brb force-pushed the pr/brb/debug-datapath-flag branch from d4f433c to ebf60c5 Compare July 20, 2020 10:12

brb changed the title ~~daemon: Add --debug-datapath flag~~ daemon: Add "datapath" opt to --debug-verbose flag Jul 20, 2020

brb requested review from christarazi and joestringer July 20, 2020 10:14

pchaigno approved these changes Jul 20, 2020

View reviewed changes

maintainer-s-little-helper Bot removed the request for review from a team July 20, 2020 12:14

nebril approved these changes Jul 20, 2020

View reviewed changes

aanm requested changes Jul 20, 2020

View reviewed changes

christarazi approved these changes Jul 20, 2020

View reviewed changes

joestringer approved these changes Jul 20, 2020

View reviewed changes

brb requested a review from aanm July 21, 2020 11:36

docs: Add note about enabling datapath dbg messages

9230ce5

Add a note to the upgrade guide how to enable datapath debug messages which were disabled by [1] (released in v1.8.0). [1]: 583e67c Signed-off-by: Martynas Pumputis <m@lambda.lt>

aanm approved these changes Jul 21, 2020

View reviewed changes

aanm merged commit 1173f18 into master Jul 21, 2020

aanm deleted the pr/brb/debug-datapath-flag branch July 21, 2020 13:18

pchaigno mentioned this pull request Jul 23, 2020

v1.8 backports 2020-07-23 #12627

Merged

pchaigno added backport-pending/1.8 and removed needs-backport/1.8 labels Jul 23, 2020

christarazi added backport-done/1.8 and removed backport-pending/1.8 labels Jul 23, 2020

Conversation

brb commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brb commented Jul 17, 2020

Uh oh!

pchaigno left a comment

Choose a reason for hiding this comment

Uh oh!

joestringer commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

brb Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

joestringer Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

borkmann commented Jul 17, 2020

Uh oh!

christarazi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joestringer commented Jul 17, 2020

Uh oh!

borkmann commented Jul 20, 2020

Uh oh!

brb commented Jul 20, 2020

Uh oh!

brb commented Jul 20, 2020

Uh oh!

brb commented Jul 20, 2020

Uh oh!

coveralls commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

aanm Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

joestringer Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

aanm Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

brb commented Jul 20, 2020

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

joestringer Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

brb commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

brb commented Jul 17, 2020 •

edited

Loading

joestringer commented Jul 17, 2020 •

edited

Loading

christarazi left a comment •

edited

Loading

coveralls commented Jul 20, 2020 •

edited

Loading

brb commented Jul 21, 2020 •

edited

Loading