Added max-unhealthy-endpoints-ratio cmdline parameter for PHC

Signed-off-by: Roman Zavodskikh <roman.zavodskikh@zalando.de>

docs/operation/operation.md

@@ -904,15 +904,20 @@ the Skipper takes a look at previous period and if the amount of requests in the
 and failed requests ratio is more than `min-drop-probability` for the given endpoints
 then Skipper will send reduced (the more `max-drop-probability` and failed requests ratio
 in previous period are, the stronger reduction is) amount of requests compared to amount sent without PHC.
+If the ratio of unhealthy endpoints is more than `max-unhealthy-endpoints-ratio` then PHC becomes fail-open. This effectively means
+if there are too many unhealthy endpoints PHC does not try to mitigate them any more and requests are sent like there is no PHC at all.
 
 Having this, we expect less requests to fail because a lot of them would be sent to endpoints that seem to be healthy instead.
 
 To enable this feature, you need to provide `-passive-health-check` option having forementioned parameters
-(`period`, `min-requests`, `min-drop-probability`, `max-drop-probability`) defined.
+(`period`, `min-requests`, `min-drop-probability`, `max-drop-probability`, `max-unhealthy-endpoints-ratio`) defined.
 `period`, `min-requests`, `max-drop-probability` are required parameters, it is not possible for PHC to be enabled without
 them explicitly defined by user. `min-drop-probability` is implicitly defined as `0.0` if not explicitly set by user.
+`max-unhealthy-endpoints-ratio` is defined as `1.0` if not expllicilty set by user.
 Valid examples of `-passive-health-check` are:
 
++ `-passive-health-check=period=1s,min-requests=10,min-drop-probability=0.05,max-drop-probability=0.9,max-unhealthy-endpoints-ratio=0.3`
++ `-passive-health-check=period=1s,min-requests=10,max-drop-probability=0.9,max-unhealthy-endpoints-ratio=0.3`
 + `-passive-health-check=period=1s,min-requests=10,min-drop-probability=0.05,max-drop-probability=0.9`
 + `-passive-health-check=period=1s,min-requests=10,max-drop-probability=0.9`
 
@@ -926,6 +931,7 @@ The parameters of `-passive-health-check` option are:
 + `min-drop-probabilty=<0.0 <= p <= 1.0>` - the minimum possible probability of unhealthy endpoint being not considered while choosing the endpoint for the given request. The same value is in fact used as minimal failed requests ratio for PHC to be enabled for this endpoint
 + `max-drop-probabilty=<0.0 <= p <= 1.0>` - the maximum possible probability of unhealthy endpoint being not considered
 while choosing the endpoint for the given request
++ `max-unhealthy-endpoints-ratio=<0.0 <= r <= 1.0>` - the maximum ratio of unhealthy endpoints for PHC to try to mitigate ongoing requests
 
 ### Metrics
 

proxy/healthy_endpoints.go

@@ -19,20 +19,22 @@ func (h *healthyEndpoints) filterHealthyEndpoints(ctx *context, endpoints []rout
 
 	p := h.rnd.Float64()
 
+	unhealthyEndpointsCount := 0
 	filtered := make([]routing.LBEndpoint, 0, len(endpoints))
 	for _, e := range endpoints {
 		dropProbability := e.Metrics.HealthCheckDropProbability()
 		if p < dropProbability {
 			ctx.Logger().Infof("Dropping endpoint %q due to passive health check: p=%0.2f, dropProbability=%0.2f",
 				e.Host, p, dropProbability)
 			metrics.IncCounter("passive-health-check.endpoints.dropped")
+			unhealthyEndpointsCount++
 		} else {
 			filtered = append(filtered, e)
 		}
-	}
 
-	if len(filtered) == 0 {
-		return endpoints
+		if float64(unhealthyEndpointsCount) > float64(len(endpoints))*e.Metrics.MaxUnhealthyEndpointsRatio() {
+			return endpoints
+		}
 	}
 
 	if len(filtered) < len(endpoints) {

proxy/healthy_endpoints_test.go

@@ -29,6 +29,7 @@ func defaultEndpointRegistry() *routing.EndpointRegistry {
 		MinRequests:                   2,
 		MaxHealthCheckDropProbability: 0.95,
 		MinHealthCheckDropProbability: 0.01,
+		MaxUnhealthyEndpointsRatio:    1.0,
 	})
 }
 
@@ -210,6 +211,7 @@ func TestPHC(t *testing.T) {
 					MinRequests:                   1, // with 2 test case fails on github actions
 					MaxHealthCheckDropProbability: 0.95,
 					MinHealthCheckDropProbability: 0.01,
+					MaxUnhealthyEndpointsRatio:    1.0,
 				})
 				mockMetrics, ps := setupProxyWithCustomEndpointRegisty(t, fmt.Sprintf(`* -> backendTimeout("5ms") -> consistentHashKey("${request.header.ConsistentHashKey}") -> <consistentHash, %s>`,
 					servicesString), consistantHashCustomEndpointRegistry)
@@ -228,6 +230,7 @@ func TestPHC(t *testing.T) {
 					MinRequests:                   1, // with 2 test case fails on github actions
 					MaxHealthCheckDropProbability: 0.95,
 					MinHealthCheckDropProbability: 0.01,
+					MaxUnhealthyEndpointsRatio:    1.0,
 				})
 				mockMetrics, ps := setupProxyWithCustomEndpointRegisty(t, fmt.Sprintf(`* -> backendTimeout("5ms") -> consistentHashKey("${request.header.ConsistentHashKey}") -> consistentHashBalanceFactor(1.25) -> <consistentHash, %s>`,
 					servicesString), consistantHashCustomEndpointRegistry)

proxy/proxy.go

@@ -162,14 +162,20 @@ type PassiveHealthCheck struct {
 
 	// The maximum probability of unhealthy endpoint to be dropped out from load balancing for every specific request
 	MaxDropProbability float64
+
+	// The maximum ratio of unhealthy endpoints in the list of all endpoints PHC will check in case of all endpoints
+	// are unhealthy
+	MaxUnhealthyEndpointsRatio float64
 }
 
 func InitPassiveHealthChecker(o map[string]string) (bool, *PassiveHealthCheck, error) {
 	if len(o) == 0 {
 		return false, &PassiveHealthCheck{}, nil
 	}
 
-	result := &PassiveHealthCheck{}
+	result := &PassiveHealthCheck{
+		MaxUnhealthyEndpointsRatio: 1.0,
+	}
 	requiredParams := map[string]struct{}{
 		"period":               {},
 		"max-drop-probability": {},
@@ -218,6 +224,15 @@ func InitPassiveHealthChecker(o map[string]string) (bool, *PassiveHealthCheck, e
 				return false, nil, fmt.Errorf("passive health check: invalid minDropProbability value: %s", value)
 			}
 			result.MinDropProbability = minDropProbability
+		case "max-unhealthy-endpoints-ratio":
+			maxUnhealthyEndpointsRatio, err := strconv.ParseFloat(value, 64)
+			if err != nil {
+				return false, nil, fmt.Errorf("passive health check: invalid maxUnhealthyEndpointsRatio value: %s", value)
+			}
+			if maxUnhealthyEndpointsRatio < 0 || maxUnhealthyEndpointsRatio > 1 {
+				return false, nil, fmt.Errorf("passive health check: invalid maxUnhealthyEndpointsRatio value: %s", value)
+			}
+			result.MaxUnhealthyEndpointsRatio = maxUnhealthyEndpointsRatio
 		default:
 			return false, nil, fmt.Errorf("passive health check: invalid parameter: key=%s,value=%s", key, value)
 		}

proxy/proxy_test.go

@@ -2296,148 +2296,162 @@ func TestInitPassiveHealthChecker(t *testing.T) {
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "somethingInvalid",
-				"min-requests":         "10",
-				"max-drop-probability": "0.9",
-				"min-drop-probability": "0.05",
+				"period":                        "somethingInvalid",
+				"min-requests":                  "10",
+				"max-drop-probability":          "0.9",
+				"min-drop-probability":          "0.05",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
 			expectedError:   fmt.Errorf("passive health check: invalid period value: somethingInvalid"),
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "1m",
-				"min-requests":         "10",
-				"max-drop-probability": "0.9",
-				"min-drop-probability": "0.05",
+				"period":                        "1m",
+				"min-requests":                  "10",
+				"max-drop-probability":          "0.9",
+				"min-drop-probability":          "0.05",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: true,
 			expectedParams: &PassiveHealthCheck{
-				Period:             1 * time.Minute,
-				MinRequests:        10,
-				MaxDropProbability: 0.9,
-				MinDropProbability: 0.05,
+				Period:                     1 * time.Minute,
+				MinRequests:                10,
+				MaxDropProbability:         0.9,
+				MinDropProbability:         0.05,
+				MaxUnhealthyEndpointsRatio: 0.3,
 			},
 			expectedError: nil,
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "-1m",
-				"min-requests":         "10",
-				"max-drop-probability": "0.9",
-				"min-drop-probability": "0.05",
+				"period":                        "-1m",
+				"min-requests":                  "10",
+				"max-drop-probability":          "0.9",
+				"min-drop-probability":          "0.05",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
 			expectedError:   fmt.Errorf("passive health check: invalid period value: -1m"),
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "1m",
-				"min-requests":         "somethingInvalid",
-				"max-drop-probability": "0.9",
-				"min-drop-probability": "0.05",
+				"period":                        "1m",
+				"min-requests":                  "somethingInvalid",
+				"max-drop-probability":          "0.9",
+				"min-drop-probability":          "0.05",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
 			expectedError:   fmt.Errorf("passive health check: invalid minRequests value: somethingInvalid"),
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "1m",
-				"min-requests":         "-10",
-				"max-drop-probability": "0.9",
-				"min-drop-probability": "0.05",
+				"period":                        "1m",
+				"min-requests":                  "-10",
+				"max-drop-probability":          "0.9",
+				"min-drop-probability":          "0.05",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
 			expectedError:   fmt.Errorf("passive health check: invalid minRequests value: -10"),
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "1m",
-				"min-requests":         "10",
-				"max-drop-probability": "somethingInvalid",
-				"min-drop-probability": "0.05",
+				"period":                        "1m",
+				"min-requests":                  "10",
+				"max-drop-probability":          "somethingInvalid",
+				"min-drop-probability":          "0.05",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
 			expectedError:   fmt.Errorf("passive health check: invalid maxDropProbability value: somethingInvalid"),
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "1m",
-				"min-requests":         "10",
-				"max-drop-probability": "-0.1",
-				"min-drop-probability": "0.05",
+				"period":                        "1m",
+				"min-requests":                  "10",
+				"max-drop-probability":          "-0.1",
+				"min-drop-probability":          "0.05",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
 			expectedError:   fmt.Errorf("passive health check: invalid maxDropProbability value: -0.1"),
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "1m",
-				"min-requests":         "10",
-				"max-drop-probability": "3.1415",
-				"min-drop-probability": "0.05",
+				"period":                        "1m",
+				"min-requests":                  "10",
+				"max-drop-probability":          "3.1415",
+				"min-drop-probability":          "0.05",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
 			expectedError:   fmt.Errorf("passive health check: invalid maxDropProbability value: 3.1415"),
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "1m",
-				"min-requests":         "10",
-				"max-drop-probability": "0.9",
-				"min-drop-probability": "somethingInvalid",
+				"period":                        "1m",
+				"min-requests":                  "10",
+				"max-drop-probability":          "0.9",
+				"min-drop-probability":          "somethingInvalid",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
 			expectedError:   fmt.Errorf("passive health check: invalid minDropProbability value: somethingInvalid"),
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "1m",
-				"min-requests":         "10",
-				"max-drop-probability": "0.9",
-				"min-drop-probability": "-0.1",
+				"period":                        "1m",
+				"min-requests":                  "10",
+				"max-drop-probability":          "0.9",
+				"min-drop-probability":          "-0.1",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
 			expectedError:   fmt.Errorf("passive health check: invalid minDropProbability value: -0.1"),
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "1m",
-				"min-requests":         "10",
-				"max-drop-probability": "0.9",
-				"min-drop-probability": "3.1415",
+				"period":                        "1m",
+				"min-requests":                  "10",
+				"max-drop-probability":          "0.9",
+				"min-drop-probability":          "3.1415",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
 			expectedError:   fmt.Errorf("passive health check: invalid minDropProbability value: 3.1415"),
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "1m",
-				"min-requests":         "10",
-				"max-drop-probability": "0.05",
-				"min-drop-probability": "0.9",
+				"period":                        "1m",
+				"min-requests":                  "10",
+				"max-drop-probability":          "0.05",
+				"min-drop-probability":          "0.9",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
 			expectedError:   fmt.Errorf("passive health check: minDropProbability should be less than maxDropProbability"),
 		},
 		{
 			inputArg: map[string]string{
-				"period":               "1m",
-				"min-requests":         "10",
-				"max-drop-probability": "0.9",
-				"min-drop-probability": "0.05",
-				"non-existing":         "non-existing",
+				"period":                        "1m",
+				"min-requests":                  "10",
+				"max-drop-probability":          "0.9",
+				"min-drop-probability":          "0.05",
+				"max-unhealthy-endpoints-ratio": "0.3",
+				"non-existing":                  "non-existing",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
@@ -2448,7 +2462,8 @@ func TestInitPassiveHealthChecker(t *testing.T) {
 				"period":       "1m",
 				"min-requests": "10",
 				/* forgot max-drop-probability */
-				"min-drop-probability": "0.05",
+				"min-drop-probability":          "0.05",
+				"max-unhealthy-endpoints-ratio": "0.3",
 			},
 			expectedEnabled: false,
 			expectedParams:  nil,
@@ -2459,14 +2474,34 @@ func TestInitPassiveHealthChecker(t *testing.T) {
 				"period":               "1m",
 				"min-requests":         "10",
 				"max-drop-probability": "0.9",
-				/* using default max-drop-probability */
+				/* using default min-drop-probability */
+				"max-unhealthy-endpoints-ratio": "0.3",
+			},
+			expectedEnabled: true,
+			expectedParams: &PassiveHealthCheck{
+				Period:                     1 * time.Minute,
+				MinRequests:                10,
+				MaxDropProbability:         0.9,
+				MinDropProbability:         0.0,
+				MaxUnhealthyEndpointsRatio: 0.3,
+			},
+			expectedError: nil,
+		},
+		{
+			inputArg: map[string]string{
+				"period":               "1m",
+				"min-requests":         "10",
+				"max-drop-probability": "0.9",
+				"min-drop-probability": "0.05",
+				/* using default max-unhealthy-endpoints-ratio */
 			},
 			expectedEnabled: true,
 			expectedParams: &PassiveHealthCheck{
-				Period:             1 * time.Minute,
-				MinRequests:        10,
-				MaxDropProbability: 0.9,
-				MinDropProbability: 0.0,
+				Period:                     1 * time.Minute,
+				MinRequests:                10,
+				MaxDropProbability:         0.9,
+				MinDropProbability:         0.05,
+				MaxUnhealthyEndpointsRatio: 1.0,
 			},
 			expectedError: nil,
 		},