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

Make dockershim support kata-containers #67009

Closed
wants to merge 3 commits into from
Closed

Make dockershim support kata-containers #67009

wants to merge 3 commits into from

Conversation

miaoyq
Copy link

@miaoyq miaoyq commented Aug 6, 2018

What this PR does / why we need it:
This PR contains 3 commits

  1. return IP info when setting up network

  2. cache Pod IP
    Above two commits, mainly to obtain IP address of sandbox when cni pugin is called, and cache the sandbox IP locally.

    Because the IP address that set by cni plugin will be removed from the veth device in network namespace by kata-runtime when creating the VM (in which containers will be created), if we use kata-containers as the oci-runtime, This will cause dockershim to fail to get the pod IP through cni plugin. Just like this issue description in containerd/cri. So, we should cache the pod IP.

  3. make k8s(dockershim) to support selecting oci-runtime
    Add an annotation field in pod definition, so that dockershim can select the oci-runtime according to pod definition.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #67007

Special notes for your reviewer:

Release note:

None

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 6, 2018
@k8s-ci-robot k8s-ci-robot requested review from nicksardo and tmrts August 6, 2018 08:58
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: miaoyq
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: random-liu

If they are not already assigned, you can assign the PR to them by writing /assign @random-liu in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@miaoyq
Copy link
Author

miaoyq commented Aug 6, 2018

/assign @Random-Liu

Yanqiang Miao added 2 commits August 6, 2018 17:21
@miaoyq
Copy link
Author

miaoyq commented Aug 6, 2018

/retest

@dixudx
Copy link
Member

dixudx commented Aug 6, 2018

/kind node

@miaoyq Nice work!

@dixudx
Copy link
Member

dixudx commented Aug 6, 2018

/cc @kubernetes/sig-node-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Aug 6, 2018
@resouer
Copy link
Contributor

resouer commented Aug 7, 2018 •
edited
Loading

Sorry, but I don't think it's reasonable to expand upstream dockershim + Kata like this, as dockershim is not even a independent running CRI shim, this will not align with RuntimeClass proposal #67049 we are trying to push recently.

In my mind, keeping dockershim as simple as specific for Docker, and use RuntimeClass to route request to another Kata shim (containerd, cri-o, frakti or DIY) sounds more reasonable.

@miaoyq
Copy link
Author

miaoyq commented Aug 7, 2018

@resouer Thanks for sharing the new feature.
Make sense, but docker still the main and default container runtime engine for k8s at present, also if we switch to other runtime, there may be many incompatible scenarios. So I think, as a transition period, it is appropriate to support VM-Based oci-runtime in dockershim(only a few modifications). :-)

make k8s(dockershim) to support selecting oci-runtime
3cfffcb 
make k8s(dockershim) to support selecting oci-runtime,
and fixes the unit test failure.

Signed-off-by: Yanqiang Miao <[email protected]>
@miaoyq
Copy link
Author

miaoyq commented Aug 7, 2018

/retest

@k8s-ci-robot
Copy link
Contributor

@miaoyq: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 3cfffcb link /test pull-kubernetes-e2e-kops-aws
pull-kubernetes-e2e-gce 3cfffcb link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@xiangpengzhao
Copy link
Contributor

I don't read through the RuntimeClass proposal. I'm not sure if there is still the issue this PR wants to fix even if we adopt RuntimeClass. If so, the fix is still needed :)
@kubernetes/sig-node-pr-reviews any thoughts?

@resouer
Copy link
Contributor

resouer commented Aug 10, 2018 •
edited
Loading

So I think, as a transition period, it is appropriate to support VM-Based oci-runtime in dockershim(only a few modifications). :-)

This actually concerns me.

I don't feel it's the right direction to use dockershim + Kata as I've mentioned, dockershim is not even a independent CRI implementation and is tightly coupled with kubelet code base, I'm afraid of shipping another temporary workaround will only make things worse ...

Would like to know your thoughts, especially why not cri-containerd + Kata instead.

@miaoyq
Copy link
Author

miaoyq commented Aug 10, 2018

Would like to know your thoughts, especially why not cri-containerd + Kata instead.

@resouer
I think cri-containerd + Kata is a better solution. Kubernetes Containerd Integration Goes GA
But In many production environments, docker still is the primary container runtime, switching to other runtimes that may affect the stability of the whole system and the compatibility with other component also. Maybe we should leave some time for the switch. :-)

@tallclair
Copy link
Member

/assign

@yujuhong
Copy link
Contributor

Making more changes to in-tree dockershim to support different types of runtimes defeats the purpose of creating CRI. I think in general, we should carefully examine new features being added to dockershim. In this case, I'd prefer not adding the support.

@tallclair
Copy link
Member

Agree with @yujuhong . That said, we have discussed supporting RuntimeClass in dockershim.

@yujuhong
Copy link
Contributor

RuntimeClass is a new api in kubernetes & CRI, which makes it very different from extending dockershim to support various runtimes that we don't even have tests for :-)

@miaoyq
Copy link
Author

miaoyq commented Aug 14, 2018

That said, we have discussed supporting RuntimeClass in dockershim.

@tallclair It's a good news. That will do the same thing as this PR.
But if the runtimeHandler is VM-Based oci-runtime, we still need to cache the pod IP in dockershim.

@k8s-ci-robot
Copy link
Contributor

@miaoyq: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2018
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 14, 2018
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 14, 2018
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicksardo nicksardo Awaiting requested review from nicksardo

@tmrts tmrts Awaiting requested review from tmrts

Assignees

@Random-Liu Random-Liu

@tallclair tallclair

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make dockershim support kata-containers
9 participants
@miaoyq @k8s-ci-robot @dixudx @resouer @xiangpengzhao @tallclair @yujuhong @fejta-bot @Random-Liu