Improve doc for debug by jodh-intel · Pull Request #846 · clearcontainers/runtime

jodh-intel · 2017-12-06T09:14:25Z

No description provided.

jodh-intel · 2017-12-06T09:15:55Z

Note that, strictly, this is blocked by clearcontainers/proxy#181.

jodh-intel · 2017-12-06T09:16:08Z

Hi @grahamwhaley, @iphutch - please could you review?

clearcontainersbot · 2017-12-06T09:45:04Z

kubernetes qa-passed 👍

grahamwhaley

Some nitpicks, but nothing that really needs to block this, so I'll also give my
lgtm

grahamwhaley · 2017-12-06T10:25:45Z

 latter is useful as it is independent of the lifecycle of each container.

-To view runtime log output,
+To view runtime log output:


silly nit - technically I think that is a colon :, not a semi-colon ; :-)

Wow, that's embarrassing! Fixing... ;)

grahamwhaley · 2017-12-06T10:27:00Z


-The runtime, the shim (`cc-shim`), and the hypervisor all have separate debug
-options in the [configuration file](#Configuration).
+The runtime, the shim (`cc-shim`), the hypervisor and the proxy all have separate `enable_debug=` debug


if we are going to call out cc-shim, do we want to call out cc-proxy as well?

Yeah, I'll re-order the elements and mention the proxy binary name...

grahamwhaley · 2017-12-06T10:30:09Z

@@ -94,11 +94,30 @@ $ sudo journalctl -t cc-runtime

 ### Enabling debug for various components


What do you think to changing this title to:
Enabling and viewing debug for other components
to cover the fact we now show enabling and viewing, and we cover the runtime separately in the above section ?

I take your point but this section also enables debug for the runtime. I'll rework both sections to make this clearer...

jodh-intel · 2017-12-06T10:56:23Z

Hi @grahamwhaley - branch updated.

clearcontainersbot · 2017-12-06T11:05:20Z

kubernetes qa-passed 👍

grahamwhaley

One suggestion - not a blocker.
lgtm

grahamwhaley · 2017-12-06T11:05:43Z

+
+```bash
+$ sudo sed -i -e 's/^#\(enable_debug\).*=.*$/\1 = true/g' /usr/share/defaults/clear-containers/configuration.toml
+```


Looking much better, thanks @jodh-intel
Sorry to be a mild pain, but one more suggestion - do you think we should also link from here to the more detailed/specific debug docs we now have as well?
https://github.com/clearcontainers/runtime/blob/master/docs/debug-agent.md
https://github.com/clearcontainers/runtime/blob/master/docs/debug-kernel.md

Done. Note that those 2 docs had rotted a bit so I've had to update them. In fact, I'm wondering if we want to merge them (maybe into the README itself) since as you can see, they are now very similar.

Thanks @jodh-intel
lgtm

clearcontainersbot · 2017-12-06T11:26:26Z

kubernetes qa-passed 👍

clearcontainersbot · 2017-12-06T12:27:38Z

kubernetes qa-passed 👍

clearcontainersbot · 2017-12-06T13:08:48Z

kubernetes qa-passed 👍

jodh-intel · 2017-12-07T08:34:41Z

Hi @klynnrif, @rcaballeromx - please could you review?

jodh-intel · 2017-12-08T17:12:07Z

Hi @klynnrif, @rcaballeromx - could you squeeze in a review for this one today possibly?

klynnrif · 2017-12-08T17:19:47Z

@jodh-intel @rcaballeromx I should have time to take a look today.

klynnrif

Completed a scrub for grammar and style guide consistency. Thanks!

klynnrif · 2017-12-08T21:29:38Z

-## Debugging
+## Logging

 The runtime provides `--log=` and `--log-format=` options. However, it can


Suggested rewrite to stay active: However, you can also configure it to log...

klynnrif · 2017-12-08T21:37:24Z


-The runtime, the shim (`cc-shim`), and the hypervisor all have separate debug
-options in the [configuration file](#Configuration).
+Note that the proxy log entries also include output from the agent (`cc-agent`) and the


I am unsure how notes are dealt with here, but I would typically format any sentence(s) as a note if it begins with "Note that..." For example,
..note::
The proxy log entries also include output from the agent (cc-agent) and the hypervisor, which includes the guest kernel boot-time messages.

klynnrif · 2017-12-08T21:39:16Z

+
+## Debugging
+
+The runtime, the shim (`cc-shim`), the proxy (`cc-proxy`) and the hypervisor all have separate `enable_debug=` debug


Including the final comma (based on the style guide): The runtime, the shim (cc-shim), the proxy (cc-proxy), and the hypervisor all have searate enable_debug= debug...

klynnrif · 2017-12-08T21:41:02Z

+
+The runtime, the shim (`cc-shim`), the proxy (`cc-proxy`) and the hypervisor all have separate `enable_debug=` debug
+options in the [configuration file](#Configuration). By default, all these
+debug options are disabled. Look at the comments in the installed


See the comments in the...

klynnrif · 2017-12-08T21:41:14Z

+debug options are disabled. Look at the comments in the installed
+configuration file for further details.
+
+If you wish to enable debug for all components, assuming a standard configuration file path, run:


If you want to enable debug...

klynnrif · 2017-12-08T21:48:33Z

-(standard program when running containers on bare metal).
+The Clear Containers agent relies heavily on [`libcontainer`](https://github.com/opencontainers/runc/tree/master/libcontainer) used by [`runc`](https://github.com/opencontainers/runc/) (standard program when running containers on bare metal).

 To provide a debug log of any agent activity on a guest, the Clear Containers


Lines 12-14 suggested rewrite (possible this changes the meaning): To provide a debug log of any agent activity on a guest, the Clear Containers agent sends a log through a QEMU serial console, which is collected by cc-proxy] (https://github.com/clearcontainers/proxy) and shown in the proxy's logs.

klynnrif · 2017-12-08T21:51:21Z

+1. Filter the agent debug logs from the `cc-proxy` logs
+
+ The `cc-proxy` logs show the sources of its collated information. To only see
+ the agent debug logs, filter `cc-proxy` logs by the QEMU serial console (the


... the agent debug logs, filter cc-proxy logs by the QEMU serial console that sent the agent logs.

klynnrif · 2017-12-08T22:02:29Z

-1. Enable debug in `configuration.toml`
+1. Enable kernel boot messages
+
+ Set the `enable_debug=` option in the `[proxy.cc]` section to `true` (assumes a standard configuration file path):


Set the enable_debut= option in the [proxy.cc] section to true, which assumes a standard configuration file path:

klynnrif · 2017-12-08T22:03:42Z

+
+1. Enable proxy debug
+
+ Set the `enable_debug=` option in the `[proxy.cc]` section to `true` (assumes a standard configuration file path):


Set the enable_debug= option in the [proxy.cc] section to true, which assumes a standard configuration file path:

klynnrif · 2017-12-08T22:04:59Z


-The Clear Containers Agent relies heavily on libcontainer used by runc 
-(standard program when running containers on bare metal).
+The Clear Containers agent relies heavily on [`libcontainer`](https://github.com/opencontainers/runc/tree/master/libcontainer) used by [`runc`](https://github.com/opencontainers/runc/) (standard program when running containers on bare metal).


... used by [runc] (https://github.com/opencontainers/runc/), which is the standard program when running containers on bare metal.

jodh-intel · 2017-12-11T08:56:45Z

Hi @klynnrif - thanks for reviewing. Branch updated.

(Note that as some of your suggested changes weren't strictly part of the changes I'd made on this PR, I've created a new commit specifically to address all the review changes).

jodh-intel · 2017-12-12T11:11:41Z

Hi @klynnrif, @rcaballeromx - could one of you take a look at this please? I'd really like to get this landed this week if possible.

klynnrif · 2017-12-12T17:12:10Z

@jodh-intel @rcaballeromx I can take a look at this today. Thanks!

klynnrif

A couple minor changes suggested. Thanks!

klynnrif · 2017-12-12T22:36:23Z

+Note:
+
+The proxy log entries also include output from the agent (`cc-agent`) and the
+hypervisor (which includes the guest kernel boot-time messages).


... hypervisor, which includes the guest kernel boot-time messages.

klynnrif · 2017-12-12T22:47:28Z

+
+The runtime, the shim (`cc-shim`), the proxy (`cc-proxy`),
+and the hypervisor all have separate `enable_debug=` debug
+options in the [configuration file](#Configuration). By default, all these


Lines 116-117 suggested rewrite: All of these debug options are disables by default.

clearcontainersbot · 2017-12-14T02:28:41Z

kubernetes qa-passed 👍

All commands should be introduced with a sentence that ends with a colon. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Renamed "Debugging" section to "Logging" and explained how to view log output for all system components. Also created a top-level "Debugging" section which explains how to enable full debug. Fixesclearcontainers#845. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

The agent and kernel debug documents had deviated from reality. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Apply further changes to the debug and logging documents based on review feedback. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel · 2017-12-14T13:05:52Z

Hi @klynnrif - thanks for re-reviewing. Branch updated.

clearcontainersbot · 2017-12-14T13:26:38Z

kubernetes qa-passed 👍

jodh-intel · 2017-12-15T14:25:27Z

Hi @klynnrif - could you re-review this today if possible?

klynnrif · 2017-12-15T20:32:07Z

@jodh-intel Yes, I can. Thanks!

rcaballeromx

Great job @jodh-intel @klynnrif

jodh-intel · 2017-12-18T13:15:39Z

Hi @klynnrif - please can you check this today?

We have sufficient approvals (thanks Rodrigo) but I want to give you one last chance to review (as we still have the red cross against your name). If I haven't heard anything by COB today, I'll assume you are happy for me to force-merge this.

jodh-intel · 2017-12-19T12:37:11Z

We have sufficient approvals, so merging as we really need this to land...

…oints network: Marshal BridgedMacvlanEndpoint and MacvtapEndpoint

jodh-intel added the in progress label Dec 6, 2017

grahamwhaley approved these changes Dec 6, 2017
View reviewed changes

jodh-intel force-pushed the improve-doc-for-debug branch 2 times, most recently from a761e8c to 4a55e72Compare December 6, 2017 10:55

grahamwhaley approved these changes Dec 6, 2017
View reviewed changes

jodh-intel force-pushed the improve-doc-for-debug branch 3 times, most recently from d51328e to 001ed84Compare December 6, 2017 12:11

klynnrif suggested changes Dec 8, 2017
View reviewed changes

klynnrif suggested changes Dec 12, 2017
View reviewed changes

James O. D. Hunt added 4 commits December 14, 2017 13:04

doc: Use colon for consistency
f4e26f9
All commands should be introduced with a sentence that ends with a colon. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

doc: Update agent and kernel debug documents
0bedb71
The agent and kernel debug documents had deviated from reality. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

doc: Update logging and debug docs based on review
56aa874
Apply further changes to the debug and logging documents based on review feedback. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel force-pushed the improve-doc-for-debug branch from bdcb1fe to 56aa874Compare December 14, 2017 13:05

rcaballeromx approved these changes Dec 15, 2017
View reviewed changes

jodh-intel merged commit 346b6d9 into clearcontainers:masterDec 19, 2017

jodh-intel removed the in progress label Dec 19, 2017

mcastelino pushed a commit to mcastelino/runtime that referenced this pull request Dec 6, 2018
Merge pull requestclearcontainers#846from amshinde/marshal-net-endp…
40ee885
…oints network: Marshal BridgedMacvlanEndpoint and MacvtapEndpoint

		@@ -94,11 +94,30 @@ $ sudo journalctl -t cc-runtime

		### Enabling debug for various components


		## Debugging

		The runtime, the shim (`cc-shim`), the proxy (`cc-proxy`) and the hypervisor all have separate `enable_debug=` debug


		1. Enable proxy debug

		Set the `enable_debug=` option in the `[proxy.cc]` section to `true` (assumes a standard configuration file path):

Uh oh!

Conversation

jodh-intel commented Dec 6, 2017

Uh oh!

jodh-intel commented Dec 6, 2017

Uh oh!

jodh-intel commented Dec 6, 2017

Uh oh!

clearcontainersbot commented Dec 6, 2017

Uh oh!

grahamwhaley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Dec 6, 2017

Uh oh!

clearcontainersbot commented Dec 6, 2017

Uh oh!

grahamwhaley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clearcontainersbot commented Dec 6, 2017

Uh oh!

clearcontainersbot commented Dec 6, 2017

Uh oh!

clearcontainersbot commented Dec 6, 2017

Uh oh!

jodh-intel commented Dec 7, 2017

Uh oh!

jodh-intel commented Dec 8, 2017

Uh oh!

klynnrif commented Dec 8, 2017

Uh oh!

klynnrif left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Dec 11, 2017

Uh oh!

jodh-intel commented Dec 12, 2017

Uh oh!

klynnrif commented Dec 12, 2017

Uh oh!

klynnrif left a comment