Preserve API group order in discovery, prefer extensions over apps #43553

liggitt · 2017-03-23T03:45:13Z

Fixes #42392, supercedes #43543

Kubectl 1.5 still uses compiled in types in kubectl edit and apply.

Because kubectl 1.5 does not have the apps/v1beta1/deployment resource compiled in, the preferred group order must have extensions come before apps. The preference order is determined by the order that the groups are listed by the discovery service, with the first elements preferred over the last elements.

This PR:

updates the discovery code to preserve the order groups were registered in
updates the registration order to move the apps group to the end of the list (for the same result as Prefer extensions group over apps group in Kubernetes 1.6. #43543)

This has the side benefit of making all TPR API groups (regardless of group name) come after the core API groups, instead of potentially appearing earlier in discovery order

The API server discovery document now prioritizes the `extensions` API group over the `apps` API group. This ensures certain commands in 1.5 versions of kubectl (such as `kubectl edit deployment`) continue to function against a 1.6 API.

k8s-reviewable · 2017-03-23T03:45:17Z

This change is

liggitt · 2017-03-23T03:53:24Z

cc @kubernetes/sig-api-machinery-pr-reviews @deads2k @lavalamp

pwittrock · 2017-03-23T04:20:46Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

-		}
-		sort.Strings(groupNames)
-		for _, groupName := range groupNames {
+		for _, groupName := range s.apiGroupNamesForDiscovery {


Consider adding a comment that this maintains the insertion order.

comment added

pwittrock · 2017-03-23T04:21:35Z

/lgtm

ethernetdan · 2017-03-23T05:10:43Z

@bgrant0607 @lavalamp @wojtek-t could you PTAL at this release blocker?

liggitt · 2017-03-23T05:25:26Z

@pwittrock @ymqytw wasn't sure if you wanted a release note on this PR or if you were planning to add them in manually... feel free to edit or remove the one I put in the description

liggitt · 2017-03-23T06:00:32Z

@deads2k would like your take on how (or if) you see this interacting with the 1.7 registration stuff you have in progress (or if the aggregator bypasses this)

deads2k · 2017-03-23T11:37:42Z

@deads2k would like your take on how (or if) you see this interacting with the 1.7 registration stuff you have in progress (or if the aggregator bypasses this)

It will be ok. Minor fitting pains, but nothing severe.

deads2k · 2017-03-23T12:11:22Z

lgtm

liggitt · 2017-03-23T15:12:09Z

added comment, retagging
@lavalamp for approval

pwittrock · 2017-03-23T16:20:53Z

lgtm

pwittrock · 2017-03-23T16:21:38Z

@liggitt Updated the release note to be focussed on what to expect when upgrading a cluster from 1.5 -> 1.6

bgrant0607 · 2017-03-23T16:53:49Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

-		sort.Strings(groupNames)
-		for _, groupName := range groupNames {
+
+		// ranging over apiGroupNamesForDiscovery preserves the registration order


Please document that the order is preserved in the function description, also.

+1 Add a godoc to AddAPIGroupForDiscovery that the call order is significant.

opened #43623 with doc

bgrant0607 · 2017-03-23T16:54:39Z

@liggitt @pwittrock

I like this much better, thanks. One minor comment. Given the time crunch, I'm ok with it not being addressed.

/approve

cc @caesarxuchao

k8s-github-robot · 2017-03-23T16:54:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgrant0607, liggitt, pwittrock

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

~~OWNERS~~ [bgrant0607]

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

liggitt · 2017-03-23T16:56:23Z

will open a follow up with additional godoc once master reopens

bgrant0607 · 2017-03-23T16:56:50Z

ref #43542

bgrant0607 · 2017-03-23T16:57:06Z

Thanks, @liggitt

lavalamp · 2017-03-23T17:41:03Z

Yeah, thanks, this is much better.

k8s-github-robot · 2017-03-23T19:45:28Z

Automatic merge from submit-queue

k8s-github-robot · 2017-03-23T19:45:33Z

@liggitt PR needs rebase

Due to backwards compatibility issues (kubernetes#42392) introduced in 1.6 when adding new API group we've set extensions to be preferred group over apps (kubernetes#43553). Now that extensions is entirely deprecated it's time to revert this change and prefer correct API groups.

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

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 23, 2017

k8s-github-robot assigned yifan-gu Mar 23, 2017

liggitt force-pushed the discovery-order branch from d88361a to 8fab539 Compare March 23, 2017 03:46

pwittrock self-assigned this Mar 23, 2017

liggitt mentioned this pull request Mar 23, 2017

Prefer extensions group over apps group in Kubernetes 1.6. #43543

Closed

k8s-github-robot added the release-note-label-needed label Mar 23, 2017

liggitt added this to the v1.6 milestone Mar 23, 2017

liggitt assigned deads2k and lavalamp Mar 23, 2017

liggitt added area/apiserver area/kubectl kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Mar 23, 2017

liggitt force-pushed the discovery-order branch from 8fab539 to c36421d Compare March 23, 2017 04:20

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 23, 2017

pwittrock reviewed Mar 23, 2017

View reviewed changes

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

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 23, 2017

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Mar 23, 2017

liggitt removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 23, 2017

Preserve API group order in discovery, prefer extensions over apps

707f0fb

liggitt force-pushed the discovery-order branch from c36421d to 707f0fb Compare March 23, 2017 15:10

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

liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2017

pwittrock added the release-blocker label Mar 23, 2017

bgrant0607 reviewed Mar 23, 2017

View reviewed changes

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

k8s-github-robot merged commit b1e665f into kubernetes:master Mar 23, 2017

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

liggitt mentioned this pull request Mar 23, 2017

Patch applies defaults from other group #43589

Closed

janetkuo mentioned this pull request Mar 23, 2017

Create empty release notes doc for commenting kubernetes/enhancements#203

Merged

liggitt deleted the discovery-order branch March 24, 2017 14:09

soltysh mentioned this pull request Feb 13, 2020

Prefer apps version over deprecated extensions #88119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve API group order in discovery, prefer extensions over apps #43553

Preserve API group order in discovery, prefer extensions over apps #43553

liggitt commented Mar 23, 2017 •

edited

Loading

k8s-reviewable commented Mar 23, 2017

liggitt commented Mar 23, 2017

pwittrock Mar 23, 2017 •

edited

Loading

liggitt Mar 23, 2017

pwittrock commented Mar 23, 2017

ethernetdan commented Mar 23, 2017

liggitt commented Mar 23, 2017

liggitt commented Mar 23, 2017

deads2k commented Mar 23, 2017

deads2k commented Mar 23, 2017

liggitt commented Mar 23, 2017 •

edited

Loading

pwittrock commented Mar 23, 2017

pwittrock commented Mar 23, 2017

bgrant0607 Mar 23, 2017

lavalamp Mar 23, 2017

liggitt Mar 24, 2017

bgrant0607 commented Mar 23, 2017

k8s-github-robot commented Mar 23, 2017

liggitt commented Mar 23, 2017

bgrant0607 commented Mar 23, 2017

bgrant0607 commented Mar 23, 2017

lavalamp commented Mar 23, 2017

k8s-github-robot commented Mar 23, 2017

k8s-github-robot commented Mar 23, 2017

Preserve API group order in discovery, prefer extensions over apps #43553

Preserve API group order in discovery, prefer extensions over apps #43553

Conversation

liggitt commented Mar 23, 2017 • edited Loading

k8s-reviewable commented Mar 23, 2017

liggitt commented Mar 23, 2017

pwittrock Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

liggitt Mar 23, 2017

Choose a reason for hiding this comment

pwittrock commented Mar 23, 2017

ethernetdan commented Mar 23, 2017

liggitt commented Mar 23, 2017

liggitt commented Mar 23, 2017

deads2k commented Mar 23, 2017

deads2k commented Mar 23, 2017

liggitt commented Mar 23, 2017 • edited Loading

pwittrock commented Mar 23, 2017

pwittrock commented Mar 23, 2017

bgrant0607 Mar 23, 2017

Choose a reason for hiding this comment

lavalamp Mar 23, 2017

Choose a reason for hiding this comment

liggitt Mar 24, 2017

Choose a reason for hiding this comment

bgrant0607 commented Mar 23, 2017

k8s-github-robot commented Mar 23, 2017

liggitt commented Mar 23, 2017

bgrant0607 commented Mar 23, 2017

bgrant0607 commented Mar 23, 2017

lavalamp commented Mar 23, 2017

k8s-github-robot commented Mar 23, 2017

k8s-github-robot commented Mar 23, 2017

liggitt commented Mar 23, 2017 •

edited

Loading

pwittrock Mar 23, 2017 •

edited

Loading

liggitt commented Mar 23, 2017 •

edited

Loading