internal: Allow retry policy, num retries to be zero #4117

stevesloka · 2021-10-15T17:11:47Z

The field, NumRetries (e.g. count), in the RetryPolicy allows for a zero to be
specified, however Contour's internal logic would see that as "undefined"
and set it back to the Envoy default of 1.

This checks for the existance of the variable and uses the value specified,
otherwise defaults to 1 matching Envoy.

Signed-off-by: Steve Sloka [email protected]

codecov · 2021-10-15T17:21:17Z

Codecov Report

Merging #4117 (ef9ec5b) into main (9799e70) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4117      +/-   ##
==========================================
+ Coverage   74.63%   74.66%   +0.03%     
==========================================
  Files         112      112              
  Lines        9709     9722      +13     
==========================================
+ Hits         7246     7259      +13     
  Misses       2307     2307              
  Partials      156      156

Impacted Files	Coverage Δ
internal/annotation/annotations.go	`100.00% <100.00%> (ø)`
internal/dag/policy.go	`94.06% <100.00%> (+0.02%)`	⬆️

skriss · 2021-10-19T14:57:07Z

apis/projectcontour/v1/httpproxy.go

 	// +optional
 	// +kubebuilder:validation:Minimum=0
-	NumRetries int64 `json:"count"`
+	NumRetries *int64 `json:"count"`


My only concern here is that this is a breaking change from a Go perspective, since the type is changing, even though it's backwards compatible from a YAML perspective. I don't think we've been 100% explicit on whether something like this is allowed or not, currently looking for any Kubernetes guidance on this..

Yeah I don't know if it's a big deal or not. The meaning of the variable isn't changing.

The alternatives would be to bump the api version, or add a new field like countZero: true or something would be confusing.

Another option, we could do something silly for now like make -1 mean "no retries" and leave 0 as "use Envoy default" (e.g. 1), and then clean it up in the v2 API.

Or we could do some kubebuilder defaulting tricks: default to adding a retryPolicy to every HTTPProxy with a count of 1, and then change the behavior when retryPolicy.count = 0 to actually pass 0 to Envoy. This should keep the default behavior as 1 retry, while allowing an explicit 0 to be specified.

Let me play with the kubebuilder route and see if that route would work.

I guess one problem with the kubebuilder approach is pre-existing HTTPProxies would not get the default populated, so their behavior would end up changing. It would only apply to new HTTPProxies.

Yeah. hmm. Maybe the -1 route is the most straight forward and we can disable from setting the value to zero in kubebuilder validation.

site/content/docs/main/config/request-routing.md

Since the current field for NumRetries is not a pointer, Contour doesn't have a good way to understand if the field is unspecified, or set to zero. To keep the v1 API contract, Contour will allow this field to be set to -1 which means the numRetries should be zero. If unspecified or set to zero, then the Envoy default of 1 is used. Signed-off-by: Steve Sloka <[email protected]>

Signed-off-by: Steve Sloka <[email protected]>

internal/dag/policy_test.go

sunjayBhatia · 2021-10-21T20:22:23Z

site/content/docs/main/config/request-routing.md

 - `retryPolicy`: A retry will be attempted if the server returns an error code in the 5xx range, or if the server takes more than `retryPolicy.perTryTimeout` to process a request.

- `retryPolicy.count` specifies the maximum number of retries allowed. This parameter is optional and defaults to 1.
+- `retryPolicy.count` specifies the maximum number of retries allowed. This parameter is optional and defaults to 1. Set to -1 to disable. If set to 0, the Envoy default of 1 is used.


given we say "defaults to 1" might also be instructive to users to set the kubebuilder default to 1 so they see it in their YAML as well if not set? regardless with how we've implemented you do end up with a default of 1

👍 I think this makes sense, would make things a bit more obvious for the default case.

sunjayBhatia

just some small nits, only slightly bigger change could be to add the kubebuilder default to 1 so we don't end up with new resources with retries count=0, but the -1 seems like it works otherwise since disabling retries was what people wanted

in Contour's Config CRD we should maybe make an issue to make the relevant field a pointer (to unsigned int 👀 ) early so we don't have to deal with this again!

Signed-off-by: Steve Sloka <[email protected]>

skriss

LGTM, I think this is a reasonable way to approach given API compatibility constraints.

skriss · 2021-10-25T14:57:07Z

site/content/docs/main/config/request-routing.md

 - `retryPolicy`: A retry will be attempted if the server returns an error code in the 5xx range, or if the server takes more than `retryPolicy.perTryTimeout` to process a request.

