separate group and version priority #46800

deads2k · 2017-06-01T20:35:30Z

This just modifies the API and does the minimal plumbing. I can extend this pull or do another to fix the priority problem.

liggitt · 2017-06-01T20:49:26Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go

-	Priority int64
+	// We'd recommend something like: *.k8s.io (except extensions) at 1000, extensions at 2000
+	// PaaSes (OpenShift, Deis) are recommended to be in the 3000s
+	GroupPriority int64


liggitt · 2017-06-01T20:49:29Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go

+	// The primary sort is based on VersionPriority, ordered lowest number to highest (10 before 20).
+	// The secondary sort is based on the alphabetical comparison of the name of the object.  (v1.bar before v1.foo)
+	// Since its inside of a group, the number can be small, probably in the 10s
+	VersionPriority int64


liggitt · 2017-06-01T20:50:48Z

cmd/kube-apiserver/app/aggregator.go

@@ -182,7 +183,7 @@ func apiServicesToRegister(delegateAPIServer genericapiserver.DelegationTarget,
 		// but for now its a special case
 		// apps has to come last for compatibility with 1.5 kubectl clients
 		if apiService.Spec.Group == "apps" {


actually, I think moving "extensions" earlier would be a better approach... the only order we've cared about so far is with resources that exist both in extensions and in their final group (deployments, networkpolicy, etc)

liggitt · 2017-06-01T20:51:39Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go

-	// The primary sort is based on priority, ordered lowest number to highest (10 before 20).
+	// GroupPriority controls the ordering of the group with respect to other groups. The lowest of all GroupPriorities
+	// for the same group is used.  Must be greater than zero.
+	// The primary sort is based on GroupPriority, ordered lowest number to highest (10 before 20).
 	// The secondary sort is based on the alphabetical comparison of the name of the object.  (v1.bar before v1.foo)


fix secondary sort comment

liggitt · 2017-06-01T20:52:11Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go

+	// VersionPriority controls the ordering of this API version inside of its group.
+	// The primary sort is based on VersionPriority, ordered lowest number to highest (10 before 20).
+	// The secondary sort is based on the alphabetical comparison of the name of the object.  (v1.bar before v1.foo)
+	// Since its inside of a group, the number can be small, probably in the 10s


liggitt · 2017-06-01T20:54:14Z

API change makes sense to me. A couple nits on types. With betas already cut, I'm wondering if defaulting is required.

@kubernetes/sig-api-machinery-api-reviews @kubernetes/api-reviewers

deads2k · 2017-06-02T15:13:54Z

With betas already cut, I'm wondering if defaulting is required.

We already tagged a beta level of 1.7? If not, data from alphas isn't guaranteed stable, right?

liggitt · 2017-06-02T15:24:56Z

https://github.com/kubernetes/kubernetes/releases/tag/v1.7.0-beta.0

deads2k · 2017-06-02T15:38:51Z

Well that's interesting. Beta before freeze. I'll update the proto numbers to avoid conflicts and default to 5k

smarterclayton · 2017-06-02T18:05:35Z

Just because we cut a beta does not alter the guarantees we provide. Features are beta until release, not beta until beta. Let's get a release team and API review team decision, I don't want accidents at code freeze to linger forever.

deads2k · 2017-06-02T20:36:15Z

cmd/kube-apiserver/app/aggregator.go

+	apiServicePriority, ok := apiVersionPriorities[gv]
+	if !ok {
+		// if we aren't found, assume we're a CRD
+		apiServicePriority = priority{group: 5000, version: 20}


I should skip instead of doing this. Has to be silent though.

liggitt · 2017-06-02T20:47:01Z

Let's get a release team and API review team decision, I don't want accidents at code freeze to linger forever.

The changes in this PR will flatten priority of existing APIService objects (ones created on the beta tag cut last night), and require group and version priority be specified when creating or updating spec (status updates don't validate anything in spec, so existing objects will still be able to have status updated).

The controller that creates these to represent primary API groups, TPRs, and CRDs will happily update them with corrected group/version priorities on upgrade. I would recommend we proceed with the change.

liggitt · 2017-06-02T20:50:02Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/v1beta1/types.go

+	VersionPriority int32 `json:"versionPriority" protobuf:"varint,8,opt,name=versionPriority"`
+
+	// Priority is completely ignored.  It never appeared in any permanent level of kubernetes, but it was in a beta API in a beta
+	// tagged level, so we tolerate the field being present.
 	Priority int64 `json:"priority" protobuf:"varint,6,opt,name=priority"`


removing doesn't cause problems with anything other than client-side validation if someone tried to create off a 1.7.0-beta.0 manifest, right? any other reason to keep it?

liggitt · 2017-06-02T20:52:59Z

cmd/kube-apiserver/app/aggregator.go

+	schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1beta1"}:        priority{group: 1200, version: 15},
+	schema.GroupVersion{Group: "networking.k8s.io", Version: "v1"}:               priority{group: 1200, version: 10},
+	schema.GroupVersion{Group: "policy", Version: "v1beta1"}:                     priority{group: 1200, version: 15},
+	schema.GroupVersion{Group: "rbac.authorization.k8s.io", Version: "v1beta1"}:  priority{group: 1200, version: 14},


14, eh?

Figured v1 would end up at 10.

liggitt · 2017-06-02T21:04:49Z

cmd/kube-apiserver/app/aggregator.go

+
+// The proper way to resolve this problem is to refactor the genericapiserver.DelegationTarget to include a list of priorities based on which APIs were installed.
+// This requires the APIGroupInfo struct to evolve and include the concept of priorities and to avoid mistakes, the core storage map there needs to be updated.
+// That ripples out every bit as far as you'd expect, so for 1.7 we'll include the list here instead of being built up during storage.


I'm really unhappy losing strong VersionPreferenceOrder. At least include a breadcrumb in the place we build the storage map until we can refactor it to preserve the info.

liggitt · 2017-06-02T21:07:23Z

cmd/kube-apiserver/app/aggregator.go

+	// can reasonably expect seems questionable.
+	schema.GroupVersion{Group: "extensions", Version: "v1beta1"}: priority{group: 1100, version: 15},
+	// to my knowledge, nothing below here collides
+	schema.GroupVersion{Group: "apps", Version: "v1beta1"}:                       priority{group: 1200, version: 15},


gofmt says you should simplify this declaration

lavalamp · 2017-06-05T20:19:46Z

cmd/kube-apiserver/app/aggregator.go

+	version int32
+}
+
+// The proper way to resolve this problem is to refactor the genericapiserver.DelegationTarget to include a list of priorities based on which APIs were installed.


What is "this problem"?

letting the aggregator know the desired group and version-within-group order of the underlying servers

(i.e., add to the comment)

lavalamp · 2017-06-05T20:29:36Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go

+	// The tertiary sort is based on the alphabetical comparison of the name of the object.  (v1.bar before v1.foo)
+	// We'd recommend something like: *.k8s.io (except extensions) at 1000, extensions at 2000
+	// PaaSes (OpenShift, Deis) are recommended to be in the 3000s
+	GroupPriority int32


What if different groupversions provide different values for the groupPriority field?

it gets min(groupPriority), as it does today (it is as important as the most important APIService providing something for that group)

IMO we do not need two priority numbers to represent that.

If I need to change the preferred order of versions within a single group, I shouldn't have to inspect all API service objects to see if changing a single priority field will have the side effect of changing my group's priority as well

IMO we do not need two priority numbers to represent that.

I was convinced by the reordering version case. It would be a really annoying and unexpected side effect.

David argued that that folks might want different versions served from different places in case of a migration or something, a use case I'm not completely convinced is actually legit

Letting you register a service representing v2 of an API, backed by a pool of v2 servers, while keeping the v1 service as-is, is hard to argue against

It's not like it's impossible to work around the existing Priority field to get things to show up in the order you want (use the first 2 digits for the group and the last two for the version, like I suggested).

merging two intents into a single int field by convention seems like a really bad idea (though the phrase "int intent" makes me smile)

Letting you register a service representing v2 of an API, backed by a pool of v2 servers, while keeping the v1 service as-is, is hard to argue against

Such arrangements must be doing something funky to get the v1 objects to show through the v2 api and vice versa. It depends on the storage mechanism whether that is possible or not.

merging two intents into a single int field by convention seems like a really bad idea (though the phrase "int intent" makes me smile)

It is totally future proof, though--once we address the group priority in some better way the versions will continue to sort correctly.

Such arrangements must be doing something funky to get the v1 objects to show through the v2 api and vice versa. It depends on the storage mechanism whether that is possible or not.

At the time, we were looking at ways of allowing unknown fields to pass serialization unmolested. Had I known then that it wouldn't move forward, I may have chosen differently, but I still think that the goal of separating bits sufficiently to be able to supply new versions as unpreferred options without risking my currently working servers is exactly a thing that I want.

My 2 cents:

We actually define a MinGroupPriority here (let's call it this way to make this explicit!). I would prefer a way that avoids this conceptual complexity, but I agree with the arguments of "self-contained APIServer" and "different servers for group versions" leading to this. So I guess we have to accept this complexity. Neither of the proposed solutions (with the exception of a Groups resources which we don't want for other reasons) solves this, only changes the encoding.

And in this light, splitting MinGroupPriority and VersionPriority is at least a step forward as it splits a group-local order from a global order. Try to explain to somebody to decrease the priority of a version, but not too much because as it might influence the group order.

Calling it GroupPriorityMinimum actually is a big improvement, thanks.

deads2k · 2017-06-13T23:02:59Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

liggitt · 2017-06-14T01:01:47Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go

-	// We'd recommend something like: *.k8s.io (except extensions) at 100, extensions at 150
-	// PaaSes (OpenShift, Deis) are recommended to be in the 200s
-	Priority int64
+	// We'd recommend something like: *.k8s.io (except extensions) at 10000 and


make the recommendation more or less match the autogenerated APIServices (those start at 18000, this recommends 10000)

liggitt · 2017-06-14T01:02:04Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go

+	GroupPriorityMinimum int32
+
+	// VersionPriority controls the ordering of this API version inside of its group.  Must be greater than zero.
+	// The primary sort is based on VersionPriority, ordered v number to lowest (20 before 10).


ordered v number to lowest - cut error?

liggitt · 2017-06-14T01:02:29Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/v1beta1/generated.proto

-  // PaaSes (OpenShift, Deis) are recommended to be in the 200s
-  optional int64 priority = 6;
+  // We'd recommend something like: *.k8s.io (except extensions) at 10000 and
+  // PaaSes (OpenShift, Deis) are recommended to be in the 2000s


same comment comments

liggitt · 2017-06-14T01:03:15Z

nits, lgtm

deads2k · 2017-06-14T13:30:04Z

comments fixed

sttts · 2017-06-14T13:42:45Z

api/openapi-spec/swagger.json

@@ -46758,22 +46759,27 @@
      "description": "Group is the API group name this server hosts",
      "type": "string"
     },
+     "groupPriorityMinimum": {
+      "description": "GroupPriorityMininum is the priority this group should have at least. Higher priority means that the group is prefered by clients over lower priority ones. Note that other versions of this group might specify even higher GroupPriorityMininum values such that the whole group gets a higher priority. The primary sort is based on GroupPriorityMinimum, ordered highest number to lowest (20 before 10). The secondary sort is based on the alphabetical comparison of the name of the object.  (v1.bar before v1.foo) We'd recommend something like: *.k8s.io (except extensions) at 18000 and PaaSes (OpenShift, Deis) are recommended to be in the 2000s",


18000 makes me smile.

sttts · 2017-06-14T13:49:45Z

Looks good from my side.

liggitt · 2017-06-14T17:24:43Z

lgtm as well. @lavalamp, any final comments?

deads2k · 2017-06-14T17:25:39Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

lavalamp · 2017-06-14T21:03:34Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go

+	// VersionPriority controls the ordering of this API version inside of its group.  Must be greater than zero.
+	// The primary sort is based on VersionPriority, ordered highest to lowest (20 before 10).
+	// The secondary sort is based on the alphabetical comparison of the name of the object.  (v1.bar before v1.foo)
+	// Since it's inside of a group, the number can be small, probably in the 10s.


Can probably even start at 0, no?

lavalamp · 2017-06-14T21:03:50Z

/lgtm

lavalamp · 2017-06-14T21:04:17Z

Thanks for humoring me, I think this makes a lot more sense now.

k8s-github-robot · 2017-06-14T21:04:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, lavalamp

Associated issue: 46322

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~~ [lavalamp]
~~cmd/kube-apiserver/OWNERS~~ [deads2k,lavalamp]
~~pkg/master/OWNERS~~ [deads2k,lavalamp]
~~pkg/registry/OWNERS~~ [deads2k,lavalamp]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k,lavalamp]
~~test/OWNERS~~ [deads2k,lavalamp]

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-06-15T04:43:42Z

Automatic merge from submit-queue (batch tested with PRs 47492, 47542, 46800, 47545, 45764)

…gistration Automatic merge from submit-queue (batch tested with PRs 47626, 47674, 47683, 47290, 47688) add admissionregistration to the list Fix #47686 The bug is introduced by #46800 Any suggestion on how to write a unit test? Or don't bother because the hardcoded list will be gone soon after 1.7?

deads2k added this to the v1.7 milestone Jun 1, 2017

deads2k assigned liggitt Jun 1, 2017

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

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jun 1, 2017

liggitt reviewed Jun 1, 2017

View reviewed changes

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

deads2k force-pushed the agg-33-priority branch from 990d211 to d264d33 Compare June 2, 2017 19:52

deads2k commented Jun 2, 2017

View reviewed changes

liggitt reviewed Jun 2, 2017

View reviewed changes

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2017

lavalamp reviewed Jun 5, 2017

View reviewed changes

dchen1107 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2017

deads2k force-pushed the agg-33-priority branch from e00be57 to 00928e7 Compare June 13, 2017 18:53

liggitt reviewed Jun 14, 2017

View reviewed changes

deads2k added 2 commits June 14, 2017 09:29

add group priority to apiservice

d13ad28

sort current API versions and fallback for others

963c85e

deads2k force-pushed the agg-33-priority branch from 00928e7 to 963c85e Compare June 14, 2017 13:29

sttts reviewed Jun 14, 2017

View reviewed changes

sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 14, 2017

lavalamp reviewed Jun 14, 2017

View reviewed changes

k8s-ci-robot assigned lavalamp Jun 14, 2017

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

k8s-github-robot merged commit 08c705e into kubernetes:master Jun 15, 2017

caesarxuchao mentioned this pull request Jun 17, 2017

add admissionregistration to the list #47688

Merged

liggitt mentioned this pull request Jun 26, 2017

Make the service-catalog chart setup more secure with RBAC kubernetes-retired/service-catalog#981

Closed

deads2k deleted the agg-33-priority branch August 3, 2017 20:10

stu-gott mentioned this pull request Mar 15, 2018

VirtualMachinePresets are un-ordered kubevirt/kubevirt#805

Closed

yue9944882 mentioned this pull request Dec 5, 2018

The field priority does not exist in apiregistration.k8s.io/v1beta kubernetes-sigs/apiserver-builder-alpha#274

Closed

separate group and version priority #46800

separate group and version priority #46800

Conversation

deads2k commented Jun 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Jun 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jun 1, 2017

deads2k commented Jun 2, 2017

liggitt commented Jun 2, 2017

deads2k commented Jun 2, 2017

smarterclayton commented Jun 2, 2017 via email

Choose a reason for hiding this comment

liggitt commented Jun 2, 2017

liggitt Jun 2, 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

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

liggitt Jun 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k Jun 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jun 14, 2017

deads2k commented Jun 14, 2017

Choose a reason for hiding this comment

sttts commented Jun 14, 2017

liggitt commented Jun 14, 2017

deads2k commented Jun 14, 2017

Choose a reason for hiding this comment

lavalamp commented Jun 14, 2017

lavalamp commented Jun 14, 2017

k8s-github-robot commented Jun 14, 2017

k8s-github-robot commented Jun 15, 2017

deads2k commented Jun 1, 2017 •

edited

liggitt Jun 1, 2017 •

edited

liggitt Jun 2, 2017 •

edited

liggitt Jun 7, 2017 •

edited

deads2k Jun 7, 2017 •

edited