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

leases: add more details on API Server Identity #38196

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

andrewsykim
Copy link
Member

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

Follow-up to #37921 (comment)

@k8s-ci-robot k8s-ci-robot added this to the 1.26 milestone Nov 30, 2022
@netlify
Copy link

netlify bot commented Nov 30, 2022

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

Name Link
🔨 Latest commit 917dee9
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6387a1649dea6a0008ee4213

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

/assign @mo @sftim

@k8s-ci-robot
Copy link
Contributor

@andrewsykim: GitHub didn't allow me to assign the following users: mo.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @mo @sftim

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.

```

The SHA256 hash used in the lease name is based on the OS hostname as seen by kube-apiserver. Each kube-apiserver should be
configured to use a hostname that is unique within the cluster. New instances of kube-apiserver that use the same hostname
Copy link
Member

Choose a reason for hiding this comment

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

Can we document the use of the kubernetes.io/hostname label as well?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@sftim
Copy link
Contributor

sftim commented Nov 30, 2022

@enj would you be willing to propose those changes in a follow-up PR? I especially want the essentials to land, even as I also want this new page to see improvements.

Oops. This is the follow up PR!

@sftim
Copy link
Contributor

sftim commented Nov 30, 2022

We should get a tech review on this one.

@enj
Copy link
Member

enj commented Nov 30, 2022

We should get a tech review on this one.

@deads2k and @lavalamp are good candidates here

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

@enj enj left a comment

Choose a reason for hiding this comment

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

One minor nit.

Comment on lines 58 to 63
```
$ kubectl -n kube-system get lease kube-apiserver-c4vwjftbvpc5os2vvzle4qg27a -o yaml
apiVersion: coordination.k8s.io/v1
Copy link
Member

Choose a reason for hiding this comment

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

Can you split this into two code blocks, one with shell syntax, the other with yaml syntax?

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!

@leonardpahlke
Copy link
Member

/cc @deads2k @lavalamp
PTAL, thanks!

@deads2k
Copy link
Contributor

deads2k commented Dec 1, 2022

/lgtm

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

LGTM label has been added.

Git tree hash: 0d317e8f2d3291f5808b70ae78582ebba14fdca9

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.

Some more feedback (see inline comments)


The label k8s.io/component is not registered. Please document it in https://kubernetes.io/docs/reference/labels-annotations-taints/

(if feasible: switch to using control-plane.kubernetes.io/component, before the v1.26 release, so that end users don't confuse it with the app.kubernetes.io/component label).

We can also release with this and then deprecate k8s.io/component in v1.27 but that's a bit of a shame to have to do.


Optionally, document what happens when an API server runs as a Pod (eg for nesting a control plane).

That might be best saved for a post-release blog for the release when this feature graduates to stable.

Comment on lines +46 to +47
$ kubectl -n kube-system get lease -l k8s.io/component=kube-apiserver
NAME HOLDER AGE
Copy link
Contributor

Choose a reason for hiding this comment

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

kube-apiserver-fyloo45sdenffw2ugwaz3likua kube-apiserver-fyloo45sdenffw2ugwaz3likua_c5ffa286-8a9a-45d4-91e7-61118ed58d2e 4m43s
```

The SHA256 hash used in the lease name is based on the OS hostname as seen by kube-apiserver. Each kube-apiserver should be
Copy link
Contributor

Choose a reason for hiding this comment

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

Which is true:

Suggested change
The SHA256 hash used in the lease name is based on the OS hostname as seen by kube-apiserver. Each kube-apiserver should be
The SHA256 hash used in the Lease name is based on the OS hostname as seen by kube-apiserver.
The API server coerces its hostname string to lowercase before hashing it. Each kube-apiserver
should be

or

Suggested change
The SHA256 hash used in the lease name is based on the OS hostname as seen by kube-apiserver. Each kube-apiserver should be
The SHA256 hash used in the Lease name is based on the OS hostname as seen by kube-apiserver.
Each kube-apiserver should be

?

creationTimestamp: "2022-11-30T15:37:15Z"
labels:
k8s.io/component: kube-apiserver
kubernetes.io/hostname: kind-control-plane
Copy link
Contributor

Choose a reason for hiding this comment

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

https://kubernetes.io/docs/reference/labels-annotations-taints/ lists that this label is used on Nodes (only)

Either:


The SHA256 hash used in the lease name is based on the OS hostname as seen by kube-apiserver. Each kube-apiserver should be
configured to use a hostname that is unique within the cluster. New instances of kube-apiserver that use the same hostname
will take over existing Leases using a new holder identity, as opposed to instantiating new lease objects. You can check the
Copy link
Contributor

@sftim sftim Dec 1, 2022

Choose a reason for hiding this comment

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

(nit)

Suggested change
will take over existing Leases using a new holder identity, as opposed to instantiating new lease objects. You can check the
will take over existing Leases using a new holder identity, as opposed to instantiating new Leases.
You can check the

spec:
holderIdentity: kube-apiserver-c4vwjftbvpc5os2vvzle4qg27a_9cbf54e5-1136-44bd-8f9a-1dcd15c346b4
leaseDurationSeconds: 3600
renewTime: "2022-11-30T18:04:27.912073Z"
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) Ideally, pick a date that is close to the v1.26 release.

Comment on lines 38 to 40
discover how many instances of `kube-apiserver` are operating the Kubernetes control plane.
Existence of kube-apiserver leases enables future capabilities that may require coordination between
each 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.

Suggested change
discover how many instances of `kube-apiserver` are operating the Kubernetes control plane.
Existence of kube-apiserver leases enables future capabilities that may require coordination between
each kube-apiserver.
The API server creates these leases in the `kube-system` namespace.
The existence of kube-apiserver Leases also enables adding future capabilities to Kubernetes,
that may require coordination between each kube-apiserver (for example, during cluster
upgrades).

@@ -38,3 +38,43 @@ rest of the system. While not particularly useful on its own, this provides a me
discover how many instances of `kube-apiserver` are operating the Kubernetes control plane.
Existence of kube-apiserver leases enables future capabilities that may require coordination between
each kube-apiserver.

You can inspect Leases owned by each kube-apiserver by checking for lease objects in the `kube-system` namespace
Copy link
Contributor

@sftim sftim Dec 1, 2022

Choose a reason for hiding this comment

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

(nit)

Suggested change
You can inspect Leases owned by each kube-apiserver by checking for lease objects in the `kube-system` namespace
You can inspect Leases owned by each kube-apiserver by checking for Leases in the `kube-system` namespace

apiVersion: coordination.k8s.io/v1
kind: Lease
metadata:
creationTimestamp: "2022-11-30T15:37:15Z"
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) Ideally, pick a date that is close to the v1.26 release.

renewTime: "2022-11-30T18:04:27.912073Z"
```

Expired leases from kube-apiservers that no longer exist are garbage collected by new kube-apiservers after 1 hour.
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 right:

Suggested change
Expired leases from kube-apiservers that no longer exist are garbage collected by new kube-apiservers after 1 hour.
### API server Lease garbage collection
Expired Leases from kube-apiservers that no longer exist are garbage collected by live kube-apiservers.
The control plane leaves the expired Leases for at least one hour before removing them.

?

We should update https://kubernetes.io/docs/concepts/architecture/garbage-collection/ to link to that heading, which is why I added it.

@sftim
Copy link
Contributor

sftim commented Dec 5, 2022

Good enough for alpha, I reckon.

/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 Dec 5, 2022
@k8s-ci-robot k8s-ci-robot merged commit b5881a3 into kubernetes:dev-1.26 Dec 5, 2022
@enj
Copy link
Member

enj commented Dec 6, 2022

Good enough for alpha, I reckon.

Except the feature is beta 😂

@sftim
Copy link
Contributor

sftim commented Dec 6, 2022

More polish is welcome.

@sftim sftim mentioned this pull request Dec 6, 2022
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.

6 participants