Consider rewriting scraper goroutine scheduling

[serathius]

Metrics Server scraper is responsible for creating paralleling metric collection. Every cycle it lists all the nodes in the cluster and creates goroutine to scrape each node. Each go-routine waits some random time (to avoid scrapping at the same time), collects metrics and then is removed. Problems with this approach:

• goroutine churn - each cycle allocates and deletes goroutines
• memory churn - all data needs to be copied into one batch before saving to storage
• bursty CPU and IO - scraping is executed only at the beginning of cycle
• freshness - saving metrics requires to wait for the slowest node

What would you like to be added:

Instead of listing nodes every cycle and churning goroutines, we should maintain a goroutine per node and use informer event handler to add/remove goroutines as nodes are updated. Similar approach is used by Prometheus Server.

Things to consider:

• Measuring performance improvement in scraper
• How storage would consume partial data, decide to remove old one and performance overhead caused by that
• How to test code and prevent leaking goroutines

Why is this needed:

I have listed some problems, but we should test it properly before committing to it. We should create benchmarks and validate scraper and storage performance before merging any code, as in worst case we could result in more complicated code without addressing any of the problems.

/cc @yangjunmyfm192085 @dgrisonnet

/kind feature

[yangjunmyfm192085]

Sounds good, We really need to consider and test properly before committing, and so when we plan to start it?
After #777 merged?

[serathius]

I don't think this is nesesery blocked by #777 as they are pretty independent changes. I think we can start to do performance testing.

[yangjunmyfm192085]

ok, Let me take a look, and try to do it?

[dgrisonnet]

How storage would consume partial data, decide to remove old one and performance overhead caused by that

I don't think this is possible with the current storage implementation.
To throw a suggestion to work around that, maybe we could have 1 store per node. I don't really see any drawbacks to that, except that getting PodMetrics will become more complex.

How to test code and prevent leaking goroutines

We could check for leaks with: https://github.com/uber-go/goleak
Also, I don't know if metrics-server registers the client_golang go collector, but it could give us insights regarding the number of goroutines that are currently running with the go_goroutines metrics.

[yangjunmyfm192085]

/assign

[yangjunmyfm192085]

I am working on this issue recently, I have a few questions

• Do we need to deal with scraper and storag both in the goroutine? or only scrape is ok?
• If deal with scrape and store both in the goroutine , do we need to modify the process of the store
• use informer event handler to add/remove goroutines, do we need to deal with update event?
  /cc @serathius @dgrisonnet

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[dgrisonnet]

/remove-lifecycle rotten
/lifecycle frozen

[serathius]

This should implement Redesign goroutine management to reduce scheduling cost (will require redesigning storage) point from #857

[jasonliu747]

/remove-lifecycle frozen
@yangjunmyfm192085 Hi, thanks for your contribution, any update on this issue?

[serathius]

/lifecycle frozen
/cc @shuaich

[yangjunmyfm192085]

Sorry, I delayed this issue due to lack of time and other things. I modified it at https://github.com/yangjunmyfm192085/metrics-server/tree/rewriting-scraper but not submit pr. I will submit the pr as soon as possible, and need help me to review whether the modification is reasonable. If there is no problem, I will do a comparison test @serathius.
Of course, @shuaich is also welcome to participate