Rate-limit reading proc files #912

2opremio · 2016-02-04T12:34:43Z

Second attempt at #814 after seeing #812 (comment)
Improves #812

2opremio · 2016-02-08T12:52:12Z

@peterbourgon Can you PTAL? I will rebase before merging.

probe/endpoint/procspy/background_reader_linux.go

+	if !br.running {
+		return fmt.Errorf("background reader already not running")
+	}
+	br.pleaseStop = true


probe/endpoint/procspy/background_reader_linux.go

+	defer br.mtx.Unlock()
+
+	reader := bytes.NewReader(br.readyBuf.Bytes())
+	buf.ReadFrom(reader)


peterbourgon · 2016-02-08T12:59:34Z

I still think @paulbellamy should take a look for overall cohesion, but I like this approach.

probe/endpoint/procspy/background_reader_linux.go

+	br.mtx.Lock()
+	defer br.mtx.Unlock()
+	if !br.running {
+		return fmt.Errorf("background reader already not running")


Use a reader in the background, dynamically rate-limited, reading the required files in a loop

2opremio · 2016-02-08T13:43:39Z

Rebased to get client test fixes.

probe/endpoint/procspy/background_reader_linux.go

+
+		// Adjust rate limit to more-accurately meet the target walk time in next iteration
+		scaledRateLimitPeriod := targetWalkTimeF / walkTimeF * float64(rateLimitPeriod)
+		rateLimitPeriod = time.Duration(math.Min(scaledRateLimitPeriod, maxRateLimitPeriodF))


paulbellamy · 2016-02-08T14:06:45Z

Overall, it's a substantial improvement. I wonder if it could be improved with contexts?

2opremio · 2016-02-08T18:13:22Z

@peterbourgon @paulbellamy please take a thorough look, I am still a bit of a newbie with Golang channels.

probe/endpoint/procspy/proc_linux.go

 		dirName := strconv.Itoa(p.PID)
 		fdBase := filepath.Join(procRoot, dirName, "fd")

+		if fdBlockCount > w.fdBlockSize {
+			// we surpassed the filedescriptor rate limit
+			// TODO: worth selecting on w.stopc?


probe/endpoint/procspy/background_reader_linux.go

+		begin           time.Time                      // when we started the last performWalk
+		tickc           = time.After(time.Millisecond) // fire immediately
+		walkc           chan map[uint64]*Proc          // initially nil, i.e. off
+		walkBuf         = bytes.NewBuffer(make([]byte, 0, 5000))


probe/endpoint/procspy/background_reader_linux.go

+	// when to start next walk
+	restInterval = targetWalkTime - took
+
+	return


peterbourgon · 2016-02-09T10:27:05Z

We could do another iteration over the loop body and clean it up a good bit, but I won't frustrate you any more :) LGTM after those changes.

2opremio · 2016-02-09T10:39:26Z

@peterbourgon Please go ahead and comment. I would like to avoid refactoring other parts of the code not directly related to this change though.

Rate-limit reading proc files

2opremio · 2016-02-09T11:20:34Z

I should had rebased, oh well.

2opremio force-pushed the 812-rate-limit-proc branch 2 times, most recently from 488f6c6 to 45ab210 Compare February 4, 2016 12:49

2opremio mentioned this pull request Feb 4, 2016

Probe using 70% CPU #812

Closed

2opremio force-pushed the 812-rate-limit-proc branch 2 times, most recently from c7a60b6 to 1e79273 Compare February 8, 2016 12:27

2opremio changed the title ~~[WIP] Rate-limit reading proc files~~ Rate-limit reading proc files Feb 8, 2016

peterbourgon reviewed Feb 8, 2016
View reviewed changes

probe/endpoint/procspy/background_reader_linux.go

if !br.running {

return fmt.Errorf("background reader already not running")

}

br.pleaseStop = true

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

2opremio force-pushed the 812-rate-limit-proc branch from 78c8fde to 93d75c1 Compare February 8, 2016 12:57

peterbourgon reviewed Feb 8, 2016
View reviewed changes

probe/endpoint/procspy/background_reader_linux.go

defer br.mtx.Unlock()

reader := bytes.NewReader(br.readyBuf.Bytes())

buf.ReadFrom(reader)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

2opremio force-pushed the 812-rate-limit-proc branch from 93d75c1 to 016d9d4 Compare February 8, 2016 13:17

paulbellamy reviewed Feb 8, 2016
View reviewed changes

probe/endpoint/procspy/background_reader_linux.go

br.mtx.Lock()

defer br.mtx.Unlock()

if !br.running {

return fmt.Errorf("background reader already not running")

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

Alfonso Acosta added 12 commits February 8, 2016 13:42

Rate-limit reading proc files

f922ea1

Use a reader in the background, dynamically rate-limited, reading the required files in a loop

Batch rate-limit to stats of /proc/*/fd/*

87dd43f

Cleanup

6deeca0

Get rid of the package-level Connections func

d4c68f4

Make linter happy

b93c323

Fix tests

6240187

Use levelled logging

8c3c899

Adjust file descript rate-limit block

c09ad9e

Use uint64 for fd counter

ccabaf5

Clarify rate-limiting further

08969ec

Make linter happy

b08c427

Review feedback

3dd2d45

2opremio force-pushed the 812-rate-limit-proc branch from 016d9d4 to 3dd2d45 Compare February 8, 2016 13:43

paulbellamy reviewed Feb 8, 2016
View reviewed changes

2opremio force-pushed the 812-rate-limit-proc branch from 5939ac3 to df4f72b Compare February 8, 2016 18:12

2opremio reviewed Feb 8, 2016
View reviewed changes

Review feedback

53bc710

2opremio force-pushed the 812-rate-limit-proc branch from df4f72b to 53bc710 Compare February 8, 2016 19:29

Fix variable scope bug

0545d9a

2opremio force-pushed the 812-rate-limit-proc branch from 98423d3 to 0545d9a Compare February 8, 2016 21:04

Raise maximum rate limit

eb52adb

peterbourgon reviewed Feb 9, 2016
View reviewed changes

Review feedback

dfc1369

2opremio assigned peterbourgon Feb 9, 2016

peterbourgon reviewed Feb 9, 2016
View reviewed changes

probe/endpoint/procspy/background_reader_linux.go

// when to start next walk

restInterval = targetWalkTime - took

return

This comment was marked as abuse.

Sign in to view

Review comments

d4b114d

2opremio pushed a commit that referenced this pull request Feb 9, 2016

Merge pull request #912 from weaveworks/812-rate-limit-proc

26ccb49

Rate-limit reading proc files

2opremio merged commit 26ccb49 into master Feb 9, 2016

2opremio deleted the 812-rate-limit-proc branch February 9, 2016 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rate-limit reading proc files #912

Rate-limit reading proc files #912

2opremio commented Feb 4, 2016

2opremio commented Feb 8, 2016

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

peterbourgon commented Feb 8, 2016

This comment was marked as abuse.

This comment was marked as abuse.

2opremio commented Feb 8, 2016

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

paulbellamy commented Feb 8, 2016

2opremio commented Feb 8, 2016

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

peterbourgon commented Feb 9, 2016

2opremio commented Feb 9, 2016

2opremio commented Feb 9, 2016

Rate-limit reading proc files #912

Rate-limit reading proc files #912

Conversation

2opremio commented Feb 4, 2016

2opremio commented Feb 8, 2016

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

peterbourgon commented Feb 8, 2016

This comment was marked as abuse.

This comment was marked as abuse.

2opremio commented Feb 8, 2016

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

paulbellamy commented Feb 8, 2016

2opremio commented Feb 8, 2016

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

peterbourgon commented Feb 9, 2016

2opremio commented Feb 9, 2016

2opremio commented Feb 9, 2016