Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: Document --debug-verbose=datapath in debugging datapath section #16022

Merged

Conversation

navarrothiago
Copy link
Contributor

Adds --debug-verbose=datapath option for debugging.

Signed-off-by: Thiago Navarro navarro@accuknox.com

@navarrothiago navarrothiago requested review from a team as code owners May 5, 2021 11:25
@maintainer-s-little-helper 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 May 5, 2021
Copy link
Member

@qmonnet qmonnet 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 doc update!
Please find some feedback below.

Note also that you're using Markdown-style single backquotes to mark inline literals, but RST uses double backquotes. If you wanted to preview the changes, you can generate the HTML document locally with make -C Documentation live-preview.

Documentation/contributing/development/dev_setup.rst Outdated Show resolved Hide resolved
Documentation/contributing/development/dev_setup.rst Outdated Show resolved Hide resolved
Documentation/contributing/development/dev_setup.rst Outdated Show resolved Hide resolved
Documentation/contributing/development/dev_setup.rst Outdated Show resolved Hide resolved
@qmonnet qmonnet changed the title docs: Updates debugging datapath section docs: Document --debug-verbose=datapath in debugging datapath section May 5, 2021
@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels May 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 5, 2021
@navarrothiago
Copy link
Contributor Author

@qmonnet, thanks for your comments. Let me know if you have more suggestions!

