Skip to content

Commit

Permalink
Make min-drop-probability of PHC optional
Browse files Browse the repository at this point in the history
The default value (0.0) makes some sense and is equivalent to how
PHC was working before.

Signed-off-by: Roman Zavodskikh <roman.zavodskikh@zalando.de>
  • Loading branch information
Roman Zavodskikh committed May 7, 2024
1 parent f15ffba commit 464a07c
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 18 deletions.
14 changes: 9 additions & 5 deletions docs/operation/operation.md
Original file line number Diff line number Diff line change
Expand Up @@ -907,13 +907,17 @@ in previous period are, the stronger reduction is) amount of requests compared t

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 all forementioned parameters
(`period`, `min-requests`, `min-drop-probability`, `max-drop-probability`) defined,
for instance: `-passive-health-check=period=1s,min-requests=10,min-drop-probability=0.05,max-drop-probability=0.9`.
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`, `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.
Valid examples of `-passive-health-check` are:

You need to define all parameters on your side, there are no defaults, and skipper will not run if PHC params are passed only partially.
+ `-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`

However, Skipper will run without this feature, if no `-passive-health-check` is provided at all.
If `-passive-health-check` option is provided, but some required parameters are not defined, Skipper will not start.
Skipper will run without this feature, if no `-passive-health-check` is provided at all.

The parameters of `-passive-health-check` option are:

Expand Down
32 changes: 19 additions & 13 deletions proxy/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,16 @@ func InitPassiveHealthChecker(o map[string]string) (bool, *PassiveHealthCheck, e
}

result := &PassiveHealthCheck{}
keysInitialized := make(map[string]struct{})
requiredParams := map[string]struct{}{
"period": {},
"max-drop-probability": {},
"min-requests": {},
}

for key, value := range o {
delete(requiredParams, key)
switch key {
/* required parameters */
case "period":
period, err := time.ParseDuration(value)
if err != nil {
Expand All @@ -191,15 +197,6 @@ func InitPassiveHealthChecker(o map[string]string) (bool, *PassiveHealthCheck, e
return false, nil, fmt.Errorf("passive health check: invalid minRequests value: %s", value)
}
result.MinRequests = int64(minRequests)
case "min-drop-probability":
minDropProbability, err := strconv.ParseFloat(value, 64)
if err != nil {
return false, nil, fmt.Errorf("passive health check: invalid minDropProbability value: %s", value)
}
if minDropProbability < 0 || minDropProbability > 1 {
return false, nil, fmt.Errorf("passive health check: invalid minDropProbability value: %s", value)
}
result.MinDropProbability = minDropProbability
case "max-drop-probability":
maxDropProbability, err := strconv.ParseFloat(value, 64)
if err != nil {
Expand All @@ -209,14 +206,23 @@ func InitPassiveHealthChecker(o map[string]string) (bool, *PassiveHealthCheck, e
return false, nil, fmt.Errorf("passive health check: invalid maxDropProbability value: %s", value)
}
result.MaxDropProbability = maxDropProbability

/* optional parameters */
case "min-drop-probability":
minDropProbability, err := strconv.ParseFloat(value, 64)
if err != nil {
return false, nil, fmt.Errorf("passive health check: invalid minDropProbability value: %s", value)
}
if minDropProbability < 0 || minDropProbability > 1 {
return false, nil, fmt.Errorf("passive health check: invalid minDropProbability value: %s", value)
}
result.MinDropProbability = minDropProbability
default:
return false, nil, fmt.Errorf("passive health check: invalid parameter: key=%s,value=%s", key, value)
}

keysInitialized[key] = struct{}{}
}

if len(keysInitialized) != 4 {
if len(requiredParams) != 0 {
return false, nil, fmt.Errorf("passive health check: missing required parameters")
}
if result.MinDropProbability >= result.MaxDropProbability {
Expand Down
16 changes: 16 additions & 0 deletions proxy/proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2454,6 +2454,22 @@ func TestInitPassiveHealthChecker(t *testing.T) {
expectedParams: nil,
expectedError: fmt.Errorf("passive health check: missing required parameters"),
},
{
inputArg: map[string]string{
"period": "1m",
"min-requests": "10",
"max-drop-probability": "0.9",
/* using default max-drop-probability */
},
expectedEnabled: true,
expectedParams: &PassiveHealthCheck{
Period: 1 * time.Minute,
MinRequests: 10,
MaxDropProbability: 0.9,
MinDropProbability: 0.0,
},
expectedError: nil,
},
} {
t.Run(fmt.Sprintf("case_%d", i), func(t *testing.T) {
enabled, params, err := InitPassiveHealthChecker(ti.inputArg)
Expand Down

0 comments on commit 464a07c

Please sign in to comment.