- `retryPolicy.count` specifies the maximum number of retries allowed. This parameter is optional and defaults to 1.
+- `retryPolicy.count` specifies the maximum number of retries allowed. This parameter is optional and defaults to 1. Set to -1 to disable. If set to 0, the Envoy default of 1 is used.


👍 I think this makes sense, would make things a bit more obvious for the default case.

…4117) Since the current field for NumRetries is not a pointer, Contour doesn't have a good way to understand if the field is unspecified, or set to zero. To keep the v1 API contract, Contour will allow this field to be set to -1 which means the numRetries should be zero. If unspecified or set to zero, then the Envoy default of 1 is used. Signed-off-by: Steve Sloka <[email protected]>

…4117) (#4135) Since the current field for NumRetries is not a pointer, Contour doesn't have a good way to understand if the field is unspecified, or set to zero. To keep the v1 API contract, Contour will allow this field to be set to -1 which means the numRetries should be zero. If unspecified or set to zero, then the Envoy default of 1 is used. Signed-off-by: Steve Sloka <[email protected]>

Since the current field for NumRetries is not a pointer, Contour doesn't have a good way to understand if the field is unspecified, or set to zero. To keep the v1 API contract, Contour will allow this field to be set to -1 which means the numRetries should be zero. If unspecified or set to zero, then the Envoy default of 1 is used. Signed-off-by: Steve Sloka <[email protected]>

…4117) Since the current field for NumRetries is not a pointer, Contour doesn't have a good way to understand if the field is unspecified, or set to zero. To keep the v1 API contract, Contour will allow this field to be set to -1 which means the numRetries should be zero. If unspecified or set to zero, then the Envoy default of 1 is used. Signed-off-by: Steve Sloka <[email protected]>

#4174) * internal: Allow retry policy, num retries to be zero (#4117) Since the current field for NumRetries is not a pointer, Contour doesn't have a good way to understand if the field is unspecified, or set to zero. To keep the v1 API contract, Contour will allow this field to be set to -1 which means the numRetries should be zero. If unspecified or set to zero, then the Envoy default of 1 is used. Signed-off-by: Steve Sloka <[email protected]> Co-authored-by: Nick Young <[email protected]>

stevesloka requested a review from a team as a code owner October 15, 2021 17:11

stevesloka requested review from skriss and youngnick and removed request for a team October 15, 2021 17:11

stevesloka added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Oct 15, 2021

skriss reviewed Oct 19, 2021

View reviewed changes

stevesloka force-pushed the allowZeroRetries branch from 9535e4e to fd0d3e2 Compare October 21, 2021 17:13

sunjayBhatia reviewed Oct 21, 2021

View reviewed changes

site/content/docs/main/config/request-routing.md Outdated Show resolved Hide resolved

stevesloka force-pushed the allowZeroRetries branch from fd0d3e2 to dc3e79e Compare October 21, 2021 19:07

add changelog

0a1b8ab

Signed-off-by: Steve Sloka <[email protected]>

sunjayBhatia reviewed Oct 21, 2021

View reviewed changes

internal/dag/policy_test.go Outdated Show resolved Hide resolved

sunjayBhatia reviewed Oct 21, 2021

View reviewed changes

sunjayBhatia approved these changes Oct 21, 2021

View reviewed changes

stevesloka added 2 commits October 24, 2021 17:29

review comments

f345978

Signed-off-by: Steve Sloka <[email protected]>

set a kubebuilder default of 1 on nunmretries

a17efd8

Signed-off-by: Steve Sloka <[email protected]>

stevesloka added this to the 1.19.1 milestone Oct 24, 2021

regen

ef9ec5b

Signed-off-by: Steve Sloka <[email protected]>

skriss approved these changes Oct 25, 2021

View reviewed changes

stevesloka merged commit 0fadd8f into projectcontour:main Oct 25, 2021

stevesloka deleted the allowZeroRetries branch October 25, 2021 17:15

youngnick mentioned this pull request Nov 11, 2021

[Release 1.18] Cherry-pick: Allow retry policy, num retries to be zero #4174

Merged

internal: Allow retry policy, num retries to be zero #4117

internal: Allow retry policy, num retries to be zero #4117

Uh oh!

Conversation

stevesloka commented Oct 15, 2021

Uh oh!

codecov bot commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunjayBhatia left a comment

Choose a reason for hiding this comment

Uh oh!

skriss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Oct 15, 2021 •

edited

Loading