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.

Sign up for GitHub

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

adding verbose debug output for inspect & inspects #206

Merged
merged 3 commits into from
Dec 4, 2017
Merged

adding verbose debug output for inspect & inspects #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
faa6682
adding verbose output switch for inspect
mikebrow Nov 29, 2017
0a7a812
added inspects options for verbose debug out
mikebrow Dec 1, 2017
0e48c9e
adds inspecti options for verbose debug out
mikebrow Dec 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions cmd/crictl/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ var containerStatusCommand = cli.Command{
Name: "output, o",
Usage: "Output format, One of: json|yaml|table",
Copy link
Contributor

@Random-Liu Random-Liu Nov 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@feiskyer @mrunalp @runcom @mikebrow Do you still think we should have the table format?
I think for inspect, we may just need json|yaml. Just like crictl info. See an example here
The problem is that timestamp in CRI is unix timestamp, we may want to do a conversion in crictl.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of table, let's drop it in favor of machine readable output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a big fan of table formats either, but as @Random-Liu says the unix timestamp is not human readable output:

crictl --runtime-endpoint /var/run/cri-containerd.sock inspect -v -o json 44025055b48da66cb67bfc221f800fe50f1256d3e22f1cf714294344b29d582e 
{
  "status": {
    "id": "44025055b48da66cb67bfc221f800fe50f1256d3e22f1cf714294344b29d582e",
    "metadata": {
      "name": "busybox",
      "attempt": 0
    },
    "state": "CONTAINER_CREATED",
    "createdAt": "1511994785429097447",
    "startedAt": "0",
    "finishedAt": "0",
    "exitCode": 0,
    "image": {
      "image": "docker.io/library/busybox:latest"
    },
    "imageRef": "docker.io/library/busybox@sha256:bbc3a03235220b170ba48a157dd097dd1379299370e1ed99ce976df0355d24f0",
    "reason": "",
    "message": "",
    "logPath": ""
  },

vs:

crictl --runtime-endpoint /var/run/cri-containerd.sock inspect -v 44025055b48da66cb67bfc221f800fe50f1256d3e22f1cf714294344b29d582e 
ID: 44025055b48da66cb67bfc221f800fe50f1256d3e22f1cf714294344b29d582e
Name: busybox
State: CONTAINER_CREATED
Created: 28 minutes ago

Copy link
Contributor

Choose a reason for hiding this comment

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

We can do some conversion there, not a big deal

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we should do some conversion. :)

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, table is not useful for inspect.

Copy link
Contributor Author

@mikebrow mikebrow Dec 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

so.. @Random-Liu @runcom @feiskyer wrt. "some conversion"
I suggest converting the timestamps to RFC3339Nano
"2006-01-02T15:04:05.999999999Z07:00"
Thoughts?

},
cli.BoolFlag{
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for inspect, we should always set verbose=true in crictl.
verbose=false is for kubelet using CRI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Random-Liu so you are saying you want a --quiet option for less verbose? or take out the option all-together? There were already other verbose flags so I figured that's what we would want with this verbose option.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like by default we should enable verbose, or else it's really troublesome to do -v every time. :)

As for whether we need a quiet option, I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Random-Liu I hear ya.. think it over no rush, easy enough to switch it to verbose by default, or quiet mode option. But I think when you see the verbose mode from the CRI-O team you'll be begging to turn down the volume :-) @runcom

Copy link
Contributor

Choose a reason for hiding this comment

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

? Not following, what about CRI-O? We aren't even using that verbose stuff and we have no plan to output anything.
In any case, I'm in favor of going verbose by default like Lantao

Copy link
Member

Choose a reason for hiding this comment

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

+1 of enabling verbose.

Copy link
Contributor Author

@mikebrow mikebrow Nov 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I was just making light of the amount of metadata CRI-O keeps around that could possibly used in a verbose output of status about the container, thus the smiley face :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but that's exactly the verbose intention, despite what runtimes want to output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I vote inspect to be verbose by default for crictl (and provide a silent flag to turn verbose off).
I vote to make the json format the default (and I don't care if you want to kill the table format or keep it as an option).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done..

Name: "quiet, q",
Usage: "Do not show verbose information",
},
},
Action: func(context *cli.Context) error {
containerID := context.Args().First()
Expand All @@ -232,7 +236,7 @@ var containerStatusCommand = cli.Command{
return err
}

err := ContainerStatus(runtimeClient, containerID, context.String("output"))
err := ContainerStatus(runtimeClient, containerID, context.String("output"), context.Bool("quiet"))
if err != nil {
return fmt.Errorf("Getting the status of the container failed: %v", err)
}
Expand Down Expand Up @@ -462,12 +466,17 @@ func RemoveContainer(client pb.RuntimeServiceClient, ID string) error {

// ContainerStatus sends a ContainerStatusRequest to the server, and parses
// the returned ContainerStatusResponse.
func ContainerStatus(client pb.RuntimeServiceClient, ID, output string) error {
func ContainerStatus(client pb.RuntimeServiceClient, ID, output string, quiet bool) error {
verbose := !(quiet)
if output == "" { // default to json output
output = "json"
}
if ID == "" {
return fmt.Errorf("ID cannot be empty")
}
request := &pb.ContainerStatusRequest{
ContainerId: ID,
Verbose: verbose,
}
logrus.Debugf("ContainerStatusRequest: %v", request)
r, err := client.ContainerStatus(context.Background(), request)
Expand All @@ -476,11 +485,15 @@ func ContainerStatus(client pb.RuntimeServiceClient, ID, output string) error {
return err
}

if output == "json" || output == "yaml" {
switch output {
case "json", "yaml":
return outputStatusInfo(r.Status, r.Info, output)
case "table": // table output is after this switch block
default:
return fmt.Errorf("output option cannot be %s", output)
}

// output in table format by default.
// output in table format
fmt.Printf("ID: %s\n", r.Status.Id)
if r.Status.Metadata != nil {
if r.Status.Metadata.Name != "" {
Expand All @@ -504,6 +517,9 @@ func ContainerStatus(client pb.RuntimeServiceClient, ID, output string) error {
}
fmt.Printf("Exit Code: %v\n", r.Status.ExitCode)
}
if verbose {
fmt.Printf("Info: %v\n", r.GetInfo())
}

return nil
}
Expand Down
32 changes: 26 additions & 6 deletions cmd/crictl/image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,10 @@ var imageStatusCommand = cli.Command{
Name: "output, o",
Usage: "Output format, One of: json|yaml|table",
},
cli.BoolFlag{
Name: "quiet, q",
Usage: "Do not show verbose information",
},
},
Action: func(context *cli.Context) error {
id := context.Args().First()
Expand All @@ -198,7 +202,8 @@ var imageStatusCommand = cli.Command{
return err
}

r, err := ImageStatus(imageClient, id)
verbose := !(context.Bool("quiet"))
r, err := ImageStatus(imageClient, id, verbose)
if err != nil {
return fmt.Errorf("image status request failed: %v", err)
}
Expand All @@ -208,11 +213,19 @@ var imageStatusCommand = cli.Command{
}

output := context.String("output")
if output == "json" || output == "yaml" {
if output == "" { // default to json output
output = "json"
}

switch output {
case "json", "yaml":
return outputStatusInfo(r.Image, r.Info, output)
case "table": // table output is after this switch block
default:
return fmt.Errorf("output option cannot be %s", output)
}

// output in table format by default.
// otherwise output in table format
fmt.Printf("ID: %s\n", image.Id)
for _, tag := range image.RepoTags {
fmt.Printf("Tag: %s\n", tag)
Expand All @@ -222,6 +235,9 @@ var imageStatusCommand = cli.Command{
}
size := units.HumanSizeWithPrecision(float64(image.GetSize_()), 3)
fmt.Printf("Size: %s\n", size)
if verbose {
fmt.Printf("Info: %v\n", r.GetInfo())
}

return nil
},
Expand All @@ -241,7 +257,8 @@ var removeImageCommand = cli.Command{
return err
}

status, err := ImageStatus(imageClient, id)
var verbose = false
status, err := ImageStatus(imageClient, id, verbose)
if err != nil {
return fmt.Errorf("image status request failed: %v", err)
}
Expand Down Expand Up @@ -342,8 +359,11 @@ func ListImages(client pb.ImageServiceClient, image string) (resp *pb.ListImages

// ImageStatus sends an ImageStatusRequest to the server, and parses
// the returned ImageStatusResponse.
func ImageStatus(client pb.ImageServiceClient, image string) (resp *pb.ImageStatusResponse, err error) {
request := &pb.ImageStatusRequest{Image: &pb.ImageSpec{Image: image}}
func ImageStatus(client pb.ImageServiceClient, image string, verbose bool) (resp *pb.ImageStatusResponse, err error) {
request := &pb.ImageStatusRequest{
Image: &pb.ImageSpec{Image: image},
Verbose: verbose,
}
logrus.Debugf("ImageStatusRequest: %v", request)
resp, err = client.ImageStatus(context.Background(), request)
logrus.Debugf("ImageStatusResponse: %v", resp)
Expand Down
30 changes: 25 additions & 5 deletions cmd/crictl/sandbox.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,10 @@ var podSandboxStatusCommand = cli.Command{
Name: "output, o",
Usage: "Output format, One of: json|yaml|table",
},
cli.BoolFlag{
Name: "quiet, q",
Usage: "Do not show verbose information",
},
},
Action: func(context *cli.Context) error {
id := context.Args().First()
Expand All @@ -139,7 +143,7 @@ var podSandboxStatusCommand = cli.Command{
return err
}

err := PodSandboxStatus(runtimeClient, id, context.String("output"))
err := PodSandboxStatus(runtimeClient, id, context.String("output"), context.Bool("quiet"))
if err != nil {
return fmt.Errorf("getting the pod sandbox status failed: %v", err)
}
Expand Down Expand Up @@ -275,20 +279,32 @@ func RemovePodSandbox(client pb.RuntimeServiceClient, ID string) error {

// PodSandboxStatus sends a PodSandboxStatusRequest to the server, and parses
// the returned PodSandboxStatusResponse.
func PodSandboxStatus(client pb.RuntimeServiceClient, ID, output string) error {
func PodSandboxStatus(client pb.RuntimeServiceClient, ID, output string, quiet bool) error {
verbose := !(quiet)
if output == "" { // default to json output
output = "json"
}
if ID == "" {
return fmt.Errorf("ID cannot be empty")
}
request := &pb.PodSandboxStatusRequest{PodSandboxId: ID}

request := &pb.PodSandboxStatusRequest{
PodSandboxId: ID,
Verbose: verbose,
}
logrus.Debugf("PodSandboxStatusRequest: %v", request)
r, err := client.PodSandboxStatus(context.Background(), &pb.PodSandboxStatusRequest{PodSandboxId: ID})
r, err := client.PodSandboxStatus(context.Background(), request)
logrus.Debugf("PodSandboxStatusResponse: %v", r)
if err != nil {
return err
}

if output == "json" || output == "yaml" {
switch output {
case "json", "yaml":
return outputStatusInfo(r.Status, r.Info, output)
case "table": // table output is after this switch block
default:
return fmt.Errorf("output option cannot be %s", output)
}

// output in table format by default.
Expand Down Expand Up @@ -324,6 +340,10 @@ func PodSandboxStatus(client pb.RuntimeServiceClient, ID, output string) error {
fmt.Printf("\t%s -> %s\n", k, r.Status.Annotations[k])
}
}
if verbose {
fmt.Printf("Info: %v\n", r.GetInfo())
}

return nil
}

Expand Down