Added qps and burst to server's client #956

ffd2subroutine · 2018-10-18T17:50:31Z

Signed-off-by: Darko Radisic [email protected]

Fixes #109

rosskukulinski · 2018-10-18T19:04:20Z

Hi @ffd2subroutine - thanks for the PR! I noticed you marked this as work-in-progress. Is there anything in particular you're still planning to do or is this ready for review from someone from the Ark team?

ffd2subroutine · 2018-10-18T19:22:07Z

Hi @rosskukulinski, yeah it's ready for review, maybe @ncdc can take a look at this one.

nrb · 2018-10-18T20:14:57Z

Thanks for the PR @ffd2subroutine. I'll give this a read and test and get back to you.

nrb

This looks reasonable to me, based on the issue and other examples of overriding the same parameters I've found in other projects.

@ncdc and @skriss do we want to keep the defaults the same, or up them at all?

ncdc · 2018-10-19T14:01:10Z

I think upping the defaults makes sense. I just looked at Kubernetes 1.11, and it looks like the controller-manager's defaults are qps=20, burst=30. I like those values - what do you all think?

skriss · 2018-10-19T15:14:47Z

@ncdc going with controller-manager's defaults sounds reasonable to me!

nrb

Thanks for updating that @ffd2subroutine!

2 more things (I missed this the first time): can you squash the commits into 1, as well as add a sign off line? git commit --amend --sign-off will add the signed off line for you.

ncdc · 2018-10-19T15:54:08Z

@skriss @nrb should we try wordsmithing the flag help text a little bit?

nrb · 2018-10-19T15:55:42Z

@ncdc Probably worth doing, yeah.

ncdc · 2018-10-19T16:06:55Z

pkg/cmd/server/server.go

