-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support action continuations in the controller #3202
Conversation
worth an email to the mailing list, this is a sweet feature. |
docs? (i.e. .md) |
The code is documented. I'll add a docs/compositions.md. But the real power of compositions will come from composer and I am not going to document this here. |
…tion compositions
I rewrote the commit history as requested. |
Am I wrong to be worried that this may confuse folks with Composer? Like - I assumed this was related and it seems to have some similarities but if it is truly unique, maybe the name "composition" should be changed? |
"The annotation is ignored on sequences" |
We can support the annotation on sequences. But I would like to do sequences as a separate PR as this one is quite large already. |
Composer is being rewritten to take advantage of this PR internally. The composer API is unchanged. The PR makes it possible to execute action compositions efficiently. Composer will continue to help author action compositions by automatically generating the conductor action code from a high-level specification. For example, code equivalent to the double increment |
... interesting. So a user would have 2 main ways to make compositions. Either using Composer or "by hand". Of course, when I work with Composer I'm working with a local file anyway. It feels like this could be - potentially - a bit confusing for the end user. I'm not saying this is bad in anyway, but I worry about how I'd explain this to a group of devs, know what I mean? Could you elaborate on when I'd use Composer vs compositions? (And since Compser makes compositions, you can see already it's a bit confusing. ;) |
Always use Composer! It is much easier, less error prone, and there is no real drawback. There is a good reason I have not written docs for compositions. :D |
So if it fair to say: "This PR changes stuff beneath the covers and Ray you can stop paying attention." Cuz I'm totally fine with ignoring this if so. :) |
We have been discussing ways to run composer server-side, i.e., upload the composer source directly as opposed to having to compile the composition locally. I suspect we will end up supporting this mode of operation in some way. |
Yes you can think of this PR as improving the Composer backend. The conductor action "language" is not really meant for end users. But the PR will have visible positive impact on Composer. First, the distinction between actions and apps will basically disappear. Second, REDIS will be an option (for compositions with large amounts of state) not a requirement. |
Thank you - I appreciate you taking the time to explain this to me! (I wanted to do more than just a thumbs up - I really am thankful.) |
The distinction between composer and compositions is not gratuitous... This PR with the addition of action compositions is about adding a fundamental capability to the OpenWhisk controller with as little a bias as possible. It does not favor a specific approach to building composition. Composer on the other hand embodies many design choices. For instance, it currently targets Javascript. In contrast, the PR is language/runtime agnostic. While I don't think writing compositions by hand is a compelling proposition for OpenWhisk users, there could be many alternatives to Composer. All would benefit from the PR in the same way. |
Ah the JS bias in Composer is a very good point. This would definitely help the non-JS folks. |
This is a change that end users would not use directly, but is a change in the primitives to a core/kernel component (controller) to allow more efficient (store and forward state) and language agnostic applications. This is a feature to be use by someone building a framework or tooling for end user (i.e. downstream integrators/providers), but not end users (serverless developers). So the docs should be written for a downstream integrator/provider, on how the primitive works and some examples how to construct an orchestration to illustrate the concepts. Similar on what you did in your opening comment above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to clarify on the need of mutability/non-deterministic concurrency methods in the code. This is not a concern of making it pretty but very much a concern of me having had a very hard time following the code around.
* @param components the current count of component actions already invoked | ||
* @param conductors the current count of conductor actions already invoked | ||
*/ | ||
private case class CompositionAccounting(var components: Int = 0, var conductors: Int = 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the mutability needed? Could we keep the object immutable and create copies instead? In general that's much more readable and much easier to follow, flow-wise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to mutability, it is clear that there is only one CompositionAccounting object ever allocated in the course of the execution of a composition. Therefore there is only one set of counters. With object copies on the other hand, it is hard to keep track of the copies. Why are copies being made, where, when? Are the multiple sets of counters concurrently valid and useful? Moreover, immutability does not prevent forking the object, hence concurrency issues.
Give me ownership types and strong updates... and then I buy your argument. But without ownership control, I am afraid we are discussing style more than safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, although I tend to disagree. Having immutable state forces locality of changes vs. being able to pass an object around and magically updating it somewhere else. If you'd want to update state in a method you'd need to make it clear through the return type, that you're returning a new version of that state vs. magically manipulating it elsewhere.
I think though, once the Promises are out of the picture, that notion might become more prominent. I'd be okay focusing on getting the Promise out as the concern is much more around that than around the mutable state.
cause: Option[ActivationId], | ||
var duration: Long, | ||
var maxMemory: Int, | ||
var state: Option[JsObject], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment on mutability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer. I want one Session object for one session. I believe this one to one mapping makes the code easier to follow. Part of what constitutes a session-its state-changes over the course of the execution, so the corresponding fields of the object are mutable.
If I made this class immutable and used object copies instead as suggested, then the distinction between immutable session elements and mutable session elements would disappear from the type and become buried in the code and comments. I would argue this obfuscates intend and hinder readability.
accounting: CompositionAccounting, | ||
logs: Buffer[ActivationId], | ||
caller: Option[Session], | ||
result: Option[Promise[Either[ActivationId, WhiskActivation]]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll put the comment here, but the concern is around the other functions as well: Do you need to pass a Promise around vs. chaining all actions together using Futures?
When glancing over the code it was very hard to tell for me, where this promise is used and which of the other methods does which side-effects. They all return Unit
. There is no way for me to safely tell what is going on if I do not read all of the source-code.
I believe this is a perfect use-case to make use of Future
s and chain the methods together and I think the code will very much profit from that. It'll make it virtually impossible to miss cases and/or have dead-ends in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll replace the promise with a future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed.
case Failure(t) => | ||
// parsing failure | ||
Future.successful(ActivationResponse.applicationError(compositionComponentInvalid(next))) | ||
case Success(next) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could save a level of nesting by doing this:
case Success(next) if (session.accounting.components >= actionSequenceLimit) =>
Future.successful(ActivationResponse.applicationError(compositionIsTooLong))
case Success(next) => the rest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes!
} else { | ||
waitForResponse map { timeout => | ||
// blocking invoke, wait until timeout | ||
response.withAlternativeAfterTimeout(waitForResponse.get, Future.successful(Left(session.activationId))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't waitForResponse.get == timeout
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! will fix
|
||
logging.debug(this, s"recording activation '${activation.activationId}'") | ||
WhiskActivation.put(activationStore, activation)(transid, notifier = None) onComplete { | ||
case Success(id) => logging.info(this, s"recorded activation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging.debug; to follow in general log reduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or just drop it entirely, it's logged in the dbstore anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused. The code already invokes logging.debug
. Should this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 549:
case Success(id) => logging.info(this, s"recorded activation")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I see it.
I agree that the code with a future is simpler than the code with a promise so I am ok with the change. But, FWIW, there was a decent reason to use the more complex concept that I try to summarize below (before I forget). The future-based implementation is now similar to the sequence implementation. It creates one pending future (ie pending continuation) for each level of nesting (sequence inside a sequence, composition inside a composition). In contrast the promise-based implementation only requires one pending continuation irrespective of nesting. It used instead a call stack (now removed caller field in session object) and unwinds the stack upon termination of a nested composition. Concretely, the callee resumes the execution of the caller explicitly with the promise implementation, whereas with the future implementation all the stack levels simultaneously exist as pending tasks in the scheduler. In short, I believe the future implementation is by nature more expensive. But since we cap the recursion depth to a low value, it is most likely irrelevant in practice. |
activation.annotations.get("limits").foreach { limitsAnnotation => | ||
limitsAnnotation.asJsObject.getFields("memory") match { | ||
case Seq(JsNumber(memory)) => | ||
session.maxMemory = Math.max(session.maxMemory, memory.toInt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this does not ignore the "not a sequence of number" cases but throws a MatchException
.
You're looking for:
activation.annotations.get("limits")
.flatMap(_.asJsObject.fields.get("memory"))
.flatMap(memory => Try(memory.convertTo[Int]).toOption)
.foreach(memory => session.maxMemory = memory max session.maxMemory
To be relatively sure it doesn't throw (asJsObject can still throw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The memory
annotation in activation records is generated. If the annotation is present without the proper type, we should get an internal error. MatchException
does that. I don't think silently ignoring the error makes more sense. We might want to have a more specific error message but I don't think this is needed either. The current code follows directly from the SequenceActions implementation.
case Right(activation) => // successful invocation | ||
session.logs += activation.activationId | ||
// activation.duration should be defined but this is not reflected by the type so be defensive | ||
session.duration += activation.duration.getOrElse(activation.end.toEpochMilli - activation.start.toEpochMilli) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe go for end
- start
straight away to avoid confusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand the suggestion here: duration
is not equal to end - start
and we have to use the correct value. I'll update the comment to be more clear.
action: ExecutableWhiskActionMetaData, | ||
cause: Option[ActivationId], | ||
var duration: Long, | ||
var maxMemory: Int, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a ByteSize
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Updating.
start: Instant, | ||
action: ExecutableWhiskActionMetaData, | ||
cause: Option[ActivationId], | ||
var duration: Long, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a FiniteDuration
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I don't think so. This current type follows from:
case class WhiskActivation(namespace: EntityPath,
...
duration: Option[Long] = None)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo the comments I included. I'll leave for @rabbah to make the final call
docs/conductors.md
Outdated
|
||
A _conductor action_ is an action with a _conductor_ annotation with a value that is not _falsy_, i.e., a value that is different from zero, null, false, and the empty string. | ||
|
||
At this time, sequence actions cannot be conductor actions. The conductor annotation on sequence actions is ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we phrase this differently? sequence actions are implicit conductors, and adding a conductor annotation has no effect/is ignored?
docs/conductors.md
Outdated
|
||
## Invocation | ||
|
||
A conductor action is invoked like a regular action, for instance: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps link to actions.md section on working with actions?
docs/conductors.md
Outdated
|
||
- one activation of the _triple_ action with input `{ value: 3 }` and output `{ value: 9 }`, | ||
- one activation of the _increment_ action with input `{ value: 9 }` and output `{ value: 10 }`, | ||
- three _secondary_ activations of the _tripleAndIncrement_ action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not immediately clear:
+3624ad829d4044afa4ad829d40e4af60 tripleAndIncrement
+4f91f9ed0d874aaa91f9ed0d87baaa07 tripleAndIncrement
4f9
is the outermost activation record, whereas 362
is the activation of the conductor action itself. This is a little subtle and perhaps warrants a bit more explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see it below - perhaps foreshadow this explanation then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tweaking...
docs/conductors.md
Outdated
|
||
Only an internal error (invocation failure or timeout) may result in an even number of derived activations. | ||
|
||
### Primary activation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plural.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
docs/conductors.md
Outdated
|
||
While secondary activations of the conductor action reflect executions of the action code, the primary activation record for the invocation of a conductor action does not. It is a synthetic record similar to the activation record of a sequence action. Concretely in our example, the code in the main function of the _tripleAndIncrement_ conductor action runs exactly three times since there are three secondary activations. | ||
|
||
The primary activation record summarizes the series of derived activations: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: flip primary to come first, secondary section to come second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
||
There are limits on the number of component action activations and secondary conductor activations in a conductor action invocation. These limits are assessed globally, i.e., if some components of a conductor action invocation are themselves conductor actions, the limits apply to the combined counts of activations across all the conductor action invocations. | ||
|
||
The maximum number _n_ of permitted component activations is equal to the maximum number of components in a sequence action. It is configured via the same configuration parameter. The maximum number of secondary conductor activations is _2n+1_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add this to references.md (i dont think we document sequence limit from our past discussion).
we can do that separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
This commit introduces a new type of actions called conductors. Conductors are a kind of sequence with dynamically computed steps. While the components of a sequence must be specified before invocation, components of a conductor can be decided as the composition is running. Like the sequence implementation, the bulk of the implementation for supporting conductors lies in the controller. It chains the invocations of the component actions. It keeps track of the execution state. It produces the final activation record for the top level conductor invocation. In contrast with sequences and in an attempt to keep the changes contained in the controller code, a conductor is not a new kind of action but rather a action with a marker annotation. This means no CLI changes for instance. A conductor action is an action with a defined "conductor" annotation. The value of the annotation must be a truthy to have effect. The annotation is ignored on sequences. Conductor actions drive the execution of compositions (functions invoked one after the other). A conductor action may either return a final result or a triplet { action, params, state }. In the latter case, the specified component action is invoked on the specified params object. Upon completion of this action the conductor action is reinvoked with a payload that combines the output of the action with the state returned by the previous conductor invocation. The composition result is the result of the final conductor invocation in the chain of invocations. A composition is therefore a sequence of invocations alternating invocations of the (fixed) conductor action with invocations of the dynamically specified components of the composition. In a sense, the conductor action acts as a scheduler that decodes and executes a sequential program. Conductor actions make it possible to support dynamic composition in OpenWhisk without committing to any particular execution model (e.g., finite state machines of one kind or another), or invocation conventions. Compositions may be nested, i.e, a step in a composition may itself be a composition. The controller recognizes this pattern and handles it efficiently. Like a sequence, one composition is materialized by one action. Creating a composition requires the definition of one conductor action. Deleting the composition consists in deleting the corresponding action. Like a sequence, a composition refers to the composed actions by name (i.e., by reference). There is no distinction between invoking a conductor and an action. Both invocations returns an activation id that can be used to fetch the result of the execution of the action or composition. Both invocations can be blocking. Both can be web actions. As for sequences, there is a configurable upper bound on the length of a composition (number of steps). In contrast with sequences, the limit is assessed at run time, and the composition invocation is aborted if the limit is exceeded. An entitlement check verifies that each composed action is accessible prior to its execution. A composition invocation like a sequence invocation counts as one invocation viz. throttling. In particular, a composition will not be aborted during execution due to throttling. Therefore a composition designed to have fewer steps than the limit will not abort due to quotas once it has started execution. This commit comes with unit tests for the controller as well as integration tests. Example: $ cat increment.js function main({ value }) { return { value: value + 1 } } $ cat conductor.js function main(params) { switch (params.$step || 0) { case 0: delete params.$step; return { params, action: 'increment', state: { $step: 1 } } case 1: delete params.$step; return { params, action: 'increment', state: { $step: 2 } } case 2: delete params.$step; return params } } $ wsk action update increment increment.js $ wsk action update conductor conductor.js -a conductor true $ wsk action invoke conductor -r -p value 3 { "value": 5 } This example demonstrates a simple conductor with two increment steps. The progress of the execution is tracked via the $step field of the parameter object that is propagated from a conductor invocation to the next but hidden from the composed actions.
This PR introduces a new type of actions called compositions. Compositions are a kind of sequences with dynamically computed steps. While the components of a sequence must be specified before invocation, components of a composition can be decided as the composition is running.
Like the sequence implementation, the bulk of the composition implementation lies in the controller. It chains the invocations of the component actions in the composition. It keeps track of the execution state. It produces the final activation record for the composition invocation.
In contrast with sequences and in an attempt to keep the changes contained in the controller code, a composition is not a new kind of action but rather a action with a marker annotation. This means no CLI changes for instance.
A conductor action is an action with a defined “conductor” annotation. The value of the annotation does not matter. For simplicity, sequences cannot be conductors. The annotation is ignored on sequences. But sequences may be components of a composition.
Conductor actions drive the execution of compositions. A conductor action may either return a final result or a triplet { action, params, state }. In the latter case, the specified component action is invoked on the specified params object. Upon completion of this action the conductor action is reinvoked with a payload that combines the output of the action with the state returned by the previous conductor invocation. The composition result is the result of the final conductor invocation in the chain of invocations.
A composition is therefore a sequence of invocations alternating invocations of the (fixed) conductor action with invocations of the dynamically specified components of the composition. In a sense, the conductor action acts as a scheduler that decodes and executes a sequential program. Conductor actions make it possible to support dynamic composition in OpenWhisk without committing to any particular execution model (e.g., finite state machines of one kind or another), or invocation conventions.
Compositions may be nested, i.e, a step in a composition may itself be a composition. The controller recognizes this pattern and handles it efficiently.
Like a sequence, one composition is materialized by one action. Creating a composition requires the definition of one conductor action. Deleting the composition consists in deleting the corresponding action. Like a sequence, a composition refers to the composed actions by name (i.e., by reference).
The PR enables a much better implementation of the composer library for OpenWhisk (https://github.com/ibm-functions/composer) that offers faster performance, lower cost (no double billing), and dramatically improves usability. With this PR, a composition is an action. There is no distinction between invoking a composition and an action. Both invocations returns an activation id that can be used to fetch the result of the execution of the action or composition. Both invocations can be blocking. Both can be web actions.
As for sequences, there is a configurable upper bound on the length of a composition (number of steps). In contrast with sequences, the limit is assessed at run time, and the composition invocation is aborted if the limit is exceeded. An entitlement check verify that each composed action is accessible prior to its execution. A composition invocation like a sequence invocation counts as one invocation viz. throttling. In particular, a composition will not be aborted during execution due to throttling. Therefore a composition designed to have fewer steps than the limit will not abort due to quotas once it has started execution.
This PR comes with unit tests for the controller as well as integration tests.
Example:
This example demonstrates a simple composition with two increment steps. The progress of the execution is tracked via the
$step
field of the parameter object that is propagated from a conductor invocation to the next but hidden from the composed actions.