Observe .spec.publishNotReadyAddresses. #61056
Conversation
k8s-ci-robot
added
release-note
|
Hi @bmcstdio. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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. |
k8s-ci-robot
added
needs-ok-to-test
|
/kind bug |
k8s-ci-robot
added
kind/bug
|
Thanks for looking into this @bmcstdio 😃 |
|
/assign @mikedanese |
| tolerateUnreadyEndpoints = b | ||
| } else { | ||
| utilruntime.HandleError(fmt.Errorf("Failed to parse annotation %v: %v", TolerateUnreadyEndpointsAnnotation, err)) | ||
| if service.Spec.PublishNotReadyAddresses == true { |
There was a problem hiding this comment.
This has to be the other way - the annotation has to be preferred if it exists. Otherwise it is not fully backwards compatible
There was a problem hiding this comment.
@thockin thanks! I have amended it.
Signed-off-by: Bruno Miguel Custodio <[email protected]>
bmcustodio
force-pushed
the
bcustodio/publish-not-ready-addresses
branch
from
e1069e3 to
4157c24
Compare
|
LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bmcstdio, mikedanese 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 |
k8s-ci-robot
added
the
approved
|
/hold I believe @kow3ns considered this approach (make the field mean exactly the same thing as the original annotation) and decided not to do it because the way the original annotation works is hacky (conflates endpoint readiness and endpoint publishing). The discussion and new plan are in #49239. To summarize: The annotation works by lying about Unready endpoints, telling downstream consumers that they are Ready. The proposed solution for the new field is to tell downstream consumers which endpoints are Ready, which endpoints are Unready, and whether we want to publish the Unready ones anyway (hence the name Unfortunately, the process stalled because the next step is to update downstream components that don't live in this repo (kube-dns, coredns?). I'd love to see progress on this issue, but I don't think this PR is the way we want to go unless the consensus on #49239 is reversed. |
k8s-ci-robot
added
the
do-not-merge/hold
|
@enisoc I'm still a bit ignorant to most things Kubernetes & just trying to follow along, so excuse the possibly silly question: should users expect different semantics between what's implemented in the annotation & what's planned for publishNotReadyAddresses? If so, what might those differences be? |
|
@thomaslee If all you care about is having a headless service that publishes a DNS entry for each endpoint regardless of Readiness, I don't think you would notice a difference between the current annotation's behavior and the plan for the |
|
@enisoc great, glad to hear it -- thank you! |
|
@bmcstdio: PR needs rebase. 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. |
k8s-ci-robot
added
the
needs-rebase
|
@bmcstdio Thanks for your work. Closing. /close |
What this PR does / why we need it:
This PR makes a service's
.spec.publishNotReadyAddressesfield be observed when set totrue. If set tofalseor not set, the value toservice.alpha.kubernetes.io/tolerate-unready-endpointsis used.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #58662.
Special notes for your reviewer:
I am not sure what the best way to test this would be. Would appreciate some guidance.
Release note: