New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added request cache and queue to AWS provider and use it from variable resolution #4518

Merged
merged 2 commits into from Dec 6, 2017

Conversation

Projects
None yet
4 participants
@HyperBrain
Member

HyperBrain commented Dec 4, 2017

What did you implement:

Closes #3821
Closes #4466

How did you implement it:

Added an on-demand request cache to the AWS provider. The cache can be utilized and activated by adding a new last parameter to the provider.request method. It defaults to false to keep the current behavior and has to be enabled explicitly for locations that can use cached results.

The requests intiated from the variable system will set the cached flag to let the variable resolution request static info like the stack descriptions only once.

This PR complements #4499 which added the generic cache for the variable system. Together the fixes will eliminate the rate exceeded errors.

How can we verify it:

Add lots of external variable references (e.g. cf, ssm, etc) to your serverless configuration that led to a rate exceeded error with 1.24.1. The deployment should now succeed when using this branch.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@HyperBrain HyperBrain self-assigned this Dec 4, 2017

@HyperBrain HyperBrain changed the title from Added request cache to AWS provider and use it from variable resolution [WIP] to Added request cache to AWS provider and use it from variable resolution Dec 4, 2017

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 4, 2017

Have to adjust the unit tests. They fail, because they test for several invokes of the SSM API which is now cached if called for the same parameter.

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 4, 2017

@e-e-e I changed the cache to the same semantics as used for the variable cache now (implicit promise queue). Now it should be safe against excessive request calls. Please double check.

I will add an additional queue, as requests should be done in parallel with bluebird queue.

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 4, 2017

I added a specific unit test to the provider tests, that does 1000 request calls in parallel and checks that all callers get the expected correct result, but the request is only sent out once. This works and proves that the implicit promise caching works.

@HyperBrain HyperBrain force-pushed the cache-AWS-requests branch from 73b4b93 to c4a3c35 Dec 4, 2017

@HyperBrain HyperBrain requested review from RafalWilinski and horike37 Dec 4, 2017

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 4, 2017

@e-e-e Now it uses a queue for all requests sent (and restricts the maximum parallel requests to 2 at a time) in combination with the result caching. So subsequent duplicate requests are cached on-demand, and all different requests eventually submitted are queued.

@HyperBrain HyperBrain changed the title from Added request cache to AWS provider and use it from variable resolution to Added request cache and queue to AWS provider and use it from variable resolution Dec 4, 2017

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 4, 2017

Additionally did a dependency security check (to test for the new dependencies). No security issues have been found:
https://www.bithound.io/github/serverless/serverless/cache-AWS-requests/dependencies/npm#filter-insecure-dep

@e-e-e

This comment has been minimized.

Contributor

e-e-e commented Dec 4, 2017

@HyperBrain nice work - this should solve the issue. 👍

@horike37

This comment has been minimized.

Member

horike37 commented Dec 5, 2017

Thank you for implementing @HyperBrain and following up @e-e-e 👍 🎉
For doing this test, I created serverless.yml has a lot of S3: variable references and tried the rate limit error occurs with sls current version at first, but I was not able to do that...

How did you guys use the yml file looks like?

@horike37

Just tested it and it works like a charm @HyperBrain 👍 Rate exceeded issue has been resolved.
The code also is perfect from my view.
Thanks!

@horike37

This comment has been minimized.

Member

horike37 commented Dec 5, 2017

FYI: @RafalWilinski

Here is the yml file which the rate exceeded error occurs.
You might want to use if you were not able to reproduce it with your yml file.

serverless.yml

service: prototype-sls-cfn-rate

custom:
  conf: ${file(conf.yml)}
  deploymentBucket: ${self:custom.conf.deploymentBucket}
  var1: ${self:custom.conf.appBucket}1
  var2: ${self:custom.conf.appBucket}2
  var3: ${self:custom.conf.appBucket}3
  var4: ${self:custom.conf.appBucket}4
  var5: ${self:custom.conf.appBucket}5
  var6: ${self:custom.conf.appBucket}6
  var7: ${self:custom.conf.appBucket}7
  var8: ${self:custom.conf.appBucket}8
  var9: ${self:custom.conf.appBucket}9

provider:
  name: aws
  region: us-east-1
  deploymentBucket: ${self:custom.conf.deploymentBucket}

resources:
  Resources:
    AppBucket:
      Type: 'AWS::S3::Bucket'
      Properties:
        BucketName: ${self:custom.conf.appBucket}

conf.yml

deploymentBucket: ${cf:anotherstack.ServerlessDeploymentBucketName}
appBucket: slsAppBucket
testtest: ${cf:anotherstack.HelloLambdaFunctionArn}
aa: ${cf:anotherstack.ServiceEndpoint}
@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 5, 2017

I'll squash the commits now, so that it does not pollute master with 8 commits :)

HyperBrain added some commits Dec 4, 2017

Added request cache and queue to AWS provider
Make sure that requests are throttled. Use a queue for the requests.

Added new expected parameter to request() call in variables test

Added request cache tests
Check that request is indeed called 1000 times

Cache promises, not values

Use request cache for AWS variable requests

Use fromCallback instead of "new Promise" anti-pattern.

Added request cache to AWS provider

@HyperBrain HyperBrain force-pushed the cache-AWS-requests branch from 91f8325 to 8cd604c Dec 5, 2017

@RafalWilinski

This comment has been minimized.

Contributor

RafalWilinski commented Dec 5, 2017

Thanks for providing steps to reproduce @horike37. I'll test it locally later today. From the code perspective, everything seems fine to me.

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 6, 2017

I'll merge it now, so that I can rebase the circular vars branch again.

@HyperBrain HyperBrain merged commit c53b0b2 into master Dec 6, 2017

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 87.962%
Details

@HyperBrain HyperBrain deleted the cache-AWS-requests branch Dec 6, 2017

TsvetanMilanov added a commit to TsvetanMilanov/serverless-base-path-mapping that referenced this pull request Apr 12, 2018

Fix "Object argument required." error
There is a small breaking change in the serverless AWS provider api.
The `params` parameter in the request method is now required. This
happened after adding request cache -
serverless/serverless#4518
The solution is to pass empty object to the request method when we try
to get the rest apis.

TsvetanMilanov added a commit to Icenium/serverless-custom-domain that referenced this pull request Jun 6, 2018

Fix "Object argument required." error
There is a small breaking change in the serverless AWS provider api.
The `params` parameter in the request method is now required. This
happened after adding request cache -
serverless/serverless#4518
The solution is to pass empty object to the request method when we try
to get the rest apis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment