Limit of binary actions is too low. #3835

cbickel · 2018-07-03T08:42:12Z

On creating an action, the code is sent to the controller. Afterwards the controller checks, that the code is not too big. If the code itself is sent directly (e.g. uploading a single .js file), the limit is 48MB (today).
If an archive is uploaded, it will be encoded with base64. The problem here is, that base64 has an overhead of 1/3. This means, if you have an archive, that has a size of e.g. 45MB, 60MB will be sent to the controller. So the request will be rejected. This is not expected for the user, as a limit of 48MB was proposed to him.

This PR fixes that bug.

Related issue and scope

I opened an issue to propose and discuss this change (apache-mailing-list)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
Enhancement or new feature (adds new functionality).
Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

I signed an Apache CLA.
I reviewed the style guides and followed the recommendations (Travis CI will check :).
I added tests to cover my changes.
My changes require further changes to the documentation.
I updated the documentation where necessary.

rabbah · 2018-07-03T16:59:01Z

@cbickel will this PR also close #3712?

cbickel · 2018-07-04T06:29:37Z

@rabbah Thanks for the pointer. I wasn't aware of this issue. But I think this PR will close this issue as well.

codecov-io · 2018-07-06T07:36:52Z

Codecov Report

Merging #3835 into master will decrease coverage by 4.74%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3835      +/-   ##
==========================================
- Coverage   75.81%   71.06%   -4.75%     
==========================================
  Files         146      146              
  Lines        6902     6906       +4     
  Branches      428      428              
==========================================
- Hits         5233     4908     -325     
- Misses       1669     1998     +329

Impacted Files	Coverage Δ
...src/main/scala/whisk/core/entity/WhiskEntity.scala	`91.66% <ø> (ø)`	⬆️
.../scala/src/main/scala/whisk/core/entity/Exec.scala	`83.87% <100%> (+0.1%)`	⬆️
.../scala/src/main/scala/whisk/core/WhiskConfig.scala	`92.06% <100%> (+0.06%)`	⬆️
.../scala/src/main/scala/whisk/core/entity/Size.scala	`96.29% <100%> (+0.14%)`	⬆️
...a/src/main/scala/whisk/core/entity/Parameter.scala	`95.34% <100%> (ø)`	⬆️
...core/database/cosmosdb/RxObservableImplicits.scala	`0% <0%> (-100%)`	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0% <0%> (-95.08%)`	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	`0% <0%> (-92.6%)`	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	`0% <0%> (-81.82%)`	⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala	`0% <0%> (-58.83%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 967f005...69cd891. Read the comment docs.

cbickel · 2018-07-09T11:30:29Z

PG5#514 🔵

sven-lange-last

Overall, very good pull request. Only minor suggestions regarding addition of tests and comment clarification.

sven-lange-last · 2018-07-17T12:12:37Z

ansible/group_vars/all

@@ -56,6 +56,8 @@ limits:
  concurrentInvocationsSystem:  "{{ limit_invocations_concurrent_system | default(5000) }}"
  firesPerMinute: "{{ limit_fires_per_minute | default(60) }}"
  sequenceMaxLength: "{{ limit_sequence_max_length | default(50) }}"
+  actionCodeMb: "{{ limit_action_code_mb | default(48) }}"


I suggest to add a comment indicating this is the net action code limit while the gross limit is 4/3 * the net limit because of Base64 encoding.

sven-lange-last · 2018-07-17T13:52:10Z

ansible/group_vars/all

@@ -56,6 +56,8 @@ limits:
  concurrentInvocationsSystem:  "{{ limit_invocations_concurrent_system | default(5000) }}"
  firesPerMinute: "{{ limit_fires_per_minute | default(60) }}"
  sequenceMaxLength: "{{ limit_sequence_max_length | default(50) }}"
+  actionCodeMb: "{{ limit_action_code_mb | default(48) }}"
+  parametersMb: "{{ limit_parameters_mb | default(1) }}"


I suggest to add a comment indicating that this size limit also applies to annotations, not only to parameters. And in contrast to the action code size limit, this is a gross limit because we do not perform any Base64 encoding for parameters / annotations.

sven-lange-last · 2018-07-17T14:05:53Z

ansible/roles/controller/tasks/deploy.yml

@@ -124,6 +124,10 @@
      "COMPONENT_NAME": "{{ controller_name }}"
      "PORT": 8080

+      # Allow base64 overhead of action-code, parameters and annotations.


This comment is a little misleading because there is no Base64 overhead for parameters and annotations.

sven-lange-last · 2018-07-17T14:45:15Z

ansible/roles/controller/tasks/deploy.yml

@@ -124,6 +124,10 @@
      "COMPONENT_NAME": "{{ controller_name }}"
      "PORT": 8080

+      # Allow base64 overhead of action-code, parameters and annotations.
+      "CONFIG_akka_http_server_parsing_maxContentLength": "{{ (limits.actionCodeMb | int * 4 / 3 + limits.parametersMb | int * 2) | round(0, 'ceil') | int }}m"


It may be valuable to add a comment that this setting does only consider the code, parameters and annotations parts in JSON messages because these are potentially the largest parts. If somebody uses very large values for code, a long name value may still overflow the message body.

sven-lange-last · 2018-07-18T09:18:05Z

tests/src/test/scala/whisk/core/limits/ActionLimitsTests.scala

@@ -328,6 +328,25 @@ class ActionLimitsTests extends TestHelpers with WskTestHelpers with WskActorSys
    actionCode.delete
  }

+  it should "succeed on creating an binary-action with binary exec which is not too big before encoding" in withAssetCleaner(


I think you should add a test that actually invokes a very large action to make sure that the DB client in the invoker is able to GET a large action JSON. This should be a large action which is Base64 encoded.

Maybe you could also add a component test for adding / updating / deleting a large action to https://github.com/apache/incubator-openwhisk/blob/master/tests/src/test/scala/whisk/core/entity/test/DatastoreTests.scala?

sven-lange-last · 2018-07-18T09:31:02Z

ansible/roles/nginx/templates/nginx.conf.j2

@@ -10,7 +10,8 @@ events {

 http {
 {# allow large uploads, need to thread proper limit into here #}
-    client_max_body_size 50M;
+    # Allow base64 overhead of action-code, parameters and annotations.


This comment is a little misleading because there is no Base64 overhead for parameters and annotations.

sven-lange-last · 2018-07-18T10:05:26Z

tests/src/test/scala/whisk/core/entity/test/SizeTests.scala

@@ -93,6 +93,30 @@ class SizeTests extends FlatSpec with Matchers {
    }
  }

+  // Multiplication


Add a test for: 1.B * 0. What happens if you multiply with / divide by negative numbers? Should this be supported?

sven-lange-last · 2018-07-18T11:05:05Z

tests/src/test/scala/whisk/core/entity/test/SizeTests.scala

+    1.MB / 512 should be(2 KB)
+  }
+
+  it should "throw an exception if division is through 0" in {


... if division is by 0 ...

sven-lange-last · 2018-07-18T12:57:02Z

A more general finding: PR #3209 introduced a configurable limit for parameters in an activation. I guess it makes sense to use the same configuration settings for parameter size and activation parameter size. Can you please check how this could be done?

style95 · 2019-01-25T02:31:18Z

Is there any task in-progress regarding this issue?
Or is it superseded by #4156 ?

rabbah · 2019-02-09T14:33:22Z

@cbickel this has rotted - what's the plan?

cbickel · 2019-02-11T11:46:40Z

I'll close the PR for now, due to long inactivity from my side.

cbickel requested a review from markusthoemmes July 3, 2018 08:51

cbickel assigned markusthoemmes Jul 3, 2018

cbickel added the review Review for this PR has been requested and yet needs to be done. label Jul 3, 2018

cbickel force-pushed the lim branch from aeb5799 to 663e259 Compare July 3, 2018 11:50

cbickel force-pushed the lim branch from 663e259 to a07bbc4 Compare July 9, 2018 06:12

cbickel force-pushed the lim branch 2 times, most recently from 965676c to 69cd891 Compare July 16, 2018 07:55

sven-lange-last requested changes Jul 18, 2018

View reviewed changes

tz70s mentioned this pull request Jul 23, 2018

Introduce configurable action blob size limit. #3757

Closed

21 tasks

markusthoemmes removed their request for review July 24, 2018 16:42

markusthoemmes assigned sven-lange-last and unassigned markusthoemmes Jul 24, 2018

cbickel added 7 commits July 25, 2018 15:55

Add division and multiplication to ByteSize.

b6d390b

Handle base64 overhaead in Action size calculation.

3ee8268

Allow base64 overhead in nginx and akka.

1685059

Make code-limit and parameters-limit configurable.

9c2f69d

Add test to verify that binary action will be accepted.

dcb0bbe

Refactoring.

947a6c5

Review.

0260672

cbickel force-pushed the lim branch from 69cd891 to 0260672 Compare July 26, 2018 09:45

rabbah mentioned this pull request Dec 4, 2018

Make exec aka code max size configurable #4156

Merged

21 tasks

cbickel closed this Feb 11, 2019

cbickel deleted the lim branch February 11, 2019 11:46

style95 mentioned this pull request Jul 12, 2022

Increase the payload limit of Nginx. #5279

Merged

22 tasks

Limit of binary actions is too low. #3835

Limit of binary actions is too low. #3835

Uh oh!

Conversation

cbickel commented Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

rabbah commented Jul 3, 2018

Uh oh!

cbickel commented Jul 4, 2018

Uh oh!

codecov-io commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cbickel commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sven-lange-last left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sven-lange-last commented Jul 18, 2018

Uh oh!

style95 commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rabbah commented Feb 9, 2019

Uh oh!

cbickel commented Feb 11, 2019

Uh oh!

Uh oh!

cbickel commented Jul 3, 2018 •

edited

Loading

codecov-io commented Jul 6, 2018 •

edited

Loading

cbickel commented Jul 9, 2018 •

edited

Loading

style95 commented Jan 25, 2019 •

edited

Loading