Add `binding` annotation to record an action path not resolved #4211

upgle · 2019-01-09T03:16:56Z

Description

If the user invokes an action in a package binding, they can't know a package binding path from which action is invoked. because the current activation structure only records the resolved action path.

for example, suppose we have the following entities:

package : pkg1
- action : pkg1/action1
package pkg2 bind pkg1
package pkg3 bind pkg1

If user invokes the pkg2/action1 action and the pkg3/action1 action, all path annotations of invoked action in binding packages are saved as namespace/pkg1/action1.

    "annotations": [
        {
            "key": "path",
            "value": "namespace/pkg1/action1"
        },
     ]

Suggestion

So i added an annotation to distinguish between resolved action path and invoked action path and opend this PR.

{
    "activationId": "f4250bb2721d4e4ca50bb2721d2e4cdd",
    "annotations": [
        {
            "key": "path",
            "value": "namespace/pkg1/action1"
        },
        {
            "key": "originPath",
            "value": "namespace/pkg2/action1"
        }
    ]
}

Related issue and scope

I opened an issue to propose and discuss this change (#????)

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.

codecov-io · 2019-01-09T09:03:18Z

Codecov Report

Merging #4211 into master will decrease coverage by 4.17%.
The diff coverage is 91.42%.

@@            Coverage Diff             @@
##           master    #4211      +/-   ##
==========================================
- Coverage   85.56%   81.39%   -4.18%     
==========================================
  Files         169      169              
  Lines        7833     7840       +7     
  Branches      536      525      -11     
==========================================
- Hits         6702     6381     -321     
- Misses       1131     1459     +328

Impacted Files	Coverage Δ
.../apache/openwhisk/core/controller/WebActions.scala	`92.33% <ø> (ø)`	⬆️
...enwhisk/core/entity/FullyQualifiedEntityName.scala	`91.42% <100%> (ø)`	⬆️
...apache/openwhisk/core/entity/WhiskActivation.scala	`92.06% <100%> (+0.12%)`	⬆️
...org/apache/openwhisk/core/controller/Actions.scala	`93.03% <100%> (-0.34%)`	⬇️
.../openwhisk/core/containerpool/ContainerProxy.scala	`91.79% <100%> (+0.03%)`	⬆️
...hisk/core/controller/actions/SequenceActions.scala	`92.3% <100%> (+0.2%)`	⬆️
...isk/core/controller/actions/PrimitiveActions.scala	`87.16% <100%> (+0.08%)`	⬆️
...org/apache/openwhisk/core/entity/WhiskAction.scala	`91.06% <78.57%> (+4.15%)`	⬆️
...core/database/cosmosdb/RxObservableImplicits.scala	`0% <0%> (-100%)`	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0% <0%> (-95.24%)`	⬇️
... and 10 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 58e3ddd...3044b13. Read the comment docs.

rabbah · 2019-01-09T19:07:58Z

I wonder if instead of modifying the Action type, we can add an alias to FullyQualifiedName. This would mean - if it works out - that you don't have to change the activation message and the name of the action and its alias (i.e., package and binding) are coupled more tightly.

I would use binding in the annotation and leave the annotation as is otherwise (shows the package name).

This is a suggestion for discussion.

upgle · 2019-01-10T09:58:21Z

As you mentioned, it would be better to add an alias to FullyQualifiedName.
I'll make a commit soon. 👍

upgle · 2019-01-15T06:00:21Z

@rabbah
I added binding field to FullyQualifiedName to not change the activation message, and renamed annotation key from originPath to binding.

this binding annotation contains only entityPath of a package binding.

{
    "activationId": "f4250bb2721d4e4ca50bb2721d2e4cdd",
    "annotations": [
        {
            "key": "binding",
            "value": "whisk.system/p2"
        },
        {
            "key": "path",
            "value": "guest/p1/hello"
        },
    ]
}

style95 · 2019-02-28T03:26:50Z

@rabbah
Is it possible to include this change? or any other direction is being considered?
In any case, I hope this issue is addressed so that we can resolve many binding related issues.

rabbah · 2019-02-28T09:31:53Z

Thanks for the nudge. Will get on it.

upgle · 2019-02-28T13:41:18Z

conflict has been resolved 😃

rabbah

this looks good generally but i think can be simplified further. You cleverly use toExecutableWhiskAction to add the binding and I suspect that you can do the same with the same method in the WhiskActionMetaData class --- this might end up removing some of the threading of binding throughout.

So that I'm not sending down the rabbit hole, I can dig into that a bit if you like.

rabbah · 2019-03-15T12:11:55Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala

@@ -303,6 +327,10 @@ case class ExecutableWhiskActionMetaData(namespace: EntityPath,
 def toWhiskAction =
 WhiskActionMetaData(namespace, name, exec, parameters, limits, version, publish, annotations)
 .revision[WhiskActionMetaData](rev)
+
+ def bindingFullyQualifiedName =


i suggest adding a type decorator on the method for the return value and a comment, explaining that the result is Some fully qualified name only if there's a binding, else None.

rabbah · 2019-03-15T12:12:18Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala

@@ -254,7 +274,8 @@ case class WhiskActionMetaData(namespace: EntityPath,
 * @param limits the limits to impose on the action
 * @param version the semantic version
 * @param publish true to share the action or false otherwise
- * @param annotation the set of annotations to attribute to the action
+ * @param annotations the set of annotations to attribute to the action
+ * @param binding the path of the package binding


-> @param binding the path of the package binding if any

rabbah · 2019-03-15T12:14:28Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskActivation.scala

@@ -137,6 +137,7 @@ object WhiskActivation

 /** Some field names for annotations */
 val pathAnnotation = "path"
+ val bindingAnnotation = "binding"


will check down below but this raises the opportunity to have the name in the activation be that of the binding, and the annotation is that of the package.

I'm sorry, I don't understand this.

In the example below, I think the name triple in the activation can not be the name of the binding whisk.system/p2

there is an action whisk.system/pkg1/triple

and the whisk.system/p2 is binding the whisk.system/pkg1

and i invoke an action whisk.system/p2/triple

{ "namespace": "whisk.system", "name": "triple", "version": "0.0.1", "subject": "whisk.system", "activationId": "b86606b16b8a4371a606b16b8a437182", "start": 1552900408086, "end": 1552900408090, "duration": 4, "annotations": [ { "key": "binding", "value": "whisk.system/p2" }, { "key": "path", "value": "whisk.system/pkg1/triple" }, ...

style95 · 2019-04-03T02:23:31Z

Any update on this PR?

upgle · 2019-04-08T01:39:50Z

Sorry. I should resolve a conflict. I can handle today.

upgle · 2019-04-08T07:33:02Z

@rabbah A conflict is resolved. could you please review again?

c.c. @style95

style95 · 2019-04-23T08:24:10Z

...controller/src/main/scala/org/apache/openwhisk/core/controller/actions/SequenceActions.scala

 * @param accounting the state of the sequence activation, contains the dynamic activation count, logs and payload for the next action
 * @param cause the activation id of the first sequence containing this activations
 * @return a future which resolves with updated accounting for a sequence, including the last result, duration, and activation ids
 */
 private def invokeNextAction(
 user: Identity,
 futureAction: Future[WhiskActionMetaData],
+ originAction: FullyQualifiedEntityName,


Is this being used in the call tree?

It seems not using in the call tree. I'll fix it.

style95 · 2019-04-23T08:25:28Z

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala

- * Once the package is resolved, the operation is dispatched to the action in the package
- * namespace.
- */
- private def mergeActionWithPackageAndDispatch(method: HttpMethod,


@upgle Could you elaborate the reason to remove this method a little bit?
It would help people who are not familiar with this change like me to get the intention more easily.

If an action is resolved before it's dispatched, there is no way to know its original name in the activate method because it's already resolved.
So, I modified it to resolve inside the activate method after it has been dispatched.

override def activate(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])( implicit transid: TransactionId) = { implicit transid: TransactionId)

upgle · 2019-04-29T04:59:19Z

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala

@@ -253,7 +247,7 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
 'result ? false,
 'timeout.as[FiniteDuration] ? WhiskActionsApi.maxWaitForBlockingActivation) { (blocking, result, waitOverride) =>
 entity(as[Option[JsObject]]) { payload =>
- getEntity(WhiskActionMetaData.get(entityStore, entityName.toDocId), Some {
+ getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {


@style95 the action is resolved here after it's dispatched.

ok, this is the similar way which is taken for web action.

style95 · 2019-04-29T07:14:38Z

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala

@@ -253,7 +247,7 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
 'result ? false,
 'timeout.as[FiniteDuration] ? WhiskActionsApi.maxWaitForBlockingActivation) { (blocking, result, waitOverride) =>
 entity(as[Option[JsObject]]) { payload =>
- getEntity(WhiskActionMetaData.get(entityStore, entityName.toDocId), Some {
+ getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {


ok, this is the similar way which is taken for web action.

style95 · 2019-04-30T06:39:29Z

@rabbah Do you have any comments on this?

style95 · 2019-05-07T05:11:56Z

I would merge this in 72 hours if there is no objection based on silent consent.

…e#4211) * Add origin path * Update document * Apply review * Update annotations and add a type decorator * Refactor code * Fix testcase * Reformat code * Refactor code * Remove unused code

upgle force-pushed the feature/add-origin-path branch from 3d632f3 to 5d6d2ef Compare January 9, 2019 08:29

upgle changed the title ~~Add originPath annotation to record the not resolved action path~~ Add originPath annotation to record an action path not resolved Jan 9, 2019

rabbah mentioned this pull request Jan 9, 2019

Impossible the see usage of a shared package #3241

Open

rabbah self-requested a review January 9, 2019 19:13

rabbah self-assigned this Jan 9, 2019

rabbah added enhancement controller awaits-contributor The contributor needs to respond to comments from reviewer. labels Jan 9, 2019

upgle force-pushed the feature/add-origin-path branch 2 times, most recently from 2a748b2 to ba5bcc8 Compare January 15, 2019 04:50

upgle changed the title ~~Add originPath annotation to record an action path not resolved~~ Add binding annotation to record an action path not resolved Jan 15, 2019

upgle force-pushed the feature/add-origin-path branch from ba5bcc8 to e8c06a2 Compare February 28, 2019 12:41

rabbah added review Review for this PR has been requested and yet needs to be done. and removed awaits-contributor The contributor needs to respond to comments from reviewer. labels Feb 28, 2019

rabbah reviewed Mar 15, 2019

View reviewed changes

upgle force-pushed the feature/add-origin-path branch from cba6677 to 848dbea Compare March 18, 2019 15:22

upgle added 5 commits April 8, 2019 10:54

Add origin path

5b5708b

Update document

de5a09b

Apply review

78aad4b

Update annotations and add a type decorator

2b331a7

Refactor code

316ecc5

upgle added 2 commits April 8, 2019 10:54

Fix testcase

dc3006f

Reformat code

2e79409

upgle force-pushed the feature/add-origin-path branch from 3044b13 to 2e79409 Compare April 8, 2019 01:54

Refactor code

210a5ff

style95 reviewed Apr 23, 2019

View reviewed changes

upgle commented Apr 29, 2019

View reviewed changes

Remove unused code

b36b6f4

style95 approved these changes Apr 29, 2019

View reviewed changes

style95 merged commit 7a304c1 into apache:master May 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `binding` annotation to record an action path not resolved #4211

Add `binding` annotation to record an action path not resolved #4211

upgle commented Jan 9, 2019 •

edited

Loading

codecov-io commented Jan 9, 2019 •

edited

Loading

rabbah commented Jan 9, 2019

upgle commented Jan 10, 2019 •

edited

Loading

upgle commented Jan 15, 2019 •

edited

Loading

style95 commented Feb 28, 2019

rabbah commented Feb 28, 2019

upgle commented Feb 28, 2019 •

edited

Loading

rabbah left a comment

rabbah Mar 15, 2019

rabbah Mar 15, 2019

rabbah Mar 15, 2019

upgle Mar 18, 2019 •

edited

Loading

style95 commented Apr 3, 2019

upgle commented Apr 8, 2019

upgle commented Apr 8, 2019

style95 Apr 23, 2019

upgle Apr 29, 2019

style95 Apr 23, 2019

upgle Apr 29, 2019

upgle Apr 29, 2019

style95 Apr 29, 2019

style95 Apr 29, 2019

style95 commented Apr 30, 2019

style95 commented May 7, 2019

Add binding annotation to record an action path not resolved #4211

Add binding annotation to record an action path not resolved #4211

Conversation

upgle commented Jan 9, 2019 • edited Loading

Description

Suggestion

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

codecov-io commented Jan 9, 2019 • edited Loading

Codecov Report

rabbah commented Jan 9, 2019

upgle commented Jan 10, 2019 • edited Loading

upgle commented Jan 15, 2019 • edited Loading

style95 commented Feb 28, 2019

rabbah commented Feb 28, 2019

upgle commented Feb 28, 2019 • edited Loading

rabbah 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

upgle Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

style95 commented Apr 3, 2019

upgle commented Apr 8, 2019

upgle commented Apr 8, 2019

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

style95 commented Apr 30, 2019

style95 commented May 7, 2019

Add `binding` annotation to record an action path not resolved #4211

Add `binding` annotation to record an action path not resolved #4211

upgle commented Jan 9, 2019 •

edited

Loading

codecov-io commented Jan 9, 2019 •

edited

Loading

upgle commented Jan 10, 2019 •

edited

Loading

upgle commented Jan 15, 2019 •

edited

Loading

upgle commented Feb 28, 2019 •

edited

Loading

upgle Mar 18, 2019 •

edited

Loading