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

fix: allow to stats not-running container #2802

Merged
merged 1 commit into from
Apr 23, 2019

Conversation

zhuangqh
Copy link
Contributor

Ⅰ. Describe what this PR did

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

Both docker api or CRI allows to get stats from not-running container.
For docker api, just return empty statistics result expect the systemCPUUsage.
For CRI, return empty statistics result expect the container meta data.

Ⅱ. Does this pull request fix one issue?

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

TO BE DESIGN

Ⅳ. Describe how to verify it

crictl stats would show the statistics from containers which just running by default. -a options would list those who are not in running state.

[root@pouch ~]# pouch ps -a
Name                                                            ID       Status       Created       Image                                                                 Runtime
k8s_ubuntu_nnginx-sandbox_default_hdishd83djaidwnduwk28bcsb_0   09bece   created      9 hours ago   registry.hub.docker.com/library/busybox:latest                        runc
k8s_POD_nnginx-sandbox_default_hdishd83djaidwnduwk28bcsb_1      8505cc   Up 9 hours   9 hours ago   registry.cn-hangzhou.aliyuncs.com/google-containers/pause-amd64:3.0   runc
k8s_ubuntu_nginx-sandbox_default_hdishd83djaidwnduwk28bcsb_0    9d6a9c   Up 9 hours   9 hours ago   registry.hub.docker.com/library/busybox:latest                        runc
k8s_POD_nginx-sandbox_default_hdishd83djaidwnduwk28bcsb_1       ca9ce8   Up 9 hours   9 hours ago   registry.cn-hangzhou.aliyuncs.com/google-containers/pause-amd64:3.0   runc
0556fc                                                          0556fc   Up 9 hours   9 hours ago   
[root@pouch ~]# crictl stats -a
CONTAINER           CPU %               MEM                 DISK                INODES
09becec027cbe       0.00                0B                  0B                  0
9d6a9c6b107fa       0.00                57.34kB             0B                  0
[root@pouch ~]# crictl stats
CONTAINER           CPU %               MEM                 DISK                INODES
9d6a9c6b107fa       0.00                57.34kB             0B                  0

Get stats from a not-running container through docker style api is allowed

[root@pouch ~]# pouch ps -a
Name                                                            ID       Status                 Created         Image                                                                 Runtime
3bec99    3bec99   Exited (0) 3 seconds   3 seconds ago   registry.hub.docker.com/library/busybox:latest                        runc

[root@pouch ~]# pouch stats --no-stream 3bec99
CONTAINER ID   NAME     CPU %   MEM %   MEM USAGE / LIMIT   NET I/O   BLOCK I/O   PIDS
3bec99c4a993   3bec99   0.00%   0.00%   0B / 0B             0B / 0B   0B / 0B     0

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Apr 14, 2019

Codecov Report

Merging #2802 into master will decrease coverage by <.01%.
The diff coverage is 37.93%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2802      +/-   ##
=========================================
- Coverage    69.4%   69.4%   -0.01%     
=========================================
  Files         278     278              
  Lines       17373   17373              
=========================================
- Hits        12058   12057       -1     
+ Misses       3971    3970       -1     
- Partials     1344    1346       +2
Flag Coverage Δ
#criv1alpha2_test 39.15% <13.79%> (-0.14%) ⬇️
#integration_test_0 36.48% <0%> (-0.08%) ⬇️
#integration_test_1 35.34% <0%> (+0.03%) ⬆️
#integration_test_2 36.5% <20.68%> (+0.04%) ⬆️
#integration_test_3 35.37% <20.68%> (+0.03%) ⬆️
#node_e2e_test 34.98% <17.24%> (-0.08%) ⬇️
#unittest 28.72% <0%> (ø) ⬆️
Impacted Files Coverage Δ
cri/v1alpha2/cri_utils.go 88.84% <100%> (+0.2%) ⬆️
daemon/mgr/container_stats.go 67.79% <28%> (-2.73%) ⬇️
ctrd/watch.go 71.83% <0%> (-5.64%) ⬇️
daemon/mgr/container.go 59.82% <0%> (-0.64%) ⬇️
ctrd/container.go 53.91% <0%> (-0.39%) ⬇️
cri/v1alpha2/cri.go 71.48% <0%> (+0.51%) ⬆️
daemon/mgr/container_utils.go 82.38% <0%> (+0.56%) ⬆️
cri/stream/runtime.go 70.23% <0%> (+2.38%) ⬆️
cri/stream/portforward/httpstream.go 77.77% <0%> (+6.83%) ⬆️

@pouchrobot pouchrobot added kind/bug This is bug report for project size/L labels Apr 14, 2019
just return a empty stats data for them

Signed-off-by: zhuangqh <zhuangqhc@gmail.com>
@zhuangqh zhuangqh force-pushed the fix-cri-stats-warn branch from af46a14 to a65bb35 Compare April 14, 2019 12:23
@zhuangqh
Copy link
Contributor Author

PTAL thanks 😀 @ZYecho

@ZYecho
Copy link
Contributor

ZYecho commented Apr 14, 2019

LGTM.

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants