Add node-api staging repo w/ RuntimeClass API #67791

tallclair · 2018-08-24T01:41:30Z

Introduce a new staging repo: k8s.io/node-api to house the node.k8s.io api group.
Adds (but doesn't implement) the RuntimeClass API proposed in https://github.com/kubernetes/community/blob/master/keps/sig-node/0014-runtime-class.md

Special notes for your reviewer:

This is intended to be an out-of tree CRD, consumed by core-components. There isn't much precedent for this, so please take a close look at the api machinery pieces.

Release note:
Covered by #67737

NONE

/sig node
/kind api-change
/priority important-soon
/milestone v1.13

tallclair · 2018-08-24T01:41:57Z

/assign @dchen1107
/assign @thockin

/hold
(for kubernetes/org#38)

tallclair · 2018-08-24T01:43:19Z

staging/src/k8s.io/node-api/pkg/apis/node/v1alpha1/doc.go

+*/
+
+// +k8s:deepcopy-gen=package
+// +k8s:openapi-gen=true


NOTE TO REVIEWERS:

Should this be here? It looks like it causes an entry in k8s.io/kubernetes/pkg/generated/openapi to be generated, but I think this is supposed to be completely out of tree?

tallclair · 2018-08-24T01:44:51Z

/cc @saad-ali @jsafrane
Similar problem to the CSI drivers, inspired/copied from #67716

thockin · 2018-08-24T17:41:20Z

Are we converged on the decision for CSI (and this by extension)?

tallclair · 2018-08-24T17:49:12Z

My interpretation of this comment from @liggitt was that we don't have any choice but to use a staging repo:

The issue is k/k vendoring an independent repo that depends on apimachinery. That cycle makes it impossible to change apimachinery in any way the external dependency would have to react to

Even if we use a dynamic client, we still want to be able to use the typed APIs in the Kubelet, so I don't think we can avoid vendoring the node-api from Kubelet?

thockin

I only reviewed API

thockin · 2018-08-28T03:34:12Z

staging/src/k8s.io/node-api/hack/update-codegen.sh

+# --output-base    because this script should also be able to run inside the vendor dir of
+#                  k8s.io/kubernetes. The output-base is needed for the generators to output into the vendor dir
+#                  instead of the $GOPATH directly. For normal projects this can be dropped.
+${CODEGEN_PKG}/generate-groups.sh "deepcopy,client,informer,lister" \


Shouldn't we be doing this in transit from staging -> own repo?

I don't know the answer to that, but I think this is the way all our other staging repos currently do it.

thockin · 2018-08-28T03:36:12Z

staging/src/k8s.io/node-api/pkg/apis/node/v1alpha1/types.go

+// RuntimeClassSpec is a specification of a RuntimeClass.
+type RuntimeClassSpec struct {
+	// RuntimeHandler specifies the underlying runtime the CRI calls to handle pod and/or container
+	// creation. The possible values are specific to a given configuration & CRI implementation.


Are we speccing the format of this string? e.g. containerd.io/containerd or cri-o.io/cri-o ?

For now I'm using the same validation as for the ObjectMeta name (DNS 1123 subdomain). We might want to expand that to allow paths later, but it's easier to start with tighter validation. I'll add this to the comment.

I don't think there's any reason to namespace the runtime handler. This field is only ever interpreted by the runtime, and the expectation is that the admin set it up to map to a specific config that the runtime understands. If we prefixed it with containerd.io, it would be up to containerd to then strip out that prefix (and I guess reject any request that didn't have that prefix).

I am confused. If a node supports multiple runtimes (or even if a cluster does), those runtimes need to self identify, right? Is that this string?

IIUC, what you're getting at is discovery? I.e. once we have scheduling support for RuntimeClass, how do nodes broadcast the runtimes they support, and how does the the cluster match that to a RuntimeClass.

I think it's an open question of whether nodes would "broadcast" supported RuntimeHandlers, or RuntimeClasses (or whether discovery should be supported at all).

Even if nodes did broadcast supported RuntimeHandlers, I think it's debateable whether 2 different CRI implementations configured with the same RuntimeHandler should be treated separately, or as 2 valid implementations of the same handler.

Given these open questions, I'd prefer to punt on this until we actually have a design for this feature.

We spoke offline. here's my proposed comments:

RuntimeHandler specifies the underlying runtime and configuration that the CRI implementation will use to handle pods of this class. The possible values are specific to the node configuration & CRI implementation. It is assumed that all handlers are available and equivalent on every node. If this is not specified, a default will be chosen by the implementation.

thockin

We spoke off-PR. This is just a case of needing to be clearer about the scope and meaning I think.

thockin · 2018-09-04T22:30:21Z

staging/src/k8s.io/node-api/pkg/apis/node/v1alpha1/types.go

+// RuntimeClassSpec is a specification of a RuntimeClass.
+type RuntimeClassSpec struct {
+	// RuntimeHandler specifies the underlying runtime the CRI calls to handle pod and/or container
+	// creation. The possible values are specific to a given configuration & CRI implementation.


We spoke offline. here's my proposed comments:

RuntimeHandler specifies the underlying runtime and configuration that the CRI implementation will use to handle pods of this class. The possible values are specific to the node configuration & CRI implementation. It is assumed that all handlers are available and equivalent on every node. If this is not specified, a default will be chosen by the implementation.

thockin · 2018-09-04T22:30:31Z

staging/src/k8s.io/node-api/pkg/apis/node/v1alpha1/types.go

+	// creation. The possible values are specific to a given configuration & CRI implementation.
+	// The empty string is equivalent to the default behavior.
+	// +optional
+	RuntimeHandler string `json:"runtimeHandler,omitempty" protobuf:"bytes,1,opt,name=runtimeHandler"`


optionals should be pointers.

thockin · 2018-09-05T01:17:48Z

API is approved.

Ping me when it is otherwise LGTM'ed

thockin · 2018-09-05T01:17:56Z

/approve

k8s-ci-robot · 2018-09-05T01:18:02Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [thockin]
~~pkg/OWNERS~~ [thockin]
~~staging/OWNERS~~ [thockin]
~~vendor/OWNERS~~ [thockin]

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

tallclair · 2018-09-05T03:01:07Z

/assign @dchen1107
For lgtm

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. Dynamic RuntimeClass implementation **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 ```release-note NONE ``` /sig node /kind feature /priority important-soon /milestone v1.12

nikhita · 2018-11-26T19:16:28Z

Can you also add this repo to hack/import-restrictions.yaml and in staging/README.md?

tallclair · 2018-11-27T18:52:29Z

@nikhita done

/hold cancel

nikhita · 2018-11-30T08:18:12Z

@tallclair Looks like this PR needs another rebase. :/

lavalamp · 2018-12-10T23:16:53Z

staging/src/k8s.io/node-api/pkg/apis/node/v1alpha1/types.go

+// +genclient:nonNamespaced
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+
+// RuntimeClass defines a class of runtime supported in the cluster.


Can we make the comments here clearer? I assume runtime means container runtime, but not everyone will have the context to realize that?

Can you document the actors that will read and/or write this object?

lavalamp · 2018-12-10T23:17:48Z

staging/src/k8s.io/node-api/pkg/apis/node/v1alpha1/types.go

+	Spec RuntimeClassSpec `json:"spec" protobuf:"bytes,2,name=spec"`
+}
+
+// RuntimeClassSpec is a specification of a RuntimeClass.


Can we make this comment more helpful? What sorts of things about RuntimeClasses need to be specified? Why would I want to specify them?

lavalamp · 2018-12-10T23:18:24Z

staging/src/k8s.io/node-api/pkg/apis/node/v1alpha1/types.go

+// RuntimeClassSpec is a specification of a RuntimeClass.
+type RuntimeClassSpec struct {
+	// RuntimeHandler specifies the underlying runtime and configuration that the
+	// CRI implementation will use to handle pods of this class. The possible


Can we spell out CRI at least once? Can we say what the relationship between this object and CRI is?

lavalamp · 2018-12-10T23:19:06Z

staging/src/k8s.io/node-api/pkg/apis/node/v1alpha1/types.go

+
+// RuntimeClassSpec is a specification of a RuntimeClass.
+type RuntimeClassSpec struct {
+	// RuntimeHandler specifies the underlying runtime and configuration that the


Can we give an example of what people might put in this string?

lavalamp · 2018-12-10T23:21:23Z

staging/src/k8s.io/node-api/pkg/apis/node/v1alpha1/types.go

+	// assumed that all handlers are available on every node, and handlers of the
+	// same name are equivalent on every node.
+	// If this is not specified, a default will be chosen by the implementation.
+	// +optional


This is optional, when should I specify it?

Is it OK for a user to provide this string with an empty value ("")?

dchen1107 · 2018-12-19T18:45:05Z

/lgtm

tallclair · 2018-12-19T23:06:38Z

/retest

fejta-bot · 2018-12-20T08:17:23Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-ci-robot added 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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Aug 24, 2018

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

k8s-ci-robot requested review from dchen1107 and eparis August 24, 2018 01:41

k8s-ci-robot assigned dchen1107 and thockin Aug 24, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2018

tallclair commented Aug 24, 2018

View reviewed changes

k8s-ci-robot requested review from jsafrane and saad-ali August 24, 2018 01:44

This was referenced Aug 27, 2018

Create new github.com/kubernetes/node-api repo kubernetes/org#38

Closed

Dynamic RuntimeClass implementation #67909

Merged

thockin reviewed Aug 28, 2018

View reviewed changes

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

thockin reviewed Sep 4, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2018

tallclair force-pushed the runtimeclass-api branch from 31e2ba9 to 7adb584 Compare November 27, 2018 18:51

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 27, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2018

tallclair force-pushed the runtimeclass-api branch from 7adb584 to 3895557 Compare November 27, 2018 18:54

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

tallclair force-pushed the runtimeclass-api branch from 3895557 to 8826271 Compare November 28, 2018 01:29

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

lavalamp reviewed Dec 10, 2018

View reviewed changes

Add new node-api repo

2f6f3be

tallclair force-pushed the runtimeclass-api branch from 8826271 to 593f215 Compare December 18, 2018 01:06

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

tallclair force-pushed the runtimeclass-api branch from 593f215 to befd2d0 Compare December 18, 2018 23:38

tallclair added 3 commits December 18, 2018 16:06

Add node.k8s.io API types

665baac

Add node-api code generators

03fdecb

Generated code

baa6ca5

tallclair force-pushed the runtimeclass-api branch from befd2d0 to baa6ca5 Compare December 19, 2018 00:07

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

k8s-ci-robot merged commit cbc3ec7 into kubernetes:master Dec 20, 2018

tallclair mentioned this pull request Jan 24, 2019

Migrate RuntimeClass support to the generated typed client #73285

Merged

Add node-api staging repo w/ RuntimeClass API #67791

Add node-api staging repo w/ RuntimeClass API #67791

Conversation

tallclair commented Aug 24, 2018 • edited Loading

tallclair commented Aug 24, 2018

Choose a reason for hiding this comment

tallclair commented Aug 24, 2018

thockin commented Aug 24, 2018

tallclair commented Aug 24, 2018

thockin left a comment

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

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

Choose a reason for hiding this comment

thockin left a comment

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

thockin commented Sep 5, 2018

thockin commented Sep 5, 2018

k8s-ci-robot commented Sep 5, 2018

tallclair commented Sep 5, 2018

nikhita commented Nov 26, 2018

tallclair commented Nov 27, 2018

nikhita commented Nov 30, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchen1107 commented Dec 19, 2018

tallclair commented Dec 19, 2018

fejta-bot commented Dec 20, 2018

tallclair commented Aug 24, 2018 •

edited

Loading