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

incompatible change of kubelet CRI package #47012

Closed
feiskyer opened this issue Jun 6, 2017 · 7 comments · Fixed by #47113
Closed

incompatible change of kubelet CRI package #47012

feiskyer opened this issue Jun 6, 2017 · 7 comments · Fixed by #47113
Assignees
@feiskyer
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/node Categorizes an issue or PR as relevant to SIG Node.
Milestone
v1.7

Comments

@feiskyer
Copy link
Member

feiskyer commented Jun 6, 2017 •
edited
Loading

We have moved CRI from api/v1alpha1/runtime to apis/cri/v1alpha1, which changed the package name of CRI. This would cause a significant problem: old-versioned runtime (based on CRI in v1.6) doesn't work with latest kubelet v1.7, and vice versa.

And based on current package format, if we move CRI to v1beta1 or v1 in the future, it will still have same problem.

To fix this problem, I think we should keep using the fixed package name (e.g. apis/cri/v1alpha1/runtime) for CRI.

cc/ @dchen1107 @yujuhong @mtaufen @kubernetes/sig-node-api-reviews
@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jun 6, 2017
@feiskyer feiskyer mentioned this issue Jun 6, 2017
Closed
@resouer resouer added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jun 6, 2017
@mtaufen
Copy link
Contributor

mtaufen commented Jun 6, 2017

Freezing the version is kind of a kludge though, because it sort of defeats the purpose of having versioned APIs.

I'm guessing that the problem is that when the name/version changes, you can't upgrade the Kubelet without also upgrading the runtime implementation, which means you have to restart (or evict) all the pods on the node to do the upgrade? This sounds like it adds some cost, but doesn't necessarily break applications (assuming the pods are robust against restarts...). Granted there could be something I'm missing.

Could we have Kubelets negotiate the CRI version with the runtime implementation on the node? How much would it take to make it so a runtime implementation could be upgraded without killing the Pods it controls?

Let me know if I'm missing the point here.

@feiskyer
Copy link
Member Author

feiskyer commented Jun 6, 2017

@mtaufen The problem is caused by grpc-go, which requires same package name between client and server. If they are not same, then no API calls could success.

@yujuhong
Copy link
Contributor

yujuhong commented Jun 7, 2017

The generated grpc code registers a service using package_name.service_name, e.g., The StopPodSandbox method is known as /v1alpha1.RuntimeService/StopPodSandbox.

Changing this does break the existing server-side implementation.

By the way, the package name of of the new path is the api version. If we use the new path, it implies that client and the server has to use the exact same version. If the server wants to support older clients, it'd have to register multiple versions of services. That's an interesting usage, but I couldn't find any existing repository doing that. It also lacks any kind of flexibility since the client can't even talk to the server to get the version.

I think the structure @feiskyer proposed (apis/cri/v1alpha1/runtime) might be a good compromise for now. Why do you think? @mtaufen

@yujuhong
Copy link
Contributor

yujuhong commented Jun 7, 2017

/cc @Random-Liu, FYI, for the containerd integration.

@yujuhong
Copy link
Contributor

yujuhong commented Jun 7, 2017

/cc @mrunalp

@feiskyer feiskyer mentioned this issue Jun 7, 2017
Merged
@mtaufen
Copy link
Contributor

mtaufen commented Jun 8, 2017

Yup, SGTM as a fix.

@dchen1107 dchen1107 added this to the v1.7 milestone Jun 9, 2017
@dchen1107
Copy link
Member

Ok, @yujuhong convinced me to approve this for 1.7 release.

@feiskyer feiskyer mentioned this issue Jun 9, 2017
Closed
k8s-github-robot pushed a commit that referenced this issue Jun 9, 2017
Merge pull request #47113 from feiskyer/cri
f754788 
Automatic merge from submit-queue

Kubelet: rename cri package name to pkg/kubelet/apis/cri/v1alpha1/runtime

**What this PR does / why we need it**:

We have moved CRI from api/v1alpha1/runtime to apis/cri/v1alpha1, which changed the package name of CRI. This would cause a significant problem: old-versioned runtime (based on CRI in v1.6) doesn't work with latest kubelet v1.7, and vice versa.

This PR renames cri package name to `pkg/kubelet/apis/cri/v1alpha1/runtime` for fixing the problem.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: 

fixes #47012

**Special notes for your reviewer**:

Should be included in v1.7.

**Release note**:

```release-note
CRI has been moved to package `pkg/kubelet/apis/cri/v1alpha1/runtime`.
```
@Random-Liu Random-Liu mentioned this issue Jun 20, 2017
Merged
@RenaudWasTaken RenaudWasTaken mentioned this issue Oct 17, 2017
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@feiskyer feiskyer

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/node Categorizes an issue or PR as relevant to SIG Node.
Projects
None yet
Milestone
v1.7
Development

Successfully merging a pull request may close this issue.

Kubelet: rename cri package name to pkg/kubelet/apis/cri/v1alpha1/runtime feiskyer/kubernetes
6 participants
@feiskyer @mtaufen @resouer @dchen1107 @yujuhong @k8s-ci-robot