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

Fix error propagation in (*K8sWatcher).addK8sPodV1 #14864

Merged
merged 2 commits into from
Feb 9, 2021
Merged

Fix error propagation in (*K8sWatcher).addK8sPodV1 #14864

merged 2 commits into from
Feb 9, 2021

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Feb 4, 2021

The first commit is a preparatory cleanup of an unused sentinel error var.

The second commit fixes the error propagation issue (as per the commit message):

In (*K8sWatcher).addK8sPodV1, the error return value of
(*K8sWatcher).updatePodHostData is ignored due to the fact that the bool
return value is always true if there was an error. This leads to
a debug message being printed rather than a warning. Also the
error is not propagated to callers of (*K8sWatcher).addK8sPodV1, which
in one case in (*K8sWatcher).createPodController leads to metrics not
being updated appropriately.

The bool return value of (*K8sWatcher).createPodController is anyway
redundant, so remove it. Instead, check the error explicitly at the call
site and log and propagate it accordingly.

@tklauser
k8s/watchers: remove errIPCacheOwnedByNonK8s
4ad3a63 
It is no longer returned anywhere since commit f6300d9 ("node:
Update ipcache entries independent of node update source").

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/misc This PR makes changes that have no direct user impact. labels Feb 4, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 4, 2021
@tklauser
Copy link
Member Author

tklauser commented Feb 4, 2021

test-me-please

@tklauser
Copy link
Member Author

tklauser commented Feb 5, 2021 •
edited

retest-net-next

known flake #12511 https://jenkins.cilium.io/job/Cilium-PR-K8s-1.13-net-next/681/

@tklauser tklauser marked this pull request as ready for review February 5, 2021 07:31
@tklauser tklauser requested review from a team and christarazi February 5, 2021 07:31
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 5, 2021
aanm
aanm requested changes Feb 5, 2021
View reviewed changes
pkg/k8s/watchers/pod.go Outdated Show resolved Hide resolved
pkg/k8s/watchers/pod.go Show resolved Hide resolved
@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 5, 2021
@tklauser tklauser force-pushed the pr/tklauser/k8s-watcher-simplify-updatepodhostdata branch from 0b4ebce to 556a148 Compare February 5, 2021 14:32
@tklauser
k8s/watchers: don't silently ignore (*K8sWatcher).updatePodHostData e…
c666e18 
…rror

In (*K8sWatcher).addK8sPodV1, the error return value of
(*K8sWatcher).updatePodHostData is ignored due to the fact that the bool
return value is always true if there was an error. This leads to
a debug message being printed rather than a warning. Also the
error is not propagated to callers of (*K8sWatcher).addK8sPodV1, which
in one case in (*K8sWatcher).createPodController leads to metrics not
being updated appropriately.

The bool return value of (*K8sWatcher).createPodController is anyway
redundant, so remove it. Instead, check the error explicitly at the call
site and log and propagate it accordingly.

Also move the host network check out of updatePodHostData as it is not
considered a warning and can be skipped. This avoids the unnecessary
podIPs lookup in that case.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser force-pushed the pr/tklauser/k8s-watcher-simplify-updatepodhostdata branch from 556a148 to c666e18 Compare February 5, 2021 14:35
@tklauser
Copy link
Member Author

tklauser commented Feb 5, 2021

test-me-please

@tklauser tklauser requested a review from aanm February 5, 2021 14:35
@tklauser
Copy link
Member Author

tklauser commented Feb 5, 2021 •
edited

retest-4.19

hit known flake #12511 https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Kernel/4650/

christarazi
christarazi reviewed Feb 5, 2021
View reviewed changes
pkg/k8s/watchers/pod.go
@@ -217,12 +217,15 @@ func (k *K8sWatcher) addK8sPodV1(pod *slim_corev1.Pod) error {
return k.deleteK8sPodV1(pod)
}

skipped := false
if pod.Spec.HostNetwork {
logger.Debug("pod is using host networking")
Copy link
Member

Choose a reason for hiding this comment

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

Now that this is a log msg, maybe we can elaborate a bit more on it? I think we should at the least conform to other log msgs so capitalize the first word

Copy link
Member

Choose a reason for hiding this comment

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

@tklauser if you get a chance, pls follow-up if needed once back

Copy link
Member Author

Choose a reason for hiding this comment

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

@christarazi as far as I can see, the logger already include necessary additional information as fields:

cilium/pkg/k8s/watchers/pod.go

Lines 202 to 208 in b875c1d

logger := log.WithFields(logrus.Fields{
logfields.K8sPodName: pod.ObjectMeta.Name,
logfields.K8sNamespace: pod.ObjectMeta.Namespace,
"podIP": pod.Status.PodIP,
"podIPs": pod.Status.PodIPs,
"hostIP": pod.Status.PodIP,
})

Or do you have any other info in mind that would be relevant for this case? Otherwise I'd just change the capitalization of the log message.

Copy link
Member

Choose a reason for hiding this comment

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

Capitalization is what I had in mind. If that's the only change you're going to make, then it's not worth making a PR for it; only if you have other changes already planned

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I have some other changes for the same package, so I can fold in the capitalization change.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 6, 2021
@borkmann borkmann merged commit da35c88 into master Feb 9, 2021
@borkmann borkmann deleted the pr/tklauser/k8s-watcher-simplify-updatepodhostdata branch February 9, 2021 13:35
@joestringer joestringer moved this from In progress to Done in 1.10.0 Feb 12, 2021
@joestringer joestringer moved this from In progress to Done in 1.10.0 Feb 12, 2021
tklauser added a commit to tklauser/cilium that referenced this pull request Feb 17, 2021
@tklauser
k8s/watchers: fix log message capitalization
a59514d 
Follow-up for
cilium#14864 (review)

Suggested-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser mentioned this pull request Feb 17, 2021
Merged
aditighag pushed a commit that referenced this pull request Feb 17, 2021
@tklauser @aditighag
k8s/watchers: fix log message capitalization
3a89fa4 
Follow-up for
#14864 (review)

Suggested-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
lyveng pushed a commit to lyveng/cilium that referenced this pull request Mar 4, 2021
@tklauser @lyveng
k8s/watchers: fix log message capitalization
de1fef6 
Follow-up for
cilium#14864 (review)

Suggested-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borkmann borkmann borkmann left review comments

@christarazi christarazi christarazi approved these changes

@aanm aanm aanm approved these changes

Assignees

@christarazi christarazi

@aanm aanm

Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tklauser @borkmann @christarazi @aanm