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

sockops: Remove duplicate error logging #16417

Merged
merged 1 commit into from Jun 4, 2021
Merged

sockops: Remove duplicate error logging #16417

merged 1 commit into from Jun 4, 2021

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Jun 3, 2021

If the compilation or loading of the sock_ops programs fail, we log the error twice, first in the SockmapEnable and SkmsgEnable functions, then in the calling function. This commit keeps only the error logging in Daemon.init():

cilium/daemon/cmd/daemon.go

Lines 239 to 242 in 7a45cd4

if err := sockops.SockmapEnable(); err != nil {
log.WithError(err).Error("Failed to enable Sockmap")
} else if err := sockops.SkmsgEnable(); err != nil {
log.WithError(err).Error("Failed to enable Sockmsg")

@pchaigno pchaigno added sig/loader Impacts the loading of BPF programs into the kernel. area/sockops release-note/misc This PR makes changes that have no direct user impact. labels Jun 3, 2021
@pchaigno pchaigno requested a review from a team as a code owner June 3, 2021 09:07
@pchaigno
sockops: Remove duplicate error logging
cf910c3 
If the compilation or loading of the sock_ops programs fail, we log the
error twice, first in the XXXEnable functions, then in the calling
function. This commit keeps only the error logging in Daemon.init().

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the remove-duplicate-error-logging branch from c9bab52 to cf910c3 Compare June 3, 2021 09:20
@pchaigno
Copy link
Member Author

pchaigno commented Jun 4, 2021 •
edited

k8s-1.21-kernel-4.9 is #13071. ConformanceAKS is just so flaky that it barely ever passes. Review is in. Marking as ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 4, 2021
@aanm aanm merged commit e7d39cf into cilium:master Jun 4, 2021
@pchaigno pchaigno deleted the remove-duplicate-error-logging branch June 4, 2021 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

Assignees
No one assigned
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/loader Impacts the loading of BPF programs into the kernel.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pchaigno @aanm