Dynamic RuntimeClass implementation #67909

tallclair · 2018-08-27T20:17:10Z

What this PR does / why we need it:

Implement RuntimeClass using the dynamic client to break the dependency on #67791

Once (if) #67791 merges, I will migrate to the typed client.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
For kubernetes/enhancements#585

Release note:
Covered by #67737

NONE

/sig node
/kind feature
/priority important-soon
/milestone v1.12

caesarxuchao · 2018-08-27T20:22:07Z

pkg/kubelet/runtimeclass/runtime_class_controller.go

+func NewController(client dynamic.Interface) *Controller {
+	rc := client.Resource(runtimeClassGVR)
+	lw := &cache.ListWatch{
+		ListFunc: func(options metav1.ListOptions) (runtime.Object, error) {


Why is the wrapper needed?

Because ListFunc is type func(options metav1.ListOptions) (runtime.Object, error), but rc.List is type List(opts metav1.ListOptions) (*unstructured.UnstructuredList, error)

Even though *unstructured.UnstructuredList implements runtime.Object, go doesn't do the conversion in function signatures.

caesarxuchao · 2018-08-27T20:30:16Z

pkg/kubelet/runtimeclass/runtimeclass_manager.go

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


Is this file redundant?

Oops, forgot to delete it when I renamed it.

tallclair

Thanks Chao.

tallclair · 2018-08-27T20:35:57Z

pkg/kubelet/runtimeclass/runtimeclass_manager.go

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


Oops, forgot to delete it when I renamed it.

tallclair · 2018-08-27T20:50:35Z

pkg/kubelet/runtimeclass/runtime_class_controller.go

+func NewController(client dynamic.Interface) *Controller {
+	rc := client.Resource(runtimeClassGVR)
+	lw := &cache.ListWatch{
+		ListFunc: func(options metav1.ListOptions) (runtime.Object, error) {


Because ListFunc is type func(options metav1.ListOptions) (runtime.Object, error), but rc.List is type List(opts metav1.ListOptions) (*unstructured.UnstructuredList, error)

Even though *unstructured.UnstructuredList implements runtime.Object, go doesn't do the conversion in function signatures.

tallclair · 2018-08-28T00:07:29Z

Ready for review.

/assign @yujuhong
/cc @dchen1107 @caesarxuchao @Random-Liu

tallclair · 2018-08-28T00:08:51Z

/cc @saad-ali @jsafrane
If you find yourselves blocked by getting k8s.io/csi-api checked in, this is an alternative approach (see pkg/kubelet/runtimeclass/runtimeclass_manager.go)

tallclair · 2018-08-29T05:41:07Z

Fixed golint & rebased.

tallclair · 2018-08-30T19:42:23Z

/assign @Random-Liu

tallclair · 2018-08-30T19:52:23Z

Shoot, it looks like I pushed an old version by mistake. Will ping once resolved.

tallclair · 2018-08-30T19:57:01Z

Actually it was just a bad rebase. Fixed. PTAL

Random-Liu

LGTM overall.

Based on #67009, dockershim doesn't support kata now even with runtime handler.

We may want to get the ip cache thing in as well, or not support runtime handler in dockershim at all and return error.

Also /cc @miaoyq

Random-Liu · 2018-09-04T09:02:27Z

cmd/kubelet/app/server.go

@@ -581,6 +584,10 @@ func run(s *options.KubeletServer, kubeDeps *kubelet.Dependencies, stopCh <-chan
 			clientCertificateManager.SetCertificateSigningRequestClient(kubeClient.CertificatesV1beta1().CertificateSigningRequests())
 			clientCertificateManager.Start()
 		}
+		dynamicKubeClient, err = dynamic.NewForConfig(clientConfig)
+		if err != nil {
+			glog.Errorf("Failed to initialize dynamic KubeClient: %v", err)


It seems that we use Warningf for the other clients.

miaoyq · 2018-09-04T10:57:11Z

Thanks @Random-Liu
The motivation of this feature is to provide a way to select between different oci runtimes.
So dockershim, as the default CRI implementation for k8s, should support this feature IMO.

I also hope that dockershim can support this feature. :-)

tallclair · 2018-09-04T18:20:23Z

On further consideration, I think the dockershim support should be split out to a separate PR. I reverted the changes and added an explicit error for the non-empty handler.

The problem with the original approach is that the RuntimeHandler is treated as a binary to call (without arguments). However, there is no way to specify the path of the binary, so it must be in the system path already. This may be OK, but still deserves its own discussion.

dchen1107 · 2018-09-04T20:39:14Z

/lgtm

Please rebase it. We are good to go.

tallclair · 2018-09-04T20:45:46Z

squashed, rebased, and fixed dockershim error message

dchen1107 · 2018-09-04T21:00:25Z

/lgtm
/approve

k8s-ci-robot · 2018-09-04T21:00:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, tallclair

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:

~~cmd/kubelet/OWNERS~~ [dchen1107]
~~pkg/kubelet/OWNERS~~ [dchen1107,tallclair]

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

lukaszo · 2018-09-05T13:58:11Z

Not sure where to ask this question so asking here. Why RuntimeClass is passed to CRI and not handled by kubelet only?

tallclair · 2018-09-05T18:12:48Z

Bumping to priority to enable merging. This was in the queue prior to freeze.
/priority critical-urgent

tallclair · 2018-09-05T18:14:18Z

@lukaszo The runtime handler selects the configuration the CRI implementation should use. There is still only a single CRI server that the kubelet talks to. As we add more features to RuntimeClass (e.g. pod overhead, scheduling & discovery, supported features, etc.) the Kubelet will do more of the work.

k8s-github-robot · 2018-09-05T21:51:45Z

Automatic merge from submit-queue (batch tested with PRs 68161, 68023, 67909, 67955, 67731). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

k8s-ci-robot added this to the v1.12 milestone Aug 27, 2018

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 27, 2018

k8s-ci-robot requested review from pmorie and vishh August 27, 2018 20:17

k8s-ci-robot added the area/kubelet label Aug 27, 2018

caesarxuchao reviewed Aug 27, 2018

View reviewed changes

tallclair force-pushed the runtimeclass-kubelet branch from 56658e6 to 4085a1a Compare August 27, 2018 23:40

tallclair commented Aug 27, 2018

View reviewed changes

tallclair force-pushed the runtimeclass-kubelet branch 2 times, most recently from 5016628 to 8bb4318 Compare August 28, 2018 00:05

tallclair changed the title ~~[WIP] dynamic RuntimeClass implementation~~ Dynamic RuntimeClass implementation Aug 28, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 28, 2018

k8s-ci-robot assigned yujuhong Aug 28, 2018

k8s-ci-robot requested review from caesarxuchao, dchen1107 and Random-Liu August 28, 2018 00:07

k8s-ci-robot requested review from jsafrane and saad-ali August 28, 2018 00:08

tallclair force-pushed the runtimeclass-kubelet branch from 8bb4318 to d5dafae Compare August 29, 2018 05:38

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2018

tallclair force-pushed the runtimeclass-kubelet branch from d5dafae to 3cd360d Compare August 29, 2018 05:40

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

k8s-ci-robot assigned Random-Liu Aug 30, 2018

tallclair force-pushed the runtimeclass-kubelet branch from 3cd360d to 00d1f15 Compare August 30, 2018 19:56

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2018

Random-Liu reviewed Sep 4, 2018

View reviewed changes

tallclair force-pushed the runtimeclass-kubelet branch from dd5b4dd to 12fcd4a Compare September 4, 2018 18:20

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

k8s-ci-robot assigned dchen1107 Sep 4, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 4, 2018

Implement RuntimeClass support for the Kubelet & CRI

63f3bc1

tallclair force-pushed the runtimeclass-kubelet branch from fe37c71 to 63f3bc1 Compare September 4, 2018 20:45

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2018

k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 5, 2018

k8s-github-robot merged commit 0df5d8d into kubernetes:master Sep 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic RuntimeClass implementation #67909

Dynamic RuntimeClass implementation #67909

tallclair commented Aug 27, 2018

caesarxuchao Aug 27, 2018

tallclair Aug 27, 2018

caesarxuchao Aug 27, 2018

tallclair Aug 27, 2018

tallclair left a comment

tallclair Aug 27, 2018

tallclair Aug 27, 2018

tallclair commented Aug 28, 2018

tallclair commented Aug 28, 2018

tallclair commented Aug 29, 2018

tallclair commented Aug 30, 2018

tallclair commented Aug 30, 2018

tallclair commented Aug 30, 2018

Random-Liu left a comment •

edited

Loading

Random-Liu Sep 4, 2018

tallclair Sep 4, 2018

miaoyq commented Sep 4, 2018

tallclair commented Sep 4, 2018

dchen1107 commented Sep 4, 2018

tallclair commented Sep 4, 2018

dchen1107 commented Sep 4, 2018

k8s-ci-robot commented Sep 4, 2018

lukaszo commented Sep 5, 2018

tallclair commented Sep 5, 2018

tallclair commented Sep 5, 2018

k8s-github-robot commented Sep 5, 2018

Dynamic RuntimeClass implementation #67909

Dynamic RuntimeClass implementation #67909

Conversation

tallclair commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair commented Aug 28, 2018

tallclair commented Aug 28, 2018

tallclair commented Aug 29, 2018

tallclair commented Aug 30, 2018

tallclair commented Aug 30, 2018

tallclair commented Aug 30, 2018

Random-Liu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miaoyq commented Sep 4, 2018

tallclair commented Sep 4, 2018

dchen1107 commented Sep 4, 2018

tallclair commented Sep 4, 2018

dchen1107 commented Sep 4, 2018

k8s-ci-robot commented Sep 4, 2018

lukaszo commented Sep 5, 2018

tallclair commented Sep 5, 2018

tallclair commented Sep 5, 2018

k8s-github-robot commented Sep 5, 2018

Random-Liu left a comment •

edited

Loading