Skip to content
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

Adds actors healthz integration tests #7394

Merged
merged 8 commits into from
Jan 18, 2024

Conversation

JoshVanL
Copy link
Contributor

@JoshVanL JoshVanL commented Jan 17, 2024

Adds integration tests for testing actor healthz behaviour when entities and or app healthz is defined.

Closes #7355

Mark actors as always healthy when there is no app channel or their have been
no actor entities (actor types) provided by the application config endpoint.
This is important for enabling actor related APIs such as workflows, which
don't rely on the application implementing actors itself.

Adds integration tests for actor healthz endpoints Most importantly, ensuring
that actors are marked as always healthy when the both the application healthz
endpoint is disabled, and the application reports no actor entities.

Mark actors as always healthy when there is no app channel or their have
been no actor entities (actor types) provided by the application config
endpoint. This is important for enabling actor related APIs such as
workflows, which don't rely on the application implementing actors
itself.

Signed-off-by: joshvanl <me@joshvanl.dev>
Most importantly, ensuring that actors are marked as always healthy when
the both the application healthz endpoint is disabled, and the
application reports no actor entities.

Signed-off-by: joshvanl <me@joshvanl.dev>
@JoshVanL JoshVanL marked this pull request as ready for review January 17, 2024 12:04
@JoshVanL JoshVanL requested review from a team as code owners January 17, 2024 12:04
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4bea838) 62.32% compared to head (2ecee30) 62.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7394      +/-   ##
==========================================
+ Coverage   62.32%   62.34%   +0.02%     
==========================================
  Files         240      240              
  Lines       22154    22154              
==========================================
+ Hits        13807    13812       +5     
+ Misses       7190     7182       -8     
- Partials     1157     1160       +3     

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

@JoshVanL JoshVanL added the P0 label Jan 17, 2024
@JoshVanL JoshVanL added this to the v1.13 milestone Jan 17, 2024
@JoshVanL JoshVanL added the autoupdate DaprBot will keep the Pull Request up to date with master branch label Jan 17, 2024
Comment on lines +71 to +81
srvNoHealthz := prochttp.New(t,
prochttp.WithHandlerFunc("/dapr/config", func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(`{}`))
}),
prochttp.WithHandlerFunc("/healthz", func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
onceNoHealthz.Do(func() {
close(n.noHealthzCalled)
})
}),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq when there are no actor types and health check is disabled, why is a healthz endpoint needed and is being called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are verifying this doesn't get called further down.

mukundansundar
mukundansundar previously approved these changes Jan 17, 2024
@mukundansundar
Copy link
Contributor

@JoshVanL The IT seem to failing in the specific test that was written in this PR.

@JoshVanL
Copy link
Contributor Author

Thanks @mukundansundar, it's clear this PR is causing some kind of unexpected side effect somewhere. I'm taking a look

@@ -180,6 +180,7 @@ func (p *actorPlacement) Start(ctx context.Context) error {
defer p.shutdownConnLoop.Done()
ch := p.appHealthFn(ctx)
if ch == nil {
p.appHealthy.Store(true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit off. I don't think we can do this because if the app has a HTTP app channel, ch will be non-nil.

Can you check pkg/actors/actors.go? Specifically, where a.checker is initialized:

https://github.com/JoshVanL/dapr/blob/a447ea75cd47e4c28256d6a5e4374dfadeb156ea/pkg/actors/actors.go#L278

Perhaps the way to fix this issue is to simply not initialize it if len(hat) == 0?

https://github.com/JoshVanL/dapr/blob/a447ea75cd47e4c28256d6a5e4374dfadeb156ea/pkg/actors/actors.go#L261

	if len(hat) > 0 {
		a.checker, err = a.getAppHealthChecker()
		if err != nil {
			return fmt.Errorf("actors: couldn't create health check: %w", err)
		}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK because ch will be nil if entities is empty

dapr/pkg/actors/actors.go

Lines 317 to 319 in ecd3798

if len(a.actorsConfig.Config.HostedActorTypes.ListActorTypes()) == 0 || a.appChannel == nil {
return nil, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hold on, then if ch is already nil when entities is empty.... There's nothing we need to do in this PR? Was the issue already fixed? (And just needed tests?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we start placement, we already set appHealthy to true:

https://github.dev/JoshVanL/dapr/blob/a447ea75cd47e4c28256d6a5e4374dfadeb156ea/pkg/actors/placement/placement.go#L159

And if ch is nil, then this can never be set to false

(And if that weren't the case, workflow tests would have failed already sinced they don't implement /healthz)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the code change made in this PR, the new test noentities will fail. i.e. actors will never become ready.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works on my machine without this line. Please try it!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoshVanL Please, double check this because we already have E2E to validate that sidecars that don't serve actors types can still invoke other actors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, happy to take the integration test part of it.

Copy link
Contributor Author

@JoshVanL JoshVanL Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! I have just included the integration tests now. I have also updated the PR title and description.

@artursouza
Copy link
Member

This is a regression because a sidecar that does not host actors can always use actors that are hosted by other instances. If this is the case, this is a hotfix.

Signed-off-by: joshvanl <me@joshvanl.dev>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
artursouza
artursouza previously approved these changes Jan 17, 2024
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
@JoshVanL JoshVanL changed the title Mark actors healthy on no app channel or actor entities Adds actors healthz integration tests Jan 17, 2024
@yaron2 yaron2 merged commit 8ff62ac into dapr:master Jan 18, 2024
20 of 22 checks passed
whytem pushed a commit to whytem/dapr that referenced this pull request Jan 22, 2024
* Mark actors healthy on no app channel or actor entities

Mark actors as always healthy when there is no app channel or their have
been no actor entities (actor types) provided by the application config
endpoint. This is important for enabling actor related APIs such as
workflows, which don't rely on the application implementing actors
itself.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Adds integration tests for actor healthz endpoints

Most importantly, ensuring that actors are marked as always healthy when
the both the application healthz endpoint is disabled, and the
application reports no actor entities.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove uneeded app health check store

Signed-off-by: joshvanl <me@joshvanl.dev>

* Update tests/integration/suite/actors/healthz/endpoint/noapp.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* Update tests/integration/suite/actors/healthz/endpoint/noappentities.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Co-authored-by: Dapr Bot <56698301+dapr-bot@users.noreply.github.com>
elena-kolevska pushed a commit to elena-kolevska/dapr that referenced this pull request Jan 24, 2024
* Mark actors healthy on no app channel or actor entities

Mark actors as always healthy when there is no app channel or their have
been no actor entities (actor types) provided by the application config
endpoint. This is important for enabling actor related APIs such as
workflows, which don't rely on the application implementing actors
itself.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Adds integration tests for actor healthz endpoints

Most importantly, ensuring that actors are marked as always healthy when
the both the application healthz endpoint is disabled, and the
application reports no actor entities.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove uneeded app health check store

Signed-off-by: joshvanl <me@joshvanl.dev>

* Update tests/integration/suite/actors/healthz/endpoint/noapp.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* Update tests/integration/suite/actors/healthz/endpoint/noappentities.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Co-authored-by: Dapr Bot <56698301+dapr-bot@users.noreply.github.com>
elena-kolevska added a commit to elena-kolevska/dapr that referenced this pull request Jan 24, 2024
elena-kolevska pushed a commit to elena-kolevska/dapr that referenced this pull request Jan 25, 2024
* Mark actors healthy on no app channel or actor entities

Mark actors as always healthy when there is no app channel or their have
been no actor entities (actor types) provided by the application config
endpoint. This is important for enabling actor related APIs such as
workflows, which don't rely on the application implementing actors
itself.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Adds integration tests for actor healthz endpoints

Most importantly, ensuring that actors are marked as always healthy when
the both the application healthz endpoint is disabled, and the
application reports no actor entities.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove uneeded app health check store

Signed-off-by: joshvanl <me@joshvanl.dev>

* Update tests/integration/suite/actors/healthz/endpoint/noapp.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* Update tests/integration/suite/actors/healthz/endpoint/noappentities.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Co-authored-by: Dapr Bot <56698301+dapr-bot@users.noreply.github.com>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
elena-kolevska added a commit to elena-kolevska/dapr that referenced this pull request Jan 25, 2024
This reverts commit 0b61910.

Signed-off-by: Elena Kolevska <elena@kolevska.com>
elena-kolevska pushed a commit to elena-kolevska/dapr that referenced this pull request Jan 25, 2024
* Mark actors healthy on no app channel or actor entities

Mark actors as always healthy when there is no app channel or their have
been no actor entities (actor types) provided by the application config
endpoint. This is important for enabling actor related APIs such as
workflows, which don't rely on the application implementing actors
itself.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Adds integration tests for actor healthz endpoints

Most importantly, ensuring that actors are marked as always healthy when
the both the application healthz endpoint is disabled, and the
application reports no actor entities.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove uneeded app health check store

Signed-off-by: joshvanl <me@joshvanl.dev>

* Update tests/integration/suite/actors/healthz/endpoint/noapp.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* Update tests/integration/suite/actors/healthz/endpoint/noappentities.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Co-authored-by: Dapr Bot <56698301+dapr-bot@users.noreply.github.com>
elena-kolevska added a commit to elena-kolevska/dapr that referenced this pull request Jan 25, 2024
elena-kolevska pushed a commit to elena-kolevska/dapr that referenced this pull request Jan 25, 2024
* Mark actors healthy on no app channel or actor entities

Mark actors as always healthy when there is no app channel or their have
been no actor entities (actor types) provided by the application config
endpoint. This is important for enabling actor related APIs such as
workflows, which don't rely on the application implementing actors
itself.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Adds integration tests for actor healthz endpoints

Most importantly, ensuring that actors are marked as always healthy when
the both the application healthz endpoint is disabled, and the
application reports no actor entities.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove uneeded app health check store

Signed-off-by: joshvanl <me@joshvanl.dev>

* Update tests/integration/suite/actors/healthz/endpoint/noapp.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* Update tests/integration/suite/actors/healthz/endpoint/noappentities.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Co-authored-by: Dapr Bot <56698301+dapr-bot@users.noreply.github.com>
elena-kolevska added a commit to elena-kolevska/dapr that referenced this pull request Jan 25, 2024
cicoyle pushed a commit to cicoyle/dapr that referenced this pull request May 24, 2024
* Mark actors healthy on no app channel or actor entities

Mark actors as always healthy when there is no app channel or their have
been no actor entities (actor types) provided by the application config
endpoint. This is important for enabling actor related APIs such as
workflows, which don't rely on the application implementing actors
itself.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Adds integration tests for actor healthz endpoints

Most importantly, ensuring that actors are marked as always healthy when
the both the application healthz endpoint is disabled, and the
application reports no actor entities.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove uneeded app health check store

Signed-off-by: joshvanl <me@joshvanl.dev>

* Update tests/integration/suite/actors/healthz/endpoint/noapp.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* Update tests/integration/suite/actors/healthz/endpoint/noappentities.go

Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Co-authored-by: Dapr Bot <56698301+dapr-bot@users.noreply.github.com>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoupdate DaprBot will keep the Pull Request up to date with master branch P0
Projects
Development

Successfully merging this pull request may close these issues.

Having healthz endpoint with appHealthCheck false
6 participants