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

e2e: Add backend health check e2e case via active http #3677

Merged
merged 12 commits into from
Jul 3, 2024

Conversation

aoledk
Copy link
Contributor

@aoledk aoledk commented Jun 26, 2024

What this PR does / why we need it:

Check whether backend health check via active http works in e2e, inspired by https://github.com/projectcontour/contour/blob/main/test/e2e/httpproxy/http_health_checks_test.go.

EG use the default 50% from Envoy as cluster panic threshold. In case of health check fail, all endpoints in cluster become unhealthy and panic mode is activated, request is distributed to all unhealthy echo-basic endpoints and get response from echo-basic, with same behavior as case of health check pass.

To make those two cases distinguishable, in case of health check fail, Envoy should not distribute request to unhealthy endpoint, respond with 503 no healthy upstream directly. This requires cluster panic mode is disabled.

Since EG doesn't have knob to configure cluster panic threshold currently, I use EnvoyPatchPolicy to disable cluster panic mode in this PR.

Issues about panic threshold in contour:
projectcontour/contour#579.
projectcontour/contour#1846

Which issue(s) this PR fixes:

xref #2417

Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
@aoledk aoledk requested a review from a team as a code owner June 26, 2024 07:53
Copy link

codecov bot commented Jun 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.01%. Comparing base (5050e36) to head (bf9cabf).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3677      +/-   ##
==========================================
- Coverage   69.02%   69.01%   -0.01%     
==========================================
  Files         176      176              
  Lines       21688    21688              
==========================================
- Hits        14971    14969       -2     
- Misses       5637     5638       +1     
- Partials     1080     1081       +1     

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

@arkodg arkodg added this to the v1.1.0 milestone Jun 26, 2024
@arkodg
Copy link
Contributor

arkodg commented Jun 26, 2024

thanks for picking this one up @aoledk !
would be great to also make sure we can check if the membership_degraded stat https://www.envoyproxy.io/docs/envoy/latest/configuration/upstream/cluster_manager/cluster_stats is also bumped
similar to whats done here

promQL := fmt.Sprintf(`envoy_connection_limit_limited_connections{envoy_connection_limit_prefix="%s",gateway_envoyproxy_io_owning_gateway_name="%s"}`, prefix, gtwName)

@aoledk
Copy link
Contributor Author

aoledk commented Jun 27, 2024

We can use membership_healthy to tell whether active http health check works, instead of membership_degraded which requires upstream host to respond with special x-envoy-degraded header 1, IMO it's out of this issue's scope.

  • In case of health check pass, membership_healthy should > 0.
  • In case of health check fail, membership_healthy should == 0.
# TYPE envoy_cluster_membership_degraded gauge
envoy_cluster_membership_degraded{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-fail/rule/0"} 0
envoy_cluster_membership_degraded{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-pass/rule/0"} 0

# TYPE envoy_cluster_membership_excluded gauge
envoy_cluster_membership_excluded{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-fail/rule/0"} 0
envoy_cluster_membership_excluded{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-pass/rule/0"} 0

# TYPE envoy_cluster_membership_healthy gauge
envoy_cluster_membership_healthy{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-fail/rule/0"} 0
envoy_cluster_membership_healthy{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-pass/rule/0"} 2

# TYPE envoy_cluster_membership_total gauge
envoy_cluster_membership_total{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-fail/rule/0"} 2
envoy_cluster_membership_total{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-pass/rule/0"} 2

Footnotes

  1. https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/health_checking#arch-overview-health-checking-degraded

@arkodg
Copy link
Contributor

arkodg commented Jun 27, 2024

membership_health works, thanks !

}
t.Logf("cluster pass health check: membership_healthy stats query count: %v", v)

if v == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be non zero ?

Copy link
Contributor

Choose a reason for hiding this comment

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

you prob need to wait until non zero is reached

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've turned to use non-zero cluster health check stats to verify whether backend health check works, for both pass and fail cases. And undo changes in e2e/utils/prometheus.sum to restore the behavior of only return nil when non-zero value is queried.

# TYPE envoy_cluster_health_check_attempt counter
envoy_cluster_health_check_attempt{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-fail/rule/0"} 2
envoy_cluster_health_check_attempt{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-pass/rule/0"} 2

# TYPE envoy_cluster_health_check_success counter
envoy_cluster_health_check_success{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-fail/rule/0"} 0
envoy_cluster_health_check_success{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-pass/rule/0"} 2

# TYPE envoy_cluster_health_check_failure counter
envoy_cluster_health_check_failure{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-fail/rule/0"} 2
envoy_cluster_health_check_failure{envoy_cluster_name="httproute/gateway-conformance-infra/http-with-health-check-active-http-pass/rule/0"} 0

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

thanks !

@arkodg arkodg requested review from a team July 1, 2024 17:23
@zirain
Copy link
Contributor

zirain commented Jul 3, 2024

/retest

@arkodg arkodg merged commit 612808c into envoyproxy:main Jul 3, 2024
26 checks passed
@aoledk aoledk deleted the backend-http-active-health-check-e2e branch July 5, 2024 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants