Multiple fixes in actors package #7119

ItalyPaleAle · 2023-10-30T23:41:41Z

This PR contains some fixes for actors. In order from the most "serious" one to the least:

If the connection to Placement goes down, which can happen because of a Placement failure (initiated by the server) or if the app becomes unhealthy (initiated by the client), deactivate all actors hosted by the current runtime/app. This is because when the placement service doesn't see the app as connected, it will direct actors to other hosts. Without telling the app that the actor has been deactivated, it can cause an actor to be active in 2 separate services.
- This is now verified through both a unit test and an integration test. In the IT, we shut down the placement service and verify that all hosted actors receive a deactivation signal.
Placement now waits for the first dissemination of tables to report readiness status. This helps actor clients as a follow-up from [release-1.12] Fix race condition in app health and actors/workflow initialization #6972: with that PR, we made all calls to actor methods blocking if the actor runtime wasn't ready. However, if the runtime is ready, but placement tables haven't been received yet, attempts to look up actors will fail, causing actor invocation to fail.
- This is verified through unit tests. Sadly, writing an IT for this isn't possible at this stage, as it would require creating a "mock" placement where we can control when tables are disseminated.
Related to the above, if the connection to Placement drops (because of a network failure or because the app becomes unhealthy), the placement tables are purged from in-memory. This is because otherwise when the service comes back online, actor invocations would use stale placement data. Now, they too have to wait for the first dissemination to happen.
- Covered by the same tests as above
Fixed an issue in the unit tests of workflow HTTP APIs that caused them to take a very long time because they were blocked on waiting for the actor runtime's initialization (issue impacted the tests only)
Improved the shutdown sequence of the actors object, to better report errors in the Close method.

1. Deactivate all actors in the app if the placement disconnects 2. Actor object now records the idle time rather than the last activation time 3. Remove actors from the active actors table even if deactivation fails 4. Improved shutdown of actors object Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

…ting readiness Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

…ctivation-refactorings

ItalyPaleAle · 2023-10-30T23:42:03Z

/ok-to-test

ItalyPaleAle · 2023-10-30T23:42:07Z

/test-sdk-all

dapr-bot · 2023-10-30T23:42:30Z

Dapr E2E test

🔗 Link to Action run

Commit ref: 1488faa

✅ Build succeeded for linux/amd64

Image tag: dapre2e822ca69582l
Test image tag: dapre2e822ca69582l

✅ Infrastructure deployed

Cluster	Resource group name	Azure region
Linux	`Dapr-E2E-dapre2e822ca69582l`	westus3
Windows	`Dapr-E2E-dapre2e822ca69582w`	westus3
Linux/arm64	`Dapr-E2E-dapre2e822ca69582la`	eastus

✅ Build succeeded for windows/amd64

Image tag: dapre2e822ca69582w
Test image tag: dapre2e822ca69582w

✅ Tests succeeded on windows/amd64

Image tag: dapre2e822ca69582w
Test image tag: dapre2e822ca69582w

✅ Tests succeeded on linux/amd64

Image tag: dapre2e822ca69582l
Test image tag: dapre2e822ca69582l

dapr-bot · 2023-10-30T23:42:34Z

Dapr SDK Python test

🔗 Link to Action run

Commit ref: 1488faa

✅ Python SDK tests passed

dapr-bot · 2023-10-30T23:42:35Z

Dapr SDK Go test

🔗 Link to Action run

Commit ref: 1488faa

✅ Go SDK tests passed

dapr-bot · 2023-10-30T23:42:35Z

Dapr SDK Java test

🔗 Link to Action run

Commit ref: 1488faa

❌ Java SDK tests failed

Please check the logs for details on the error.

dapr-bot · 2023-10-30T23:43:05Z

Dapr SDK JS test

🔗 Link to Action run

Commit ref: 1488faa

❌ JS SDK tests failed

Please check the logs for details on the error.

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

tests/integration/suite/actors/healthz/deactivate-on-unhealthy.go

Signed-off-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>

codecov · 2023-10-31T00:25:11Z

Codecov Report

Attention: 63 lines in your changes are missing coverage. Please review.

Comparison is base (70f5fd6) 65.00% compared to head (4f2ebdb) 64.86%.

Files	Patch %	Lines
pkg/actors/actors.go	40.00%	40 Missing and 2 partials ⚠️
pkg/actors/placement/placement.go	64.40%	17 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7119      +/-   ##
==========================================
- Coverage   65.00%   64.86%   -0.15%     
==========================================
  Files         221      221              
  Lines       20924    21003      +79     
==========================================
+ Hits        13602    13623      +21     
- Misses       6177     6225      +48     
- Partials     1145     1155      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JoshVanL

First pass from me 🙂

tests/integration/suite/actors/healthz/deactivate-on-unhealthy.go

pkg/actors/actors.go

pkg/actors/placement/placement.go

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

yaron2 · 2023-11-16T02:58:09Z

@JoshVanL please review again :)

JoshVanL · 2023-11-17T20:47:25Z

@yaron2 along with https://github.com/dapr/dapr/pull/7119/files#diff-7ec21ca80e96c8603d65ab85ff72d4e3ef479f9cff11eb09f993182e1e91ce3a I think we need another int tests which checks that an actor is deactivated when the app reports unhealthy. This is contingent on #7022 and #6216

…ctivation-refactorings Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle · 2023-11-19T18:52:49Z

I have fixed the merge conflicts. Is there anything else blocking this PR?

Feedback on adding a new test is valid but not actionable at this stage.

JoshVanL · 2023-11-20T09:56:49Z

Yes, I think we should block until we have full test coverage for this, i.e. the app unhealthy actor deregister.

ItalyPaleAle · 2023-11-20T15:50:55Z

Can it be done in a subsequent PR?

yaron2 · 2023-11-21T15:29:54Z

Can it be done in a subsequent PR?

Yes, please follow up on this @ItalyPaleAle

ItalyPaleAle · 2023-11-21T19:08:28Z

@JoshVanL I was looking into adding a test based on your PR. Your PR reduces the healthcheck interval, but still requires 4 failed healthchecks, each on a 5s interval, for an app to be unhealthy. That makes the test still take at least 30s. Is there a way to make that faster?

JoshVanL · 2023-11-21T19:26:05Z

@ItalyPaleAle #7119 (comment)

Yes, I think the current settings don't make much sense for even a "real" scenario and should be changed.

ItalyPaleAle · 2023-11-21T19:33:34Z

@ItalyPaleAle #7119 (comment)

Yes, I think the current settings don't make much sense for even a "real" scenario and should be changed.

Aside from "real" scenarios, perhaps there could be a way to tweak that (not sure how) so tests can set even lower values

JoshVanL · 2023-11-21T19:45:02Z

@ItalyPaleAle Unifying the health checks would make them configurable with the same CLI flags and therefore more testable.

ItalyPaleAle · 2023-11-21T20:01:58Z

@ItalyPaleAle Unifying the health checks would make them configurable with the same CLI flags and therefore more testable.

Makes sense. Will wait for that then

ItalyPaleAle added 7 commits October 30, 2023 20:33

Fixed: placement waits for the first table dissemination before repor…

98a7b4d

…ting readiness Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Some improvements to memory efficiency and tests

5590dce

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Halt all actors if connection to placement fails

60e3e8d

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Added test for deactivation

facb35f

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

💄

9d43520

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Merge branch 'master' of https://github.com/dapr/dapr into actors-dea…

1488faa

…ctivation-refactorings

ItalyPaleAle requested review from a team as code owners October 30, 2023 23:41

💄

fbf6b51

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle commented Oct 31, 2023

View reviewed changes

tests/integration/suite/actors/healthz/deactivate-on-unhealthy.go Outdated Show resolved Hide resolved

Update tests/integration/suite/actors/healthz/deactivate-on-unhealthy.go

730b009

Signed-off-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>

JoshVanL requested changes Oct 31, 2023

View reviewed changes

Address review feedback

ffa00b8

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle added the autoupdate DaprBot will keep the Pull Request up to date with master branch label Nov 1, 2023

dapr-bot and others added 7 commits November 1, 2023 22:25

Merge branch 'master' into actors-deactivation-refactorings

43c6668

Merge branch 'master' into actors-deactivation-refactorings

70e8d75

Merge branch 'master' into actors-deactivation-refactorings

3840345

Merge branch 'master' into actors-deactivation-refactorings

7575790

Merge branch 'master' into actors-deactivation-refactorings

f2e3bcd

Merge branch 'master' into actors-deactivation-refactorings

6dedbc7

Renamed test

927ab4b

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

dapr-bot added 6 commits November 13, 2023 11:23

Merge branch 'master' into actors-deactivation-refactorings

0af9c34

Merge branch 'master' into actors-deactivation-refactorings

fa31d81

Merge branch 'master' into actors-deactivation-refactorings

a5a4af8

Merge branch 'master' into actors-deactivation-refactorings

c5d0f7b

Merge branch 'master' into actors-deactivation-refactorings

1820b2b

Merge branch 'master' into actors-deactivation-refactorings

75b5381

dapr-bot added 5 commits November 15, 2023 23:53

Merge branch 'master' into actors-deactivation-refactorings

718789f

Merge branch 'master' into actors-deactivation-refactorings

deb364c

Merge branch 'master' into actors-deactivation-refactorings

c3abae1

Merge branch 'master' into actors-deactivation-refactorings

1ec07f5

Merge branch 'master' into actors-deactivation-refactorings

369381b

Merge branch 'master' of https://github.com/dapr/dapr into actors-dea…

1b388d8

…ctivation-refactorings Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

dapr-bot added 2 commits November 19, 2023 19:52

Merge branch 'master' into actors-deactivation-refactorings

4e14ef3

Merge branch 'master' into actors-deactivation-refactorings

d40d77c

Merge branch 'master' into actors-deactivation-refactorings

4f2ebdb

yaron2 approved these changes Nov 21, 2023

View reviewed changes

yaron2 merged commit 6d000be into dapr:master Nov 21, 2023
19 of 22 checks passed

ItalyPaleAle deleted the actors-deactivation-refactorings branch November 21, 2023 18:54

ItalyPaleAle added this to the v1.13 milestone Jan 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple fixes in actors package #7119

Multiple fixes in actors package #7119

ItalyPaleAle commented Oct 30, 2023 •

edited

ItalyPaleAle commented Oct 30, 2023

ItalyPaleAle commented Oct 30, 2023

dapr-bot commented Oct 30, 2023 •

edited

dapr-bot commented Oct 30, 2023 •

edited

dapr-bot commented Oct 30, 2023 •

edited

dapr-bot commented Oct 30, 2023 •

edited

dapr-bot commented Oct 30, 2023 •

edited

codecov bot commented Oct 31, 2023 •

edited

JoshVanL left a comment

yaron2 commented Nov 16, 2023

JoshVanL commented Nov 17, 2023

ItalyPaleAle commented Nov 19, 2023

JoshVanL commented Nov 20, 2023

ItalyPaleAle commented Nov 20, 2023

yaron2 commented Nov 21, 2023

ItalyPaleAle commented Nov 21, 2023

JoshVanL commented Nov 21, 2023

ItalyPaleAle commented Nov 21, 2023

JoshVanL commented Nov 21, 2023

ItalyPaleAle commented Nov 21, 2023

Multiple fixes in actors package #7119

Multiple fixes in actors package #7119

Conversation

ItalyPaleAle commented Oct 30, 2023 • edited

ItalyPaleAle commented Oct 30, 2023

ItalyPaleAle commented Oct 30, 2023

dapr-bot commented Oct 30, 2023 • edited

Dapr E2E test

✅ Build succeeded for linux/amd64

✅ Infrastructure deployed

✅ Build succeeded for windows/amd64

✅ Tests succeeded on windows/amd64

✅ Tests succeeded on linux/amd64

dapr-bot commented Oct 30, 2023 • edited

Dapr SDK Python test

✅ Python SDK tests passed

dapr-bot commented Oct 30, 2023 • edited

Dapr SDK Go test

✅ Go SDK tests passed

dapr-bot commented Oct 30, 2023 • edited

Dapr SDK Java test

❌ Java SDK tests failed

dapr-bot commented Oct 30, 2023 • edited

Dapr SDK JS test

❌ JS SDK tests failed

codecov bot commented Oct 31, 2023 • edited

Codecov Report

JoshVanL left a comment

Choose a reason for hiding this comment

yaron2 commented Nov 16, 2023

JoshVanL commented Nov 17, 2023

ItalyPaleAle commented Nov 19, 2023

JoshVanL commented Nov 20, 2023

ItalyPaleAle commented Nov 20, 2023

yaron2 commented Nov 21, 2023

ItalyPaleAle commented Nov 21, 2023

JoshVanL commented Nov 21, 2023

ItalyPaleAle commented Nov 21, 2023

JoshVanL commented Nov 21, 2023

ItalyPaleAle commented Nov 21, 2023

ItalyPaleAle commented Oct 30, 2023 •

edited

dapr-bot commented Oct 30, 2023 •

edited

dapr-bot commented Oct 30, 2023 •

edited

dapr-bot commented Oct 30, 2023 •

edited

dapr-bot commented Oct 30, 2023 •

edited

dapr-bot commented Oct 30, 2023 •

edited

codecov bot commented Oct 31, 2023 •

edited