Treat action code as attachments for created/updated actions #2847

dubee · 2017-10-10T16:07:17Z

Save action code as attachments in the database for new or updated actions.

Depends on #2832 to get whisk.core.controller.test.ActionsApiTests > Actions API should put and then get action from cache test to pass.

I combined these changes with #2730 and performed profiling on the controller.

Here is a graph of the heap monitoring when invoking a 34MB zipped action:

Previously invoking a 34MB non-cached action would result in a spike in heap usage when only using changes in #2730:

dubee · 2017-10-10T16:08:04Z

@csantanapr, FYI.

dubee · 2017-10-11T21:54:15Z

common/scala/src/main/scala/whisk/core/entity/Exec.scala

+                case _ =>
+                  obj.fields.get("binary") match {
+                    case Some(JsBoolean(b)) => b
+                    case _                  => false


Should probably error here since code and binary property is not defined.

dubee · 2017-10-11T23:03:15Z

tests/src/test/scala/whisk/core/controller/test/ActionsApiTests.scala

@@ -138,7 +138,7 @@ class ActionsApiTests extends ControllerTestCommon with WhiskActionsApi {
    Get(s"$collectionPath/${action.name}") ~> Route.seal(routes(creds)) ~> check {
      status should be(OK)
      val response = responseAs[WhiskAction]
-      response should be(action)
+      response.toJson should be(action.toJson)


Direct comparison of the WhiskAction here fails. However, comparing the actions as JSON seems to work.

dubee · 2017-10-13T19:51:01Z

common/scala/src/main/scala/whisk/core/entity/Exec.scala

              } map {
                attFmt[String].read(_)
              } getOrElse {
-                throw new DeserializationException(
-                  s"'code' must be a valid base64 string in 'exec' for '$kind' actions")
+                throw new DeserializationException(s"'code' must be a string defined in 'exec' for '$kind' actions")


Not sure we will ever get here. Since I think the function in map will work even for an empty string.

dubee · 2017-10-16T21:27:44Z

Can expand tests from #2855 after that one is merged.

dubee · 2017-10-16T21:43:27Z

whisk.core.database.test.CacheConcurrencyTests > the cache should support concurrent CRUD without bogus residual cache entries, iter 1 FAILED

Failure is a result of parallel action gets with updates. I think this test fails because there is now an extra database write for the attachment that causes the action get to fail as it does not have the correct document revision.

rabbah

This looks generally good - I'm surprised no test changes were required. I suspect because tests check for action equality which will be opaque to the attachment change.

We will need a test which shows the controller can tolerate an action in the previous schema and migrates it to the new schema.

rabbah · 2017-11-25T01:54:48Z

common/scala/src/main/scala/whisk/core/entity/Exec.scala

-                // java actions once stored the attachment in "jar" instead of "code"
-                obj.fields.get("code").orElse(obj.fields.get("jar"))
+              // java actions once stored the attachment in "jar" instead of "code"
+              val code = obj.fields.get("code").orElse(obj.fields.get("jar"))


perhaps as part of the schema migration pr, you can normalize all jar occurrences to code and we can remove this legacy bit.

As part of #2938 I have normalized jar to code. Will have to remove the jar reference here.

dubee · 2017-11-27T21:06:57Z

@rabbah, I believe I had originally planned to have #2855 merged first and update the tests in that PR for this one. I can add the tests here as well.

dubee · 2018-01-06T03:24:13Z

Pushed changes to use ansible/files/runtimes.json since the runtimes were removed from ansible/group_varz/all.

dubee · 2018-02-14T15:37:58Z

common/scala/src/main/scala/whisk/core/entity/Exec.scala

-    copy(code = Inline(encoded))
+    val code = new String(bytes, StandardCharsets.UTF_8)
+
+    if (kind == "java" && !Exec.isBinaryCode(code)) {


Allows backward compatibility with old Java actions that have attachments that are not base64.

markusthoemmes · 2018-02-28T16:29:54Z

@dubeejw What's the status here? The latest Travis failed.

codecov-io · 2018-05-01T20:12:17Z

Codecov Report

Merging #2847 into master will decrease coverage by 0.07%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2847      +/-   ##
==========================================
- Coverage    74.5%   74.42%   -0.08%     
==========================================
  Files         126      126              
  Lines        5950     5955       +5     
  Branches      378      377       -1     
==========================================
- Hits         4433     4432       -1     
- Misses       1517     1523       +6

Impacted Files	Coverage Δ
...src/main/scala/whisk/core/entity/WhiskAction.scala	`77.62% <100%> (ø)`	⬆️
.../scala/src/main/scala/whisk/core/entity/Exec.scala	`79.37% <81.81%> (-4.5%)`	⬇️
...src/main/scala/whisk/core/entity/Attachments.scala	`88.88% <0%> (+11.11%)`	⬆️

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 d13ff86...eddb1c7. Read the comment docs.

dubee · 2018-05-08T17:37:47Z

@markusthoemmes, Travis is now passing. Awaiting more feed back.

dubee · 2018-05-10T14:07:13Z

Closed in favor of #3286

dubee added controller wip labels Oct 10, 2017

dubee force-pushed the code-attach branch 4 times, most recently from 9d2e029 to 67f8765 Compare October 11, 2017 21:53

dubee commented Oct 11, 2017

View reviewed changes

dubee force-pushed the code-attach branch 2 times, most recently from 96b143b to f7f3b6a Compare October 12, 2017 06:09

dubee changed the title ~~Treat action code as attachments for created/update actions~~ Treat action code as attachments for created/updated actions Oct 12, 2017

dubee commented Oct 13, 2017

View reviewed changes

dubee requested a review from rabbah October 16, 2017 18:46

dubee added review Review for this PR has been requested and yet needs to be done. and removed wip labels Oct 16, 2017

dubee force-pushed the code-attach branch 2 times, most recently from 31ce900 to ea5e9fa Compare October 21, 2017 20:56

dubee mentioned this pull request Nov 7, 2017

Convert inline code to attachment in database #2938

Merged

rabbah self-assigned this Nov 13, 2017

dubee force-pushed the code-attach branch from ea5e9fa to 577be81 Compare November 20, 2017 21:46

rabbah reviewed Nov 25, 2017

View reviewed changes

dubee force-pushed the code-attach branch 5 times, most recently from 0639141 to c6563a0 Compare December 1, 2017 02:33

dubee force-pushed the code-attach branch 3 times, most recently from 210d98d to 740d0fb Compare December 12, 2017 05:08

dubee force-pushed the code-attach branch from 740d0fb to bf1af08 Compare January 6, 2018 02:59

dubee force-pushed the code-attach branch from bf1af08 to c90ba78 Compare January 6, 2018 04:36

markusthoemmes assigned markusthoemmes and unassigned rabbah Feb 14, 2018

dubee commented Feb 14, 2018

View reviewed changes

dubee mentioned this pull request Feb 14, 2018

Treat action code as attachments for created/updated actions #3286

Closed

markusthoemmes added the awaits-feedback A reviewer has asked a question and/or asked for a change. label Feb 28, 2018

markusthoemmes added needs-review and removed needs-review labels Feb 28, 2018

dubee force-pushed the code-attach branch 4 times, most recently from 29c12fd to e068182 Compare April 19, 2018 14:43

dubee added 6 commits May 1, 2018 15:25

Treat action code as attachments for created/update actions

907a3ef

Refactor tests

6cdbde6

Use runtime.json

ba4001b

Update exec helpers

38da82c

Fix unicode issue when saving actions

88ba0c6

Allow 404s and 200s for concurrency get+update test

eddb1c7

dubee force-pushed the code-attach branch from e068182 to eddb1c7 Compare May 1, 2018 19:25

dubee closed this May 10, 2018

chetanmeh mentioned this pull request Aug 7, 2018

Treat action code as attachments #3945

Merged

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat action code as attachments for created/updated actions #2847

Treat action code as attachments for created/updated actions #2847

dubee commented Oct 10, 2017 •

edited

Loading

dubee commented Oct 10, 2017

dubee Oct 11, 2017 •

edited

Loading

dubee Oct 11, 2017

dubee Oct 13, 2017 •

edited

Loading

dubee commented Oct 16, 2017 •

edited

Loading

dubee commented Oct 16, 2017

rabbah left a comment

rabbah Nov 25, 2017

dubee Nov 27, 2017

dubee commented Nov 27, 2017

dubee commented Jan 6, 2018

dubee Feb 14, 2018

markusthoemmes commented Feb 28, 2018

codecov-io commented May 1, 2018

dubee commented May 8, 2018

dubee commented May 10, 2018

Treat action code as attachments for created/updated actions #2847

Treat action code as attachments for created/updated actions #2847

Conversation

dubee commented Oct 10, 2017 • edited Loading

dubee commented Oct 10, 2017

dubee Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

dubee Oct 11, 2017

Choose a reason for hiding this comment

dubee Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

dubee commented Oct 16, 2017 • edited Loading

dubee commented Oct 16, 2017

rabbah left a comment

Choose a reason for hiding this comment

rabbah Nov 25, 2017

Choose a reason for hiding this comment

dubee Nov 27, 2017

Choose a reason for hiding this comment

dubee commented Nov 27, 2017

dubee commented Jan 6, 2018

dubee Feb 14, 2018

Choose a reason for hiding this comment

markusthoemmes commented Feb 28, 2018

codecov-io commented May 1, 2018

Codecov Report

dubee commented May 8, 2018

dubee commented May 10, 2018

dubee commented Oct 10, 2017 •

edited

Loading

dubee Oct 11, 2017 •

edited

Loading

dubee Oct 13, 2017 •

edited

Loading

dubee commented Oct 16, 2017 •

edited

Loading