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 execSync of cri manager #629

Merged
merged 1 commit into from
Jan 24, 2018

Conversation

YaoZengzeng
Copy link
Contributor

@YaoZengzeng YaoZengzeng commented Jan 24, 2018

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 execSyn container:

[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
bf1b6fc8cf9ec4c496fe620f1df101266208203bc73f3e01ef2cb7027fd7cec3
[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 bf1b6fc8cf9ec4c496fe620f1df101266208203bc73f3e01ef2cb7027fd7cec3 container-config.json sandbox-config.json
6bd4a19122fe56f3972968aa21e5c72b90bfdad1e162c5aa54038959ee49df2a
[root@pouch-test cri-tools]# crictl start 6bd4a19122fe56f3972968aa21e5c72b90bfdad1e162c5aa54038959ee49df2a
[root@pouch-test cri-tools]# crictl exec -s 6bd4a19122fe56f3972968aa21e5c72b90bfdad1e162c5aa54038959ee49df2a echo "hello world"
hello world

5.Special notes for reviews

@YaoZengzeng
Copy link
Contributor Author

@allencloud PTAL.

It has already passed the corresponding test in cri-tools.

Since #617 has not been merged, we could not add basic operations on container test set into CI
temporarily :)

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #629      +/-   ##
==========================================
- Coverage   13.47%   13.41%   -0.07%     
==========================================
  Files          65       65              
  Lines        3719     3736      +17     
==========================================
  Hits          501      501              
- Misses       3168     3185      +17     
  Partials       50       50
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 fe9dd75...9dc56b2. Read the comment docs.

@allencloud allencloud added this to the v0.2-milestone milestone Jan 24, 2018
@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 24, 2018
@allencloud
Copy link
Collaborator

allencloud commented Jan 24, 2018

I think we still have some tiny issues in #617, so maybe we need some time fix this.
Ping @0x04C2, could you help solve the reference package refactoring thing ASAP?
Since CRI implementation thing may be blocked by your work.

More feature demand, you may refer to @zeppp . We can communicate with each other any time.

@fuweid
Copy link
Contributor

fuweid commented Jan 24, 2018

OK. I will do this in two days @allencloud

@allencloud
Copy link
Collaborator

Thanks @0x04C2
And I am merging this now.

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/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants