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

health_check: add Cached custom health checker #24742

Closed
wants to merge 21 commits into from

Conversation

hudayou
Copy link
Contributor

@hudayou hudayou commented Jan 3, 2023

Signed-off-by: Jacky Hu jacky.hu@walmart.com

Commit Message: health_check: add Cached custom health checker
Additional Description: Allow use of cached health check result from an external third party health checker
Risk Level: L
Testing: unit test, manual testing
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
@repokitteh-read-only repokitteh-read-only bot added api deps Approval required for changes to Envoy's external dependencies labels Jan 3, 2023
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @RyanTheOptimist

🐱

Caused by: #24742 was opened by hudayou.

see: more, trace.

Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
@hudayou
Copy link
Contributor Author

hudayou commented Jan 3, 2023

Need to fix some format errors
/wait

Jacky Hu added 7 commits January 3, 2023 13:15
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Jacky Hu added 6 commits January 3, 2023 14:54
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
@hudayou
Copy link
Contributor Author

hudayou commented Jan 4, 2023

Need to figure out:

  1. how to fix windows build
  2. why compile_time_options ci failed?
  3. add more tests for code coverage

/wait

Jacky Hu added 2 commits January 3, 2023 20:09
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Jacky Hu added 3 commits January 3, 2023 20:27
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks.
Can you provide some more background about this extension in the PR description (also is there a tracking issue)?

// [#extension: envoy.health_checkers.cached]

// [#next-free-field: 9]
message Cached {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can/should the cached server be an upstream cluster?
This should take care of all the connection parameters.

Copy link
Contributor Author

@hudayou hudayou Jan 4, 2023

Choose a reason for hiding this comment

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

No, this parameters are for a cache server
The purpose is to avoid query upstream cluster and just use the cache

https://github.com/envoyproxy/envoy/blob/badad5f2aadde7523be4e422146ff10a099055db/source/extensions/health_checkers/cached/hiredis.cc

@hudayou
Copy link
Contributor Author

hudayou commented Jan 4, 2023

Thanks. Can you provide some more background about this extension in the PR description (also is there a tracking issue)?

Yep, we are using a redis cache to avoid excessive/unnecessary health checks to our backend servers.

Below is some description about it.

I'll update the PR description about it. There is no tracking issue, if needed by you I can create one.

https://github.com/envoyproxy/envoy/blob/badad5f2aadde7523be4e422146ff10a099055db/docs/root/configuration/upstream/health_checkers/cached.rst

@hudayou
Copy link
Contributor Author

hudayou commented Jan 4, 2023

Need to fix windows build issue by providing alias to static libs as required
/wait

Signed-off-by: Jacky Hu <jacky.hu@walmart.com>
@mattklein123
Copy link
Member

Hi,

Thanks for the contribution. We can't accept a contribution this large without a tracking issue, small design doc, etc. Also, this extension would need a sponsor to go in the main repo vs. contrib. Please read the contributing guide. I'm going to close this for now until we sort that out.

Thanks

@hudayou
Copy link
Contributor Author

hudayou commented Jan 4, 2023

Sorry, I apologize for not providing a issue, I'll provide it today.

@hudayou
Copy link
Contributor Author

hudayou commented Jan 4, 2023

Created issue as required here.

#24757

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants