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 a metric for max observed endpoint ifindex #27953

Merged
merged 4 commits into from Sep 12, 2023

Conversation

asauber
Copy link
Member

@asauber asauber commented Sep 5, 2023

Adds a metric endpoint_max_ifindex, which is the current maximum
interface index for all endpoints. The metric is updated during the
periodic invocation of syncHostIPs.

This metric can be used to determine if the interface index for the next
Pod will exceed Cilium's limit of max(uint16) on older kernels. On
kernels which do not provide ifindex via the FIB, Cilium must store the
ifindex in the CT map, and 16 bits are reserved per entry for this
purpose. This presents a limit of 65535 lifetime interfaces, which can
be approached quickly with sufficient pod churn.

Users who are subject to this limitation (typically a kernel version
less than 5.10), are advised to reboot or roll the host in this case.

Its default-enabled status is dynamic. On kernels which provide ifindex
via the FIB, the metric is disabled (since Cilium's ifindex limits are
not subject to max(uint16) in this case).

It can be enabled with the following Helm values:

prometheus:
  enabled: true
  metrics:
    - +cilium_endpoint_max_ifindex

On kernels which do not provide ifindex via the FIB, the metric is enabled by default.

Dynamic default based on Kernel version

On Linux 4.19 without metric explicitly enabled

$ cat contrib/testing/kind-values.yaml | grep -A 1 prom
prometheus:
  enabled: true

$ curl 2>/dev/null 172.18.0.4:9962/metrics | grep ifindex
# HELP cilium_endpoint_max_ifindex Maximum interface index observed for existing endpoints
# TYPE cilium_endpoint_max_ifindex gauge
cilium_endpoint_max_ifindex 7

$ uname -r
kind-4.19

On Linux 6.4 without metric explicitly enabled

$ cat contrib/testing/kind-values.yaml | grep -A 1 prom
prometheus:
  enabled: true
  
$ curl 2>/dev/null 172.19.0.3:9962/metrics | grep ifindex

$ 

On Linux 6.4 with metric explicitly enabled

$ cat contrib/testing/kind-values.yaml | grep -A 3 prom
prometheus:
  enabled: true
  metrics:
    - +cilium_endpoint_max_ifindex

$ curl 2>/dev/null 172.19.0.3:9962/metrics | grep ifindex
# HELP cilium_endpoint_max_ifindex Maximum interface index observed for existing endpoints
# TYPE cilium_endpoint_max_ifindex gauge
cilium_endpoint_max_ifindex 11

$ uname -r
6.4.12-arch1-1

Longer-running example

Roll a pod or two

$ k delete pod -n kube-system          coredns-5d78c9869d-5w8pc
pod "coredns-5d78c9869d-5w8pc" deleted

and the ifindex increases

$ curl 172.19.0.2:9962/metrics 2>/dev/null | grep ifindex
# HELP cilium_endpoint_max_ifindex Maximum interface index observed for existing endpoints
# TYPE cilium_endpoint_max_ifindex gauge
cilium_endpoint_max_ifindex 43

Addresses: #17259
Addresses: #16260

metrics: add a metric for max observed endpoint ifindex

@asauber asauber requested review from a team as code owners September 5, 2023 21:08
@maintainer-s-little-helper
Copy link

Commit 5ec7915 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 5, 2023
@asauber asauber force-pushed the pr/asauber/max-ifindex-metric branch from 76f5baf to 4cea910 Compare September 5, 2023 21:10
@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 Sep 5, 2023
@asauber asauber added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Sep 5, 2023
@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 Sep 5, 2023
@asauber
Copy link
Member Author

asauber commented Sep 5, 2023

The metric was tested on Linux 4.19 interactively using an lvh VM:
https://docs.cilium.io/en/stable/contributing/testing/e2e/#running-tests-in-vm

After bringing up the VM:

On the local machine

# build new container images for kind
make kind-image

# get our recently built cilium image onto the VM
docker save -o ciliumdev.tar localhost:5000/cilium/cilium-dev:local
scp -P 2224 ciliumdev.tar root@localhost:/host/cilium

# get our recently built cilium operator image onto the VM
docker save -o ciliumop.tar localhost:5000/cilium/operator-generic:local

# get cilium CLI on to the VM
scp -P 2224 $(which cilium) root@localhost:/host/ciliumcli

On the VM

alias cilium=/host/ciliumcli
alias k=kubectl

# load Cilium image
docker load -i ciliumdev.tar 
kind load docker-image localhost:5000/cilium/cilium-dev:local

# load Cilium operator image
docker load -i ciliumop.tar
kind load docker-image localhost:5000/cilium/operator-generic:local

cilium uninstall
cilium install \
	--chart-directory=/host/cilium/install/kubernetes/cilium \
	--helm-values=/host/cilium/contrib/testing/kind-values.yaml \
	--version=

k get pods -A -owide

@asauber
Copy link
Member Author

asauber commented Sep 5, 2023

Upon the first run of CI for this PR, an import cycle was detected. This commit fixes that import cycle, with details in the commit message:

9c4a431

@asauber
Copy link
Member Author

asauber commented Sep 5, 2023

/test

@asauber asauber force-pushed the pr/asauber/max-ifindex-metric branch from caf9516 to be5ce5e Compare September 6, 2023 00:22
@borkmann
Copy link
Member

borkmann commented Sep 6, 2023

@asauber could we add backport 1.14 label given the ifindex fix went also in there?

@asauber asauber added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Sep 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.2 Sep 6, 2023
@asauber
Copy link
Member Author

asauber commented Sep 6, 2023

@derailed yes, this can be used to implement an alert to determine if the cluster is nearing the max ifindex of 65535 (if on an affected kernel version)

The current suggested mitigation in that case is to roll or reboot the Node.

Copy link
Member

@christarazi christarazi 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 the PR!

A few comments below. One other thing is that I don't see any commit descriptions for the commits and all the context is instead in the PR. As a project, we are not opposed to a PR description, but we state in our contrib guide that PRs should contain commits with a description. Plus, this is beneficial when bisecting / git blame.

pkg/metrics/metrics.go Show resolved Hide resolved
Documentation/observability/metrics.rst Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Show resolved Hide resolved
@christarazi christarazi added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/daemon Impacts operation of the Cilium daemon. area/metrics Impacts statistics / metrics gathering, eg via Prometheus. labels Sep 7, 2023
For the sake of ease of development, we test in CI that the Ginkgo suite
found in /test can compile on multiple platforms, including darwin.

This test suite includes a test of the Hive framework, which initializes
an instance of the daemon Cell. This Cell requires the MetricsRegistry,
and so these tests depend on the MetricsRegistry. Some metrics may soon
depend upon Linux feature probes, and so make the Linux probes package
an indirect dependency of the Ginkgo tests.

In order to support this build dependency, we allow the probes package
to compile on darwin by adding stub values for netlink constants used by
the package, and a linux-specific declaration of the constants using the
netlink package.

Signed-off-by: Andrew Sauber <2046750+asauber@users.noreply.github.com>
To avoid a circular import, move testutils/ipam.go into its own package.

The circular import was observed when attempting to use datapath kernel
feature probes in the metrics package. It was affecting only the probe
package's tests.

import cycle:
github.com/cilium/cilium/pkg/datapath/linux/probes
    [github.com/cilium/cilium/pkg/datapath/linux/probes.test]
github.com/cilium/cilium/pkg/testutils
    [github.com/cilium/cilium/pkg/datapath/linux/probes.test]
github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2
    [github.com/cilium/cilium/pkg/datapath/linux/probes.test]
github.com/cilium/cilium/pkg/policy/api
    [github.com/cilium/cilium/pkg/datapath/linux/probes.test]
github.com/cilium/cilium/pkg/metrics
    [github.com/cilium/cilium/pkg/datapath/linux/probes.test]

Moving this package avoids the import cycle

Signed-off-by: Andrew Sauber <andrew.sauber@isovalent.com>
@asauber asauber force-pushed the pr/asauber/max-ifindex-metric branch from be5ce5e to 75ad939 Compare September 7, 2023 19:51
@asauber asauber requested a review from a team as a code owner September 7, 2023 19:51
@asauber asauber requested a review from rgo3 September 7, 2023 19:51
Add a metric for the current maximum observed interface index

Adds a metric `endpoint_max_ifindex`, which is the current maximum
interface index for all endpoints. The metric is updated during the
periodic invocation of `syncHostIPs`.

This metric can be used to determine if the interface index for the next
Pod will exceed Cilium's limit of max(uint16) on older kernels. On
kernels which do not provide ifindex via the FIB, Cilium must store the
ifindex in the CT map, and 16 bits are reserved per entry for this
purpose. This presents a limit of 65535 lifetime interfaces, which can
be approached quickly with sufficient pod churn.

Users who are subject to this limitation (typically a kernel version
less than 5.10), are advised to reboot or roll the host in this case.

Its default-enabled status is dynamic. On kernels which provide ifindex
via the FIB, the metric is disabled (since Cilium's ifindex limits are
not subject to max(uint16) in this case).

Addresses: cilium#17259
Addresses: cilium#16260

Signed-off-by: Andrew Sauber <2046750+asauber@users.noreply.github.com>
@asauber asauber force-pushed the pr/asauber/max-ifindex-metric branch from 75ad939 to daae407 Compare September 7, 2023 20:38
Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

Looks great on docs! Thank you for adding the description as to the dynamic default status, awesome stuff.

@michi-covalent michi-covalent added this to Needs backport from main in 1.14.3 Sep 9, 2023
@michi-covalent michi-covalent removed this from Needs backport from main in 1.14.2 Sep 9, 2023
Signed-off-by: Andrew Sauber <andrew.sauber@isovalent.com>
@asauber
Copy link
Member Author

asauber commented Sep 11, 2023

/test

@michi-covalent michi-covalent merged commit d9c9337 into cilium:main Sep 12, 2023
61 checks passed
@giorio94 giorio94 mentioned this pull request Sep 26, 2023
22 tasks
@giorio94 giorio94 added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Sep 26, 2023
@aanm aanm added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Sep 29, 2023
@jrajahalme jrajahalme moved this from Needs backport from main to Backport done to v1.14 in 1.14.3 Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon Impacts operation of the Cilium daemon. area/metrics Impacts statistics / metrics gathering, eg via Prometheus. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.14.3
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

None yet