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

Add CRI v1 proto #96387

Merged

k8s-ci-robot merged 1 commit into kubernetes:master from mrunalp:cri_add_beta

Nov 12, 2020

Contributor

mrunalp commented Nov 9, 2020

@derekwaynecarr @SergeyKanzhelev @saschagrunert @mikebrow

Signed-off-by: Mrunal Patel mpatel@redhat.com

What type of PR is this?
/kind feature

What this PR does / why we need it:

Add CRI v1beta proto and generated code.
We need both v1beta1 and v1alpha2 side by side so that
containerd and CRI-O could be updated. Once the runtimes
are updated and in the CI, we can switch the kubelet
to use v1beta1.

Does this PR introduce a user-facing change?:

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

k8s-ci-robot added release-note-none kind/feature size/XXL cncf-cla: yes do-not-merge/needs-sig needs-triage labels

Contributor

k8s-ci-robot commented Nov 9, 2020

@mrunalp: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 added needs-priority area/kubelet sig/node and removed do-not-merge/needs-sig labels

k8s-ci-robot requested review from hwdef and matthyx

November 9, 2020 22:01

mrunalp mentioned this pull request

Promote cri-api to v1beta1 #96325

Closed

mrunalp force-pushed the cri_add_beta branch from cc4c9ef to aa51439 Compare

November 9, 2020 23:05

SergeyKanzhelev reviewed

View reviewed changes