@navarrothiago navarrothiago requested a review from qmonnet May 5, 2021 13:59
@navarrothiago navarrothiago marked this pull request as draft May 10, 2021 13:39
@navarrothiago navarrothiago force-pushed the update-doc-debug-datapath branch 2 times, most recently from e8a6d06 to 7666a1c Compare May 10, 2021 18:57
@navarrothiago navarrothiago marked this pull request as ready for review May 10, 2021 18:58
@navarrothiago navarrothiago marked this pull request as draft May 10, 2021 19:13
@navarrothiago navarrothiago marked this pull request as ready for review May 11, 2021 01:03
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks! A few comments:

  • I think cilium_dbg() should be the preferred method over printk(), because we already have many calls in place in the code, and it integrates well in cilium monitor whereas printk() logs may be harder to track in the datapath flow. Plus printk() is supposed to have lower performance. So I would mention cilium monitor/cilium_dbg() first, and bpftool prog tracelog/printk() last.

  • --debug and --debug-verbose=datapath are not equivalent. I suspect (but didn't check) that the later also sets the former. My understanding is that --debug-verbose=datapath also defines the LB_DEBUG macros used in bpf/lib/lb.h to provide additional logs about the load-balancer. But I do not believe these LB_DEBUG macros condition the usage of either cilium_dbg() or printk().

  • By the way, cilium config debug=true does not seem to enable DebugLB, like --debug-verbose=datapath does. I think that cilium config debugLB=true is needed (but conversely, this won't set Debug).

Overall it is not clear when to use printk() or cilium_dbg(). I'm not sure there is a clear answer to that, I think cilium_dbg() should be favoured, but the more “traditional” printk() method remains available for developers who prefer to handle their debug logs outside of cilium monitor's framework.

I would also try to make the distinction between functions and options more explicit. I do understand the paragraph because I know what the options or debug functions do, but it would be more accessible, I think, if we stated explicitly that:

  1. There are options to print logs from the datapath with cilium monitor: ...
  2. There are options to get even more logs, for the load-balancer: ...
  3. Depending on which options are enabled, developers can also add their own debug messages. Two functions are available: cilium_dbg() and printk(), they behave differently and their output should be retrieved differently and blah blah blah.

What do you think?
[Asking a review from Joe who may have some valuable input here.]

@joestringer
Copy link
Member

My understanding is that --debug-verbose=datapath also defines the LB_DEBUG macros used in bpf/lib/lb.h to provide additional logs about the load-balancer. But I do not believe they condition the usage of either cilium_dbg() or printk().

--debug-verbose=datapath enables the datapath DEBUG option (via option.Config.Opts) in a somewhat sneaky way:

$ git grep -i -B 2 debugdatapath
daemon/cmd/daemon_main.go-
daemon/cmd/daemon_main.go-func initEnv(cmd *cobra.Command) {
daemon/cmd/daemon_main.go:      var debugDatapath bool
--
daemon/cmd/daemon_main.go-              case argDebugVerboseDatapath:
daemon/cmd/daemon_main.go-                      log.Debugf("Enabling datapath debug messages")
daemon/cmd/daemon_main.go:                      debugDatapath = true
--
daemon/cmd/daemon_main.go-      bpf.CheckOrMountFS(option.Config.BPFRoot, k8s.IsEnabled())
daemon/cmd/daemon_main.go-
daemon/cmd/daemon_main.go:      option.Config.Opts.SetBool(option.Debug, debugDatapath)
daemon/cmd/daemon_main.go:      option.Config.Opts.SetBool(option.DebugLB, debugDatapath)

(Plus later the option.Config.Opts is serialized into individual endpoint headers to enable debug for the endpoint).

@navarrothiago navarrothiago marked this pull request as draft May 12, 2021 19:33
@navarrothiago navarrothiago force-pushed the update-doc-debug-datapath branch 2 times, most recently from 490b008 to 7346708 Compare May 21, 2021 20:34
@navarrothiago navarrothiago force-pushed the update-doc-debug-datapath branch 3 times, most recently from 6aec9f5 to 97d2f88 Compare May 24, 2021 12:11
@navarrothiago navarrothiago marked this pull request as ready for review May 24, 2021 12:25
@navarrothiago navarrothiago force-pushed the update-doc-debug-datapath branch 3 times, most recently from 7f14be1 to d52981a Compare May 24, 2021 12:38
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks, nice rework! It looks more correct and makes the different options much clearer, I find it much better than the previous versions of the PR.

Now for the small details :). A few observations below, mostly I think the paragraph starts a bit abruptly for users who come here just looking for a way to debug their issue, I think we should put the commands first and the explanations after that. See suggestions below.

Documentation/contributing/development/dev_setup.rst Outdated Show resolved Hide resolved
Documentation/contributing/development/dev_setup.rst Outdated Show resolved Hide resolved
Documentation/contributing/development/dev_setup.rst Outdated Show resolved Hide resolved
@qmonnet
Copy link
Member

qmonnet commented May 24, 2021

Ah, one other thing, we are trying to move away from running commands inside of Cilium pods, but to use the cilium CLI instead. I haven't checked if it can be used to enable debug messages (and the debugLB?), would you happen to know? It would be nice if we could use this generic CLI instead of the old one which is just available on the agent pods.

@navarrothiago
Copy link
Contributor Author

Ah, one other thing, we are trying to move away from running commands inside of Cilium pods, but to use the cilium CLI instead. I haven't checked if it can be used to enable debug messages (and the debugLB?), would you happen to know? It would be nice if we could use this generic CLI instead of the old one which is just available on the agent pods.

Oh! I did not know that. I have never tried to used the cilium CLI. Because of that, could we follow this PR focusing on the idea that we are doing (without CLI)? I believe it will took me a little bit more to understand about it. If you have other suggestion (corrections), please, let me know! And, thank you again :)

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

OK let's keep the cilium-cli for another PR.
I find the current version really neat, thanks a lot for this work!

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Nitpick: maybe we can unify style of diagrams in the docs? E.g. https://docs.cilium.io/en/v1.10/gettingstarted/cni-chaining-aws-cni/

@navarrothiago navarrothiago requested a review from brb May 27, 2021 11:45
@navarrothiago navarrothiago force-pushed the update-doc-debug-datapath branch 2 times, most recently from 11c0da4 to 436475e Compare May 27, 2021 11:55
Adds `--debug-verbose=datapath` option for debugging eBPF datapath.

Signed-off-by: Thiago Navarro <navarro@accuknox.com>
Copy link
Contributor

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

As someone who doesn't know much about this, I find this additional information very helpful! Thanks very much!

@nathanjsweet nathanjsweet merged commit 2fd5c0f into cilium:master Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants