Update k8s to 53965 and add `verbose` for `crictl status` #167

miaoyq · 2017-10-26T12:38:30Z

Partly fixes #166

Update k8s to 53965
Rename info to version
Add verbose for crictl status

Maybe we could rename status to info .

/cc @Random-Liu @feiskyer

Signed-off-by: Yanqiang Miao [email protected]

Signed-off-by: Yanqiang Miao <[email protected]>

Random-Liu · 2017-10-26T16:59:30Z

cmd/crictl/status.go

@@ -43,8 +51,8 @@ var runtimeStatusCommand = cli.Command{
 }

 // Status sends a StatusRequest to the server, and parses the returned StatusResponse.
-func Status(client pb.RuntimeServiceClient) error {
-	request := &pb.StatusRequest{}
+func Status(client pb.RuntimeServiceClient, verbose bool) error {


I'm not sure about this.

I think the extra information should be part of crictl info which is similar with docker info.

Questions:

If we already have crictl info, do we still need a separate crictl status. I think we don't.

How do we display crictl info? Do we need to organize the information a little bit or just print json output? I feel like json is good enough for now, we could keep the yaml option.

Whether we should add the verbose flag? I feel like we should always retrieve verbose information, and display it.

How do we display crictl info? Do we need to organize the information a little bit or just print json output? I feel like json is good enough for now, we could keep the yaml option.

In map<string, string> info = 2, the value have been in json format, so if we encode the info to json format, the value will not be formatted. Is there any way to make the value formatted?

we have both version and status API in CRI, I'm ok with mapping info command to status API while version command to version API.

if we encode the info to json format, the value will not be formatted

what about convert the whole info to json first and then convert to json or yaml?

Also not work, I make a test locally:

func main() { test := make(map[string]interface{}) test["Int"] = 100 test["String"] = "string" jsonMap := make(map[string]interface{}) jsonMap["Num"] = 123 jsonMap["Str"] = "abc" jsonByt, _ := json.Marshal(&jsonMap) test["JsonStr"] = string(jsonByt) byt, _ := json.Marshal(&test) fmt.Println("test-json:\n", string(byt)) marshaledYaml, _ := yaml.Marshal(&test) fmt.Println("\ntest-yaml:\n",string(marshaledYaml)) strJson := string(byt) marshaledYaml, _ = yaml.Marshal(&strJson) fmt.Println("test-yaml-json:\n", string(marshaledYaml)) marshaledJson, _ := json.Marshal(&strJson) fmt.Println("test-json-json:\n", string(marshaledJson)) }

The results are as follows：

$ go run main.go test-json: {"Int":100,"JsonStr":"{\"Num\":123,\"Str\":\"abc\"}","String":"string"} test-yaml: Int: 100 JsonStr: '{"Num":123,"Str":"abc"}' String: string test-yaml-json: '{"Int":100,"JsonStr":"{\"Num\":123,\"Str\":\"abc\"}","String":"string"}' test-json-json: "{\"Int\":100,\"JsonStr\":\"{\\\"Num\\\":123,\\\"Str\\\":\\\"abc\\\"}\",\"String\":\"string\"}"

Random-Liu · 2017-10-26T17:14:42Z

cmd/crictl/version.go

@@ -30,7 +30,7 @@ const (
 )

 var runtimeVersionCommand = cli.Command{
-	Name:  "info",
+	Name:  "version",


I'm fine with this change.

miaoyq · 2017-10-26T23:57:01Z

If we already have crictl info, do we still need a separate crictl status. I think we don't.

Agree with you. I have rename crictl info to crictl version, so we can add a new crictl info, make status as a part of info.

miaoyq · 2017-10-31T14:47:52Z

If we already have crictl info, do we still need a separate crictl status. I think we don't.

How do we display crictl info? Do we need to organize the information a little bit or just print json output? I feel like json is good enough for now, we could keep the yaml option.

Whether we should add the verbose flag? I feel like we should always retrieve verbose information, and display it.

@Random-Liu @feiskyer I have addressed above comments, PTAL

crictl info print:

# crictl info
{
  "Status": {
    "conditions": [
      {
        "type": "RuntimeReady",
        "status": true
      },
      {
        "type": "NetworkReady",
        "status": true
      }
    ]
  },
  "ExtrInfo": null
}

Random-Liu · 2017-11-01T00:14:11Z

cmd/crictl/version.go

 		if err != nil {
-			return fmt.Errorf("Getting status of runtime failed: %v", err)
+			return fmt.Errorf("Getting the runtime version failed: %v", err)


s/Getting/getting.

It seems that the cri-tools repo doesn't follow convention very well. Let's try to change it gradually.

Please address this.

Random-Liu · 2017-11-01T00:17:46Z

cmd/crictl/info.go

-	Usage: "Display runtime version information",
+var runtimeStatusCommand = cli.Command{
+	Name:      "info",
+	Usage:     "Display status and extra information of the container runtime",


Display information of the container runtime.

nit: change usage according to Random's comments.

Sorry for missing this.

Random-Liu · 2017-11-01T00:18:39Z

cmd/crictl/info.go

 		if err != nil {
-			return fmt.Errorf("Getting the runtime version failed: %v", err)
+			return fmt.Errorf("Getting status of runtime failed: %v", err)


s/Getting/getting

Random-Liu · 2017-11-01T00:20:15Z

cmd/crictl/info.go

+
+	statusInfo := struct {
+		Status   *pb.RuntimeStatus
+		ExtrInfo map[string]interface{}


I prefer combine the extra information into the result directly.

Random-Liu · 2017-11-01T01:08:02Z

cmd/crictl/info.go

+		if err != nil {
+			return err
+		}
+		statusInfo.ExtrInfo[key] = buf


I guess in this way buf will be escaped, which is not what we want.
@miaoyq Could you verify?

@Random-Liu I have verified， it worked.

This is my validation codes:

package main import "fmt" import "encoding/json" func main() { test := make(map[string]interface{}) test["Int"] = 100 test["String"] = "string" jsonMap := make(map[string]interface{}) jsonMap["Num"] = 123 jsonMap["Str"] = "abc" jsonByt, _ := json.Marshal(&jsonMap) test["JsonStr"] = string(jsonByt) var jsonMap1 map[string]interface{} if err := json.Unmarshal(jsonByt, &jsonMap1); err == nil { fmt.Println(jsonMap1) } test["JsonStr"] = jsonMap1 byt, _ := json.Marshal(&test) fmt.Println("test-json-New:\n", string(byt)) var testMap map[string]interface{} if err := json.Unmarshal(byt, &testMap); err == nil { fmt.Printf("%#v \n",testMap) } }

output:

# go run main.go map[Num:123 Str:abc] test-json-New: {"Int":100,"JsonStr":{"Num":123,"Str":"abc"},"String":"string"} map[string]interface {}{"Int":100, "JsonStr":map[string]interface {}{"Num":123, "Str":"abc"}, "String":"string"}

But seems not strict, I will use your opinion. :)

The value in map itself could be a json.

Random-Liu · 2017-11-01T01:08:58Z

@miaoyq @feiskyer Actually we just need to do something like this:

package main

import (
        "bytes"
        "encoding/json"
        "os"
)

type Status struct {
        Name  string
        Value string
}

func main() {
        status := Status{
                Name:  "name",
                Value: "value",
        }
        info := map[string]string{
                "Pid": "1234",
                "Config": `{
    "a": "b",
    "c": [
        "d",
        "e"
    ]
}`,
        }
        statusB, err := json.Marshal(&status)
        if err != nil {
                panic(err)
        }
        value := "{" + "\"Status\":" + string(statusB) + ","                                                                                       
        for k, v := range info {
                value += "\"" + k + "\"" + ":" + v + "," 
        }
        value = value[:len(value)-1]
        value += "}" 
        var out bytes.Buffer
        if err := json.Indent(&out, []byte(value), "", "  "); err != nil {
                panic(err)
        }
        out.WriteTo(os.Stdout)
}

$ ./test 
{
  "Status": {
    "Name": "name",
    "Value": "value"
  },
  "Pid": 1234,
  "Config": {
    "a": "b",
    "c": [
      "d",
      "e"
    ]
  }
}

The code is ugly, we could clean it up a little bit, but this works. :)

Random-Liu · 2017-11-01T01:10:26Z

And we may need to decide whether field should start with capitalized letter or not.
We should make it consistent.

feiskyer · 2017-11-01T01:59:57Z

Actually we just need to do something like this:

Cool. @miaoyq could you format json in this way?

And we may need to decide whether field should start with capitalized letter or not.

Shouldn't we keep consistent with what runtimes returns?

miaoyq · 2017-11-01T02:06:27Z

@feiskyer OK, I'm working on this.

miaoyq · 2017-11-01T02:39:01Z

@feiskyer @Random-Liu Updated.

feiskyer · 2017-11-01T03:12:03Z

@miaoyq Could you also format output when user chooses yaml format?

@Random-Liu Does containerd provide those extra info now? need to verity the output.

miaoyq · 2017-11-01T03:13:22Z

@feiskyer OK.

miaoyq · 2017-11-01T05:00:32Z

@feiskyer Updated.

feiskyer

The change LGTM.

@Random-Liu Does containerd provide those extra info now? need to verity the output.

miaoyq · 2017-11-01T07:03:30Z

@feiskyer Thanks for reminder, Done.

Random-Liu · 2017-11-02T04:26:23Z

Does containerd provide those extra info now? need to verity the output.

Not yet. We are planning to add it this release. @miaoyq Could you add some basic information for info in containerd? For the first step, maybe we could just return current cri-containerd config in json format.

Random-Liu · 2017-11-02T04:28:05Z

cmd/crictl/info.go

+		return err
+	}
+
+	statusByte, err := json.Marshal(r.Status)


We can make this a helper function, because we'll need it for container/sandbox/image status.
I'm fine with doing that in another PR.

Will do late. :)

Random-Liu · 2017-11-02T04:28:33Z

cmd/crictl/version.go

 		if err != nil {
-			return fmt.Errorf("Getting status of runtime failed: %v", err)
+			return fmt.Errorf("Getting the runtime version failed: %v", err)


Please address this.

Random-Liu · 2017-11-02T04:28:57Z

LGTM other than one comment.

miaoyq · 2017-11-02T05:15:58Z

@Random-Liu Addressed.

…l status` Signed-off-by: Yanqiang Miao <[email protected]>

miaoyq · 2017-11-02T05:56:45Z

@miaoyq Could you add some basic information for info in containerd? For the first step, maybe we could just return current cri-containerd config in json format.

@Random-Liu Do you means adding some basic information for info in crictl?

Random-Liu · 2017-11-02T06:24:16Z

@miaoyq I mean return those information from Status function of cri-containerd.

Then crictl will print it out automatically with your PR, right? That's why we add the extra information field.

Random-Liu · 2017-11-02T06:25:05Z

/lgtm

miaoyq · 2017-11-02T06:49:59Z

@Random-Liu Ok, got it, thanks.

Update the CRI API

dfc2e77

Signed-off-by: Yanqiang Miao <[email protected]>

k8s-ci-robot requested review from feiskyer and Random-Liu October 26, 2017 12:38

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 26, 2017

Random-Liu reviewed Oct 26, 2017

View reviewed changes

miaoyq force-pushed the update-k8s-to-53965 branch from 841ddb6 to 93c8113 Compare October 31, 2017 14:40

Random-Liu reviewed Nov 1, 2017

View reviewed changes

miaoyq force-pushed the update-k8s-to-53965 branch from 93c8113 to 75791e8 Compare November 1, 2017 02:38

miaoyq force-pushed the update-k8s-to-53965 branch from 75791e8 to 4131c65 Compare November 1, 2017 04:59

feiskyer reviewed Nov 1, 2017

View reviewed changes

miaoyq force-pushed the update-k8s-to-53965 branch from 4131c65 to 7403734 Compare November 1, 2017 07:02

Random-Liu reviewed Nov 2, 2017

View reviewed changes

miaoyq force-pushed the update-k8s-to-53965 branch from 7403734 to 2512e54 Compare November 2, 2017 05:15

Rename 'crictl info' to 'crictl version' and add verbose for `crict…

2512e54

…l status` Signed-off-by: Yanqiang Miao <[email protected]>

k8s-ci-robot assigned Random-Liu Nov 2, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2017

Random-Liu merged commit 8681fe8 into kubernetes-sigs:master Nov 2, 2017

miaoyq mentioned this pull request Nov 2, 2017

Extract the output codes of status as a util function #175

Merged

spiffxp unassigned Random-Liu Sep 6, 2018

Update k8s to 53965 and add verbose for crictl status #167

Update k8s to 53965 and add verbose for crictl status #167

Conversation

miaoyq commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miaoyq commented Oct 26, 2017

miaoyq commented Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Nov 1, 2017 • edited Loading

Random-Liu commented Nov 1, 2017

feiskyer commented Nov 1, 2017

miaoyq commented Nov 1, 2017

miaoyq commented Nov 1, 2017

feiskyer commented Nov 1, 2017

miaoyq commented Nov 1, 2017

miaoyq commented Nov 1, 2017

feiskyer left a comment

Choose a reason for hiding this comment

miaoyq commented Nov 1, 2017

Random-Liu commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Nov 2, 2017

miaoyq commented Nov 2, 2017

miaoyq commented Nov 2, 2017

Random-Liu commented Nov 2, 2017 • edited Loading

Random-Liu commented Nov 2, 2017

miaoyq commented Nov 2, 2017

Update k8s to 53965 and add `verbose` for `crictl status` #167

Update k8s to 53965 and add `verbose` for `crictl status` #167

feiskyer Oct 27, 2017 •

edited

Loading

miaoyq commented Oct 31, 2017 •

edited

Loading

Random-Liu Nov 1, 2017 •

edited

Loading

Random-Liu commented Nov 1, 2017 •

edited

Loading

Random-Liu commented Nov 2, 2017 •

edited

Loading