-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
docs: service.kubernetes.io/topology-aware-hints #38997
docs: service.kubernetes.io/topology-aware-hints #38997
Conversation
dcb356d
to
09b19ba
Compare
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
@kubernetes/sig-network-misc , Do you have time to review the changes? |
If this PR is open after the 1.27 release is out (scheduled for April 11, one week from the time of this comment), this annotation needs confirmation from SIG Network as there is a name change from Topology Aware Hints to Topology Aware Routing in 1.27 |
@reylejano For now we waiting for SIG Network review. If it will be available after 1.27 freeze, I will change naming |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
@sftim Can we review it in any way? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sergeyshevch! Left some recommended changes to capture the latest updates to the KEP.
@@ -395,6 +395,19 @@ Used on: Service | |||
|
|||
The control plane adds this label to an Endpoints object when the owning Service is headless. | |||
|
|||
### service.kubernetes.io/topology-aware-hints {#servicekubernetesiotopology-aware-hints} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the updated terminology throughout:
- The annotation should be
service.kubernetes.io/topology-mode
- The hints annotation is deprecated
- The value should be "Auto" (note the capitalization)
- Links should go to https://kubernetes.io/docs/concepts/services-networking/topology-aware-routing/
- Domain-prefixed values are supported for custom/implementation-specific values per https://github.com/kubernetes/kubernetes/blob/99190634ab252604a4496882912ac328542d649d/staging/src/k8s.io/api/core/v1/annotation_key_constants.go#L154-L161
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR, we're documenting service.kubernetes.io/topology-aware-hints
@robscott, as a fix for #36830.
Please review this as a PR to belatedly register and document service.kubernetes.io/topology-aware-hints
.
PRs to document service.kubernetes.io/topology-mode
are also welcome; I know that this is undocumented as of right now. Anyone is welcome to open a PR that documents the new annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
service.kubernetes.io/topology-aware-hints
was replaced by service.kubernetes.io/topology-mode
, any reason we should document the old annotation? For reference, any use of the hints annotation will return a warning pointing to the new annotation: https://github.com/kubernetes/kubernetes/blob/99190634ab252604a4496882912ac328542d649d/pkg/api/service/warnings.go#L34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We register (and document) all the annotations that we've ever used in official released code. Even if using them returns a warning, they should still be registered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(if we don't do that, we won't resolve issue #36830)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on feedback so far on the PR.
/lgtm
LGTM label has been added. Git tree hash: 1cc0fe5e5c9acac597bc026a2342daf6f2804ab5
|
/remove-lifecycle stale |
@robscott : Please would you be able to provide feedback from a technical POV and see if this is all okay to merge? |
@sergeyshevch While we wait for another tech review to make sure we can move forward, would you mind pushing an empty commit due to the checks failing? This should help kickstart them so we can eventually merge – thanks! |
@natalisucks Sure. Will make it tomorrow |
As I commented above, I'd personally rather document the current annotation instead of (or at least before) the deprecated annotation since I think this could lead to confusion. This does look like correct documentation for the deprecated annotation, so LGTM for sig-network. |
The best time to document annotations is before / as we start to use them. The second best time - in my view - is now. Please do feel free to document the missing newer annotation. PRs to fix gaps like that are always welcome. @sergeyshevch please try rebasing this against main from upstream. Your base branch is fairly stale (and Netlify doesn't automate testing against a simulated merge). |
Co-authored-by: Tim Bannister <[email protected]>
Co-authored-by: Tim Bannister <[email protected]>
Co-authored-by: Tim Bannister <[email protected]>
Co-authored-by: Tim Bannister <[email protected]>
Co-authored-by: Tim Bannister <[email protected]>
87f4d88
to
ca5dc47
Compare
Rebased on actual main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
/lgtm
/approve
LGTM label has been added. Git tree hash: 4549763100282e1d410633d5c9bfe5c5af097719
|
[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 |
Fixes: #36830