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.

Sign up for GitHub

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

Jump to bottom

Endpoints and EndpointSlices should not publish IPs for terminal pods #110255

Merged
merged 6 commits into from May 27, 2022
Merged

Endpoints and EndpointSlices should not publish IPs for terminal pods #110255

merged 6 commits into from May 27, 2022

Conversation

robscott
Copy link
Member

What type of PR is this?

/kind bug

What this PR does / why we need it:

Since 1.22 the pod phase lifecycle guarantees that terminal pods, those whose states are Unready or Succeeded , can not regress and will have all container stopped. Hence, terminal PodIPs will never been able to be reachable and should not be published on the endpoints or endpoints slices, independently of the TolerateUnready option

Which issue(s) this PR fixes:

Fixes: #109414, #109718

Special notes for your reviewer:

This is identical to @aojea's earlier PR (#110115) with one additional commit tacked on to resolve the final bits of feedback. I'm fine to just incorporate these into the original PR if that's easier, just want to get this fix in ASAP.

Does this PR introduce a user-facing change?

The pod phase lifecycle guarantees that terminal Pods, those whose states are Unready or Succeeded, can not regress and will have all container stopped. Hence, terminal Pods will never be reachable and should not publish their IP addresses on the Endpoints or EndpointSlices, independently of the Service TolerateUnready option.

/sig network
/priority important-soon
/cc @aojea @bowei @swetharepakula
/assign @smarterclayton @thockin

aojea and others added 6 commits May 27, 2022 06:42
@aojea
add pod util to verify pod is terminal
d16d23e 
pods on phase succeeded or failed are guaranteed to have all containers
stopped and to not ever regress
@aojea
endpointslices: terminal pods doesn't receive enpoints
b905c28
@aojea
endpoints controller: don't consider terminal endpoints
aa35f6f 
Terminal pods, whose phase its Failed or Succeeded, are guaranteed
to never regress and to be stopped, so their IPs never should
be published on the Endpoints.
@aojea
e2e test for evicted pods
ffdbce6
@aojea
e2e: services with evicted pods doesn't have endpoints
3a8edca
@robscott
Updating e2e test to check EndpointSlices and Endpoints as well
aa02b7a
@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 27, 2022
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. labels May 27, 2022
@k8s-ci-robot k8s-ci-robot requested review from aojea and bowei May 27, 2022 21:16
@k8s-ci-robot k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label May 27, 2022
@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 27, 2022
@k8s-ci-robot
Copy link
Contributor

@robscott: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. area/kubelet area/test sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels May 27, 2022
@thockin
Copy link
Member

thockin commented May 27, 2022

Thanks - is it important to merge this so you can start cherrypicks? Or should we just wait for Antonio to update?

@robscott
Copy link
Member Author

@thockin I'd been hoping to get cherry picks going today if possible, but this can wait until next week if needed.

@thockin
Copy link
Member

thockin commented May 27, 2022

Thanks!

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, thockin

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 May 27, 2022
@aojea
Copy link
Member

aojea commented May 27, 2022

/lgtm

@aojea aojea mentioned this pull request May 27, 2022
Closed
@k8s-ci-robot
Copy link
Contributor

@robscott: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-e2e-ubuntu-gce-network-policies aa02b7a link false /test pull-kubernetes-e2e-ubuntu-gce-network-policies

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot merged commit 8b6dd06 into kubernetes:master May 27, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone May 27, 2022
This was referenced May 27, 2022
Merged
Merged
Merged
@saschagrunert saschagrunert mentioned this pull request Jun 1, 2022
Closed
This was referenced Jun 1, 2022
Closed
Closed
k8s-ci-robot added a commit that referenced this pull request Jun 3, 2022
@k8s-ci-robot
Merge pull request #110260 from robscott/automated-cherry-pick-of-#11…
0d99df5 
…0255-upstream-release-1.22

Automated cherry pick of #110255: Endpoints and EndpointSlices should not publish IPs for terminal pods
k8s-ci-robot added a commit that referenced this pull request Jun 3, 2022
@k8s-ci-robot
Merge pull request #110259 from robscott/automated-cherry-pick-of-#11…
375a755 
…0255-upstream-release-1.23

Automated cherry pick of #110255: Endpoints and EndpointSlices should not publish IPs for terminal pods
k8s-ci-robot added a commit that referenced this pull request Jun 3, 2022
@k8s-ci-robot
Merge pull request #110258 from robscott/automated-cherry-pick-of-#11…
a6b031e 
…0255-upstream-release-1.24

Automated cherry pick of #110255: Endpoints and EndpointSlices should not publish IPs for terminal pods
@thockin thockin mentioned this pull request Jun 8, 2022
Merged
@robscott robscott mentioned this pull request Jun 14, 2022
Closed
@pio-janik pio-janik mentioned this pull request Jun 17, 2022
Closed
@navidnadali navidnadali mentioned this pull request Jul 18, 2022
Open
@bbreijer bbreijer mentioned this pull request Sep 8, 2022
Open
pacoxu added a commit to pacoxu/kubernetes that referenced this pull request Oct 24, 2022
@pacoxu
update release-note for cherry-picks of kubernetes#110255
214a293
@pacoxu pacoxu mentioned this pull request Oct 24, 2022
Merged
k8s-ci-robot added a commit that referenced this pull request Dec 13, 2022
@k8s-ci-robot
Merge pull request #113285 from pacoxu/release-note-update
10376ad 
update release-note for cherry-picks of #110255
@aojea aojea mentioned this pull request Dec 20, 2022
Closed
ivelichkovich pushed a commit to ivelichkovich/kubernetes that referenced this pull request Dec 20, 2022
@pacoxu @ivelichkovich
update release-note for cherry-picks of kubernetes#110255
091f994
jaehnri pushed a commit to jaehnri/kubernetes that referenced this pull request Jan 3, 2023
@pacoxu @jaehnri
update release-note for cherry-picks of kubernetes#110255
eeea972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea Awaiting requested review from aojea

@bowei bowei Awaiting requested review from bowei

@swetharepakula swetharepakula Awaiting requested review from swetharepakula

Assignees

@smarterclayton smarterclayton

@thockin thockin

@aojea aojea

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

Pods with failed status IP address reused on new pods, but traffic still going to old pods across namespaces.
5 participants
@robscott @k8s-ci-robot @thockin @aojea @smarterclayton