Support request rate limiting #201

harveylowndes · 2018-05-16T11:53:37Z

Resolves: #108

prydie

Looking good bar a few minor changes 👍

prydie · 2018-05-16T12:00:02Z

pkg/oci/client/client.go

@@ -40,16 +41,24 @@ type Interface interface {
 	Networking() NetworkingInterface
 }

+// OperationPollRateLimiter reader and writer.
+type OperationPollRateLimiter struct {


OperationPollRateLimiter implies to me that the rate limiter is only used for polling operations (e.g. polling work requests). It's used for all API requests, however. Suggest just RateLimiter.

prydie · 2018-05-16T12:04:12Z

pkg/oci/client/errors.go

+	if isWrite {
+		opType = "write"
+	}
+	return fmt.Errorf("Rate limited(%s) for operation: %s", opType, opName)


Should use errors.Errorf to capture stack and error strings shouldn't start with a capital.

prydie · 2018-05-16T12:05:11Z

pkg/oci/client/load_balancer.go

@@ -48,6 +49,11 @@ type LoadBalancerInterface interface {
 }

 func (c *client) GetLoadBalancer(ctx context.Context, id string) (*loadbalancer.LoadBalancer, error) {
+	// Read rate limiting


Probably not needed. Would ditch this and similar comments.

prydie · 2018-05-16T12:08:37Z

pkg/oci/client/compute.go

@@ -48,6 +53,11 @@ func (c *client) GetInstance(ctx context.Context, id string) (*core.Instance, er
 }

 func (c *client) getInstanceByDisplayName(ctx context.Context, compartmentID, displayName string) (*core.Instance, error) {
+	// Read rate limiting
+	if !c.rateLimiter.Reader.TryAccept() {


As getInstanceByDisplayName (potentially) makes multiple calls to the SDK this should be within the for loop prior to the call to c.compute.ListInstances.

prydie · 2018-05-16T12:09:57Z

pkg/oci/ccm.go

@@ -73,7 +79,46 @@ func NewCloudProvider(config *Config) (cloudprovider.Interface, error) {
 	if err != nil {
 		return nil, err
 	}
-	c, err := client.New(cp)
+
+	operationPollRateLimiterRead := flowcontrol.NewFakeAlwaysRateLimiter()


Don't think these two lines are required as the rate limiter isn't optional

prydie · 2018-05-16T12:11:34Z

pkg/oci/client/load_balancer.go

@@ -273,14 +339,19 @@ func (c *client) AwaitWorkRequest(ctx context.Context, id string) (*loadbalancer
 			wr = twr
 			return true, nil
 		case loadbalancer.WorkRequestLifecycleStateFailed:
-			return false, errors.Errorf("WorkRequest %q failed: %s", id, twr.Message)
+			return false, errors.Errorf("WorkRequest %q failed: %s", id, *twr.Message)


Is WorkRequest.Message a required field? If not will need to nil check.

prydie · 2018-05-16T12:12:27Z

pkg/oci/client/networking.go

@@ -71,6 +81,11 @@ func (c *client) GetSubnet(ctx context.Context, id string) (*core.Subnet, error)
 // GetSubnetFromCacheByIP checks to see if the given IP is contained by any subnet CIDR block in the subnet cache
 // If no hits were found then no subnet and no error will be returned (nil, nil)
 func (c *client) GetSubnetFromCacheByIP(ip string) (*core.Subnet, error) {
+	// Read rate limiting
+	if !c.rateLimiter.Reader.TryAccept() {


No need to rate limit access to the local cache

prydie · 2018-05-16T12:13:14Z

pkg/oci/client/networking.go

@@ -46,6 +51,11 @@ func (c *client) getVNIC(ctx context.Context, id string) (*core.Vnic, error) {
 }

 func (c *client) GetSubnet(ctx context.Context, id string) (*core.Subnet, error) {
+	// Read rate limiting
+	if !c.rateLimiter.Reader.TryAccept() {


Only need to rate limit if we get a cache miss.

prydie · 2018-05-16T12:18:20Z

pkg/oci/client/load_balancer.go

@@ -61,6 +67,11 @@ func (c *client) GetLoadBalancer(ctx context.Context, id string) (*loadbalancer.
 }

 func (c *client) GetLoadBalancerByName(ctx context.Context, compartmentID, name string) (*loadbalancer.LoadBalancer, error) {
+	// Read rate limiting
+	if !c.rateLimiter.Reader.TryAccept() {


As GetLoadBalancerByName (potentially) makes multiple calls to the SDK this should be within the for loop prior to the call to c.compute.ListLoadBalancers().

prydie · 2018-05-16T12:19:43Z

pkg/oci/client/load_balancer.go

@@ -259,6 +320,11 @@ func (c *client) UpdateListener(ctx context.Context, lbID, name string, details
 }

 func (c *client) AwaitWorkRequest(ctx context.Context, id string) (*loadbalancer.WorkRequest, error) {
+	// Read rate limiting
+	if !c.rateLimiter.Reader.TryAccept() {


No need to rate limit here. The rate limiting is applied in the call to c.GetWorkRequest() on a per OCI API request basis.

simonlord · 2018-05-22T13:44:53Z

pkg/oci/client/client_test.go

+
+type mockLoadBalancerClient struct{}
+
+func TestRateLimiting(t *testing.T) {


Not sure i see much value in this test as what's being tested is the kubernetes code from the flowcontrol pkg

simonlord · 2018-05-22T13:47:53Z

pkg/oci/ccm.go

@@ -218,3 +229,39 @@ func buildConfigurationProvider(config *Config) (common.ConfigurationProvider, e
 	)
 	return cp, nil
 }
+
+// BuildRateLimiter ...


Could do with a better comment here

prydie

Some minor changes and query re constants but otherwise LGTM 👍

prydie · 2018-07-03T15:00:01Z

pkg/oci/ccm.go

@@ -43,6 +44,13 @@ import (
 	"github.com/oracle/oci-cloud-controller-manager/pkg/oci/util"
 )

+const (
+	//RateLimitQPSDefault sets a value for the default queries per second.


Missing leading space before comment

prydie · 2018-07-03T15:00:06Z

pkg/oci/ccm.go

+const (
+	//RateLimitQPSDefault sets a value for the default queries per second.
+	RateLimitQPSDefault = 1.0
+	//RateLimitBucketDefault sets a value for the default token bucket burst size.


Missing leading space before comment

prydie · 2018-07-03T15:00:20Z

pkg/oci/ccm.go

@@ -43,6 +44,13 @@ import (
 	"github.com/oracle/oci-cloud-controller-manager/pkg/oci/util"
 )

+const (
+	//RateLimitQPSDefault sets a value for the default queries per second.
+	RateLimitQPSDefault = 1.0


What is the rationale behind these constants?

prydie · 2018-07-03T15:01:23Z

pkg/oci/ccm.go

+
+// buildNewRateLimiter builds and returns a struct containing read and write
+// rate limiters. Defaults are used where no (0) value is provided.
+func buildNewRateLimiter(config *RateLimiterConfig) client.RateLimiter {


func newRateLimiter(config *RateLimiterConfig) client.RateLimiter

prydie · 2018-07-03T15:01:48Z

pkg/oci/ccm_test.go

+)
+
+func TestBuildRateLimiterWithConfig(t *testing.T) {
+	var qpsRead float32 = 6.0


qpsRead := float32(6.0)

prydie · 2018-07-03T15:03:25Z

pkg/oci/client/compute.go

@@ -53,6 +57,9 @@ func (c *client) getInstanceByDisplayName(ctx context.Context, compartmentID, di
 		instances []core.Instance
 	)
 	for {
+		if !c.rateLimiter.Reader.TryAccept() {
+			return nil, RateLimitError(false, "getInstanceByDisplayName")


s/getInstanceByDisplayName/ListInstances/

prydie · 2018-07-03T15:03:59Z

pkg/oci/client/compute.go

@@ -81,6 +88,10 @@ func (c *client) getInstanceByDisplayName(ctx context.Context, compartmentID, di
 }

 func (c *client) listVNICAttachments(ctx context.Context, req core.ListVnicAttachmentsRequest) (core.ListVnicAttachmentsResponse, error) {
+	if !c.rateLimiter.Reader.TryAccept() {
+		return core.ListVnicAttachmentsResponse{}, RateLimitError(false, "listVNICAttachments")


s/listVNICAttachments/ListVnicAttachments/

prydie · 2018-07-03T15:04:33Z

pkg/oci/client/load_balancer.go

@@ -63,6 +68,9 @@ func (c *client) GetLoadBalancer(ctx context.Context, id string) (*loadbalancer.
 func (c *client) GetLoadBalancerByName(ctx context.Context, compartmentID, name string) (*loadbalancer.LoadBalancer, error) {
 	var page *string
 	for {
+		if !c.rateLimiter.Reader.TryAccept() {
+			return nil, RateLimitError(false, "GetLoadBalancerByName")


s/GetLoadBalancerByName/ListLoadBalancers/

prydie · 2018-07-03T15:05:11Z

pkg/oci/client/load_balancer.go

@@ -113,6 +129,10 @@ func (c *client) DeleteLoadBalancer(ctx context.Context, id string) (string, err
 }

 func (c *client) GetCertificateByName(ctx context.Context, lbID, name string) (*loadbalancer.Certificate, error) {
+	if !c.rateLimiter.Reader.TryAccept() {
+		return nil, RateLimitError(false, "GetCertificateByName")


s/GetCertificateByName/ListCertificates/

prydie · 2018-07-03T15:06:05Z

pkg/oci/client/networking.go

@@ -33,6 +33,10 @@ type NetworkingInterface interface {
 }

 func (c *client) getVNIC(ctx context.Context, id string) (*core.Vnic, error) {
+	if !c.rateLimiter.Reader.TryAccept() {
+		return nil, RateLimitError(false, "getVNIC")


s/getVNIC/GetVnic/

prydie

One minor nit but otherwise 👍

prydie · 2018-08-02T15:00:42Z

pkg/oci/client/networking.go

-func (c *client) getVNIC(ctx context.Context, id string) (*core.Vnic, error) {
+func (c *client) GetVNIC(ctx context.Context, id string) (*core.Vnic, error) {
+	if !c.rateLimiter.Reader.TryAccept() {
+		return nil, RateLimitError(false, "getVNIC")


s/getVNIC/GetVNIC/

prydie · 2018-08-02T15:05:00Z

pkg/oci/ccm.go

@@ -43,6 +44,13 @@ import (
 	"github.com/oracle/oci-cloud-controller-manager/pkg/oci/util"
 )

+const (
+	// RateLimitQPSDefault sets a value for the default queries per second.
+	RateLimitQPSDefault = 1.0


@jhorwit2 Do you have any insight into appropriate values for these constants?

@harveylowndes After talking to @jhorwit2 offline it seems like a value of 20.0 should be ok here.

prydie · 2018-08-15T12:05:43Z

@harveylowndes Can you give this a rebase please to fix the conflicts (NOTE: you'll need to pull the branch again as I've made some modifications)?

prydie

Couple of minor nits but otherwise LGTM 👍

prydie · 2018-08-17T08:59:45Z

pkg/oci/ccm.go

+// newRateLimiter builds and returns a struct containing read and write
+// rate limiters. Defaults are used where no (0) value is provided.
+func newRateLimiter(config *RateLimiterConfig) client.RateLimiter {
+


prydie · 2018-08-17T08:59:58Z

pkg/oci/ccm.go

+		config.RateLimitQPSRead,
+		config.RateLimitBucketRead)
+
+	glog.V(2).Infof("OCI using write rate limit configuration: QPS=%g, bucket=%d",


Use zap directly rather than glog

prydie

Nearly there!

prydie · 2018-08-17T09:35:04Z

pkg/oci/ccm.go

@@ -269,11 +267,11 @@ func newRateLimiter(config *RateLimiterConfig) client.RateLimiter {
 			config.RateLimitBucketWrite),
 	}

-	glog.V(2).Infof("OCI using read rate limit configuration: QPS=%g, bucket=%d",
+	zap.S().Infof("OCI using read rate limit configuration: QPS=%g, bucket=%d",


Don't use the global logger; pass in a logger to newRateLimiter.

prydie · 2018-08-17T10:21:26Z

LGTM 👍

owainlewis · 2018-08-17T10:23:16Z

pkg/oci/ccm_test.go

@@ -0,0 +1,59 @@
+// Copyright 2017 Oracle and/or its affiliates. All rights reserved.


2017 => 2018

simonlord

Couple of things

simonlord · 2018-08-17T10:26:16Z

pkg/oci/ccm.go

@@ -42,6 +43,13 @@ import (
 	"github.com/oracle/oci-cloud-controller-manager/pkg/oci/util"
 )

+const (
+	// RateLimitQPSDefault sets a value for the default queries per second.


Remove comment

simonlord · 2018-08-17T10:27:03Z

pkg/oci/ccm.go

@@ -42,6 +43,13 @@ import (
 	"github.com/oracle/oci-cloud-controller-manager/pkg/oci/util"
 )

+const (
+	// RateLimitQPSDefault sets a value for the default queries per second.
+	RateLimitQPSDefault = 20.0


Doesn't need to start with a capital (which reduces req for a comment too) as i don't think it's used outside this pkg?

simonlord · 2018-08-17T10:27:13Z

pkg/oci/ccm.go

+const (
+	// RateLimitQPSDefault sets a value for the default queries per second.
+	RateLimitQPSDefault = 20.0
+	// RateLimitBucketDefault sets a value for the default token bucket burst size.


Remove comment

simonlord · 2018-08-17T10:27:19Z

pkg/oci/ccm.go

+	// RateLimitQPSDefault sets a value for the default queries per second.
+	RateLimitQPSDefault = 20.0
+	// RateLimitBucketDefault sets a value for the default token bucket burst size.
+	RateLimitBucketDefault = 5


Doesn't need to start with a capital (which reduces req for a comment too) as i don't think it's used outside this pkg?

owainlewis

Approved for merge.

* Support request rate limiting

prydie suggested changes May 16, 2018

View reviewed changes

harveylowndes force-pushed the support-request-rate-limiting branch 9 times, most recently from efcfa74 to 47dccc7 Compare May 22, 2018 13:20

harveylowndes requested a review from simonlord May 22, 2018 13:22

simonlord reviewed May 22, 2018

View reviewed changes

harveylowndes force-pushed the support-request-rate-limiting branch from 47dccc7 to ec00172 Compare May 22, 2018 14:42

prydie suggested changes Jul 3, 2018

View reviewed changes

prydie suggested changes Aug 2, 2018

View reviewed changes

prydie reviewed Aug 2, 2018

View reviewed changes

prydie force-pushed the support-request-rate-limiting branch from 5562f27 to 1fcffc0 Compare August 2, 2018 15:42

harveylowndes force-pushed the support-request-rate-limiting branch from 1fcffc0 to 5165184 Compare August 3, 2018 10:00

prydie approved these changes Aug 8, 2018

View reviewed changes

prydie changed the title ~~WIP: Support request rate limiting~~ Support request rate limiting Aug 8, 2018

prydie force-pushed the support-request-rate-limiting branch from 5165184 to 0cad1b7 Compare August 13, 2018 09:30

prydie added this to the 0.6.0 milestone Aug 14, 2018

Harvey Lowndes added 3 commits August 15, 2018 15:52

Support request rate limiting

d65b631

Added tests for request rate limiting

dead5c3

Review changes

0660372

harveylowndes force-pushed the support-request-rate-limiting branch 2 times, most recently from 779711b to 237498c Compare August 15, 2018 15:06

Support request rate limiting

2d6e127

harveylowndes force-pushed the support-request-rate-limiting branch from 237498c to 2d6e127 Compare August 17, 2018 08:26

prydie suggested changes Aug 17, 2018

View reviewed changes

Add documentation

36df839

prydie suggested changes Aug 17, 2018

View reviewed changes

harveylowndes force-pushed the support-request-rate-limiting branch 2 times, most recently from dd96ed9 to ab8fe53 Compare August 17, 2018 10:12

prydie approved these changes Aug 17, 2018

View reviewed changes

owainlewis self-assigned this Aug 17, 2018

owainlewis self-requested a review August 17, 2018 10:21

owainlewis reviewed Aug 17, 2018

View reviewed changes

pkg/oci/ccm_test.go

@@ -0,0 +1,59 @@

// Copyright 2017 Oracle and/or its affiliates. All rights reserved.

Copy link

Member

owainlewis Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2017 => 2018

harveylowndes force-pushed the support-request-rate-limiting branch from ab8fe53 to eb8da02 Compare August 17, 2018 10:28

simonlord reviewed Aug 17, 2018

View reviewed changes

Review changes

24bc0f4

harveylowndes force-pushed the support-request-rate-limiting branch from eb8da02 to 24bc0f4 Compare August 17, 2018 12:54

owainlewis added enhancement feature labels Aug 17, 2018

owainlewis approved these changes Aug 17, 2018

View reviewed changes

simonlord approved these changes Aug 17, 2018

View reviewed changes

owainlewis merged commit 8d62bc4 into master Aug 17, 2018

prydie deleted the support-request-rate-limiting branch September 21, 2018 16:29

ayushverma14 pushed a commit to ayushverma14/oci-cloud-controller-manager that referenced this pull request Jul 18, 2022

Support request rate limiting (oracle#201)

3e2ee82

* Support request rate limiting


		type mockLoadBalancerClient struct{}

		func TestRateLimiting(t *testing.T) {

		@@ -0,0 +1,59 @@
		// Copyright 2017 Oracle and/or its affiliates. All rights reserved.

Support request rate limiting #201

Support request rate limiting #201

Conversation

harveylowndes commented May 16, 2018

prydie left a 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

prydie left a 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

prydie left a 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

prydie commented Aug 15, 2018

prydie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydie commented Aug 17, 2018

Choose a reason for hiding this comment

simonlord left a 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

owainlewis left a comment

Choose a reason for hiding this comment