feat(translator): Implement BTP Timeout API by guydc · Pull Request #2454 · envoyproxy/gateway

guydc · 2024-01-16T23:53:21Z

What this PR does / why we need it:
Implement the BackendTrafficPolicy Timeout API

Which issue(s) this PR fixes:
Fixes #2401

codecov · 2024-01-16T23:57:08Z

Codecov Report

Attention: 84 lines in your changes are missing coverage. Please review.

Comparison is base (8d19d77) 64.70% compared to head (ed5ca57) 64.61%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
internal/ir/zz_generated.deepcopy.go	0.00%	61 Missing and 1 partial ⚠️
internal/gatewayapi/backendtrafficpolicy.go	86.41%	7 Missing and 4 partials ⚠️
internal/gatewayapi/route.go	57.69%	9 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2454      +/-   ##
==========================================
- Coverage   64.70%   64.61%   -0.09%     
==========================================
  Files         116      116              
  Lines       17613    17797     +184     
==========================================
+ Hits        11396    11500     +104     
- Misses       5486     5560      +74     
- Partials      731      737       +6

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

arkodg · 2024-01-17T01:15:27Z

internal/gatewayapi/backendtrafficpolicy.go

can we use lower case here to make the func private - setBackend....

arkodg · 2024-01-17T01:16:51Z

internal/ir/xds.go

prefer if this was renamed to Timeout and we moved the existing Timeout field into it and renamed it to RequestTimeout under HTTP

shawnh2 · 2024-01-17T04:37:13Z

internal/xds/translator/cluster.go

these two duration should be checked (whether is nil ptr) before assign values toIdleTimeout and MaxConnectionDuration, nor it will raise a nil pointer panic.

http: - name: "first-listener" #... routes: - name: "first-route" backendTimeout: tcp: connectTimeout: "31s" http: connectionIdleTimeout: "32s" #maxConnectionDuration: "33s" # paniced #...

shawnh2

lgtm, thanks

zhaohuabing

This implementation LGTM, thanks!

A minor comment on the name of Timeout structure in the API: we may want to change it to BackendTimeout because the TCP ConnectTimeout can't be reused for the future client-side Timeouts.

// BackendTimeout defines configuration for timeouts related to the backend.
type BackendTimeout struct {
	// Timeout settings for TCP.
	//
	// +optional
	TCP *TCPTimeout `json:"tcp,omitempty"`

	// Timeout settings for HTTP.
	//
	// +optional
	HTTP *HTTPTimeout `json:"http,omitempty"`
}

guydc · 2024-01-23T12:22:54Z

A minor comment on the name of Timeout structure in the API: we may want to change it to BackendTimeout because the TCP ConnectTimeout can't be reused for the future client-side Timeouts.

Can we translate this to transport socket timeout for downstream connections? If so, it can be reused...

arkodg · 2024-01-23T18:20:15Z

internal/gatewayapi/route.go

nit: policy is computed/translated post route translation, so we can assume its empty here

I wanted to this to be safe in case there's a re-arrangement of the computation order. If you feel that we can make this assumption, i'll update the implementation accordingly.

sure, if thats the case, can you also add a comment here, so the next dev can understand why this done this way similar to the comment you have in policy around request timeout

arkodg · 2024-01-23T19:19:09Z

internal/gatewayapi/backendtrafficpolicy.go

we should log error in status and continue processing instead of returning early here

arkodg · 2024-01-23T19:19:15Z

internal/gatewayapi/backendtrafficpolicy.go

same as above

arkodg · 2024-01-23T19:19:22Z

internal/gatewayapi/backendtrafficpolicy.go

same as above

arkodg · 2024-01-23T19:26:36Z

internal/xds/translator/testdata/out/xds-ir/http-route-timeout.routes.yaml

what happened here, why did 5s get deleted ?

arkodg · 2024-01-23T19:28:09Z

internal/xds/translator/testdata/in/xds-ir/http-route-timeout.yaml

test is broken, probably needs below config

timeout: http: requestTimeout: 5s

zhaohuabing · 2024-01-24T02:15:37Z

A minor comment on the name of Timeout structure in the API: we may want to change it to BackendTimeout because the TCP ConnectTimeout can't be reused for the future client-side Timeouts.

Can we translate this to transport socket timeout for downstream connections? If so, it can be reused...

