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

Check outbound healthz endpoint on sidecar wait #611

Closed
mukundansundar opened this issue Oct 3, 2023 · 5 comments · Fixed by #670
Closed

Check outbound healthz endpoint on sidecar wait #611

mukundansundar opened this issue Oct 3, 2023 · 5 comments · Fixed by #670
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed kind/enhancement New feature or request size/XS 2 days work
Milestone

Comments

@mukundansundar
Copy link
Contributor

mukundansundar commented Oct 3, 2023

Feature request

Similar to dotnet sdk check the outbound healthz endpoint for checking if sidecar is ready.
https://github.com/dapr/dotnet-sdk/blob/master/src/Dapr.Client/DaprClientGrpc.cs#L1790

Even if the port is open, Dapr might not be ready yet.

Current behavior

Python only waits to check if the port is available and socket connection can be created.
https://github.com/dapr/python-sdk/blob/master/dapr/clients/grpc/client.py#L1431

RELEASE NOTE:

@mukundansundar mukundansundar added the kind/bug Something isn't working label Oct 3, 2023
@mukundansundar
Copy link
Contributor Author

@dapr/maintainers-python-sdk For this one, can I modify the existing wait method in the gRPC clients (sync and async), to directly check against the HTTP endpoint "/v1.0/healthz/outbound" instead of the the socket check that we do right now.
This will change the behavior of wait method, but will not be a breaking change since this was supposed to be the actual health check and current implementation is considered a bug.

@mukundansundar mukundansundar self-assigned this Oct 3, 2023
@berndverst berndverst added kind/enhancement New feature or request and removed kind/bug Something isn't working labels Oct 3, 2023
@berndverst
Copy link
Member

Making HTTP calls within the gRPC client might be unexpected behavior for some users:

The good news is that there is already precedent for using both protocols at the same time: The service invocation client in the Dapr Python SDK defaults to HTTP unless otherwise configured. And Actors are HTTP only right now! Officially the recommended way is to perform service invocation with native libraries however, rather than using the Dapr SDK.
For folks that truly only use gRPC, or do not use service invocation however this could theoretically be a breaking change if they have misconfigured their (unused) Dapr HTTP Port or the Python SDK.

I would prefer if the runtime were to implement a native gRPC health method. But short of that, I think we can create a blocking utility method that calls the HTTP outbound health endpoint and retries until that passes.
I do not want this HTTP code to live directly inline within the gRPC code, hence the utility method.

I suggest we add a new helpers.py under dapr/clients/http which performs this healthz check. And then you import that function from the wait method in the grpc client as you mentioned. Additionally, you also need to use this in the Dapr Actor Client and Dapr Service Invocation Client.

@berndverst berndverst added this to the v1.12 milestone Oct 3, 2023
@berndverst berndverst added help wanted Extra attention is needed size/XS 2 days work good first issue Good for newcomers labels Oct 4, 2023
@mukundansundar
Copy link
Contributor Author

Additionally, you also need to use this in the Dapr Actor Client and Dapr Service Invocation Client.

Based on the current usages, I only see the wait method being called in examples. Can you tell me why we need to use this in Actor Client and Service Invocation Client?

@berndverst berndverst modified the milestones: v1.12, v1.13 Oct 31, 2023
@berndverst
Copy link
Member

Additionally, you also need to use this in the Dapr Actor Client and Dapr Service Invocation Client.

Based on the current usages, I only see the wait method being called in examples. Can you tell me why we need to use this in Actor Client and Service Invocation Client?

It might be nice for anything which talks to the sidecar to first wait to make sure Dapr is ready. So if it's missing it should be added I think.

@elena-kolevska elena-kolevska mentioned this issue Feb 7, 2024
3 tasks
@elena-kolevska elena-kolevska linked a pull request Feb 13, 2024 that will close this issue
3 tasks
@elena-kolevska
Copy link
Contributor

Done in #670

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed kind/enhancement New feature or request size/XS 2 days work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants