Dapr WF engine implementation starter #5301

cgillum · 2022-10-03T18:47:05Z

Description

This is the initial implementation of the Dapr WF engine. This PR contains the following changes to dapr/dapr:

A new dependency on https://github.com/microsoft/durabletask-go (currently targeting the dapr branch)
A new InternalActor abstraction that the workflow engine will rely on, including changes to the existing actor runtime to invoke them
A new backend implementation for the durable task engine based on internal actors, including the first internal actor implementation (of type dapr.internal.wfengine.workflow)
Changes to the runtime initialization code to initialize the workflow engine, including registering a new gRPC endpoint on the API server

I've implemented just enough of the engine to support executing a "no-op" orchestration using the existing, unmodified .NET Durable Task SDK, which tells us that the core parts are working and that the embedded engine is compatible with the gRPC protocol used by the existing Durable Task SDKs. However, I have not yet implemented any state storage, so the workflows are not yet scalable or reliable. There are also many more features that need to be implemented. However, I wanted to get this PR started to checkpoint my progress and get any initial feedback.

The most important files to review are the existing files, like go.mod, actors.go, runtime.go and server.go. This is a PR to the feature/workflows feature branch, so I don't think we don't need a full-blown review for this.

I've also added a new README.md where I add more information, including some basic instructions for how to build and test the new feature.

FYI @johnewart @artursouza @yaron2

artursouza

I have just reviewed the actors and runtime changes for now. I like that it is keeping the wf code relatively separate from the runtime code.

pkg/actors/actors.go

cgillum

Thanks for your feedback. Please see my comments.

pkg/actors/actors.go

codecov · 2022-10-04T18:50:00Z

Codecov Report

Merging #5301 (034d1b8) into feature/workflows (1961fa6) will increase coverage by 0.01%.
The diff coverage is 62.61%.

@@                  Coverage Diff                  @@
##           feature/workflows    #5301      +/-   ##
=====================================================
+ Coverage              65.63%   65.64%   +0.01%     
=====================================================
  Files                    125      126       +1     
  Lines                  13313    13400      +87     
=====================================================
+ Hits                    8738     8797      +59     
- Misses                  3946     3953       +7     
- Partials                 629      650      +21

Impacted Files	Coverage Δ
pkg/messaging/v1/util.go	`50.47% <ø> (ø)`
pkg/actors/internal_actor.go	`50.00% <50.00%> (ø)`
pkg/runtime/runtime.go	`68.02% <66.66%> (-0.42%)`	⬇️
pkg/actors/actors.go	`60.30% <80.95%> (+1.56%)`	⬆️
pkg/grpc/server.go	`39.61% <84.61%> (-0.13%)`	⬇️
pkg/diagnostics/grpc_tracing.go	`67.33% <100.00%> (+0.21%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cgillum · 2022-10-04T20:26:49Z

Per feedback from @artursouza, I'm going to change the design a bit. Rather than have the actor runtime worry about local vs. internal actors, we'll instead have the actor runtime support two types of app channels, the regular one and also an internal one. Internal actors will be implemented inside the internal app channel. This should reduce some of the churn in the actor runtime code.

yaron2 · 2022-10-04T21:24:19Z

Per feedback from @artursouza, I'm going to change the design a bit. Rather than have the actor runtime worry about local vs. internal actors, we'll instead have the actor runtime support two types of app channels, the regular one and also an internal one. Internal actors will be implemented inside the internal app channel. This should reduce some of the churn in the actor runtime code.

Can we break that up into a separate PR?

cgillum · 2022-10-04T21:47:29Z

Per feedback from @artursouza, I'm going to change the design a bit. Rather than have the actor runtime worry about local vs. internal actors, we'll instead have the actor runtime support two types of app channels, the regular one and also an internal one. Internal actors will be implemented inside the internal app channel. This should reduce some of the churn in the actor runtime code.

Can we break that up into a separate PR?

Sorry @yaron2, I only just now noticed your comment and already made this change. It reduces the complexity of the actor runtime changes slightly, so hopefully that makes things a little easier to review.

artursouza

I like how the runtime changes go now. I still need to take a look at the wfengine/* code.

pkg/actors/actors.go

johnewart · 2022-10-05T19:03:28Z

pkg/runtime/wfengine/backend.go

+
+func NewActorBackend() *actorBackend {
+	return &actorBackend{
+		orchestrationWorkItemChan: make(chan *backend.OrchestrationWorkItem),


Do you deliberately want these channels un-buffered?

Let me revisit this. I didn't realize that unbuffered channels were an option.

pkg/runtime/wfengine/backend.go

pkg/runtime/wfengine/workflow.go

johnewart · 2022-10-05T19:24:42Z

I think this is looking pretty good overall; personally I think that we should move some of the actor-relate bits over into the actor package, and there are a few places where code will indefinitely block without timeouts (which may be okay if intentional) or we could bump into NPEs (which would probably be bad 😄)

yaron2 · 2022-10-05T20:48:58Z

Having the actor package know about the workflow package feels wrong; As Workflows are a consumer of actors it makes sense to have workflow import actors only and not the other way around.

cgillum · 2022-10-05T23:05:35Z

Having the actor package know about the workflow package feels wrong; As Workflows are a consumer of actors it makes sense to have workflow import actors only and not the other way around.

Sounds good. This will be fixed in the next iteration.

cgillum

Thanks for the feedback so far. Here's a quick summary of the changes in this latest iteration:

The actor runtime now knows nothing about workflows. The only new concept it understands is internal actors.
I made the InternalActor a first-class member of the actors package that anyone can use. See internal_actor.go.
We have two AppChannel variables: externalAppChannel (just a rename of the existing field) and internalAppChannel, which is responsible for invoking all internal actors, regardless of who creates them.

I've also responded to all comments. Let me know if anything else is required for this PR.

pkg/actors/actors.go

pkg/runtime/runtime.go

pkg/runtime/wfengine/actors.go

pkg/runtime/wfengine/workflow.go

johnewart

I think we are in a pretty good place for this round -- thanks for shuffling around the actors / workflow engine relationship bits!

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

pkg/runtime/wfengine/workflow.go

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

cgillum · 2022-10-07T20:53:05Z

Thanks @johnewart and @RyanLettieri for the reviews! I made a few more small changes in the latest commits:

Merged the latest changes from feature/workflows, which included several conflicting changes (mostly around constructor parameters)
I updated GetActiveActorsCount to exclucde internal actors. This was caught by a failing e2e test. Before this change, GetActiveActorsCount was returning the internal workflow actor, which I don't think we want.
Made some of the nil checks more explicit to avoid reader confusion.

Let me know if anyone thinks there's anything else we need to merge this initial PR. Fingers crossed that the CI will be happy this time. 🤞🏽

cgillum requested review from a team as code owners October 3, 2022 18:47

nyemade-uversky added this to In progress in Dapr Workflows Oct 3, 2022

artursouza reviewed Oct 3, 2022

View reviewed changes

pkg/actors/actors.go Outdated Show resolved Hide resolved

pkg/actors/actors.go Outdated Show resolved Hide resolved

pkg/actors/actors.go Outdated Show resolved Hide resolved

pkg/actors/actors.go Outdated Show resolved Hide resolved

pkg/actors/actors.go Outdated Show resolved Hide resolved

cgillum commented Oct 3, 2022

View reviewed changes

pkg/actors/actors.go Outdated Show resolved Hide resolved

pkg/actors/actors.go Outdated Show resolved Hide resolved

pkg/actors/actors.go Outdated Show resolved Hide resolved

pkg/actors/actors.go Outdated Show resolved Hide resolved

pkg/actors/actors.go Outdated Show resolved Hide resolved

cgillum commented Oct 3, 2022

View reviewed changes

pkg/actors/actors.go Outdated Show resolved Hide resolved

cgillum force-pushed the feature/workflows branch from 8f12a60 to 70c891c Compare October 3, 2022 21:54

cgillum requested a review from artursouza October 3, 2022 21:57

cgillum force-pushed the feature/workflows branch from 3b4507e to 4eebbf0 Compare October 4, 2022 21:42

artursouza reviewed Oct 5, 2022

View reviewed changes

johnewart suggested changes Oct 5, 2022

View reviewed changes

cgillum commented Oct 6, 2022

View reviewed changes

johnewart approved these changes Oct 7, 2022

View reviewed changes

cgillum added 5 commits October 7, 2022 18:43

Dapr WF engine implementation starter

1354c7a

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

Infof -> Debugf and linter fixes

69eb194

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

Update Durable Task dependency for logging fixes

091be3e

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

Switch to AppChannel abstraction

f960b5e

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

Undo accidental variable scope change

b334859

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

RyanLettieri reviewed Oct 7, 2022

View reviewed changes

pkg/runtime/wfengine/workflow.go Show resolved Hide resolved

RyanLettieri reviewed Oct 7, 2022

View reviewed changes

pkg/runtime/wfengine/workflow.go Show resolved Hide resolved

RyanLettieri reviewed Oct 7, 2022

View reviewed changes

pkg/runtime/wfengine/workflow.go Show resolved Hide resolved

cgillum added 2 commits October 7, 2022 20:02

Refactoring based on PR feedback

824f7bf

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

Small refactor to fix failing unit test

6f4592d

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

cgillum added 4 commits October 7, 2022 20:03

Added unit test for internal actors

559062a

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

Fix linter error

900f94a

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

Fix for E2E test + PR feedback on nil checks

bf26824

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

Merge latest from upstream/feature/workflows

034d1b8

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

cgillum force-pushed the feature/workflows branch from 29c00fd to 034d1b8 Compare October 7, 2022 20:37

artursouza approved these changes Oct 8, 2022

View reviewed changes

artursouza merged commit 12f8b64 into dapr:feature/workflows Oct 8, 2022

Dapr Workflows automation moved this from In progress to Done Oct 8, 2022

This was referenced Oct 8, 2022

[Proposal] Workflow building block and engine #4576

Closed

Core WF engine features: activities, timers, sub-workflows, etc. #5344

Merged

cgillum mentioned this pull request Jan 13, 2023

Dapr Workflow Engine Part 1: Internal Actors #5761

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dapr WF engine implementation starter #5301

Dapr WF engine implementation starter #5301

cgillum commented Oct 3, 2022 •

edited

artursouza left a comment

cgillum left a comment

codecov bot commented Oct 4, 2022 •

edited

cgillum commented Oct 4, 2022

yaron2 commented Oct 4, 2022

cgillum commented Oct 4, 2022

artursouza left a comment

johnewart Oct 5, 2022

cgillum Oct 5, 2022

johnewart commented Oct 5, 2022

yaron2 commented Oct 5, 2022

cgillum commented Oct 5, 2022

cgillum left a comment

johnewart left a comment

cgillum commented Oct 7, 2022

Dapr WF engine implementation starter #5301

Dapr WF engine implementation starter #5301

Conversation

cgillum commented Oct 3, 2022 • edited

Description

artursouza left a comment

Choose a reason for hiding this comment

cgillum left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 4, 2022 • edited

Codecov Report

cgillum commented Oct 4, 2022

yaron2 commented Oct 4, 2022

cgillum commented Oct 4, 2022

artursouza left a comment

Choose a reason for hiding this comment

johnewart Oct 5, 2022

Choose a reason for hiding this comment

cgillum Oct 5, 2022

Choose a reason for hiding this comment

johnewart commented Oct 5, 2022

yaron2 commented Oct 5, 2022

cgillum commented Oct 5, 2022

cgillum left a comment

Choose a reason for hiding this comment

johnewart left a comment

Choose a reason for hiding this comment

cgillum commented Oct 7, 2022

cgillum commented Oct 3, 2022 •

edited

codecov bot commented Oct 4, 2022 •

edited