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

Virtlet diagnostics #712

Merged
merged 15 commits into from
Jul 11, 2018
Merged

Virtlet diagnostics #712

merged 15 commits into from
Jul 11, 2018

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Jul 10, 2018

This adds a possibility to retrieve Virtlet diagnostics information using sonobuoy or virtletctl. When using sonobuoy, a plugin is added that uses virtlet image to invoke virtletctl diag.
virtletctl dumpmetadata command is removed as its output is now available as part
of what virtletctl diag dump produces, but it's obtained in a safer manner.

  • diagnostics framework (collecting info, unpacking info from JSON to files&dirs)
  • command-based diagnostics (ip r etc.)
  • collecting qemu log files
  • dumping Go stack
  • examining network namespaces
  • dumping metadata info
  • dumping libvirt xml info
  • virtlet --diag (grabbing diagnostics info from within the Virtlet pod)
  • add virtletctl diag dump that retrieves diagnostics info from each virtlet pod (--json to get raw json w/o unpacking)
  • add virtletctl diag unpack that unpacks Virtlet diagnostics from JSON file
  • add virtletctl diag sonobuoy that patches sonobuoy yaml to inject Virtlet plugin
  • retrieve virtlet pod logs during virtletctl diag dump [convenience]
  • retrieve criproxy logs on the node, if found [convenience]
  • fix CodeClimate issues
  • remove virtletctl dumpmetadata
  • add docs

This change is Reviewable

Ivan Shvedunov added 6 commits July 10, 2018 22:25
This command patches sonobuoy-generated yaml to include the Virtlet
plugin.
It's functionality is now covered by `virtletctl diag dump` in a safer
manner.
@ivan4th ivan4th force-pushed the ivan4th/diag branch 3 times, most recently from 816b8cd to 38eed99 Compare July 10, 2018 23:44
@ivan4th ivan4th changed the title [WiP] Virtlet diagnostics Virtlet diagnostics Jul 10, 2018
@ivan4th
Copy link
Contributor Author

ivan4th commented Jul 10, 2018

The PR is ready for review.

Copy link
Contributor

@jellonek jellonek left a comment

Choose a reason for hiding this comment

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

Please remove from PR changes in docs which are only changing timestamp.

Reviewed 43 of 50 files at r1, 1 of 1 files at r2.
Reviewable status: 0 of 2 LGTMs obtained


docs/diagnostics.md, line 6 at r2 (raw file):

[virtletctl diag](virtletctl/virtletctl_diag.md) commands that can
help with troubleshooting. The diagnostics can be invoked either
directly or buy means of a

s/buy/by/


pkg/diag/diag.go, line 109 at r2 (raw file):

	switch {
	case dr.Name == "":
		return errors.New("Result name is not set")

error message should started from lower case character.


pkg/diag/diag.go, line 114 at r2 (raw file):

		return nil
	case !dr.IsDir && len(dr.Children) != 0:
		return errors.New("Result can't contain both Data and Children")

same as above

Copy link
Contributor Author

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

As of the timestamps, I re-generated docs several times in different commits. I'm not sure filtering each commit or adding another one on top of them to revert the changes is worth it ;(

Reviewable status: 0 of 2 LGTMs obtained


docs/diagnostics.md, line 6 at r2 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

s/buy/by/

Done.


pkg/diag/diag.go, line 109 at r2 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

error message should started from lower case character.

This is a message that reports a malformed data structure. The structure is called Result (starting from capital letter). This error shouldn't happen under normal circumstances, so it's kind of "internal"


pkg/diag/diag.go, line 114 at r2 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

same as above

same, capitalized Result is the name of the structure

Copy link
Contributor

@jellonek jellonek left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 LGTMs obtained, and 1 stale


pkg/diag/diag.go, line 109 at r2 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

This is a message that reports a malformed data structure. The structure is called Result (starting from capital letter). This error shouldn't happen under normal circumstances, so it's kind of "internal"

Ah, right. My mistake.

Copy link
Contributor

@lukaszo lukaszo left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 of 2 LGTMs obtained, and 1 stale

@pigmej pigmej merged commit 7724a0a into master Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants