Improve doc for debug #846

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

README.md

 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

README.md


-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

README.md

 $ 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

README.md

+
+```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

README.md

-## 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

README.md


-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

README.md

+
+## 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

README.md

+
+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

README.md

+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

docs/debug-agent.md

-(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

docs/debug-agent.md

+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

docs/debug-kernel.md

-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

docs/debug-agent.md

+
+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

docs/debug-agent.md


-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

README.md

+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

README.md

+
+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. Fixes clearcontainers#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 4a55e72 Compare 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 001ed84 Compare 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 56aa874 Compare December 14, 2017 13:05

rcaballeromx approved these changes Dec 15, 2017

View reviewed changes

jodh-intel merged commit 346b6d9 into clearcontainers:master Dec 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 request clearcontainers#846 from amshinde/marshal-net-endp…

40ee885

…oints network: Marshal BridgedMacvlanEndpoint and MacvtapEndpoint


		## 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):

Improve doc for debug #846

Improve doc for debug #846

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