@@ -153,6 +161,8 @@ func NewCommand() *cobra.Command {
 	command.Flags().StringSliceVar(&config.restoreResourcePriorities, "restore-resource-priorities", config.restoreResourcePriorities, "desired order of resource restores; any resource not in the list will be restored alphabetically after the prioritized resources")
 	command.Flags().StringVar(&config.defaultBackupLocation, "default-backup-storage-location", config.defaultBackupLocation, "name of the default backup storage location")
 	command.Flags().Var(&volumeSnapshotLocations, "default-volume-snapshot-locations", "list of unique volume providers and default volume snapshot location (provider1:location-01,provider2:location-02,...)")
+	command.Flags().Float32Var(&config.clientQPS, "client-qps", config.clientQPS, "server's client qps")


We should probably be more descriptive here. Something along the lines that this is the maximum number of requests per second the server uses when communicating with Kubernetes when backing up and restoring.

maximum number of requests per second the server uses when communicating with Kubernetes when backing up and restoring

Would that be okay?

Wanted to jot this down before I forget - I'm not sure if "maximum" is the best word here, since it's possible to burst above the qps. It's more like the average sustained requests rate.

"quota" seems like one of the right words here. Something like "standard quota of requests per second to Kubernetes"?

Okay, let me know the final wording for qps and burst param descriptions!

ncdc · 2018-10-19T16:25:43Z

pkg/cmd/server/server.go

@@ -153,6 +161,8 @@ func NewCommand() *cobra.Command {
 	command.Flags().StringSliceVar(&config.restoreResourcePriorities, "restore-resource-priorities", config.restoreResourcePriorities, "desired order of resource restores; any resource not in the list will be restored alphabetically after the prioritized resources")
 	command.Flags().StringVar(&config.defaultBackupLocation, "default-backup-storage-location", config.defaultBackupLocation, "name of the default backup storage location")
 	command.Flags().Var(&volumeSnapshotLocations, "default-volume-snapshot-locations", "list of unique volume providers and default volume snapshot location (provider1:location-01,provider2:location-02,...)")
+	command.Flags().Float32Var(&config.clientQPS, "client-qps", config.clientQPS, "server's client qps")
+	command.Flags().IntVar(&config.clientBurst, "client-burst", config.clientBurst, "server's client burst")


I'm not sure the best way to write this clearly. It's a higher-than-average number of requests (more than QPS) that are allowed in a small window of time.

What about this?

Given that the QPS is the normal number of requests per second, the burst is the ability to exceed the QPS for a short period of time. Here's how it works:

There is a "bucket" that has a fixed capacity. This capacity is the "burst" value. Let's use an example of 30. The bucket is initially filled to capacity, so it has 30 tokens. You can pull tokens out as fast as possible, which means you can initially do 30 requests. The bucket also has a rate at which it is filled - this is the QPS. Here let's use an example of 20. This means a new token is added to the bucket approximately every 1/20 of a second. If you are constantly making requests, the steady rate will be 20/second. If you pause for long enough, the bucket will fill to capacity == burst == 30 in this example.

I'm still thinking about wording.

and thanks for the explanation!

It is a maximum value, but it's not a sustained rate.

For example, if the burst is 100 and the qps is 5, and you have a steady stream of requests you need to make, what happens is initially you can make 100 requests as fast as you're able to do so, and all additional requests will average 5 per second. The only way you get back to 100 (the burst) is if there is a pause in requests for (burst / qps) seconds to allow the bucket to refill (100/5 = 20 seconds).

what kind of usable string length requirements do we have here? (pondering further ...)

"temporary maximum ..." ? "emergency maximum ..." ?

or if a longer string would work:

"one-time maximum number of requests to exceed QPS before pausing to refresh burst"

"short-term maximum number of requests to exceed QPS before pause to refresh burst"

I think this one or the other longer alternative are not too bad (actually useful) if we can accommodate longer string

I think "short-term" or "temporary" is probably good

skriss

Hi @ffd2subroutine - will you be able to get this updated so we can include it in the upcoming release? Thanks!

skriss · 2018-10-30T15:40:57Z

pkg/cmd/server/server.go

@@ -446,6 +458,8 @@ func (s *server) validateBackupStorageLocations() error {
 // - Limit ranges go before pods or controllers so pods can use them.
 // - Pods go before controllers so they can be explicitly restored and potentially
 //	 have restic restores run before controllers adopt the pods.
+// - Custom Resource Definitions come before Custom Resource so that they can be


This has already made it into master so can be removed from this PR

sorry I was squashing commits for the first time and it squashed everything in between, will remove it

skriss · 2018-10-30T15:41:02Z

pkg/cmd/server/server.go

@@ -456,6 +470,7 @@ var defaultRestorePriorities = []string{
 	"limitranges",
 	"pods",
 	"replicaset",
+	"customresourcedefinitions",


This has already made it into master so can be removed from this PR

skriss · 2018-11-08T19:47:37Z

@ncdc @Bradamant3 would like to get this into v0.10, so we just need to finalize help text. WDYT about these:

client-burst: maximum number of requests by the server to the Kubernetes API in a short period of time
client-qps: maximum number of requests per second by the server to the Kubernetes API once the burst limit has been reached

ncdc · 2018-11-08T19:48:25Z

Sounds good enough for now!

skriss · 2018-11-08T22:02:52Z

@ffd2subroutine if you can update the help text to what I proposed just above, we should be good to merge - thanks!

ncdc · 2018-11-09T16:16:28Z

@ffd2subroutine thanks - would you mind rewording the commit message so it doesn't have [wip] in it any more?

skriss

Approved pending removal of [wip] from commit message :)

ncdc · 2018-11-09T16:34:53Z

pkg/cmd/server/server.go

@@ -197,6 +207,8 @@ func newServer(namespace, baseName string, config serverConfig, logger *logrus.L
 	if err != nil {
 		return nil, err
 	}
+	clientConfig.QPS = config.clientQPS


I don't know why I didn't think of this sooner - should we validate these are positive numbers?

yeah makes sense, I also forgot

if config.clientQPS <= 0 { return errors.New("client-qps can be only a positive number") }

Something like this? Maybe some better wording, not a native english speaker:)

That's good, or client-qps must be positive

Changes addressed, but haven't reviewed latest code.

Signed-off-by: Darko Radisic <[email protected]>

skriss · 2018-11-09T19:41:30Z

LGTM, thanks @ffd2subroutine!

nrb changed the title ~~[wip] Added qps and burst to server's client~~ Added qps and burst to server's client Oct 18, 2018

nrb requested review from skriss and nrb October 18, 2018 20:15

nrb approved these changes Oct 18, 2018

View reviewed changes

rosskukulinski added this to the v0.10.0 milestone Oct 18, 2018

nrb previously requested changes Oct 19, 2018

View reviewed changes

ncdc reviewed Oct 19, 2018

View reviewed changes

ffd2subroutine force-pushed the qps-burst-configurable branch from bdbc268 to 9b0a4e9 Compare October 19, 2018 17:33

skriss suggested changes Oct 30, 2018

View reviewed changes

ffd2subroutine force-pushed the qps-burst-configurable branch from d157ec7 to 5b89fc2 Compare October 30, 2018 16:53

ffd2subroutine force-pushed the qps-burst-configurable branch from 5b89fc2 to 28f56b6 Compare November 7, 2018 17:25

ffd2subroutine force-pushed the qps-burst-configurable branch from 28f56b6 to 50a1e71 Compare November 8, 2018 22:26

skriss approved these changes Nov 9, 2018

View reviewed changes

ffd2subroutine force-pushed the qps-burst-configurable branch from 50a1e71 to 0aac667 Compare November 9, 2018 16:18

ncdc reviewed Nov 9, 2018

View reviewed changes

Added qps and burst to server's client

e5556fe

Signed-off-by: Darko Radisic <[email protected]>

ffd2subroutine force-pushed the qps-burst-configurable branch from 0aac667 to e5556fe Compare November 9, 2018 19:16

skriss merged commit 2781e4e into vmware-tanzu:master Nov 9, 2018

Added qps and burst to server's client #956

Added qps and burst to server's client #956

Conversation

ffd2subroutine commented Oct 18, 2018

rosskukulinski commented Oct 18, 2018

ffd2subroutine commented Oct 18, 2018

nrb commented Oct 18, 2018

nrb left a comment

Choose a reason for hiding this comment

ncdc commented Oct 19, 2018

skriss commented Oct 19, 2018

nrb left a comment

Choose a reason for hiding this comment

ncdc commented Oct 19, 2018

nrb commented Oct 19, 2018

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

Bradamant3 Oct 30, 2018 • 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

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffd2subroutine Oct 30, 2018 • 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

skriss commented Nov 8, 2018

ncdc commented Nov 8, 2018

skriss commented Nov 8, 2018

ncdc commented Nov 9, 2018

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffd2subroutine Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Nov 9, 2018

Bradamant3 Oct 30, 2018 •

edited

Loading

ffd2subroutine Oct 30, 2018 •

edited

Loading

ffd2subroutine Nov 9, 2018 •

edited

Loading