Use latest code if action's revision is mismatched #4954

upgle · 2020-08-28T02:11:13Z

Description

It fixes #4953

If the action is updated while the sequence action is executing, the old revision is referred to when the sub action is executed.

1. [InvokerReactive] guest/B guest 8cd79... 38-41cde62e7493f5ffd83f31939dce8a84
2. [InvokerReactive] guest/B guest 2db43... 39-e8576e618c86251c246fc8c6804dbce2
3. [InvokerReactive] guest/B guest 72d6f... 40-a3892ba52d244f27b00a6526e5388e00
4. [InvokerReactive] guest/B guest 969be... 41-ad44c086275d0d7e4631298a3107f96c
5. [InvokerReactive] guest/B guest 0f7d3... 37-9f5e794ffcfc7d556a9154c39a1ce700 <- old revision is reffered

At this time, if the DB server returns the latest revision, then a boxed error occurs due to an assert condition.

assert(doc.rev.rev == null || doc.rev.rev == responseRev, "Returned revision should match original argument")

error message

{
    "response": {
        "status": "whisk internal error",
        "statusCode": 0,
        "success": false,
        "result": {
            "error": "Action could not be fetched."
        }
    }
}

To avoid this error, I've updated the code to retry with the latest code if the revision is different.

Related issue and scope

Internal error occurs when updating an sub-action while running a sequence action (Boxed error) #4953

My changes affect the following components

Invoker

Types of changes

Bug fix (generally a non-breaking change which closes an issue).

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.

style95 · 2020-08-28T02:45:32Z

core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InvokerReactive.scala

+        case DocumentRevisionMismatchException(_) =>
+          // if revision is mismatched, the action may have been updated,
+          // so try again with the latest code
+          handleActivationMessage(msg.copy(revision = DocRevision.empty))


This can be controversial but I think the system should take care of this.
This is to handle the case where the underlying actions are updated while a sequence action is being invoked.
If we consider the sequence as just a coordinator for underlying actions, it would be fine for it to always invoke the latest code. Once a sequence action is defined with some actions, we don't need to update the sequence action whenever the underlying actions are updated. It means the sequence action itself does not care about the version of actions in it but just focuses on the relation and the execution order of them.
So I think it is reasonable to invoke the latest codes all the time.

And regarding the implementation, while this is great, can we differentiate the sequence case with the others?
I feel like there can be some side effects.

For example, if any activation sent to Kafka arrives at the invoker side late, it could happen.
In such a case the activation is intended for the old codes but the latest code will be invoked with this change.

I did not look into code deeply yet, but can we make the controller setup subsequent activations with the latest codes while invoking a sequence action?

Thank you for your review and I understand your concern.

For example, if any activation sent to Kafka arrives at the invoker side late, it could happen.
In such a case the activation is intended for the old codes but the latest code will be invoked with this change.

Currently, Openwhisk doesn't have a versioning feature. So I don't think there's any problem with always serving the latest code if the DB can't fetch the older version, because action developers are responsible for compatibility between old and new action codes.

HDYT?

Not quite sure about this.
It would be great to listen to other reviewers' opinions as well.
From some point of view, it could be a semantic change as activations that are supposed to be rejected would be invoked successfully.
(With an assumption that the codes are backward compatible.)

How about sending an email to the dev list to get more people to attend this PR?

Can we leave at least a warning log to describe the system takes a fallback to the latest codes because of the revision mismatch?

Since you're also working on versioning - I think this change should be considered in that broader context.

If the sequence or composition references actions specifically by version, then it should be an error to invoke an alternate version. If the sequence uses "latest" then this change is acceptable.

Was the intent for this change strictly to address compositions?

Yes.
That needs to be considered in the action versioning feature I think.

CC: @jiangpengcheng

upgle · 2020-08-31T07:55:35Z

tests/src/test/scala/system/basic/WskSequenceTests.scala

@@ -501,6 +501,46 @@ class WskSequenceTests extends TestHelpers with WskTestHelpers with StreamLoggin
    checkEchoSeqRuleResult(newRun, seqName, JsObject(newPayload))
  }

+  it should "run a sub-action even if it is updated while the sequence action is running" in withAssetCleaner(wskprops) {


I've added a test case. And this test currently does not pass on the master branch.

style95

LGTM with a minor nit.

style95 · 2020-09-29T04:27:56Z

core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InvokerReactive.scala

+        case DocumentRevisionMismatchException(_) =>
+          // if revision is mismatched, the action may have been updated,
+          // so try again with the latest code
+          handleActivationMessage(msg.copy(revision = DocRevision.empty))


Can we leave at least a warning log to describe the system takes a fallback to the latest codes because of the revision mismatch?

rabbah

Sorry for the late questions, I couldn't look sooner.

rabbah · 2020-10-07T13:57:16Z

core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InvokerReactive.scala

-            }
-            .recoverWith {
-              case t =>
-                // If the action cannot be found, the user has concurrently deleted it,


This comment is still valid - why was it removed?

rabbah · 2020-10-07T13:59:42Z

core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InvokerReactive.scala

+        case DocumentRevisionMismatchException(_) =>
+          // if revision is mismatched, the action may have been updated,
+          // so try again with the latest code
+          handleActivationMessage(msg.copy(revision = DocRevision.empty))


Since you're also working on versioning - I think this change should be considered in that broader context.

If the sequence or composition references actions specifically by version, then it should be an error to invoke an alternate version. If the sequence uses "latest" then this change is acceptable.

Was the intent for this change strictly to address compositions?

style95 reviewed Aug 28, 2020

View reviewed changes

Use latest code if action's revision is mismatched

f2e25f1

upgle force-pushed the fix-boxed-error branch from 75abd79 to f2e25f1 Compare August 28, 2020 06:48

Add test casae

ce7846e

upgle commented Aug 31, 2020

View reviewed changes

style95 approved these changes Sep 29, 2020

View reviewed changes

style95 merged commit ef55353 into apache:master Oct 5, 2020

rabbah reviewed Oct 7, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use latest code if action's revision is mismatched #4954

Use latest code if action's revision is mismatched #4954

upgle commented Aug 28, 2020 •

edited

style95 Aug 28, 2020 •

edited

upgle Aug 28, 2020 •

edited

style95 Aug 31, 2020

style95 Sep 29, 2020 •

edited

rabbah Oct 7, 2020

style95 Oct 8, 2020

upgle Aug 31, 2020

style95 left a comment

style95 Sep 29, 2020 •

edited

rabbah left a comment

rabbah Oct 7, 2020

rabbah Oct 7, 2020

Use latest code if action's revision is mismatched #4954

Use latest code if action's revision is mismatched #4954

Conversation

upgle commented Aug 28, 2020 • edited

Description

error message

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

style95 Aug 28, 2020 • edited

Choose a reason for hiding this comment

upgle Aug 28, 2020 • edited

Choose a reason for hiding this comment

style95 Aug 31, 2020

Choose a reason for hiding this comment

style95 Sep 29, 2020 • edited

Choose a reason for hiding this comment

rabbah Oct 7, 2020

Choose a reason for hiding this comment

style95 Oct 8, 2020

Choose a reason for hiding this comment

upgle Aug 31, 2020

Choose a reason for hiding this comment

style95 left a comment

Choose a reason for hiding this comment

style95 Sep 29, 2020 • edited

Choose a reason for hiding this comment

rabbah left a comment

Choose a reason for hiding this comment

rabbah Oct 7, 2020

Choose a reason for hiding this comment

rabbah Oct 7, 2020

Choose a reason for hiding this comment

upgle commented Aug 28, 2020 •

edited

style95 Aug 28, 2020 •

edited

upgle Aug 28, 2020 •

edited

style95 Sep 29, 2020 •

edited

style95 Sep 29, 2020 •

edited