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

nat: Do not increment delete error metric on nat entry GC #15587

Merged
merged 1 commit into from
Apr 13, 2021
Merged

nat: Do not increment delete error metric on nat entry GC #15587

merged 1 commit into from
Apr 13, 2021

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Apr 7, 2021 •
edited

This fixes the spurious increments of the snat_v[46]_external delete
fail metric by extending bpf.Map with SilentDelete() which allows
deleting map entries that may not exist without error counter increments
or deferred repeated delete via resolveErrors().

Fixes: #11485

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 7, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Apr 7, 2021
@joamaki joamaki added kind/bug This is a bug in the Cilium logic. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Apr 7, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 7, 2021
joamaki
joamaki commented Apr 7, 2021
View reviewed changes
pkg/bpf/map_linux.go Outdated Show resolved Hide resolved
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 7, 2021
@joamaki joamaki force-pushed the pr/joamaki/no-inc-error-metric-on-ct-gc branch from 65fef90 to d347507 Compare April 7, 2021 14:17
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 7, 2021
@joamaki
Copy link
Contributor Author

joamaki commented Apr 7, 2021 •
edited

Please review that only the relevant call sites to Map.Delete have been changed to Map.SilentDelete.

@joamaki joamaki force-pushed the pr/joamaki/no-inc-error-metric-on-ct-gc branch from d347507 to 1860ab2 Compare April 7, 2021 14:56
brb
brb reviewed Apr 8, 2021
View reviewed changes
pkg/maps/ctmap/ctmap.go Outdated Show resolved Hide resolved
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 8, 2021
@joamaki joamaki force-pushed the pr/joamaki/no-inc-error-metric-on-ct-gc branch from 99fbbdb to e903e2a Compare April 8, 2021 09:55
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 8, 2021
@joamaki
Copy link
Contributor Author

joamaki commented Apr 8, 2021

Tested this locally and with e903e2a the increments of failed snat_v4_external are now gone.

@joamaki joamaki force-pushed the pr/joamaki/no-inc-error-metric-on-ct-gc branch 2 times, most recently from cff0584 to 2516d94 Compare April 8, 2021 11:54
@joamaki joamaki marked this pull request as ready for review April 8, 2021 12:22
@joamaki joamaki requested review from a team and jrfastab April 8, 2021 12:22
@joamaki joamaki force-pushed the pr/joamaki/no-inc-error-metric-on-ct-gc branch from 2516d94 to 19b479a Compare April 8, 2021 13:57
@joamaki
Copy link
Contributor Author

joamaki commented Apr 8, 2021

test-me-please

@joamaki joamaki force-pushed the pr/joamaki/no-inc-error-metric-on-ct-gc branch from 19b479a to 88c93db Compare April 8, 2021 15:22
brb
brb reviewed Apr 9, 2021
View reviewed changes
pkg/maps/nat/nat.go Outdated Show resolved Hide resolved
@joamaki joamaki force-pushed the pr/joamaki/no-inc-error-metric-on-ct-gc branch from 88c93db to 767f9e8 Compare April 12, 2021 08:07
@joamaki
Do not increment delete error metric on delete of non-existing nat entry
cb2f4f4 
This fixes the spurious increments of the snat_v[46]_external delete
fail metric by extending bpf.Map with SilentDelete() which allows
deleting map entries that may not exist without error counter increments
or deferred repeated delete via resolveErrors().

Fixes: cilium#11485

Signed-off-by: Jussi Maki <jussi@isovalent.com>
brb
brb approved these changes Apr 12, 2021
View reviewed changes
Copy link
Member

@brb brb 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 🚀

@brb
Copy link
Member

brb commented Apr 12, 2021

There seems to be a legit Go linter issue: https://github.com/cilium/cilium/pull/15587/checks?check_run_id=2321807972

@joamaki
Copy link
Contributor Author

joamaki commented Apr 12, 2021

There seems to be a legit Go linter issue: https://github.com/cilium/cilium/pull/15587/checks?check_run_id=2321807972

Finally set up my editor to gofmt on save so hopefully won't be hitting this again. Will push a fix in a minute.

@joamaki joamaki force-pushed the pr/joamaki/no-inc-error-metric-on-ct-gc branch from 767f9e8 to cb2f4f4 Compare April 12, 2021 09:08
@pchaigno
Copy link
Member

test-me-please

@joamaki joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 13, 2021
qmonnet
qmonnet approved these changes Apr 13, 2021
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@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 Apr 13, 2021
@qmonnet qmonnet merged commit 4b4a74b into cilium:master Apr 13, 2021
1.10.0 automation moved this from In progress to Done Apr 13, 2021
This was referenced Apr 28, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb approved these changes

@qmonnet qmonnet qmonnet approved these changes

@jrfastab jrfastab Awaiting requested review from jrfastab

Assignees

@jrfastab jrfastab

Labels
kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nat: Do not inc error metric from CT GC if related NAT entry not found
5 participants
@joamaki @brb @pchaigno @qmonnet @jrfastab