hack/update-generated-runtime-dockerized.sh Outdated

    
              function generate_code() {

              	RUNTIME_API_VERSION="$1"

              	KUBE_REMOTE_RUNTIME_PATH="${KUBE_REMOTE_RUNTIME_ROOT}/${RUNTIME_API_VERSION}"

              	PATH="${gogopath}:${PATH}" \

Member

SergeyKanzhelev Nov 10, 2020

not sure how it works in bash. PATH is a global variable. first call will set the first version. Second - will have the first version already set and add a new runtime to the end. Would it pick up the second runtime at all?

Contributor Author

mrunalp Nov 10, 2020

Moving out of the function.

SergeyKanzhelev reviewed

View reviewed changes

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1beta1/constants.go Outdated

    
            @@ -0,0 +1,55 @@
          
              /*

              Copyright 2016 The Kubernetes Authors.

Member

SergeyKanzhelev Nov 10, 2020

Suggested change

      
            Copyright 2016 The Kubernetes Authors.
          
            Copyright 2020 The Kubernetes Authors.

Contributor Author

mrunalp Nov 10, 2020

Fixed

SergeyKanzhelev reviewed

View reviewed changes

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1beta1/api.proto Outdated

    
            @@ -0,0 +1,1338 @@
          
              /*

              Copyright 2018 The Kubernetes Authors.

Member

SergeyKanzhelev Nov 10, 2020

Suggested change

      
            Copyright 2018 The Kubernetes Authors.
          
            Copyright 2020 The Kubernetes Authors.

Contributor Author

mrunalp Nov 10, 2020

Fixed

SergeyKanzhelev reviewed

View reviewed changes

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1beta1/api.proto Outdated

    
                  //

                  // WARNING: Log management and how kubelet should interface with the

                  // container logs are under active discussion in

                  // https://issues.k8s.io/24677. There *may* be future change of direction

Member

SergeyKanzhelev Nov 10, 2020

this issue is closed. Should we keep the warning around?

Contributor Author

mrunalp Nov 10, 2020

Dropped the warning.

SergeyKanzhelev reviewed

View reviewed changes

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1beta1/api.proto Outdated

    
                  // If the runtime handler is unknown, this request should be rejected.  An

                  // empty string should select the default handler, equivalent to the

                  // behavior before this feature was added.

                  // See https://git.k8s.io/enhancements/keps/sig-node/runtime-class.md

Member

SergeyKanzhelev Nov 10, 2020

Suggested change

      
                // See https://git.k8s.io/enhancements/keps/sig-node/runtime-class.md
          
                // See https://git.k8s.io/enhancements/keps/sig-node/585-runtime-class/README.md

Contributor Author

mrunalp Nov 10, 2020

Fixed, thanks!

SergeyKanzhelev reviewed

View reviewed changes

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1beta1/api.proto Outdated

    
              // LinuxContainerResources specifies Linux specific configuration for

              // resources.

              // TODO: Consider using Resources from opencontainers/runtime-spec/specs-go

Member

SergeyKanzhelev Nov 10, 2020

Are we keeping TODO in beta?

Contributor Author

mrunalp Nov 10, 2020

Dropped the TODO.

SergeyKanzhelev reviewed

View reviewed changes

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1beta1/api.proto Outdated

    
                  // Variables for interactive containers, these have very specialized

                  // use-cases (e.g. debugging).

                  // TODO: Determine if we need to continue supporting these fields that are

Member

SergeyKanzhelev Nov 10, 2020

should we keep this TODO in beta?

Contributor Author

mrunalp Nov 10, 2020

No, dropped.

SergeyKanzhelev reviewed

View reviewed changes

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1beta1/api.proto Outdated

    
                  // StopContainer stops a running container with a grace period (i.e., timeout).

                  // This call is idempotent, and must not return an error if the container has

                  // already been stopped.

                  // TODO: what must the runtime do after the grace period is reached?

Member

SergeyKanzhelev Nov 10, 2020

should we keep TODO in beta?

Contributor Author

mrunalp Nov 10, 2020

Replaced TODO by a line describing that the runtime should kill on grace period expiration.

mrunalp force-pushed the cri_add_beta branch from aa51439 to 47a544d Compare

November 10, 2020 05:20

Member

SergeyKanzhelev commented Nov 10, 2020

/lgtm
/retest

k8s-ci-robot assigned SergeyKanzhelev

k8s-ci-robot added the lgtm label

Member

mikebrow commented Nov 10, 2020

thx for updating/removing the todo's I had a todo to do that and got swamped :-)

Member

dims commented Nov 10, 2020

/milestone v1.20

k8s-ci-robot added this to the v1.20 milestone

Contributor Author

mrunalp commented Nov 10, 2020

/hold we are discussing some options in sig-node to potentially rename the package to v1 to avoid multiple hops.

k8s-ci-robot added the do-not-merge/hold label

Contributor

cpuguy83 commented Nov 10, 2020

This seems a little rushed to try and pull this off for v1.20. What's the benefit in doing it now vs waiting for v1.21?

mrunalp force-pushed the cri_add_beta branch from 47a544d to d8bc4ea Compare

November 10, 2020 21:34

k8s-ci-robot removed the lgtm label

mrunalp changed the title ~~Add CRI v1beta1 proto~~ Add CRI v1 proto

mrunalp force-pushed the cri_add_beta branch from d8bc4ea to d25f1ea Compare

November 10, 2020 22:13


          Add CRI v1 proto

9fcede9

Add CRI v1 proto and generated code.
We need both v1 and v1alpha2 side by side so that
containerd and CRI-O could be updated. Once the runtimes
are updated and in the CI, we can switch the kubelet
to use v1 in 1.21 .

We are jumping to v1, so we have to avoid multiple hops to get
to GA. The package could stay v1 and declare CRI support to
be at beta and eventually GA.

Signed-off-by: Mrunal Patel <mpatel@redhat.com>

mrunalp force-pushed the cri_add_beta branch from d25f1ea to 9fcede9 Compare

November 10, 2020 22:17

Contributor Author

mrunalp commented Nov 10, 2020

/retest

Contributor Author

mrunalp commented Nov 10, 2020

This seems a little rushed to try and pull this off for v1.20. What's the benefit in doing it now vs waiting for v1.21?

We need a cycle to allow runtimes to support v1 before they can be brought back into CI and change the kubelet code. See - #96325 (comment)

Contributor Author

mrunalp commented Nov 10, 2020

/hold cancel

k8s-ci-robot removed the do-not-merge/hold label

Contributor

cpuguy83 commented Nov 10, 2020

Thanks @mrunalp, that makes sense then.

saschagrunert approved these changes

View reviewed changes

Member

saschagrunert left a comment

/lgtm

k8s-ci-robot assigned saschagrunert

k8s-ci-robot added the lgtm label

Member

dims commented Nov 11, 2020

/approve
/hold

@mrunalp please remove hold when you are ready

k8s-ci-robot added the do-not-merge/hold label

Contributor

k8s-ci-robot commented Nov 11, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, mrunalp, saschagrunert

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:

~~hack/OWNERS~~ [dims]
~~staging/src/k8s.io/cri-api/pkg/OWNERS~~ [mrunalp]

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

k8s-ci-robot added the approved label

Contributor Author

mrunalp commented Nov 11, 2020

/hold cancel

k8s-ci-robot removed the do-not-merge/hold label

mrunalp mentioned this pull request

Kubelet CRI support kubernetes/enhancements#2040

Closed

k8s-ci-robot merged commit 9d2a226 into kubernetes:master

irvifa mentioned this pull request

WIP: Add documentation for CRI API moving to v1beta1 kubernetes/website#24757

Closed

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

Labels

approved area/kubelet cncf-cla: yes kind/feature lgtm needs-priority needs-triage release-note-none sig/node size/XXL