Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

promote API priority and fairness types and APIs to beta #96527

Merged
merged 4 commits into from
Nov 14, 2020
Merged

promote API priority and fairness types and APIs to beta #96527

merged 4 commits into from
Nov 14, 2020

Conversation

adtac
Copy link
Member

@adtac adtac commented Nov 12, 2020 •
edited
Loading

What type of PR is this?

/kind api-change
-->

What this PR does / why we need it: Graduates APF types and APIs to beta. Same PR as #96213 but re-opened here to get in by the code freeze deadline (today).

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer: Min's original commit is preserved.

Does this PR introduce a user-facing change?:

ACTION REQUIRED: API priority and fairness graduated to beta. 1.19 servers with APF turned on should not be run in a multi-server cluster with 1.20+ servers.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1040-priority-and-fairness

/cc @lavalamp @yue9944882 @MikeSpreitzer @ahg-g
/sig api-machinery

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/apiserver area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Nov 12, 2020
@lavalamp
Copy link
Member

/lgtm
/approve

Let's see if the tests are happier with this PR.

@k8s-ci-robot 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 Nov 12, 2020
@lavalamp
Copy link
Member

Looks like the file .../filters/priority-and-fairness_test.go needs an update?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2020
@adtac
Copy link
Member Author

adtac commented Nov 12, 2020

Looks like the file .../filters/priority-and-fairness_test.go needs an update?

yep, that and a couple of other tests too -- added a new commit on top

@fedebongio
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 12, 2020
@adtac adtac force-pushed the apfbeta branch 2 times, most recently from 82fda19 to f2edd9c Compare November 12, 2020 21:30
MikeSpreitzer
MikeSpreitzer reviewed Nov 12, 2020
View reviewed changes
cmd/kube-apiserver/app/options/validation.go Outdated Show resolved Hide resolved
cmd/kube-apiserver/app/options/validation.go Outdated Show resolved Hide resolved
pkg/registry/flowcontrol/rest/storage_flowcontrol.go Outdated
}
apiGroupInfo.VersionedResourcesStorageMap[flowcontrolapisv1beta1.SchemeGroupVersion.Version] = v1beta1FlowControlStorage
if apiResourceConfigSource.VersionEnabled(flowcontrolapisv1alpha1.SchemeGroupVersion) {
apiGroupInfo.VersionedResourcesStorageMap[flowcontrolapisv1alpha1.SchemeGroupVersion.Version] = v1beta1FlowControlStorage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it right to put the beta storage in the alpha entry in this map?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the storage is the same for all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. I remember @lavalamp mentioning that they're supposed to share the same storage?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either rename the method to be version-neutral, or make a copy per version. having a single storage constructor means that if a new type was added to surface in the alpha version, it would simultaneously/accidentally also get surfaced in the beta version

@adtac
Copy link
Member Author

adtac commented Nov 13, 2020

/test pull-kubernetes-e2e-gce-100-performance

@lavalamp lavalamp mentioned this pull request Nov 13, 2020
Closed
@adtac
Copy link
Member Author

adtac commented Nov 13, 2020

/test pull-kubernetes-bazel-test

yue9944882 and others added 4 commits November 13, 2020 23:20
@yue9944882
APF: graduate API and types to beta
849be44 
Signed-off-by: Adhityaa Chandrasekar <adtac@google.com>
flowcontrol bootstrap: make exempt PL last
bb32d51 
Signed-off-by: Adhityaa Chandrasekar <adtac@google.com>
APF matching: fallback to catch-all if nothing matches
794a3ec 
Signed-off-by: Adhityaa Chandrasekar <adtac@google.com>
flowcontrol bootstrap: give catch-all PL more concurrency share
642b114 
Signed-off-by: Adhityaa Chandrasekar <adtac@google.com>
@lavalamp
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2020
@lavalamp lavalamp mentioned this pull request Nov 14, 2020
Closed
hasheddan
hasheddan reviewed Nov 14, 2020
View reviewed changes
Copy link
Contributor

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/milestone v1.20

@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Nov 14, 2020
@hasheddan
Copy link
Contributor

/hold cancel

@k8s-ci-robot 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 14, 2020
@hasheddan
Copy link
Contributor

/hold

Please remove when ready for merge @lavalamp @adtac

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 14, 2020
@lavalamp
Copy link
Member

/hold cancel

We're ready :)

@k8s-ci-robot 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 14, 2020
@hasheddan
Copy link
Contributor

/retest

@k8s-ci-robot k8s-ci-robot merged commit 36f5714 into kubernetes:master Nov 14, 2020
@adtac adtac mentioned this pull request Nov 16, 2020
Closed
12 tasks
@lavalamp lavalamp mentioned this pull request Nov 17, 2020
Closed
@github-actions github-actions bot mentioned this pull request Nov 18, 2020
Open
@adtac adtac mentioned this pull request Nov 23, 2020
Merged
@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 24, 2020
@rfranzke rfranzke mentioned this pull request Dec 2, 2020
Open
@sttts
Copy link
Contributor

sttts commented Dec 13, 2020

The release note

ACTION REQUIRED: API priority and fairness graduated to beta. 1.19 servers with APF turned on should not be run in a multi-server cluster with 1.20+ servers.

is an understatement. More correct:

Clusters are BRICKED after downgrade from 1.20 to 1.19 if v1beta1 FlowSchemas had been written to storage.

Note, that we have an informer for v1alpha1 FlowSchemas in 1.19 that blocks /readyz to go green. And the list call fails with

no kind "FlowSchema" is registered for version "flowcontrol.apiserver.k8s.io/v1beta1" in scheme "k8s.io/kubernetes/pkg/api/legacyscheme/scheme.go:30"

Is that an accepted behaviour for alpha->beta promotions?

@liggitt
Copy link
Member

liggitt commented Dec 13, 2020

we have an informer for v1alpha1 FlowSchemas in 1.19 that blocks /readyz to go green.

That is only true if the alpha API is enabled in the 1.19 cluster, correct?

In general, enabling an alpha feature or API removes the guarantee that upgrade/downgrade will work properly without manual steps, since alpha features can be removed or altered in a single release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasheddan hasheddan hasheddan left review comments

@MikeSpreitzer MikeSpreitzer MikeSpreitzer left review comments

@wojtek-t wojtek-t wojtek-t left review comments

@liggitt liggitt liggitt left review comments

@lavalamp lavalamp lavalamp left review comments

@ahg-g ahg-g Awaiting requested review from ahg-g

@yue9944882 yue9944882 Awaiting requested review from yue9944882

Assignees

@lavalamp lavalamp

@MikeSpreitzer MikeSpreitzer

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.