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: implement StopContainer, RemoveContainer and ListContainers of cri manager #527

Merged

Conversation

YaoZengzeng
Copy link
Contributor

Signed-off-by: YaoZengzeng [email protected]

1.Describe what this PR did

2.Does this pull request fix one issue?

fixes part of #420

3.Describe how you did it

4.Describe how to verify it

Maybe the cri-tools is what you need.

We may use the following method to verify that we can stop, remove and list 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]# crictl ps
CONTAINER ID        IMAGE                              CREATED             STATE               NAME                ATTEMPT
270b4d34783fb       docker.io/library/busybox:latest   48 years ago        CONTAINER_RUNNING   busybox             0
[root@pouch-test cri-tools]# crictl stop 3530bbb414ccc
[root@pouch-test cri-tools]# pouch ps
Name                                                            ID       Status    Image                              Runtime   Created
k8s_busybox_nginx-sandbox_default_hdishd83djaidwnduwk28bcsb_0  3530bb   stopped   docker.io/library/busybox:latest   runc      34 seconds ago
k8s_POD_nginx-sandbox_default_hdishd83djaidwnduwk28bcsb_1       30a21b   running   k8s.gcr.io/pause-amd64:3.0         runc      1 minute ago
[root@pouch-test cri-tools]# crictl rm 3530bbb414ccc
[root@pouch-test cri-tools]# pouch ps
Name                                                        ID       Status    Image                        Runtime   Created
k8s_POD_nginx-sandbox_default_hdishd83djaidwnduwk28bcsb_1   30a21b   running   k8s.gcr.io/pause-amd64:3.0   runc      1 minute ago

5.Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Jan 9, 2018

CLA assistant check
All committers have signed the CLA.

@YaoZengzeng
Copy link
Contributor Author

@allencloud PTAL.

Copy link
Collaborator

@allencloud allencloud left a comment

Choose a reason for hiding this comment

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

Could you please add unit test for function extractLabels , parseUint32 , parseContainerName and filterCRIContainers?

func parseContainerName(name string) (*runtime.ContainerMetadata, error) {
parts := strings.Split(name, nameDelimiter)
if len(parts) != 6 {
return nil, fmt.Errorf("failed to parse container name: %q", name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think here we only throw the error, but have not told users the root cause of the error.

How about fmt.Errorf("failed to parse container name %q: name format must be in format blablabla", name)

@allencloud
Copy link
Collaborator

CI fails @YaoZengzeng

@YaoZengzeng YaoZengzeng force-pushed the container-left branch 3 times, most recently from 09699d9 to f8b06b7 Compare January 9, 2018 08:12
@YaoZengzeng
Copy link
Contributor Author

@allencloud updated.

Maybe we should move tool functions like extractLabels, parseUint32... to another file like cri_utils.go and we'll do the unit test in file like cri_test.go, which would make file cri.go more clean.

I'll do it in another PR, is this OK?

@allencloud
Copy link
Collaborator

Maybe we should move tool functions like extractLabels, parseUint32... to another file like cri_utils.go

Totally agree. I think it looks not so clear in dir mgr currently.

@codecov-io
Copy link

Codecov Report

Merging #527 into master will decrease coverage by 0.96%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
- Coverage   17.38%   16.41%   -0.97%     
==========================================
  Files          36       36              
  Lines        1829     1937     +108     
==========================================
  Hits          318      318              
- Misses       1476     1584     +108     
  Partials       35       35
Impacted Files Coverage Δ
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 779515e...fa110bf. Read the comment docs.

@allencloud
Copy link
Collaborator

LGTM, please add a PR for unit test ASAP. Thanks.

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 9, 2018
@allencloud allencloud merged commit 058fe0e into AliyunContainerService:master Jan 9, 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/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants