Add PriorityClass API object under new "scheduling" API group #48377

bsalamat · 2017-07-01T00:57:36Z

What this PR does / why we need it: This PR is a part of a series of PRs to add pod priority to Kubernetes. This PR adds a new API group called "scheduling" with a new API object called "PriorityClass". PriorityClass maps the string value of priority to its integer value.

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

Special notes for your reviewer: Given the size of this PR, I will add the admission controller for the PriorityClass in a separate PR.

Release note:

Add PriorityClass API object under new "scheduling" API group

ref/ #47604
ref/ #48646

bsalamat · 2017-07-01T00:59:23Z

@kubernetes/api-approvers
@kubernetes/api-reviewers
@kubernetes/sig-scheduling-api-reviews
@kubernetes/sig-scheduling-pr-reviews

gyliu513 · 2017-07-05T02:59:57Z

pkg/apis/scheduling/types.go

+// integer value. The value can be any valid integer.
+type PriorityClass struct {
+	metav1.TypeMeta
+	// +optional


How about adding more comments here?

// Standard object metadata; More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#metadata.

Thanks, @gyliu513 ! Done.

gyliu513 · 2017-07-05T03:03:26Z

pkg/apis/scheduling/types.go

+// PriorityClassList is a collection of priority classes.
+type PriorityClassList struct {
+	metav1.TypeMeta
+	// +optional


// Standard list metadata. // More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds // +optional

gyliu513 · 2017-07-05T03:21:07Z

pkg/apis/scheduling/validation/validation.go

+	allErrs := apivalidation.ValidateObjectMetaUpdate(&pc.ObjectMeta, &oldPc.ObjectMeta, field.NewPath("metadata"))
+	// Name is immutable and is checked by the ObjectMeta validator.
+	if pc.Value != oldPc.Value {
+		allErrs = append(allErrs, field.Forbidden(field.NewPath("Value"), "may not be changed in an update."))


How about putting the Value in the error message?

I think putting the value in the error message is a bit misleading and may make users think that there is an issue with the value itself. We don't allow changing value regardless of its content.

Make sense, thanks @bsalamat

gyliu513 · 2017-07-05T03:21:30Z

pkg/apis/scheduling/validation/validation.go

+	if pc.Value != oldPc.Value {
+		allErrs = append(allErrs, field.Forbidden(field.NewPath("Value"), "may not be changed in an update."))
+	}
+	if pc.GlobalDefault != oldPc.GlobalDefault {


Why cannot change GlobalDefault? It not, the comments for this fn also need some update.

It has a similar problem to changing value, i.e., if the GlobalDefault changes, the integer value of pods without any priority class which were resolved before the change of GlobalDefault may confuse users. Besides, there is a risk of having multiple priority classes with GlobalDefault set. I updated the comment.

I revisited the idea of letting GlobalDefault be updatable today. I can prevent users from setting multiple priority classes as default in the admission controller. So, the main risk can be mitigated. The confusion problem of updating GlobalDefault stills remain, but the flexibility that allowing the update provides may justify it. WDYT?

It is always good idea to mitigate the risk even though it cannot fully resolve the issue, +1 on this, thanks @bsalamat ;-)

gyliu513 · 2017-07-05T03:21:51Z

pkg/apis/scheduling/validation/validation.go

+		allErrs = append(allErrs, field.Forbidden(field.NewPath("Value"), "may not be changed in an update."))
+	}
+	if pc.GlobalDefault != oldPc.GlobalDefault {
+		allErrs = append(allErrs, field.Forbidden(field.NewPath("GlobalDefault"), "may not be changed in an update."))


How about adding the GlobalDefault to the error message?

Similar to my comment about putting value in the error message, I think it may be misleading.

bsalamat · 2017-07-05T23:38:29Z

ref/ kubernetes/community#604

justinsb · 2017-07-06T00:20:55Z

/lgtm

This is probably api-machinery for approval though.

justinsb · 2017-07-06T00:14:55Z

pkg/apis/scheduling/validation/validation.go

+func ValidatePriorityClassUpdate(pc, oldPc *scheduling.PriorityClass) field.ErrorList {
+	allErrs := apivalidation.ValidateObjectMetaUpdate(&pc.ObjectMeta, &oldPc.ObjectMeta, field.NewPath("metadata"))
+	// Name is immutable and is checked by the ObjectMeta validator.
+	if pc.Value != oldPc.Value {


The comments say that Name and Value are immutable, but you check Value & GlobalDefault.

+1

btw, do we allow update Default?

I fixed the comment to say that Name, Value, and GlobalDefault are immutable. Name is checked by the default validation of metadata.

@k82cn no, we don't.

Got it; and there'll be only one default priority in the cluster, right? and we'll check it in admission controller.

yes, and yes!

justinsb · 2017-07-06T00:15:31Z

pkg/apis/scheduling/validation/validation_test.go

+	old := scheduling.PriorityClass{
+		ObjectMeta: metav1.ObjectMeta{Name: "tier1", Namespace: "", ResourceVersion: "1",
+			Annotations: map[string]string{
+				"kbernetes.io/description": "Used for the highest priority pods.",


typo: kbernetes

Actually, I removed the whole annotation. I learned that we should not use annotations for information which should be discoverable by users. So, I changed description from an annotation to a field and this was a remnant of the initial code. Thanks for pointing it out!

k82cn · 2017-07-06T00:25:15Z

pkg/apis/scheduling/register.go

+)
+
+// GroupName is the group name use in this package
+const GroupName = "scheduling.k8s.io"


scheduling? seems others are without k8s.io.

Our document says that we should use xyz.k8s.io. Some of the other groups use the k8s.io suffix and some don't. I think the suffix is preferred.

Can you share the doc link? if xxx.k8s.io is preferred, I think we need to open a PR/Issue to correct others.

Please see below:
https://github.com/kubernetes/community/blob/master/contributors/devel/adding-an-APIGroup.md#scripts-changes-and-auto-generated-code

Opened #48527 to align the GroupName. lgtm for this PR.

k82cn · 2017-07-06T00:30:30Z

pkg/apis/scheduling/validation/validation.go

+func ValidatePriorityClassUpdate(pc, oldPc *scheduling.PriorityClass) field.ErrorList {
+	allErrs := apivalidation.ValidateObjectMetaUpdate(&pc.ObjectMeta, &oldPc.ObjectMeta, field.NewPath("metadata"))
+	// Name is immutable and is checked by the ObjectMeta validator.
+	if pc.Value != oldPc.Value {


+1

btw, do we allow update Default?

k82cn · 2017-07-06T00:31:39Z

pkg/apis/scheduling/validation/validation_test.go

+		},
+	}
+
+	for k, v := range errorCases {


suggest merge errorCases & successCases like others.

Some of the other groups, such as autoscaling, storage, rbac, ... follow this pattern of separate error and success cases.

k82cn · 2017-07-06T00:33:52Z

pkg/apis/scheduling/validation/validation.go

+// set correctly.
+func ValidatePriorityClass(pc *scheduling.PriorityClass) field.ErrorList {
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, apivalidation.ValidateObjectMeta(&pc.ObjectMeta, false, ValidatePriorityClassName, field.NewPath("metadata"))...)


PriorityClass is global by default, so the namespace should be empty.

The API object is defined as "nonNamespaced" in staging/src/k8s.io/api/scheduling/v1alpha1/types.go. My understanding is that the system enforces the rule of not having a namespace for "nonNamespaced" resources.
We have tests that verify it does not take any namespace.

Great to know !

BTW, do you know which component handled nonNamespaced ? just want to learn :).

I don't know. I wonder if the // +nonNamespaced=true annotation in the types.go file causes some validation code to be generated that checks this.

gyliu513

/lgtm

bsalamat · 2017-07-13T18:12:53Z

/assign @smarterclayton
For approval

lavalamp · 2017-07-13T21:24:04Z

@justinsb SIG API Machinery doesn't own the contents of the API.

cblecker · 2017-07-14T01:49:14Z

cross-build is currently broken: #48887

bsalamat · 2017-07-14T05:05:19Z

@smarterclayton Fixed the comments. PTAL.

cblecker · 2017-07-14T19:53:20Z

/test pull-kubernetes-cross

timothysc · 2017-07-17T15:32:34Z

pkg/apis/scheduling/types.go

+
+// PriorityClass defines the mapping from a priority class name to the priority
+// integer value. The value can be any valid integer.
+type PriorityClass struct {


Sorry that I'm behind on reviews... trying pretty frantically to catch up.

Why exactly do we create our own api group, and won't this need to be referenced by the main pod objects? B/c we don't typically cross groups, they are self contained.

I tried to see if we can place the PriorityClass in any of the existing API groups, but it didn't really fit nicely in any of them. So, I spoke with @thockin and the conclusion was to create a new API group for scheduling.

The priority class is not being referenced by pod objects. Pod API can have a PriorityClass name which is resolved to its integer value by an admission controller. The integer value is set in the pod object and all the components of the system will work with this integer value.

That is wonky to me, what other api constructs split across groups like this?

PV.Spec.StorageClassName and storage.k8s.io/StorageClass. I agree it's weird.

Yes, storage class is similar to this as well. Why are the API groups should be self contained? What is the concern with referencing an API group from another one?

It's just a UX issue and consistency thing. Operators already have a tough time rationalizing the system. We should be sure to document this accordingly.

There is no doubt about documenting it, but my understanding is that API groups are not exposed to operators. In other words, when one adds a PriorityClass, for example, she adds it the same way no matter which API group the PriorityClass is located in.

my understanding is that API groups are not exposed to operators. In other words, when one adds a PriorityClass, for example, she adds it the same way no matter which API group the PriorityClass is located in.

That is definitely not correct. The API group is included in the object definition, and is required to fully-qualify the object kind.

That is definitely not correct. The API group is included in the object definition, and is required to fully-qualify the object kind.

True. I guess you are referring to apiVersion in object definition where API group is mentioned.

Add PriorityClass to pkg/registry Add PriorityClass to pkg/master/master.go Add PriorityClass to import_know_versions.go Update linted packages minor fix

bsalamat · 2017-07-19T22:07:28Z

ping @smarterclayton

smarterclayton · 2017-07-19T23:57:05Z

/approve

smarterclayton · 2017-07-19T23:57:28Z

/lgtm

As well

k8s-github-robot · 2017-07-19T23:57:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bsalamat, davidopp, gyliu513, justinsb, smarterclayton

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [smarterclayton]
~~cmd/libs/go2idl/client-gen/OWNERS~~ [smarterclayton]
~~cmd/libs/go2idl/go-to-protobuf/OWNERS~~ [smarterclayton]
~~docs/OWNERS~~ [smarterclayton]
~~federation/OWNERS~~ [smarterclayton]
~~hack/OWNERS~~ [smarterclayton]
~~pkg/OWNERS~~ [smarterclayton]
~~staging/OWNERS~~ [smarterclayton]
~~test/OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-07-20T02:04:28Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 1, 2017

k8s-github-robot assigned vishh and justinsb Jul 1, 2017

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jul 1, 2017

gyliu513 reviewed Jul 5, 2017

View reviewed changes

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

bsalamat force-pushed the priority_class branch 2 times, most recently from 41fd2a1 to e9cea8b Compare July 5, 2017 21:58

bsalamat force-pushed the priority_class branch from e9cea8b to 29c3ff4 Compare July 5, 2017 23:48

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

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

justinsb added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 6, 2017

justinsb reviewed Jul 6, 2017

View reviewed changes

k82cn reviewed Jul 6, 2017

View reviewed changes

bsalamat force-pushed the priority_class branch from 29c3ff4 to d4efb64 Compare July 6, 2017 01:22

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2017

k8s-ci-robot assigned gyliu513 Jul 6, 2017

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

gyliu513 reviewed Jul 6, 2017

View reviewed changes

bsalamat force-pushed the priority_class branch from d4efb64 to a288b70 Compare July 7, 2017 18:36

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2017

k8s-ci-robot assigned smarterclayton Jul 13, 2017

lavalamp added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jul 13, 2017

bsalamat force-pushed the priority_class branch from e61581d to 7a19aa7 Compare July 14, 2017 05:01

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2017

bsalamat force-pushed the priority_class branch from 7a19aa7 to f9f36f3 Compare July 14, 2017 17:29

timothysc reviewed Jul 17, 2017

View reviewed changes

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

bsalamat force-pushed the priority_class branch 2 times, most recently from 30c9f6c to 036f93d Compare July 18, 2017 22:35

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

bsalamat added 3 commits July 18, 2017 17:47

Add PriorityClass API

33e6a47

Add PriorityClass to pkg/registry Add PriorityClass to pkg/master/master.go Add PriorityClass to import_know_versions.go Update linted packages minor fix

addressed reviewer comments

af2e55e

autogenerated files

e827e1b

bsalamat force-pushed the priority_class branch from 036f93d to e827e1b Compare July 19, 2017 00:48

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

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2017

k8s-github-robot merged commit 8d26afa into kubernetes:master Jul 20, 2017

bsalamat deleted the priority_class branch July 20, 2017 04:50

davidopp mentioned this pull request Sep 7, 2017

Add user docs for pod priority and preemption kubernetes/website#5328

Merged

Add PriorityClass API object under new "scheduling" API group #48377

Add PriorityClass API object under new "scheduling" API group #48377

Conversation

bsalamat commented Jul 1, 2017 • edited

bsalamat commented Jul 1, 2017

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

bsalamat Jul 5, 2017 • edited

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

bsalamat commented Jul 5, 2017

justinsb commented Jul 6, 2017

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

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

bsalamat Jul 6, 2017 • edited

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

gyliu513 left a comment

Choose a reason for hiding this comment

bsalamat commented Jul 13, 2017

lavalamp commented Jul 13, 2017

cblecker commented Jul 14, 2017

bsalamat commented Jul 14, 2017

cblecker commented Jul 14, 2017

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

bsalamat commented Jul 19, 2017

smarterclayton commented Jul 19, 2017

smarterclayton commented Jul 19, 2017

k8s-github-robot commented Jul 19, 2017

k8s-github-robot commented Jul 20, 2017

bsalamat commented Jul 1, 2017 •

edited

bsalamat Jul 5, 2017 •

edited

bsalamat Jul 6, 2017 •

edited