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

feature: InspectExec method of Container manager to get the result of exec #711

Merged
merged 1 commit into from
Feb 7, 2018

Conversation

YaoZengzeng
Copy link
Contributor

Signed-off-by: YaoZengzeng [email protected]

Ⅰ. Describe what this PR did

Now the caller can't get the status (such as exitCode, exitTime, etc) of exec process which caused
great trouble for us to pass many CRI related test cases.

With this PR, the problem described above is gone.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Maybe the cri-tools is what you need.

We may use the following method to verify that we can create and start container in sandbox:

// Now we must ensure the sandbox image has already exists.
[root@pouch-test cri-tools]# pouch pull k8s.gcr.io/pause-amd64:3.0
[root@pouch-test cri-tools]# cat sandbox-config.json
{
    "metadata": {
        "name": "nginx-sandbox",
        "namespace": "default",
        "attempt": 1,
        "uid": "hdishd83djaidwnduwk28bcsb"
    },
    "linux": {
    }
}
[root@pouch-test cri-tools]# crictl runs sandbox-config.json
008cc69c9c9c8e9c4d78aa8d6c528e21ab26affdc64748f631fb5dcb59963216
[root@pouch-test cri-tools]# cat container-config.json 
{
  "metadata": {
      "name": "busybox"
  },
  "image":{
      "image": "docker.io/library/busybox:latest"
  },
  "command": [
      "top"
  ],
  "linux": {
  }
}
[root@pouch-test cri-tools]# crictl create 008cc69c9c9c8e9c4d78aa8d6c528e21ab26affdc64748f631fb5dcb59963216 container-config.json sandbox-config.json
3530bbb414ccce09ba52fbdf781df53b881661720f05d92edf8820280abf064d
[root@pouch-test cri-tools]# crictl start 3530bbb414ccce09ba52fbdf781df53b881661720f05d92edf8820280abf064d
[root@pouch-test cri-tools]# pouch ps
Name                                                            ID       Status    Image                              Runtime   Created
k8s_busybox_nginx-sandbox_default_hdishd83djaidwnduwk28bcsb_0   3530bb   running   docker.io/library/busybox:latest   runc      4 seconds ago
k8s_POD_nginx-sandbox_default_hdishd83djaidwnduwk28bcsb_1       008cc6   running   k8s.gcr.io/pause-amd64:3.0         runc      1 minute ago
[root@pouch-test cri-tools]# crictl exec -s 3530bbb414ccce09ba52fbdf781df53b881661720f05d92edf8820280abf064d brctl addbr abc
brctl: bridge abc: Operation not permitted
Exit code: 1
[root@pouch-test cri-tools]# 

Ⅴ. Special notes for reviews

@codecov-io
Copy link

Codecov Report

Merging #711 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
- Coverage    10.7%   10.63%   -0.07%     
==========================================
  Files          92       92              
  Lines        5383     5415      +32     
==========================================
  Hits          576      576              
- Misses       4757     4789      +32     
  Partials       50       50
Impacted Files Coverage Δ
daemon/mgr/container_types.go 15.87% <ø> (ø) ⬆️
daemon/mgr/container.go 3.52% <0%> (-0.14%) ⬇️
daemon/mgr/cri.go 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b903ebe...d652075. Read the comment docs.

@YaoZengzeng
Copy link
Contributor Author

@skoo87 @allencloud PTAL.

@skoowoo
Copy link
Contributor

skoowoo commented Feb 7, 2018

LGTM , the exec process cache must be gc, remember to do it.

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Feb 7, 2018
@allencloud allencloud merged commit 2dea120 into AliyunContainerService:master Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants