CRI: upgrade protobuf to v3 #39158

feiskyer · 2016-12-22T15:33:24Z

For #38854, this PR upgrades CRI protobuf version to v3, and also updated related packages for confirming to new api.

Release note:

CRI: upgrade protobuf version to v3.

k8s-reviewable · 2016-12-22T15:33:31Z

This change is

k8s-ci-robot · 2016-12-22T16:05:30Z

Jenkins verification failed for commit 0f4ff82694731321a618e11175a17aa6d9880f05. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-12-22T16:17:32Z

Jenkins GKE smoke e2e failed for commit 0f4ff82694731321a618e11175a17aa6d9880f05. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

feiskyer · 2016-12-23T07:08:03Z

@k8s-bot cri e2e test this

k8s-ci-robot · 2016-12-29T04:04:55Z

Jenkins Bazel Build failed for commit bd1aec921ca840154b5ba65c1e12f96fb9c1cc8a. Full PR test history.

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

feiskyer · 2016-12-29T04:07:41Z

cc @yujuhong @yifan-gu @euank

feiskyer · 2016-12-29T04:47:54Z

@k8s-bot bazel test this

feiskyer · 2016-12-29T07:44:17Z

All tests passed. cc/ @kubernetes/sig-node-misc

k8s-ci-robot · 2017-01-04T08:58:25Z

Jenkins CRI GCE Node e2e failed for commit bfa4baec254be7ba0442fa85453a4fc2f68b26b1. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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.

yifan-gu · 2017-01-18T20:04:10Z

LGTM overall,

I have questions about some of the integer-type fields and their default value being 0. Will review the rest once these are addressed.

The default 0 values are passed to runtime by GetXXX() today already, e.g. GetTimeout(), so there is no functional changes happening here.

But we should better document all those integer default values in the proto. See checklist here

yujuhong · 2017-01-18T20:11:17Z

The default 0 values are passed to runtime by GetXXX() today already, e.g. GetTimeout(), so there is no functional changes happening here.

Better document is needed for sure, especially now that there is no way to check the existence of these field even if we want to. The use of timeout is not very consistent -- in one case (StopContainer) it's used to indicate immediate termination, while in the other (ExecSync request) it means run forever. What 0 means for the resource quota/limit fields is just unclear.

As for run_as_user and uid, we do check them in the code, and I don't think we can not wrap them in proto3.

Random-Liu · 2017-01-19T02:29:57Z

t's not from the PR, but can we also rename the status to ready ? status doesn't sound like a boolean

@yifan-gu
I name it as status just to match the K8s api https://github.com/kubernetes/kubernetes/blob/master/pkg/api/types.go#L1371-L1381.

"ready" or not should be in condition type (RuntimeReady, NetworkNotReady), status should only indicate whether the condition is true or not.

feiskyer · 2017-01-19T05:38:05Z

@yujuhong @yifan-gu Addressing comments. PTAL.

yujuhong · 2017-01-19T17:57:37Z

Reviewed 17 of 41 files at r1, 15 of 20 files at r2, 11 of 11 files at r3.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

hack/verify-flags/exceptions.txt, line 70 at r3 (raw file):

cluster/saltbase/salt/opencontrail-networking-master/init.sls:      - 'SERVICE_CLUSTER_IP_RANGE': '{{ pillar.get('service_cluster_ip_range') }}'
cluster/saltbase/salt/opencontrail-networking-minion/init.sls:      - 'SERVICE_CLUSTER_IP_RANGE': '{{ pillar.get('service_cluster_ip_range') }}'
cluster/saltbase/salt/supervisor/kubelet-checker.sh:	{% set kubelet_port = pillar['kubelet_port'] -%}

Why would the PR touch this?...

pkg/kubelet/api/v1alpha1/runtime/api.proto, line 647 at r3 (raw file):

    string container_id = 1;
    // Timeout in seconds to stop the container. Must be > 0.
    int64 timeout = 2;

I think we should allow timeout=0. How about:
Timeout in seconds to wait for the container to stop before forcibly terminating it. Default: 0 (forcibly terminate the container immediately)

pkg/kubelet/api/v1alpha1/runtime/api.proto, line 823 at r3 (raw file):

    // ID of the container to which to forward the port.
    string pod_sandbox_id = 1;
    // Port to forward. Each port must be > 0 if provided.

nit: I think repeated fields should be ok because they'll get converted to a list (which could be nil).

Comments from Reviewable

yujuhong · 2017-01-19T17:58:49Z

Looks good overall with some nits.
Probably also want to squash some of the commits.

yifan-gu · 2017-01-20T00:57:28Z

pkg/kubelet/api/v1alpha1/runtime/api.proto

@@ -699,7 +705,7 @@ message Container {
    string image_ref = 5;
    // State of the container.
    ContainerState state = 6;
-    // Creation time of the container in nanoseconds.
+    // Creation time of the container in nanoseconds. Must be > 0.


while the container is not created, this can be 0 right?

yifan-gu · 2017-01-20T00:57:40Z

pkg/kubelet/api/v1alpha1/runtime/api.proto

@@ -728,13 +734,13 @@ message ContainerStatus {
    ContainerMetadata metadata = 2;
    // Status of the container.
    ContainerState state = 3;
-    // Creation time of the container in nanoseconds.
+    // Creation time of the container in nanoseconds. Must be > 0.


yifan-gu · 2017-01-20T01:01:11Z

@Random-Liu Thanks for the clarification.

@feiskyer LGTM except the comments made by @yujuhong

feiskyer · 2017-01-20T01:57:11Z

Rebased and squashed commits.

Why would the PR touch this?

hack/verify-flags/exceptions.txt is generated automatically by hack/verify-flags-underscore.py -e.

yujuhong · 2017-01-20T02:04:54Z

/lgtm

Please help watch the cri builds to see if anything breaks after the PR is merged.

feiskyer · 2017-01-20T07:35:20Z

Please help watch the cri builds to see if anything breaks after the PR is merged.

Of course. Is CRI enabled for all tests in page google-cri ?

k8s-github-robot · 2017-01-20T16:28:48Z

Automatic merge from submit-queue (batch tested with PRs 40168, 40165, 39158, 39966, 40190)

Random-Liu · 2017-01-20T18:24:13Z

Of course. Is CRI enabled for all tests in page google-cri ?

Yeah.

yujuhong · 2017-01-20T18:32:21Z

FYI, I glanced over all the cri builds and didn't see any obvious red builds.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 22, 2016

k8s-github-robot assigned vishh Dec 22, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Dec 22, 2016

feiskyer mentioned this pull request Dec 22, 2016

CRI: upgrade protobuf to v3 #38854

Closed

feiskyer force-pushed the cri-proto3 branch from 0f4ff82 to 54a635e Compare December 23, 2016 03:53

feiskyer changed the title ~~WIP: upgrade CRI to protobuf v3~~ CRI: upgrade protobuf to v3 Dec 26, 2016

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed release-note-label-needed labels Dec 29, 2016

feiskyer force-pushed the cri-proto3 branch from 54a635e to bd1aec9 Compare December 29, 2016 04:04

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 29, 2016

resouer requested review from vishh and yujuhong December 29, 2016 07:46

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 31, 2016

feiskyer force-pushed the cri-proto3 branch from bd1aec9 to bfa4bae Compare January 4, 2017 08:05

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 4, 2017

feiskyer force-pushed the cri-proto3 branch from bfa4bae to e819c6f Compare January 4, 2017 09:33

yujuhong assigned yujuhong and unassigned vishh Jan 5, 2017

yujuhong removed the request for review from vishh January 5, 2017 00:31

yifan-gu reviewed Jan 20, 2017

View reviewed changes

feiskyer added 8 commits January 20, 2017 09:54

CRI: update protobuf to v3

9cab3b4

kubelet/api: update cri to protobuf v3

97fff6a

kubelet/server: update cri to protobuf v3

8e4ea2d

kubelet/remote: update cri to protobuf v3

76afc73

kubelet/rktshim: update cri to protobuf v3

e77e61b

kubelet/dockershim: update cri to protobuf v3

d4bfcd1

kubelet/kuberuntime: update cri to protobuf v3

53c20e3

Update flags underscore

b962580

feiskyer force-pushed the cri-proto3 branch from 34304fd to b962580 Compare January 20, 2017 01:56

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2017

k8s-github-robot merged commit 0e1a166 into kubernetes:master Jan 20, 2017

feiskyer deleted the cri-proto3 branch January 20, 2017 23:57

This was referenced Jan 27, 2017

Applying k8s kubelet v3 api to cri-o server cri-o/cri-o#342

Merged

ListPodSandbox: bad wiretype for field runtime.PodSandboxFilter.State cri-o/cri-o#339

Closed

heartlock mentioned this pull request Feb 8, 2017

fix #72 Upgrade CRI to proto3 kubernetes-retired/frakti#73

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRI: upgrade protobuf to v3 #39158

CRI: upgrade protobuf to v3 #39158

feiskyer commented Dec 22, 2016 •

edited

Loading

k8s-reviewable commented Dec 22, 2016

k8s-ci-robot commented Dec 22, 2016

k8s-ci-robot commented Dec 22, 2016

feiskyer commented Dec 23, 2016

k8s-ci-robot commented Dec 29, 2016

feiskyer commented Dec 29, 2016

feiskyer commented Dec 29, 2016

feiskyer commented Dec 29, 2016

k8s-ci-robot commented Jan 4, 2017

yifan-gu commented Jan 18, 2017 •

edited

Loading

yujuhong commented Jan 18, 2017 •

edited

Loading

Random-Liu commented Jan 19, 2017 •

edited

Loading

feiskyer commented Jan 19, 2017

yujuhong commented Jan 19, 2017

yujuhong commented Jan 19, 2017

yifan-gu Jan 20, 2017

yifan-gu Jan 20, 2017

yifan-gu commented Jan 20, 2017

feiskyer commented Jan 20, 2017

yujuhong commented Jan 20, 2017

feiskyer commented Jan 20, 2017

k8s-github-robot commented Jan 20, 2017

Random-Liu commented Jan 20, 2017

yujuhong commented Jan 20, 2017

CRI: upgrade protobuf to v3 #39158

CRI: upgrade protobuf to v3 #39158

Conversation

feiskyer commented Dec 22, 2016 • edited Loading

k8s-reviewable commented Dec 22, 2016

k8s-ci-robot commented Dec 22, 2016

k8s-ci-robot commented Dec 22, 2016

feiskyer commented Dec 23, 2016

k8s-ci-robot commented Dec 29, 2016

feiskyer commented Dec 29, 2016

feiskyer commented Dec 29, 2016

feiskyer commented Dec 29, 2016

k8s-ci-robot commented Jan 4, 2017

yifan-gu commented Jan 18, 2017 • edited Loading

yujuhong commented Jan 18, 2017 • edited Loading

Random-Liu commented Jan 19, 2017 • edited Loading

feiskyer commented Jan 19, 2017

yujuhong commented Jan 19, 2017

yujuhong commented Jan 19, 2017

yifan-gu Jan 20, 2017

Choose a reason for hiding this comment

yifan-gu Jan 20, 2017

Choose a reason for hiding this comment

yifan-gu commented Jan 20, 2017

feiskyer commented Jan 20, 2017

yujuhong commented Jan 20, 2017

feiskyer commented Jan 20, 2017

k8s-github-robot commented Jan 20, 2017

Random-Liu commented Jan 20, 2017

yujuhong commented Jan 20, 2017

feiskyer commented Dec 22, 2016 •

edited

Loading

yifan-gu commented Jan 18, 2017 •

edited

Loading

yujuhong commented Jan 18, 2017 •

edited

Loading

Random-Liu commented Jan 19, 2017 •

edited

Loading