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.

Sign up for GitHub

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

Jump to bottom

feature: implement basic RunPodSandbox method for cri manager #455

Merged
merged 1 commit into from
Dec 29, 2017
Merged

feature: implement basic RunPodSandbox method for cri manager #455

merged 1 commit into from
Dec 29, 2017

Conversation

YaoZengzeng
Copy link
Contributor

@YaoZengzeng YaoZengzeng commented Dec 28, 2017 •
edited
Loading

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 use pouch to run a pod:

// 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
53bfc944e2e6b391089d441d364e9fea98ea4a51c882d831f5a83d5fd0803162
[root@pouch-test cri-tools]# pouch ps
Name                                                        ID       Status    Image                              Runtime   Created
k8s_POD_nginx-sandbox_default_hdishd83djaidwnduwk28bcsb_1   53bfc9   running   k8s.gcr.io/pause-amd64:3.0         runc      4 seconds ago

5.Special notes for reviews

@YaoZengzeng
Copy link
Contributor Author

@skoo87 @allencloud PTAL

@codecov-io
Copy link

codecov-io commented Dec 28, 2017 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #455   +/-   ##
=========================================
  Coverage          ?   19.76%           
=========================================
  Files             ?       34           
  Lines             ?     1634           
  Branches          ?        0           
=========================================
  Hits              ?      323           
  Misses            ?     1276           
  Partials          ?       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 ded3981...804d1f5. Read the comment docs.

@allencloud allencloud added this to the v0.1-Milestone milestone Dec 28, 2017
skoowoo
skoowoo reviewed Dec 29, 2017
View reviewed changes
daemon/mgr/cri.go Outdated
@@ -2,11 +2,37 @@ package mgr

import (
"fmt"
"strings"

pouchtypes "github.com/alibaba/pouch/apis/types"
Copy link
Contributor

Choose a reason for hiding this comment

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

pouchtypes -> apitypes

daemon/mgr/cri.go Outdated
@@ -2,11 +2,37 @@ package mgr

import (
"fmt"
"strings"

pouchtypes "github.com/alibaba/pouch/apis/types"

"golang.org/x/net/context"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the "context" package, I hope to use standard "context" to replace it.

We maybe need to find a way to do it.

WDYT @allencloud

@YaoZengzeng
Copy link
Contributor Author

@skoo87 updated.

allencloud
allencloud reviewed Dec 29, 2017
View reviewed changes
daemon/mgr/cri.go
// Apply a label to distinguish sandboxes from regular containers.
labels[containerTypeLabelKey] = containerTypeLabelSandbox

hc := &apitypes.HostConfig{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If #459 has been merged, this part needs change, since we should make NetworkingConfig non-nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, please ping me if it is merged :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

get merged, please rebase, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allencloud updated.

@allencloud
Copy link
Collaborator

LGTM, and please take a double check @skoo87 if you would like, otherwise I will merge this before 2018. 😄

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 29, 2017
@allencloud allencloud merged commit 8490dd3 into AliyunContainerService:master Dec 29, 2017
@YaoZengzeng YaoZengzeng mentioned this pull request Dec 29, 2017
Closed
28 tasks
@allencloud allencloud mentioned this pull request Jan 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allencloud allencloud allencloud left review comments

@skoowoo skoowoo skoowoo left review comments

Assignees
No one assigned
Labels
kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/L
Projects
None yet
Milestone
v0.1-Milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@YaoZengzeng @codecov-io @allencloud @skoowoo @pouchrobot