Change metrics store to use sync.Map #1028
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Welcome @zhengl7! |
k8s-ci-robot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zhengl7 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Results of running |
The cache map for metrics cannot be concurrently accessed in the current version. This caused a problem that heavy metrics writing to the map will cause a contention with reading, and can potentially cause Prometheus scrape call to timeout. Using sync.Map can remove the mutex.
zhengl7
force-pushed
the
syncmap_metrics_store
branch
from
620c817
to
61c94d9
Compare
k8s-ci-robot
removed
the
needs-rebase
|
Force pushing after rebasing |
|
/assign @lilic |
|
ok, tests failure, need to understand it better. Should I close the PR and come back later when it's ready? |
|
hmm those do seem like very huge performance degradation. 🤔 |
There are a few documented scenarios where `kube-state-metrics` will lock up(kubernetes#995, kubernetes#1028). I believe a much simpler solution to ensure `kube-state-metrics` doesn't lock up and require a restart to server `/metrics` requests is to add default read and write timeouts and to allow them to be configurable. At Grafana, we've experienced a few scenarios where `kube-state-metrics` running in larger clusters falls behind and starts getting scraped multiple times. When this occurs, `kube-state-metrics` becomes completely unresponsive and requires a reboot. This is somewhat easily reproduceable(I'll provide a script in an issue) and causes other critical workloads(KEDA, VPA) to fail in weird ways. Adds two flags: - `server-read-timeout` - `server-write-timeout` Updates the metrics http server to set the `ReadTimeout` and `WriteTimeout` to the configured values.
In busy/large clusters, will prevent timeouts from long living locks/concurrency issues, as the writing to the map takes overly long, blocking the metrics-reading thread and as the lock doesn't get released in a timely manner, timing out the request. Inpired by previous PR at kubernetes#1028
What this PR does / why we need it:
The cache map for metrics cannot be concurrently accessed in the current version, so there is a mutex for map reading/writing. This caused a problem that heavy metrics writing to the map will cause a contention with reading, and can potentially cause Prometheus scrape call to timeout. Using sync.Map can remove the need of the mutex.
The non-concurrent map implementation led to issues reported in #995, there is constant Prometheus scraping timeouts when the cluster reaches certain busyness of creating new pods. Applied a version of kube-state-metrics with this change to the same cluster, timeouts are completely gone.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #995