Skip to content

Add type label to the identity metric #19999

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
Merged
merged 2 commits into from
Jun 3, 2022
Merged

Add type label to the identity metric #19999

merged 2 commits into from
Jun 3, 2022

Conversation

ungureanuvladvictor
Copy link
Member

Signed-off-by: Vlad Ungureanu ungureanuvladvictor@gmail.com

@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 May 30, 2022
@ungureanuvladvictor ungureanuvladvictor added area/metrics Impacts statistics / metrics gathering, eg via Prometheus. release-note/misc This PR makes changes that have no direct user impact. labels May 30, 2022
@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 May 30, 2022
@ungureanuvladvictor
Copy link
Member Author

/test

joestringer
joestringer reviewed May 31, 2022
View reviewed changes
joestringer
joestringer reviewed May 31, 2022
View reviewed changes
daemon/cmd/daemon.go Outdated
@@ -475,12 +475,12 @@ func NewDaemon(ctx context.Context, cancel context.CancelFunc, epMgr *endpointma
}

identity.IterateReservedIdentities(func(_ identity.NumericIdentity, _ *identity.Identity) {
metrics.Identity.Inc()
metrics.Identity.WithLabelValues("reserved").Inc()
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if these metrics will double-count vs. the AllocateIdentity() defer()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so -- in my testing I did not see reserved identities being double counted (installed prometheus + grafana and plotted things). I'll try to look in the code to see the code path of AllocateIdentity if it gets called for reserved ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looked at the code and I was able to trace it to https://github.com/cilium/cilium/blob/master/pkg/identity/reserved.go#L49-L51. Given this I don't think they get allocated via the normal AllocateIdentity() method.

@ungureanuvladvictor ungureanuvladvictor force-pushed the vladu/identity-type-metrics branch from 500e932 to 34af8ca Compare May 31, 2022 21:26
@ungureanuvladvictor
Copy link
Member Author

/test

@ungureanuvladvictor ungureanuvladvictor force-pushed the vladu/identity-type-metrics branch from 34af8ca to d58a585 Compare May 31, 2022 21:58
@ungureanuvladvictor
Add type label to the identity metric
467ace6 
Signed-off-by: Vlad Ungureanu <ungureanuvladvictor@gmail.com>
@ungureanuvladvictor
Add constants for identity types
60579d3 
Signed-off-by: Vlad Ungureanu <ungureanuvladvictor@gmail.com>
@ungureanuvladvictor ungureanuvladvictor force-pushed the vladu/identity-type-metrics branch from d58a585 to 60579d3 Compare May 31, 2022 22:06
@joestringer joestringer added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels May 31, 2022
@ungureanuvladvictor ungureanuvladvictor marked this pull request as ready for review May 31, 2022 22:11
@ungureanuvladvictor ungureanuvladvictor requested a review from a team as a code owner May 31, 2022 22:11
@ungureanuvladvictor ungureanuvladvictor requested a review from a team May 31, 2022 22:11
@ungureanuvladvictor ungureanuvladvictor requested a review from a team as a code owner May 31, 2022 22:11
joestringer
joestringer approved these changes May 31, 2022
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

This makes me wonder about the "multi_cluster" identities and how we might understand their effect on the system. But these LGTM.

@joestringer
Copy link
Member

Added release-note/minor since it is potentially user-facing (could impact user dashboards for instance).

@ungureanuvladvictor
Copy link
Member Author

ungureanuvladvictor commented May 31, 2022 •
edited by maintainer-s-little-helper bot
Loading

/test

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed:

Click to show.

Test Name

K8sEgressGatewayTest tunnel vxlan with endpointRoutes disabled no egress gw policy connectivity works

Failure Output

FAIL: Expected command: kubectl exec -n kube-system log-gatherer-tjr7j -- curl --path-as-is -s -D /dev/stderr --fail --connect-timeout 5 --max-time 20 http://192.168.56.11:20080 -w "time-> DNS: '%{time_namelookup}(%{remote_ip})', Connect: '%{time_connect}',Transfer '%{time_starttransfer}', total '%{time_total}'"

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create one.

@joestringer
Copy link
Member

ConformanceKind1.19 failure is #20034

@ungureanuvladvictor
Copy link
Member Author

ungureanuvladvictor commented Jun 1, 2022 •
edited by maintainer-s-little-helper bot
Loading

/test-1.23-net-next

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed:

Click to show.

Test Name

K8sEgressGatewayTest tunnel disabled with endpointRoutes enabled no egress gw policy connectivity works

Failure Output

FAIL: Expected command: kubectl exec -n kube-system log-gatherer-2dzs7 -- curl --path-as-is -s -D /dev/stderr --fail --connect-timeout 5 --max-time 20 http://192.168.56.11:20080 -w "time-> DNS: '%{time_namelookup}(%{remote_ip})', Connect: '%{time_connect}',Transfer '%{time_starttransfer}', total '%{time_total}'"

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create one.

@ungureanuvladvictor
Copy link
Member Author

ci-awscni is disabled in GH actions and this is why it's pending here.

ldelossa
ldelossa approved these changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

LGTM

sayboras
sayboras approved these changes Jun 2, 2022
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@ungureanuvladvictor
Copy link
Member Author

/test-1.23-net-next

@joestringer
Copy link
Member

All reviews are in, only ConformanceKind was failing but that was due to a breakage in the tree. LGTM, merging.

@joestringer joestringer merged commit aa7572b into cilium:master Jun 3, 2022
@ungureanuvladvictor ungureanuvladvictor deleted the vladu/identity-type-metrics branch June 3, 2022 15:52
@joamaki joamaki mentioned this pull request Jun 7, 2022
Merged
@joamaki joamaki mentioned this pull request Jun 8, 2022
Merged
@joestringer joestringer added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Jun 9, 2022
This was referenced Jun 10, 2022
Merged
Merged
@aanm aanm mentioned this pull request Jun 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@ldelossa ldelossa ldelossa approved these changes

@sayboras sayboras sayboras approved these changes

@qmonnet qmonnet qmonnet approved these changes

@jrajahalme jrajahalme Awaiting requested review from jrajahalme

Assignees
No one assigned
Labels
area/metrics Impacts statistics / metrics gathering, eg via Prometheus. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ungureanuvladvictor @joestringer @ldelossa @sayboras @qmonnet @joamaki