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

document kube-apiserver identity #24921

Merged
merged 1 commit into from
Nov 29, 2020

Conversation

roycaihw
Copy link
Member

@roycaihw roycaihw commented Nov 6, 2020

enhancement issue: kubernetes/enhancements#1965

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 6, 2020
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Nov 6, 2020

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

Building with commit 2ad9e02

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 6, 2020
@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/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 6, 2020
@annajung
Copy link
Contributor

annajung commented Nov 6, 2020

/assign @reylejano-rxm
/milestone 1.20
/sig api-machinery
/hold pending merge kubernetes/kubernetes#95533 and kubernetes/kubernetes#95895

@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 6, 2020
@k8s-ci-robot k8s-ci-robot added this to the 1.20 milestone Nov 6, 2020
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.

Hi. Here's some early feedback - I hope it's helpful.

Comment on lines 177 to 182
The API Server Identity feature is controlled by a feature gate
and is not enabled by default. See
[Feature Gates](/docs/reference/command-line-tools-reference/feature-gates/)
for a general explanation of feature gates and how to enable and disable them. The
name of the feature gate is "APIServerIdentity". You can enable API Server Identity
by adding the following command-line flag to your `kube-apiserver` invocation:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd write this as:

Suggested change
The API Server Identity feature is controlled by a feature gate
and is not enabled by default. See
[Feature Gates](/docs/reference/command-line-tools-reference/feature-gates/)
for a general explanation of feature gates and how to enable and disable them. The
name of the feature gate is "APIServerIdentity". You can enable API Server Identity
by adding the following command-line flag to your `kube-apiserver` invocation:
The API Server Identity feature is controlled by a
[feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
and is not enabled by default. You can activate API Server Identity by enabling
the feature gate named `APIServerIdentity` when you start the
{{< glossary_tooltip text="API Server" term_id="kube-apiserver" >}}:

Copy link
Contributor

Choose a reason for hiding this comment

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

or:

Suggested change
The API Server Identity feature is controlled by a feature gate
and is not enabled by default. See
[Feature Gates](/docs/reference/command-line-tools-reference/feature-gates/)
for a general explanation of feature gates and how to enable and disable them. The
name of the feature gate is "APIServerIdentity". You can enable API Server Identity
by adding the following command-line flag to your `kube-apiserver` invocation:
_API Server identity_ is behind a
[feature gate](/docs/reference/command-line-tools-reference/feature-gates/)
named `APIServerIdentity`, that is not enabled by default.
You can enable API Server Identity by adding the following command-line
flag to your `kube-apiserver` invocation:

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Updated the paragraph with the first suggestion.

# …and other flags as usual
```

Each API server will be assigned with an unique ID on bootstrap. The list of
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
Each API server will be assigned with an unique ID on bootstrap. The list of
During bootstrap, NAME_OF_THING assigns a unique ID to each API server. Each kube-apiserver
manages a [Lease](/docs/reference/generated/kubernetes-api/{{< param "version" >}}//#lease-v1-coordination-k8s-io)
in the _kube-apiserver-lease_ {{< glossary_tooltip text="namespaces" term_id="namespace">}}.
The Lease contains the unique ID for the kube-apiserver. If another kube-apiserver tries to join
same control plane, the additional API server [picks a unique ID that is not used within the cluster?]
Enabling this feature [provides benefits?].
Each API server also exposes its own ID as a label dimension in its metrics: `TBD`.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the mechanism and the benefit.

The metrics didn't made it into alpha. Dropped the sentence.

@annajung
Copy link
Contributor

/hold cancel
k/k prs merged

hi @roycaihw with k/k prs merged in, Sig Docs would appreciate it if you could get the docs content ready to be reviewed early, if possible, before kubecon. It will give us more time to make sure we get both tech/docs review on time before the release. Thank you!

Please don't forget to change the PR title once it's ready to be reviewed. (remove wip)

@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 12, 2020
and is not enabled by default. See
[Feature Gates](/docs/reference/command-line-tools-reference/feature-gates/)
for a general explanation of feature gates and how to enable and disable them. The
name of the feature gate is "APIServerIdentity". You can enable API Server Identity
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi there, please don't forget to add APIServerIdentity to https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/ and a short description of what functionality it provides. thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 63 to 66
* `kube-system` The namespace for objects created by the Kubernetes system
* `kube-public` This namespace is created automatically and is readable by all users (including those not authenticated). This namespace is mostly reserved for cluster usage, in case that some resources should be visible and readable publicly throughout the whole cluster. The public aspect of this namespace is only a convention, not a requirement.
* `kube-node-lease` This namespace for the lease objects associated with each node which improves the performance of the node heartbeats as the cluster scales.
* `kube-apiserver-lease` TODO(roycaihw): behind feature gate
Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to change “four” to “several”. Also, I suggest sorting the list.

Here are some wording suggestions that can either go in this PR or land after this one merges.

Suggested change
* `kube-system` The namespace for objects created by the Kubernetes system
* `kube-public` This namespace is created automatically and is readable by all users (including those not authenticated). This namespace is mostly reserved for cluster usage, in case that some resources should be visible and readable publicly throughout the whole cluster. The public aspect of this namespace is only a convention, not a requirement.
* `kube-node-lease` This namespace for the lease objects associated with each node which improves the performance of the node heartbeats as the cluster scales.
* `kube-apiserver-lease` TODO(roycaihw): behind feature gate
* `kube-apiserver-lease` *⚠????TODO????TODO????⚠* _behind feature gate_
* `kube-node-lease` A namespace for the lease objects associated with each node. Node leases improve the performance of node heartbeats for large clusters.
* `kube-public` A namespace that is created automatically and is readable by all users (including those not authenticated). This namespace is mostly reserved for cluster usage, in case some resources should be visible and readable publicly throughout the whole cluster.
* `kube-system` General namespace for Kubernetes system objects.
{{< note >}}
Making the `kube-public` namespace and its contents world-readable is a convention, not a requirement.
{{< /note >}}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. We decided to reuse kube-system for this feature. Reverted the change.

@reylejano
Copy link
Member

reylejano commented Nov 18, 2020

Hi @roycaihw , please change the PR title to remove the words "wip: placeholder" when the PR is ready for review

@reylejano
Copy link
Member

@roycaihw, please keep in mind that the Docs PR Ready for Review deadline is coming up on Monday, November 23

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 20, 2020
@roycaihw roycaihw changed the title wip: placeholder for kube-apiserver identity document kube-apiserver identity Nov 20, 2020
@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 20, 2020
@roycaihw
Copy link
Member Author

It's ready for review. Please take a look. Thanks!

@reylejano
Copy link
Member

reylejano commented Nov 20, 2020

Hi @kubernetes/sig-api-machinery-pr-reviews , can we get a tech review (lgtm)
Thank you

@irvifa
Copy link
Member

irvifa commented Nov 20, 2020

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

@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 20, 2020
@irvifa
Copy link
Member

irvifa commented Nov 20, 2020

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

@irvifa
Copy link
Member

irvifa commented Nov 20, 2020

/remove-label kind/api-change

@k8s-ci-robot
Copy link
Contributor

@irvifa: The label(s) /remove-label kind/api-change 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:

/remove-label kind/api-change

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.

@sftim
Copy link
Contributor

sftim commented Nov 22, 2020

/remove-kind api-change

@k8s-ci-robot k8s-ci-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 22, 2020
sftim
sftim previously requested changes Nov 22, 2020
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.

I'm puzzled about the StorageVersionAPI feature gate that this PR is now adding.

@@ -150,6 +151,7 @@ different Kubernetes components.
| `ServiceTopology` | `false` | Alpha | 1.17 | |
| `SetHostnameAsFQDN` | `false` | Alpha | 1.19 | 1.19 |
| `SetHostnameAsFQDN` | `true` | Beta | 1.20 | |
| `StorageVersionAPI` | `false` | Alpha | 1.20 | |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intended? The PR title doesn't mention StorageVersionAPI.

If I were documenting 2 different features I'd probably do that in 2 separate commits - it's easier to revert if ever needed.

Copy link
Member

Choose a reason for hiding this comment

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

Yes there's two separate things, storage version depends on the identity flag being on. @roycaihw is there a second PR documenting storage version? Should these be moved there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Storage version API is not user-facing. The only user of it is the storage migrator. Currently there is no plan to have a k8s.io doc for it. We may document it in the storage migrator repo. cc @caesarxuchao

I removed this line.

(we have the k8s.io doc for apiserver-identity because it can be used for other features, e.g. priority & fairness)

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 24, 2020
Copy link
Member

@caesarxuchao caesarxuchao left a comment

Choose a reason for hiding this comment

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

LGTM otherwise.

in the _kube-system_ {{< glossary_tooltip text="namespaces" term_id="namespace">}}.
The Lease name is the unique ID for the kube-apiserver. The Lease contains a
label `k8s.io/component=kube-apiserver`. Each kube-apiserver manages deleting
identity leases for kube-apiservers that are gone.
Copy link
Member

Choose a reason for hiding this comment

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

Shall we also introduce the lease duration and gc period flags?

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 flags

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.

Hi, a bit more feedback.

Comment on lines 194 to 195
label `k8s.io/component=kube-apiserver`. Each kube-apiserver manages deleting
identity leases for kube-apiservers that are gone.
Copy link
Contributor

Choose a reason for hiding this comment

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

Each kube-apiserver manages deleting identity leases for kube-apiservers that are gone.

How is “gone” defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated with details. Thanks

Comment on lines 197 to 198
Enabling this feature is a prerequisite for using features that involve HA API
server coodination (e.g. the StorageVersion API feature).
Copy link
Contributor

Choose a reason for hiding this comment

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

nits (we avoid Latin abbreviations; typo fix)

Suggested change
Enabling this feature is a prerequisite for using features that involve HA API
server coodination (e.g. the StorageVersion API feature).
Enabling this feature is a prerequisite for using features that involve HA API
server coordination (for example, the StorageVersion API feature).

Is StorageVersion a kind or the name of a feature gate? I'm not sure readers can work that out.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's the feature gate. Updated

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 24, 2020
@annajung
Copy link
Contributor

Hi @roycaihw if you address some of the comments above, I think this will be able to merge soon.

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

LGTM label has been added.

Git tree hash: d31ecd48abedb8bc0d556e60c04aaa4714089886

@sftim sftim dismissed their stale review November 25, 2020 16:40

Superseded

@sftim
Copy link
Contributor

sftim commented Nov 29, 2020

/approve

@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 29, 2020
@k8s-ci-robot k8s-ci-robot merged commit 79a6030 into kubernetes:dev-1.20 Nov 29, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants