refactor: remove cri wrapper #2613

zhuangqh · 2018-12-25T11:16:20Z

Signed-off-by: zhuangqh zhuangqhc@gmail.com

Ⅰ. Describe what this PR did

using grpc interceptor to hook each grpc connection logging.

Ⅱ. Does this pull request fix one issue?

related to #2633

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

grpc logging like

$ crictl runp pod.json
INFO[2018-12-29T06:27:33.784753333Z] grpc start                                    grpc.method=RunPodSandbox grpc.request.content=&RunPodSandboxRequest{Config:&PodSandboxConfig{Metadata:&PodSandboxMetadata{Name:nginx-sandbox,Uid:hdishd83djaidwnduwk28bnnb,Namespace:default,Attempt:1,},Hostname:,LogDirectory:,DnsConfig:nil,PortMappings:[],Labels:map[string]string{},Annotations:map[string]string{},Linux:nil,},RuntimeHandler:,} grpc.service=runtime.v1alpha2.RuntimeService grpc.start_time="2018-12-29T06:27:33Z"
INFO[2018-12-29T06:27:33.78831053Z] invoke container pre-create hook in plugin
INFO[2018-12-29T06:27:33.799896345Z] write mtab file to (/var/lib/pouch/containers/e2141b5bc3561bd6f43b3acafb047b44c2dddabaecd1ce2fad164e4adab28062/rootfs/etc/mtab)
WARN[2018-12-29T06:27:33.812157386Z] get passwd file or group file is nil
INFO[2018-12-29T06:27:33.812893641Z] success to get image registry.cn-hangzhou.aliyuncs.com/google-containers/pause-amd64:3.0, container id e2141b5bc3561bd6f43b3acafb047b44c2dddabaecd1ce2fad164e4adab28062
INFO[2018-12-29T06:27:33.819013058Z] success to new container: e2141b5bc3561bd6f43b3acafb047b44c2dddabaecd1ce2fad164e4adab28062
INFO[2018-12-29T06:27:33.821998625Z] shim containerd-shim started                  address="/containerd-shim/default/e2141b5bc3561bd6f43b3acafb047b44c2dddabaecd1ce2fad164e4adab28062/shim.sock" debug=false pid=8279
INFO[2018-12-29T06:27:33.986153234Z] success to create task(pid=8295) in container(e2141b5bc3561bd6f43b3acafb047b44c2dddabaecd1ce2fad164e4adab28062)
INFO[2018-12-29T06:27:34.017795534Z] success to start task in container(e2141b5bc3561bd6f43b3acafb047b44c2dddabaecd1ce2fad164e4adab28062)
INFO[2018-12-29T06:27:34.017824179Z] success to add container, id: e2141b5bc3561bd6f43b3acafb047b44c2dddabaecd1ce2fad164e4adab28062
INFO[2018-12-29T06:27:34.02161777Z] Got pod network &{Name:nginx-sandbox Namespace:default ID:e2141b5bc3561bd6f43b3acafb047b44c2dddabaecd1ce2fad164e4adab28062 NetNS:/proc/8295/ns/net PortMappings:[] Networks:[]}
INFO[2018-12-29T06:27:34.02164234Z] About to add CNI network cni-loopback (type=loopback)
INFO[2018-12-29T06:27:34.024564394Z] Got pod network &{Name:nginx-sandbox Namespace:default ID:e2141b5bc3561bd6f43b3acafb047b44c2dddabaecd1ce2fad164e4adab28062 NetNS:/proc/8295/ns/net PortMappings:[] Networks:[]}
INFO[2018-12-29T06:27:34.024578428Z] About to add CNI network mynet (type=bridge)
INFO[2018-12-29T06:27:34.06731512Z] grpc end                                      grpc.method=RunPodSandbox grpc.response.content=&RunPodSandboxResponse{PodSandboxId:e2141b5bc3561bd6f43b3acafb047b44c2dddabaecd1ce2fad164e4adab28062,} grpc.service=runtime.v1alpha2.RuntimeService grpc.start_time="2018-12-29T06:27:33Z"

fails

INFO[2018-12-29T06:29:20.786531364Z] grpc start                                    grpc.method=StartContainer grpc.request.content=&StartContainerRequest{ContainerId:kk,} grpc.service=runtime.v1alpha2.RuntimeService grpc.start_time="2018-12-29T06:29:20Z"
ERRO[2018-12-29T06:29:20.786615725Z] grpc failed failed to start container "kk": container kk: not found  grpc.method=StartContainer grpc.response.content=nil grpc.service=runtime.v1alpha2.RuntimeService grpc.start_time="2018-12-29T06:29:20Z"

for read only interface, such as crictl pods. Just print debug log.

DEBU[2018-12-29T06:26:21.95956152Z] Get a grpc client                             elapsed=4.363µs
DEBU[2018-12-29T06:26:21.960201681Z] Get a grpc client                             elapsed=957ns
DEBU[2018-12-29T06:26:21.96054135Z] Get a grpc client                             elapsed=804ns
DEBU[2018-12-29T06:26:21.960884243Z] Get a grpc client                             elapsed=916ns
DEBU[2018-12-29T06:26:21.96122824Z] Get a grpc client                             elapsed=864ns
DEBU[2018-12-29T06:26:21.961491604Z] Get a grpc client                             elapsed=880ns
DEBU[2018-12-29T06:26:24.979544613Z] grpc start                                    grpc.method=ListPodSandbox grpc.request.content=&ListPodSandboxRequest{Filter:&PodSandboxFilter{Id:,State:nil,LabelSelector:map[string]string{},},} grpc.service=runtime.v1alpha2.RuntimeService grpc.start_time="2018-12-29T06:26:24Z"
DEBU[2018-12-29T06:26:24.979874962Z] grpc end                                      grpc.method=ListPodSandbox grpc.response.content=&ListPodSandboxResponse{Items:[&PodSandbox{Id:1c9f76075695fcbde3d42accbb0d18deb0d8a0d0a96d2b0ffe4a52c6999370bc,Metadata:&PodSandboxMetadata{Name:nginx-sandbox,Uid:hdishd83djaidwnduwk28bnnb,Namespace:default,Attempt:1,},State:SANDBOX_READY,CreatedAt:1546064756398390025,Labels:map[string]string{},Annotations:map[string]string{},}],} grpc.service=runtime.v1alpha2.RuntimeService grpc.start_time="2018-12-29T06:26:24Z"
DEBU[2018-12-29T06:26:31.959004098Z] Get a grpc client                             elapsed=4.631µs
DEBU[2018-12-29T06:26:31.959648315Z] Get a grpc client                             elapsed=902ns
DEBU[2018-12-29T06:26:31.960007343Z] Get a grpc client                             elapsed=880ns
DEBU[2018-12-29T06:26:31.960351277Z] Get a grpc client                             elapsed=1.188µs
DEBU[2018-12-29T06:26:31.960710469Z] Get a grpc client                             elapsed=812ns
DEBU[2018-12-29T06:26:31.960971361Z] Get a grpc client                             elapsed=844ns

Ⅴ. Special notes for reviews

codecov · 2018-12-25T11:16:22Z

Codecov Report

❗ No coverage uploaded for pull request base (master@22bfd53). Click here to learn what that means.
The diff coverage is 75%.

@@            Coverage Diff            @@
##             master    #2613   +/-   ##
=========================================
  Coverage          ?   69.54%           
=========================================
  Files             ?      278           
  Lines             ?    18642           
  Branches          ?        0           
=========================================
  Hits              ?    12965           
  Misses            ?     4238           
  Partials          ?     1439

Flag	Coverage Δ
#criv1alpha1test	`31.57% <0%> (?)`
#criv1alpha2test	`35.52% <61.36%> (?)`
#integrationtest	`42.14% <0%> (?)`
#nodee2etest	`32.52% <61.36%> (?)`
#unittest	`27.27% <19.14%> (?)`

Impacted Files	Coverage Δ
cri/v1alpha2/types/sandbox_meta.go	`100% <ø> (ø)`
cri/v1alpha1/cri.go	`59.43% <0%> (ø)`
cri/criservice.go	`56.62% <100%> (ø)`
cri/v1alpha2/stream_server.go	`79.2% <100%> (ø)`
cri/v1alpha2/cri.go	`67.53% <66.66%> (ø)`
cri/v1alpha2/cri_utils.go	`88.44% <68%> (ø)`
pkg/utils/utils.go	`83.26% <69.23%> (ø)`
pkg/grpc/interceptor/logging.go	`72.97% <72.97%> (ø)`
cri/v1alpha2/service.go	`93.1% <93.1%> (ø)`

starnop · 2018-12-28T07:07:07Z

Code refactoring
GRPC logging interceptor

Maybe we should split it in a similar situation next time.
Whatever, LGTM. 😄

- simplify NewStreamServer - move common util to pkg/utils Signed-off-by: zhuangqh <zhuangqhc@gmail.com>

zhuangqh · 2018-12-29T07:29:51Z

Code refactoring

GRPC logging interceptor

Maybe we should split it in a similar situation next time.
Whatever, LGTM. 😄

Has split them into two parts. remove the time cost log for every interface. @starnop

starnop · 2019-01-02T13:04:54Z

Got it, and LGTM again. 😄

fuweid

Just a suggestion for updating vendor: maybe we can provide the govendor command in git commit so that we can see how to update vendor.

fuweid · 2019-01-06T12:04:59Z

vendor/vendor.json

 		},
 		{
-			"checksumSHA1": "Z4RIWIXH05QItZqVbmbONO9mWig=",
+			"checksumSHA1": "G0aiY+KmzFsQLTNzRAGRhJNSj7A=",
 			"path": "github.com/golang/protobuf/ptypes/any",


containerd current master uses github.com/golang/protobuf@v1.1.0. I am not sure that it is conflicting item when we update containerd. If it is conflicting, we can implement WithUnaryServerChain by ourself.

pkg/grpc/interceptor/logging.go

cri/v1alpha2/service.go

fuweid

please repush to re-trigger travis-ci. thanks!

Signed-off-by: zhuangqh <zhuangqhc@gmail.com>

HusterWan · 2019-01-10T08:37:08Z

LGTM

fuweid

LGTM

pouchrobot added kind/refactor size/XXL labels Dec 25, 2018

zhuangqh force-pushed the refactor-cri-misc branch 8 times, most recently from 66429ab to a4ff7a0 Compare December 26, 2018 12:23

zhuangqh changed the title ~~[WIP] refactor: remove cri wrapper~~ refactor: remove cri wrapper Dec 26, 2018

zhuangqh mentioned this pull request Dec 27, 2018

feature: extend cri apis for support pause/unpause container #2623

Merged

refactor cri

4bd7a84

- simplify NewStreamServer - move common util to pkg/utils Signed-off-by: zhuangqh <zhuangqhc@gmail.com>

zhuangqh force-pushed the refactor-cri-misc branch 3 times, most recently from 29618d6 to 233e1f8 Compare December 29, 2018 07:26

zhuangqh mentioned this pull request Dec 30, 2018

feature: Add log for cri request #2636

Closed

zhuangqh added areas/log areas/cri labels Dec 30, 2018

allencloud assigned fuweid Jan 2, 2019

fuweid reviewed Jan 6, 2019

View reviewed changes

zhuangqh force-pushed the refactor-cri-misc branch from 233e1f8 to 64eb054 Compare January 8, 2019 08:27

fuweid suggested changes Jan 8, 2019

View reviewed changes

remove cri wrapper, using grpc interceptor

837f3f2

Signed-off-by: zhuangqh <zhuangqhc@gmail.com>

zhuangqh force-pushed the refactor-cri-misc branch from 64eb054 to 837f3f2 Compare January 8, 2019 13:33

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 10, 2019

fuweid approved these changes Jan 13, 2019

View reviewed changes

fuweid merged commit 172a387 into AliyunContainerService:master Jan 13, 2019

zhuangqh mentioned this pull request Jan 14, 2019

[proposal] Add log if a cri request is spent more than a certain period of time #2633

Closed

pouchrobot mentioned this pull request Jan 18, 2019

WeeklyReport of PouchContainer from 2019-01-11 to 2019-01-18 #2676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove cri wrapper #2613

refactor: remove cri wrapper #2613

zhuangqh commented Dec 25, 2018 •

edited

Loading

codecov bot commented Dec 25, 2018 •

edited

Loading

starnop commented Dec 28, 2018 •

edited

Loading

zhuangqh commented Dec 29, 2018

starnop commented Jan 2, 2019

fuweid left a comment

fuweid Jan 6, 2019

fuweid left a comment

HusterWan commented Jan 10, 2019

fuweid left a comment

refactor: remove cri wrapper #2613

refactor: remove cri wrapper #2613

Conversation

zhuangqh commented Dec 25, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Dec 25, 2018 • edited Loading

Codecov Report

starnop commented Dec 28, 2018 • edited Loading

zhuangqh commented Dec 29, 2018

starnop commented Jan 2, 2019

fuweid left a comment

Choose a reason for hiding this comment

fuweid Jan 6, 2019

Choose a reason for hiding this comment

fuweid left a comment

Choose a reason for hiding this comment

HusterWan commented Jan 10, 2019

fuweid left a comment

Choose a reason for hiding this comment

zhuangqh commented Dec 25, 2018 •

edited

Loading

codecov bot commented Dec 25, 2018 •

edited

Loading

starnop commented Dec 28, 2018 •

edited

Loading