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.

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

Update API Priority and Fairness doc for graduation to beta #24975

Merged
merged 1 commit into from
Nov 14, 2020

Conversation

MikeSpreitzer
Copy link
Member

Update the doc page to mention the v1beta1 API group in addition to, and in preference to, the v1alpha. Update the example to use the v1beta1 API group.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 10, 2020
@k8sio-netlify-preview-bot
Copy link
Collaborator

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit 78869f6

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5faecc43c710b6000845b6df

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 10, 2020
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Nov 10, 2020
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Nov 10, 2020
@MikeSpreitzer
Copy link
Member Author

@reylejano
Copy link
Member

reylejano commented Nov 10, 2020

/milestone 1.20
/assign @reylejano-rxm
/sig api-machinery
/hold for k/k PR 90541 and k/k PR 91389 k/k PR 96213
/cc @annajung

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Nov 10, 2020
@k8s-ci-robot k8s-ci-robot added this to the 1.20 milestone Nov 10, 2020
@MikeSpreitzer MikeSpreitzer changed the title Update API Priority and Fairness doc for graduatino to beta Update API Priority and Fairness doc for graduation to beta Nov 10, 2020
@MikeSpreitzer
Copy link
Member Author

MikeSpreitzer commented Nov 10, 2020

@reylejano-rxm : This is not dependent on k/k PRs 90541 and 91389

@reylejano
Copy link
Member

reylejano commented Nov 10, 2020

@reylejano-rxm : This is not dependent on k/k PRs 90541 and 91389

@MikeSpreitzer I updated the related k/k PR dependency

--feature-gates=APIPriorityAndFairness=true \
--runtime-config=flowcontrol.apiserver.k8s.io/v1alpha1=true \
--feature-gates=APIPriorityAndFairness=false \
--runtime-config=flowcontrol.apiserver.k8s.io/v1alpha1=false \
Copy link
Contributor

Choose a reason for hiding this comment

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

This alpha API group is enabled by default? Is that right?

Copy link
Member

Choose a reason for hiding this comment

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

good catch, no, that should read --runtime-config=flowcontrol.apiserver.k8s.io/v1beta1=false if I'm not mistaken (alpha runtime config flag is false by default)

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@adtac adtac left a comment

Choose a reason for hiding this comment

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

/lgtm

except for one minor nit

@@ -7,6 +7,7 @@ min-kubernetes-server-version: v1.18
<!-- overview -->

{{< feature-state state="alpha" for_k8s_version="v1.18" >}}
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 common practice to leave the alpha section in? I couldn't find anything similar from a brief search

I'd imagine the older versions of the docs will have the alpha tag

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, remove this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

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

adtac commented Nov 11, 2020

/lgtm cancel

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

@MikeSpreitzer , since this feature is graduating to beta, does it need to be updated in the Feature Gates list

@MikeSpreitzer
Copy link
Member Author

Ah, yes, the FeatureGates list needs to be updated too.

@@ -52,6 +52,7 @@ different Kubernetes components.
| `APIListChunking` | `false` | Alpha | 1.8 | 1.8 |
| `APIListChunking` | `true` | Beta | 1.9 | |
| `APIPriorityAndFairness` | `false` | Alpha | 1.17 | |
Copy link
Member

Choose a reason for hiding this comment

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

the convention seems to be to have the last alpha version in the Until column - do we do this only if the alpha API is entirely removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the "until" for alpha.

@annajung
Copy link
Contributor

looks like correct k/k pr for this is kubernetes/kubernetes#96527

@MikeSpreitzer
Copy link
Member Author

Oh yes, we had to switch to a different k/k PR because the original author is sleeping now.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm
Markdown makes sense, technical changes here are minor.

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

annajung commented Nov 13, 2020

Note:

/hold
Exception filed (pending approval)

@MikeSpreitzer
Copy link
Member Author

@sftim : I took your suggestion, and naturally lost your LGTM for my trouble.
The PR for which we are holding has still not landed yet but it is very close and we have filed for an exception. I expect it to merge today or tomorrow.

@sftim
Copy link
Contributor

sftim commented Nov 13, 2020

/lgtm
Markdown makes sense, technical changes here are minor.

Only minor changes since #24975 (review)

BTW - this repo does preserve LGTM through squashes

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2020
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5b36f976ac2f1720cff959599e5f1bd9ba744c9e

@MikeSpreitzer
Copy link
Member Author

The force-push to 285bc5c is a squash.

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

aand the force-push to 78869f6 added the overlooked close quote.

@sftim
Copy link
Contributor

sftim commented Nov 13, 2020

Preview

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

LGTM label has been added.

Git tree hash: 20d4b15e2a56e2a735bf5ef53fd28e192a222783

disable them. The name of the feature gate for APF is
"APIPriorityAndFairness". This feature also involves an {{<
glossary_tooltip term_id="api-group" text="API Group" >}} with: (a) a
`v1alpha1` version, disabled by default, and (b) a `v1beta1`
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes seem straightforward.
You could use a list here instead of using:
(a) some text
(b) some more text

This feature includes an {{< glossary_tooltip term_id="api-group" text="API Group" >}} with:

  • a v1alpha1 version, disabled, and
  • a v1beta1 version, enabled by default

You can disable the feature gate ...

@@ -189,12 +194,14 @@ that originate from outside your cluster.

## Resources
The flow control API involves two kinds of resources.
[PriorityLevelConfigurations](/docs/reference/generated/kubernetes-api/{{< param "version" >}}/#prioritylevelconfiguration-v1alpha1-flowcontrol-apiserver-k8s-io)
[PriorityLevelConfigurations](/docs/reference/generated/kubernetes-api/{{< param "version" >}}/#prioritylevelconfiguration-v1beta1-flowcontrol-apiserver-k8s-io)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This could be a list?

The flow control API involves two types of resources:

  • PriorityLevelConfigurations .... define the available isolation classes and the share of the available concurrency
    budget that each can handle, and allow for fine-tuning queuing behavior.
  • FlowSchemas ...

@irvifa
Copy link
Member

irvifa commented Nov 13, 2020

/label tide/method-merge-squash

@k8s-ci-robot
Copy link
Contributor

@irvifa: The label(s) /label tide/method-merge-squash cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash

In response to this:

/label tide/method-merge-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@irvifa
Copy link
Member

irvifa commented Nov 13, 2020

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 13, 2020
@kbhawkey
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbhawkey

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2020
@irvifa
Copy link
Member

irvifa commented Nov 14, 2020

/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
@k8s-ci-robot k8s-ci-robot merged commit bf23ba2 into kubernetes:dev-1.20 Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants