rewriting scraper goroutine scheduling #972

yangjunmyfm192085 · 2022-02-25T06:30:56Z

What this PR does / why we need it:
Consider rewriting scraper goroutine scheduling , and 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.
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 #778

yangjunmyfm192085 · 2022-03-01T00:50:25Z

I did a comparative performance test for the modification of metrics-server.
envirnoment:

kind v1.23.0
6 workers, 1 control-plane(Limited by resources, create up to 6 workers)
1000 pods
running watch kubectl top pods --all-namespaces

results:
cpu(v0.6.1: 20mcore, after modification: 19mcore -- command kubectl top pod statistics result):
v0.6.1
pprof-cpu1.tar.gz

after modification
pprof-cpu2.tar.gz

memory(v0.6.1: 45Mi, after modification: 40Mi -- command kubectl top pod statistics result):
v0.6.1
pprof-heap1.tar.gz

after modification
pprof-heap2.tar.gz

The number of nodes may be small. From the test results, there is no obvious difference in CPU and memory usage.
/cc @serathius @dgrisonnet

serathius · 2022-03-01T09:50:38Z

pkg/scraper/scraper.go

-		nodeLister:    nodeLister,
-		kubeletClient: client,
-		scrapeTimeout: scrapeTimeout,
+func NewManageNodeScraper(client client.KubeletMetricsGetter, scrapeTimeout time.Duration, metricResolution time.Duration, store storage.Storage) *manageNodeScraper {


Rename to ManagedNodeScraper is not needed, I don't think we will want to have both implementations in the same codebase.

ok, I will update it.

serathius · 2022-03-01T09:55:07Z

pkg/scraper/scraper.go

+	m.nodeLock.Lock()
+	defer m.nodeLock.Unlock()
+	if working, exists := m.isWorking[node.UID]; exists && working {
+		klog.V(1).ErrorS(fmt.Errorf("Scrape in node is already running"), "", "node", klog.KObj(node))


You should not call verbose V(1) with ErrorS.

Let's change it to Info log.

serathius · 2022-03-01T09:57:35Z

pkg/scraper/scraper.go

+		ticker := time.NewTicker(m.metricResolution)
+		defer ticker.Stop()
+
+		res, _ := m.ScrapeData(node)


Why return error from ScrapeData if it's silenced everywhere?

I will update it.

serathius · 2022-03-01T10:00:03Z

pkg/scraper/scraper.go

 		}
+	}()
+	if _, exists := m.isWorking[node.UID]; !exists || !m.isWorking[node.UID] {


Suggested change

if _, exists := m.isWorking[node.UID]; !exists || !m.isWorking[node.UID] {

if !m.isWorking[node.UID] {

serathius · 2022-03-01T10:01:59Z

pkg/scraper/scraper.go

 	startTime := myClock.Now()
+	baseCtx, cancel := context.WithCancel(context.Background())


Why use WithCancel if cancel is used anywhere, also 2 lines below you use WithTimeout that also gives you cancel.

serathius · 2022-03-01T10:03:19Z

pkg/scraper/scraper.go

 		requestTotal.WithLabelValues("false").Inc()
 		return nil, err
 	}
 	requestTotal.WithLabelValues("true").Inc()
-	return ms, nil
+	res := &storage.MetricsBatch{


This merging is not needed at all.

serathius · 2022-03-01T10:04:22Z

pkg/scraper/scraper.go

+	klog.V(6).InfoS("Storing metrics from node", "node", klog.KObj(node))
+	m.storage.Store(res)
+}
+func (m *manageNodeScraper) UpdateNodeScraper(node *corev1.Node) error {


Not sure how this differs from AddNodeScraper

I have defined three functions AddNodeScraper/UpdateNodeScraper/DeleteNodeScraper based on three events in pkg/server/config.go, but I am not sure if we need to deal with it based on the update event. If not, it should be deleted. it is similar to AddNodeScraper

serathius · 2022-03-01T10:05:15Z

pkg/scraper/scraper.go

+func (m *manageNodeScraper) DeleteNodeScraper(node *corev1.Node) {
+	m.nodeLock.Lock()
+	defer m.nodeLock.Unlock()
+	if working, exists := m.isWorking[node.UID]; exists && working {


Suggested change

if working, exists := m.isWorking[node.UID]; exists && working {

if m.isWorking[node.UID] {

serathius · 2022-03-01T10:05:56Z

pkg/scraper/scraper.go

+		if _, exists := m.stop[node.UID]; exists {
+			m.stop[node.UID] <- struct{}{}


Suggested change

if _, exists := m.stop[node.UID]; exists {

m.stop[node.UID] <- struct{}{}

if stop, exists := m.stop[node.UID]; exists {

stop <- struct{}{}

serathius · 2022-03-01T10:10:03Z

pkg/storage/node.go

@@ -69,8 +72,8 @@ func (s *nodeStorage) GetMetrics(nodes ...*corev1.Node) ([]metrics.NodeMetrics,
 }

 func (s *nodeStorage) Store(batch *MetricsBatch) {
-	lastNodes := make(map[string]MetricsPoint, len(batch.Nodes))
-	prevNodes := make(map[string]MetricsPoint, len(batch.Nodes))
+	lastNodes := make(map[string]MetricsPoint, len(batch.Nodes)+len(s.last))


No need to allocate space for batch.Nodes as the size of data stored should stay around the same level.

serathius · 2022-03-01T10:19:41Z

pkg/storage/node.go

@@ -96,6 +99,17 @@ func (s *nodeStorage) Store(batch *MetricsBatch) {
 			}
 		}
 	}
+	newTimeStamp := time.Now()


This code is pretty inefficient. Every time we scrape a node, we scan all nodes if their metrics are outdated. This gives us O(n^2) time and memory complexity to scrape and store metrics from n nodes.

If this was not picked up in the tests you prepared, means that they were not accurate. Please look into writing a benchmark for this.

OK. I will optimize it

Let's focus on creating a benchmark that would detect the difference.

serathius · 2022-03-01T10:24:01Z

Current code vs results make me think that we need to write a proper benchmark to reliably measure improvement.

Note: I treat this PR as a prof of concept that can be used to compare performance. @yangjunmyfm192085 no need to update/fix the tests.

/cc @shuaich

k8s-ci-robot · 2022-03-01T10:24:02Z

@serathius: GitHub didn't allow me to request PR reviews from the following users: shuaich.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Current code vs results make me think that we need to write a proper benchmark to reliably measure improvement.

Note: I treat this PR as a prof of concept that can be used to compare performance. @yangjunmyfm192085 no need to update/fix the tests.

/cc @shuaich

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

yangjunmyfm192085

/test pull-metrics-server-test-e2e

yangjunmyfm192085 · 2022-03-31T15:15:53Z

In pr #993 and pr #972 I have added a benchmark for scrape, please help to review it. If the benchmarks are ok, I'll do a comparison test

yangjunmyfm192085

/retest

k8s-triage-robot · 2022-08-04T01:06:24Z

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

yangjunmyfm192085 · 2022-08-04T01:15:50Z

/remove-lifecycle stale
Waiting to find a good way to compare the performance before and after modification

k8s-ci-robot · 2022-09-17T06:19:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yangjunmyfm192085
Once this PR has been reviewed and has the lgtm label, please assign s-urbaniak for approval by writing /assign @s-urbaniak in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-triage-robot · 2022-12-16T07:04:18Z

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-ci-robot · 2023-01-10T05:36:50Z

@yangjunmyfm192085: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-triage-robot · 2023-02-09T05:43:34Z

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle rotten

k8s-triage-robot · 2023-03-11T05:46:27Z

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

This bot triages 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 PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

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

/close

k8s-ci-robot · 2023-03-11T05:46:32Z

@k8s-triage-robot: Closed this PR.

In response to this:

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

This bot triages 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 PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

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

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jasonliu747 · 2023-05-20T03:00:33Z

/reopen
/remove-lifecycle rotten

k8s-ci-robot · 2023-05-20T03:00:36Z

@jasonliu747: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen
/remove-lifecycle rotten

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jasonliu747 · 2023-05-24T11:36:47Z

pkg/server/server.go

-
-func (s *server) tick(ctx context.Context, startTime time.Time) {
-	s.tickStatusMux.Lock()
-	s.tickLastStart = startTime


Correct me if I am wrong.
In this implementation, probeMetricCollectionTimely() will always return an error, right?

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 25, 2022

k8s-ci-robot requested review from dgrisonnet and serathius February 25, 2022 06:31

yangjunmyfm192085 force-pushed the rewritingscraper branch 4 times, most recently from f7ec7d3 to a579b29 Compare February 25, 2022 08:34

serathius reviewed Mar 1, 2022

View reviewed changes

yangjunmyfm192085 force-pushed the rewritingscraper branch 4 times, most recently from ca43dde to 6f2b94e Compare March 7, 2022 12:08

yangjunmyfm192085 commented Mar 8, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2022

yangjunmyfm192085 force-pushed the rewritingscraper branch from 6f2b94e to 7adfa24 Compare March 9, 2022 14:09

k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 9, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2022

yangjunmyfm192085 force-pushed the rewritingscraper branch from 0195d7e to 18b92b1 Compare April 27, 2022 04:34

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2022

yangjunmyfm192085 force-pushed the rewritingscraper branch from 18b92b1 to e73d3a8 Compare April 27, 2022 04:42

yangjunmyfm192085 commented May 6, 2022

View reviewed changes

yangjunmyfm192085 mentioned this pull request Jun 9, 2022

Automate testing scalability of Metrics Server add statistics on cpu and memory #1039

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 4, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 4, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2022

rewriting scraper goroutine scheduling

4cdc172

yangjunmyfm192085 force-pushed the rewritingscraper branch from e73d3a8 to 4cdc172 Compare September 17, 2022 06:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 16, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2023

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 9, 2023

k8s-ci-robot closed this Mar 11, 2023

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 20, 2023

jasonliu747 reviewed May 24, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rewriting scraper goroutine scheduling #972

rewriting scraper goroutine scheduling #972

yangjunmyfm192085 commented Feb 25, 2022

yangjunmyfm192085 commented Mar 1, 2022 •

edited

Loading

serathius Mar 1, 2022 •

edited

Loading

yangjunmyfm192085 Mar 1, 2022

serathius Mar 1, 2022

serathius Mar 1, 2022

yangjunmyfm192085 Mar 1, 2022

serathius Mar 1, 2022

serathius Mar 1, 2022

serathius Mar 1, 2022

serathius Mar 1, 2022

yangjunmyfm192085 Mar 1, 2022

serathius Mar 1, 2022

serathius Mar 1, 2022

serathius Mar 1, 2022

serathius Mar 1, 2022

yangjunmyfm192085 Mar 1, 2022

serathius Mar 7, 2022

serathius commented Mar 1, 2022

k8s-ci-robot commented Mar 1, 2022

yangjunmyfm192085 left a comment

yangjunmyfm192085 commented Mar 31, 2022

yangjunmyfm192085 left a comment

k8s-triage-robot commented Aug 4, 2022

yangjunmyfm192085 commented Aug 4, 2022

k8s-ci-robot commented Sep 17, 2022

k8s-triage-robot commented Dec 16, 2022

k8s-ci-robot commented Jan 10, 2023

k8s-triage-robot commented Feb 9, 2023

k8s-triage-robot commented Mar 11, 2023

k8s-ci-robot commented Mar 11, 2023

jasonliu747 commented May 20, 2023

k8s-ci-robot commented May 20, 2023

jasonliu747 May 24, 2023

	if _, exists := m.isWorking[node.UID]; !exists \|\| !m.isWorking[node.UID] {
	if !m.isWorking[node.UID] {

		startTime := myClock.Now()
		baseCtx, cancel := context.WithCancel(context.Background())

	if working, exists := m.isWorking[node.UID]; exists && working {
	if m.isWorking[node.UID] {

		if _, exists := m.stop[node.UID]; exists {
		m.stop[node.UID] <- struct{}{}

rewriting scraper goroutine scheduling #972

rewriting scraper goroutine scheduling #972

Conversation

yangjunmyfm192085 commented Feb 25, 2022

yangjunmyfm192085 commented Mar 1, 2022 • edited Loading

serathius Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius commented Mar 1, 2022

k8s-ci-robot commented Mar 1, 2022

yangjunmyfm192085 left a comment

Choose a reason for hiding this comment

yangjunmyfm192085 commented Mar 31, 2022

yangjunmyfm192085 left a comment

Choose a reason for hiding this comment

k8s-triage-robot commented Aug 4, 2022

yangjunmyfm192085 commented Aug 4, 2022

k8s-ci-robot commented Sep 17, 2022

k8s-triage-robot commented Dec 16, 2022

k8s-ci-robot commented Jan 10, 2023

k8s-triage-robot commented Feb 9, 2023

k8s-triage-robot commented Mar 11, 2023

k8s-ci-robot commented Mar 11, 2023

jasonliu747 commented May 20, 2023

k8s-ci-robot commented May 20, 2023

Choose a reason for hiding this comment

yangjunmyfm192085 commented Mar 1, 2022 •

edited

Loading

serathius Mar 1, 2022 •

edited

Loading