Not sure about it, they look similar but not exactly the same. Are there any other possible different settings between client and backend timeouts?

@arkodg @zirain WDYT?

arkodg · 2024-01-26T02:29:34Z

A minor comment on the name of Timeout structure in the API: we may want to change it to BackendTimeout because the TCP ConnectTimeout can't be reused for the future client-side Timeouts.

Can we translate this to transport socket timeout for downstream connections? If so, it can be reused...

Not sure about it, they look similar but not exactly the same. Are there any other possible different settings between client and backend timeouts?

@arkodg @zirain WDYT?

the go struct name can be changed anytime w/o breaking YAML API compatibility so no strong comments for now from my side

guydc · 2024-01-26T15:03:24Z

Regarding E2E test for this feature: echo server does support a /delay endpoint. However, since these settings are per-connection and not per-request, timed-out connections will not terminate active delayed requests. In other words, it's difficult to demonstrate a connection timeout on a specific request.

idle timeout: "The idle timeout is defined as the period in which there are no active requests."
max connection duration: "When max_connection_duration is reached and if there are no active streams, the connection will be closed"

One way to verify timeout settings with an e2e test is to trigger some upstream connections, wait for timeouts to occur and then check the relevant envoy cluster metrics.

I propose to handle the e2e test in a dedicated issue.

arkodg · 2024-01-26T17:25:20Z

sure @guydc lets raise seperate GH issues for e2e and docs and tackle those in different PRs

arkodg

LGTM thanks !

Signed-off-by: Guy Daich <guy.daich@sap.com>

guydc · 2024-01-29T15:21:37Z

/retest

arkodg

LGTM thanks !

guydc requested a review from a team as a code owner January 16, 2024 23:53

arkodg reviewed Jan 17, 2024

View reviewed changes

shawnh2 reviewed Jan 17, 2024

View reviewed changes

guydc force-pushed the impl-btp-timeout branch from 8485b19 to ab06b32 Compare January 22, 2024 23:57

shawnh2 approved these changes Jan 23, 2024

View reviewed changes

zirain requested a review from arkodg January 23, 2024 03:08

zhaohuabing reviewed Jan 23, 2024

View reviewed changes

arkodg reviewed Jan 23, 2024

View reviewed changes

internal/gatewayapi/backendtrafficpolicy.go Outdated

Copy link
Copy Markdown

Contributor

arkodg Jan 23, 2024

Choose a reason for hiding this comment

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

same as above

arkodg reviewed Jan 23, 2024

View reviewed changes

internal/gatewayapi/backendtrafficpolicy.go Outdated

Copy link
Copy Markdown

Contributor

arkodg Jan 23, 2024

Choose a reason for hiding this comment

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

same as above

arkodg reviewed Jan 23, 2024

View reviewed changes

guydc force-pushed the impl-btp-timeout branch from ab06b32 to f556086 Compare January 26, 2024 14:46

arkodg previously approved these changes Jan 26, 2024

View reviewed changes

guydc mentioned this pull request Jan 26, 2024

e2e: Backend Traffic Policy Timeout #2510

Closed

Implement BTP Timeout API

ed5ca57

Signed-off-by: Guy Daich <guy.daich@sap.com>

guydc dismissed arkodg’s stale review via ed5ca57 January 29, 2024 13:59

guydc force-pushed the impl-btp-timeout branch from f556086 to ed5ca57 Compare January 29, 2024 13:59

arkodg approved these changes Jan 29, 2024

View reviewed changes

arkodg merged commit f1a0a42 into envoyproxy:main Jan 29, 2024

Conversation

guydc commented Jan 16, 2024

Uh oh!

codecov bot commented Jan 16, 2024 • 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!

shawnh2 left a comment

Choose a reason for hiding this comment

Uh oh!

zhaohuabing left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guydc commented Jan 23, 2024

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!

Choose a reason for hiding this comment

Uh oh!

zhaohuabing commented Jan 24, 2024

Uh oh!

arkodg commented Jan 26, 2024

Uh oh!

guydc commented Jan 26, 2024

Uh oh!

arkodg commented Jan 26, 2024

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

guydc commented Jan 29, 2024

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jan 16, 2024 •

edited

Loading

zhaohuabing left a comment •

edited

Loading