storagenode: decline uploads when there are too many live requests #2397

egonelbre · 2019-07-01T07:42:29Z

Currently it's easy to overload storage nodes and crash them. This PR adds a maximum concurrent requests parameter which throttles upload requests.

Please describe the tests:

Test 1:
Test 2:

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

Does the PR describe what changes are being made?
Does the PR describe why the changes are being made?
Does the code follow our style guide?
Does the code follow our testing guide?
Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
Does any documentation need updating?
Do the database access patterns make sense?

kaloyan-raev · 2019-07-01T09:03:37Z

storagenode/piecestore/endpoint.go

@@ -55,6 +56,7 @@ type OldConfig struct {
 // Config defines parameters for piecestore endpoint.
 type Config struct {
 	ExpirationGracePeriod time.Duration `help:"how soon before expiration date should things be considered expired" default:"48h0m0s"`
+	MaxConcurrentRequests int           `help:"how many concurrent requests are allowed, before uploads are rejected." default:"30"`


How would SNOs know what value to configure here?

Is there a way for the storage node to self-diagnose and determine if it is overloaded or not?

Probably with some kind of benchmarking by some other party. The issue is that it's not just about the storage node itself, but also about the network it's able to deliver.

I guess we could try monitoring bandwidth usage and when it isn't increasing anymore or load or memory usage etc. But these all are much more complicated solutions than "this is how much this storage node can serve".

I think it’s totally fine to have a default, if someone notices a lot of issues he just can tune it down. We had the same mechanic in V2 and that worked fine.
About the default itself, I would recommend something in the neighborhood of 5-10 max.

Yeah, I'm not sure what to put as limits for now. Is there an easy way to test how much we can handle?

My Server Node was handling 15-20 Requests max per second, when fully cached.
The Pi 3B+ is already overwhelmed with 3 requests.

I could run a few test uploads on my local Network to see what i can come up with.

I wonder if the storagenodes could potentially obtain data from one or more satellites to help it decide how many requests it can handle and adjust the max.

With a dynamic scaling that is just adding another layer of potential issues @phutchins
Lets make a good decision/average and update the docs accordingly

Let me know when you get a number from testing @stefanbenten

I will test that after lunch 👍

jtolio · 2019-07-01T12:55:23Z

storagenode/piecestore/endpoint.go

@@ -74,6 +76,8 @@ type Endpoint struct {
 	orders      orders.DB
 	usage       bandwidth.DB
 	usedSerials UsedSerials
+
+	liveRequests int32


we should make this be one of the first struct fields, so arm alignment is better guaranteed (https://golang.org/pkg/sync/atomic/#pkg-note-BUG)

Wasn't this an issue for int64 only?

oh yeah good point, maybe worth testing on an arm device

egonelbre · 2019-07-02T18:57:32Z

internal/testplanet/storagenode.go

@@ -109,6 +109,8 @@ func (planet *Planet) newStorageNodes(count int, whitelistedSatelliteIDs []strin
 				StaticDir: filepath.Join(developmentRoot, "web/operator/"),
 			},
 			Storage2: piecestore.Config{
+				ExpirationGracePeriod: 0,
+				MaxConcurrentRequests: 100,


Leaving testplanet to 100, because having this lower will probably break some tests.

Worth testing where the limit is, in another PR of course.

stefanbenten

the rest looks good to me!

stefanbenten · 2019-07-02T18:59:49Z

storagenode/piecestore/endpoint.go

@@ -55,6 +56,7 @@ type OldConfig struct {
 // Config defines parameters for piecestore endpoint.
 type Config struct {
 	ExpirationGracePeriod time.Duration `help:"how soon before expiration date should things be considered expired" default:"48h0m0s"`
+	MaxConcurrentRequests int           `help:"how many concurrent requests are allowed, before uploads are rejected." default:"6"`


Maybe we want to define a devDefault and a releaseDefault?
to prevent storj-sim slowdowns?

…2397) (cherry picked from commit 38f3d86)

storjrobot · 2019-07-11T19:29:37Z

This pull request has been mentioned on Storj Labs Community Forum: Decentralized Cloud Storage. There might be relevant details there:

https://forum.storj.io/t/changelog-v0-14-11/433/1

add storagenode limiter

4659477

egonelbre requested a review from aleitner as a code owner July 1, 2019 07:42

cla-bot bot added the cla-signed label Jul 1, 2019

fix test

89031cb

egonelbre added the Request Code Review Code review requested label Jul 1, 2019

use int

8f0105a

kaloyan-raev reviewed Jul 1, 2019

View reviewed changes

egonelbre changed the title ~~storagenode: decline upload requests when there are too many live requests~~ storagenode: decline uploads when there are too many live requests Jul 1, 2019

jtolio reviewed Jul 1, 2019

View reviewed changes

stefanbenten added the Sprint Release Goal Sprint Release Goal label Jul 2, 2019

set max concurrent requests default to 6

4593416

egonelbre commented Jul 2, 2019

View reviewed changes

stefanbenten approved these changes Jul 2, 2019

View reviewed changes

Merge branch 'master' into ee/sno-backoff

492c009

egonelbre added the Reviewer Can Merge If all checks have passed, non-owner can merge PR label Jul 2, 2019

egonelbre added 3 commits July 3, 2019 16:21

Merge branch 'master' into ee/sno-backoff

48ef87b

log more information

5bc671c

disable test for now to prevent flakiness

f6895c0

mniewrzal approved these changes Jul 3, 2019

View reviewed changes

egonelbre merged commit 38f3d86 into master Jul 3, 2019

egonelbre deleted the ee/sno-backoff branch July 3, 2019 13:47

littleskunk pushed a commit that referenced this pull request Jul 5, 2019

storagenode: decline uploads when there are too many live requests (#…

b60cf09

…2397) (cherry picked from commit 38f3d86)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storagenode: decline uploads when there are too many live requests #2397

storagenode: decline uploads when there are too many live requests #2397

egonelbre commented Jul 1, 2019 •

edited

kaloyan-raev Jul 1, 2019

egonelbre Jul 1, 2019

stefanbenten Jul 1, 2019

egonelbre Jul 1, 2019

stefanbenten Jul 1, 2019

stefanbenten Jul 1, 2019

phutchins Jul 1, 2019

stefanbenten Jul 1, 2019

egonelbre Jul 2, 2019

stefanbenten Jul 2, 2019

jtolio Jul 1, 2019

kaloyan-raev Jul 1, 2019

jtolio Jul 1, 2019

egonelbre Jul 2, 2019

stefanbenten Jul 2, 2019

stefanbenten left a comment

stefanbenten Jul 2, 2019

storjrobot commented Jul 11, 2019

storagenode: decline uploads when there are too many live requests #2397

storagenode: decline uploads when there are too many live requests #2397

Conversation

egonelbre commented Jul 1, 2019 • edited

Code Review Checklist (to be filled out by reviewer)

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

stefanbenten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storjrobot commented Jul 11, 2019

egonelbre commented Jul 1, 2019 •

edited