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

metrics: add cilium_datapath_nat_gc_entries #12832

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

ArthurChiao
Copy link
Contributor

@ArthurChiao ArthurChiao commented Aug 10, 2020

This patch adds NAT GC metrics to current metrics list. As NAT entries are
GC'd alongside CT entries and NAT module has no dedicated GC jobs, we
calculate the NAT metrics in CT module's GC job.

Signed-off-by: ArthurChiao arthurchiao@hotmail.com

@ArthurChiao ArthurChiao requested a review from a team August 10, 2020 07:54
@ArthurChiao ArthurChiao requested a review from a team as a code owner August 10, 2020 07:54
@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 Aug 10, 2020
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.009%) to 37.109% when pulling bd04add9130af51329efe46d78bfb6850041b6c7 on ctripcloud:add_nat_metrics_github into 5195789 on cilium:master.

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.

Thanks for your PR!

Unfortunately, iterating over all NAT entries just to update metrics counters each time we do GC of CT entries is too expensive operation. We might introduce such iteration in #12686. So, I suggest to wait until the latter has been done.

@ArthurChiao
Copy link
Contributor Author

ArthurChiao commented Aug 10, 2020

Thanks @brb , then should I close this PR right now?

@brb brb marked this pull request as draft August 10, 2020 09:38
@brb
Copy link
Member

brb commented Aug 10, 2020

Thanks @brb , then should I close this PR right now?

Let's keep it open. I've just changed its state to draft.

@stale
Copy link

stale bot commented Sep 10, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 10, 2020
@sayboras sayboras removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 11, 2020
@stale
Copy link

stale bot commented Oct 11, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 11, 2020
@aanm aanm added the needs/triage This issue requires triaging to establish severity and next steps. label Oct 13, 2020
@stale stale bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 13, 2020
@stale
Copy link

stale bot commented Nov 14, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Nov 14, 2020
@brb
Copy link
Member

brb commented Nov 16, 2020

@ArthurChiao #13912 got merged. Mind rebasing your PR and adding metrics?

@ArthurChiao
Copy link
Contributor Author

Thanks for reminding me @brb.

Yeah, we definitely need the metric for it. I will add it as a follow up.

I closed this PR as i saw the above message in #13912. So not sure if my PR is still needed?

@brb
Copy link
Member

brb commented Nov 16, 2020

@ArthurChiao Yeah, the metrics part is still needed (I've only added the logging).

@ArthurChiao
Copy link
Contributor Author

I'll have a look, thanks! @brb

…onstant

Signed-off-by: ArthurChiao <arthurchiao@hotmail.com>
@ArthurChiao ArthurChiao reopened this Nov 19, 2020
Signed-off-by: ArthurChiao <arthurchiao@hotmail.com>
@ArthurChiao
Copy link
Contributor Author

Hi @brb, rebased to master, please have a review, thanks!

Note: currently this metric could only be enabled/disabled along with cilium_datapath_conntrack_gc_entries,
seems there is no need to add a dedicated flag to control NAT gc metric on/off?

Test output:

# HELP cilium_datapath_nat_gc_entries The number of alive and deleted nat entries at the end of a garbage collector run labeled by datapath family.
# TYPE cilium_datapath_nat_gc_entries gauge
cilium_datapath_nat_gc_entries{direction="egress",family="ipv4",status="deleted"} 0
cilium_datapath_nat_gc_entries{direction="ingress",family="ipv4",status="alive"} 11800
cilium_datapath_nat_gc_entries{direction="ingress",family="ipv4",status="deleted"} 0

@stale stale bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Nov 19, 2020
@ArthurChiao ArthurChiao marked this pull request as ready for review November 19, 2020 14:38
@brb
Copy link
Member

brb commented Nov 19, 2020

test-me-please

@nathanjsweet nathanjsweet merged commit 57784e3 into cilium:master Nov 19, 2020
@ArthurChiao ArthurChiao deleted the add_nat_metrics_github branch November 20, 2020 01:08
This was referenced Nov 21, 2020
@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.1 Nov 21, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.12 Nov 23, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.1 Nov 23, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.1 Nov 23, 2020
@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.6 Nov 23, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.12 Nov 30, 2020
@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.6 Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.7.12
Backport done to v1.7
1.8.6
Backport done to v1.8
1.9.1
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

None yet

9 participants