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

kep-1965: update doc references for APIServerIdentity #37921

Merged
merged 2 commits into from
Nov 30, 2022

Conversation

andrewsykim
Copy link
Member

Signed-off-by: Andrew Sy Kim [email protected]

Update feature gate reference for APIServerIdentity. Currently on the fence about whether we want more comprehensive documentation for this feature. In it's current state it is mainly going to be used for internal optimizations (i.e. StorageVersion API) and likely won't see too much use from users.

@k8s-ci-robot k8s-ci-robot added this to the 1.26 milestone Nov 16, 2022
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 16, 2022
@netlify
Copy link

netlify bot commented Nov 16, 2022

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit 991a4a3
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6386bca035048f0009ad537e

@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 16, 2022
@sftim
Copy link
Contributor

sftim commented Nov 16, 2022

I recommend adding a short, simple page inside https://kubernetes.io/docs/concepts/overview/working-with-objects/: https://kubernetes.io/docs/concepts/architecture/leases/

In that page, outline what a Lease is and where Kubernetes uses them.

@andrewsykim
Copy link
Member Author

I recommend adding a short, simple page inside https://kubernetes.io/docs/concepts/overview/working-with-objects/: https://kubernetes.io/docs/concepts/architecture/leases/

In that page, outline what a Lease is and where Kubernetes uses them.

I agree this would be useful, but admittedly also going outside the scope of this KEP to write general documentation about Leases. I will try to find the time to work on this but no promises :)

@sftim
Copy link
Contributor

sftim commented Nov 16, 2022

Second-mover disadvantage: the SIG that makes the general API gets to write up specific, whereas making good use of the existing API means the new documentation only makes sense following a tidying and reorganization.

For GA, we'll want the docs about API server identity leases to have a home somewhere (and not just in the list of former feature gates). Optional but also recommened: revise pages within https://kubernetes.io/docs/tasks/debug/debug-cluster/

@andrewsykim
Copy link
Member Author

I think general documentation about Leases and their uses (kube-apiserver identtiy being one of many) would be good to have, and I am happy to try to work on it during regular development cycle. I just don't have time to work on this right now.

@andrewsykim andrewsykim changed the title [WIP] kep-1965: update doc references for APIServerIdentity kep-1965: update doc references for APIServerIdentity Nov 17, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 17, 2022
@krol3
Copy link
Contributor

krol3 commented Nov 28, 2022

Hi @andrewsykim! This PR needs a doc review by Mon Nov 28th to get this into the release. Please reach out to required SIGs to get their review. Thank you!

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.

As well as documenting the feature gate, please document how Kubernetes behaves when this is enabled. A few lines may be enough; zero documentation is not sufficient.

See #37921 (comment)

@@ -62,7 +62,8 @@ For a reference to old feature gates that are removed, please refer to
| `APIPriorityAndFairness` | `true` | Beta | 1.20 | |
| `APIResponseCompression` | `false` | Alpha | 1.7 | 1.15 |
| `APIResponseCompression` | `true` | Beta | 1.16 | |
| `APIServerIdentity` | `false` | Alpha | 1.20 | |
| `APIServerIdentity` | `false` | Alpha | 1.20 | 1.26 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `APIServerIdentity` | `false` | Alpha | 1.20 | 1.26 |
| `APIServerIdentity` | `false` | Alpha | 1.20 | 1.25 |

@leonardpahlke
Copy link
Member

x posting my slack comment

I agree, the PR does not add sufficient documentation just yet.

About the PR comment:

(@andrewsykim) I agree this would be useful, but admittedly also going outside the scope of this KEP to write general documentation about Leases.

Seems to me like a bit like an ownership issue, who “bootstraps general docs”. Perhaps we have missed to introduce the documentation page a while back?
I am +1 adding the documentation & suggestions @sftim mentioned now.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 28, 2022
@sftim
Copy link
Contributor

sftim commented Nov 28, 2022

@andrewsykim if you write the text for the APIServerIdentity part, SIG Docs can write up any generic text that the new bit needs in order to have context.

@enj
Copy link
Member

enj commented Nov 28, 2022

/assign @enj

@enj
Copy link
Member

enj commented Nov 28, 2022

/assign @ritazh @aramase

@andrewsykim
Copy link
Member Author

@andrewsykim if you write the text for the APIServerIdentity part, SIG Docs can write up any generic text that the new bit needs in order to have context.

ack, will add this later today

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2022
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 30, 2022
Signed-off-by: Andrew Sy Kim <[email protected]>
@andrewsykim
Copy link
Member Author

I added some general docs for Leases, PTAL.

Docs definitely can use more details, but trying to be mindful of the docs deadline today. Definitely needs to be a follow-up to add more content in this area.

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 for docs

I don't think this needs tech review; I can see that these pages look right enough.

/lgtm
/approve

HA configurations, where only one instance of the component should be actively running while the other
instances are on stand-by.

## kube-apiserver identity
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## kube-apiserver identity
## API Server identity

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

LGTM label has been added.

Git tree hash: 73b37c1dbd940030dd9a766297af49747b756ba4

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

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 30, 2022
@sftim
Copy link
Contributor

sftim commented Nov 30, 2022

After the release, we can track an issue to mention that workloads / out-of-tree things are also welcome to use Leases (especially controllers and operators).

@k8s-ci-robot k8s-ci-robot merged commit 0ac0f16 into kubernetes:dev-1.26 Nov 30, 2022

{{< feature-state for_k8s_version="v1.26" state="beta" >}}

Starting in Kubernetes v1.26, each `kube-apiserver` uses the Lease API to publish its identity to the
Copy link
Member

Choose a reason for hiding this comment

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

Should we provide more details on the specifics here? Like if I want to build an external integration that uses this information, how would I know what to look for?

Copy link
Contributor

Choose a reason for hiding this comment

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

More detail is good. IMO we don't need that for beta.

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 more details in #38196

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/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants