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

pkg/k8s: add pod IP event change #16190

Merged
merged 1 commit into from Jun 2, 2021
Merged

pkg/k8s: add pod IP event change #16190

merged 1 commit into from Jun 2, 2021

Conversation

aanm
Copy link
Member

@aanm aanm commented May 17, 2021

This is a follow up of 6bd98ad ("handle IP addresses modification in running nodes and CEPs")
for more information read the commit description of that commit.

Signed-off-by: André Martins andre@cilium.io

Accidentally fixes #13136

@aanm aanm added release-note/misc This PR makes changes that have no direct user impact. needs-backport/1.8 labels May 17, 2021
@aanm aanm requested review from aditighag, a team and errordeveloper May 17, 2021 22:13
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.8 May 17, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.11 May 17, 2021
@aanm
Copy link
Member Author

aanm commented May 17, 2021

test-me-please

@aanm
Copy link
Member Author

aanm commented May 17, 2021

test-me-please

aditighag
aditighag requested changes May 21, 2021
View reviewed changes
pkg/k8s/watchers/pod.go Outdated Show resolved Hide resolved
pkg/k8s/watchers/pod.go Show resolved Hide resolved
pkg/k8s/watchers/pod.go Show resolved Hide resolved
@aanm
Copy link
Member Author

aanm commented May 25, 2021

test-me-please

@aanm aanm requested a review from aditighag May 25, 2021 00:33
aditighag
aditighag approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

aditighag
aditighag reviewed May 25, 2021
View reviewed changes
Copy link
Member

@aditighag aditighag left a comment •
edited

Choose a reason for hiding this comment

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

The PR changes look good to me, but I don't understand how the PR is fixing the tagged issue. PTAL. (I'm not selecting "request changes"). The PR does fix the tagged issue, edited my previous comment. 🚀

pkg/k8s/watchers/pod.go Show resolved Hide resolved
aditighag
aditighag requested changes May 25, 2021
View reviewed changes
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

The CI test failure might be related. I can take a look tomorrow.

pkg/k8s/watchers/pod.go Outdated
@@ -584,6 +600,12 @@ func (k *K8sWatcher) upsertHostPortMapping(oldPod, newPod *slim_corev1.Pod, oldP
if !option.Config.EnableHostPort {
return nil
}

// no-op if specs are the same
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved to updatePodHostData before the defer function that does diff, and later upsertHostPortMapping is called. 1) The check applies to regular pods as well (not just for hostPort pods) 2) The diff is unnecessary if pod spec data isn't changed regardless.

@aditighag
Copy link
Member

@aanm I took a look at the gke failure. I don't see a hostPort service entry being created, as a result, the test fails. This is the code relevant to this PR - https://github.com/cilium/cilium/blob/master/pkg/k8s/watchers/pod.go#L641.

Also, I see a few occurrences of this error log - "Unable to update ipcache map entry on pod update". AFAIU, this is supposed to be a red herring, but with this change we'll return an error, skipping processing of labels/annotations, etc.

@aanm aanm added this to Needs backport from master in 1.9.9 May 27, 2021
@aanm aanm removed this from Needs backport from master in 1.9.8 May 27, 2021
@aanm aanm force-pushed the pr/event-handle-pod branch 2 times, most recently from dd33215 to 53a5eb3 Compare May 28, 2021 22:11
@aanm aanm requested a review from a team as a code owner May 28, 2021 22:11
@aanm aanm requested review from aditighag and removed request for a team May 28, 2021 22:11
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Needs backport from master in 1.9.9 Jun 22, 2021
@pchaigno
Copy link
Member

FYI, I removed this PR from the v1.9 backport PR because it was causing CI failures.

@tklauser tklauser mentioned this pull request Jun 25, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.11 Jun 25, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Needs backport from master in 1.8.11 Jun 25, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Needs backport from master in 1.8.11 Jun 25, 2021
@twpayne twpayne mentioned this pull request Jul 5, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.9 Jul 5, 2021
@aanm aanm mentioned this pull request Jul 5, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.11 Jul 6, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.11 Jul 6, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.11 Jul 6, 2021
@joestringer joestringer added this to Backport pending to v1.9 in 1.9.10 Jul 19, 2021
@joestringer joestringer removed this from Backport pending to v1.9 in 1.9.9 Jul 19, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport done to v1.9 in 1.9.9 Jul 19, 2021
@joestringer joestringer removed this from Backport pending to v1.9 in 1.9.10 Jul 19, 2021
This was referenced Jul 19, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@errordeveloper errordeveloper errordeveloper left review comments

@aditighag aditighag aditighag approved these changes

Assignees

@errordeveloper errordeveloper

Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.11
Backport done to v1.8
1.9.9
Backport done to v1.9
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Review duplicate processing in pod watcher callbacks that can cause increased resources utilization
10 participants
@aanm @aditighag @pchaigno @errordeveloper @twpayne @gandro @nathanjsweet @tklauser @joamaki @joestringer