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

Jump to bottom

Always update pod IP in case of hostNetworking #93898

Closed
wants to merge 1 commit into from
Closed

Always update pod IP in case of hostNetworking #93898

wants to merge 1 commit into from

Conversation

pupapaik
Copy link

@pupapaik pupapaik commented Aug 11, 2020 •
edited

/kind bug

What type of PR is this?

It is bug fix.

What this PR does / why we need it:

It is need to fix bug described bellow.

Which issue(s) this PR fixes:

Fixes #93897

Special notes for your reviewer:

I tested this on my setup where I was changing IP via DHCP server leases. It works fine now without any issues.

Does this PR introduce a user-facing change?:

NONE

NONE

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 11, 2020
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

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 added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 11, 2020
@pupapaik
Copy link
Author

I signed it

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note-none Denotes a PR that doesn't merit a release note. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 11, 2020
@pupapaik
Copy link
Author

/release-note-none

@pupapaik
Copy link
Author

/milestone v1.19

@k8s-ci-robot
Copy link
Contributor

@pupapaik: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.19

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.

@pupapaik
Copy link
Author

/assign @dchen1107

@pupapaik
Copy link
Author

@mtaufen and @Random-Liu can you please review?

@pupapaik pupapaik changed the title Update pod IP in case of hostNetworking Always update pod IP in case of hostNetworking Aug 12, 2020
@pupapaik
Copy link
Author

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-integration

@pupapaik pupapaik requested a review from knight42 August 12, 2020 18:02
@aojea
Copy link
Member

aojea commented Aug 12, 2020

/lgtm
/assign @vishh

@khenidak
Copy link
Contributor

@danwinship PodIPs are mutable. Here is why: Assume PodX assigned to nodeN. PodX starts on NodeN we get PodIPs and all well. Node restarts. Node needs to start every assigned pod from scratch (i.e., the entire sandbox creation dance). The entire representation of PodX on NodeN (i.e, sandbox Id, PodIPs, etc) changed but the pod object didn't change (recreated). Hence the mutability. Same scenario if pod was forcibly delete on the node via let us say containerd or kill command.

We should be able to merge this once it passes tests and gets reviewed.

@smarterclayton
Copy link
Contributor

We never fully specified the behavior of CNI plugins - whether they had to guarantee the pod IP remains the same when various bits of state were lost.

@knobunc tested this and we confirmed a number of plugins in the ecosystem do not preserve pod IP after reset of the node and loss of the container cache.

At this point, while preservation of IP is very desirable for consumers, without a strong statement in the early days (which we didn’t make or test) the current state is “podIP can change”.

At a minimum, other clients should be aware that any podIP can change at any time and we should test this. We may wish to offer guidance and tests for CNI plugins to encourage certain behaviors. But we can’t assume they are immutable in practice nor enforce that without breaking existing plugins.

@smarterclayton
Copy link
Contributor

smarterclayton commented Sep 15, 2021 •
edited

(note i’m making a general statement about podIP changing, pod IP on host network pods is one of the most likely to change, but any pods IP can change over its lifetime)

@khenidak khenidak mentioned this pull request Sep 30, 2021
Closed
@aojea
Copy link
Member

aojea commented Sep 30, 2021 •
edited

Main concerns is because it changes the current behavior, and since is not easy to predict all the consequences of this change, we can hide it behind a FeatureGate, so we can have an easy roll back if the result is not the expected.

@khenidak khenidak mentioned this pull request Oct 1, 2021
Closed
@aojea aojea mentioned this pull request Oct 4, 2021
Closed
4 tasks
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 29, 2021
@tomkukral
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 4, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pupapaik
To complete the pull request process, please ask for approval from dchen1107 after the PR has been reviewed.

The full list of commands accepted by this bot can be found 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
Copy link
Contributor

k8s-ci-robot commented Jan 5, 2022 •
edited

@pupapaik: The following tests 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-bazel-test daff9d3a0fd3056edff13a9b43a699152c716fe7 link /test pull-kubernetes-bazel-test
pull-kubernetes-unit 83d2f6a link true /test pull-kubernetes-unit
pull-kubernetes-e2e-kind 83d2f6a link true /test pull-kubernetes-e2e-kind

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
Copy link
Contributor

@pupapaik: 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 k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2022
@aojea aojea mentioned this pull request Feb 22, 2022
Closed
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 6, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 5, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

SIG Node PR Triage automation moved this from Waiting on Author to Done Jul 5, 2022
@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

@aojea aojea mentioned this pull request Jun 26, 2023
Merged
8 tasks
@aojea aojea mentioned this pull request Sep 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtaufen mtaufen Awaiting requested review from mtaufen

@Random-Liu Random-Liu Awaiting requested review from Random-Liu

@knight42 knight42 Awaiting requested review from knight42

Assignees

@sjenning sjenning

@vishh vishh

@dchen1107 dchen1107

Labels
area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
SIG Node PR Triage
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pod IP in hostNetwork mode does not update when node IP changes