Adds request retry attributes to HTTPRoute API Type #184

danehans · 2020-05-07T20:50:56Z

Extends ForwardToTarget of type HTTPRoute to include per-target request timeouts and retries.

Partially fixes #58.

k8s-ci-robot · 2020-05-07T20:50:59Z

@danehans: GitHub didn't allow me to request PR reviews from the following users: Miciah.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Extends ForwardToTarget of type HTTPRoute to include per-target request timeouts and retries.

Partially fixes #58.

/assign @bowei
/cc @Miciah @robscott

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2020-05-07T20:51:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danehans
To complete the pull request process, please assign bowei
You can assign the PR to them by writing /assign @bowei in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hbagdi · 2020-05-08T00:40:29Z

api/v1alpha1/httproute_types.go

+	// Support: Core
+	//
+	// +optional
+	Timeout *metav1.Duration `json:"timeout,omitempty" protobuf:"bytes,3,opt,name=timeout"`


It seems this is read timeout, should we rename this to ReadTimeout?
We should be explicit here if this is TTFB (time to first byte) or not. It seems most proxies have this meaning.

Do we also need Connect Timeout for time to define timeout for the TCP handshake?

PTAL at my latest changes and let me know if you feel this is a good starting point for connection timeouts and retries.

hbagdi · 2020-05-08T00:41:56Z

api/v1alpha1/httproute_types.go

+	// +optional
+	Timeout *metav1.Duration `json:"timeout,omitempty" protobuf:"bytes,3,opt,name=timeout"`
+
+	// RetryPolicy describes a policy for resending a failed request to a targetRef.


What defines a request as failed?
Will it be an L4 failure or L7 failure (idempotent HTTP methods)?

I updated the docs. A connection is considered failed when the connection is either refused (5xx) or times out.t

danehans · 2020-05-13T02:28:52Z

@hbagdi I just pushed b76d37b that addresses some of your feedback, PTAL.

k8s-ci-robot · 2020-05-19T19:51:28Z

@danehans: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

robscott · 2020-08-13T22:35:14Z

I think this may overlap with @bowei's Service level config proposal.

hbagdi · 2020-09-10T01:21:36Z

@danehans Should we keep this open or close this and track it as part of #196 and revisit post v1alpha1?

howardjohn · 2020-10-08T22:03:45Z

api/v1alpha1/httproute_types.go

@@ -264,6 +264,59 @@ type ForwardToTarget struct {
 	//
 	// +optional
 	TargetPort *TargetPort `json:"targetPort" protobuf:"bytes,2,opt,name=targetPort"`
+
+	// TimeoutPolicy describes a policy for configuring connection timeouts to targetRef.


I am not sure how much we want to cater to specific implementations but I don't think Envoy supports this, specifically the timeout and retry config is at the route level, so you cannot set it differently for each weighted cluster destination.

danehans · 2020-12-01T17:22:50Z

Note that Knative has deprecated managing connection timeouts: https://github.com/knative/networking/blob/4b21f11ccfa7fbd2ab5c0a978e78e79f4fe728d8/pkg/apis/networking/v1alpha1/ingress_types.go#L240

Miciah · 2020-12-04T04:37:13Z

api/v1alpha1/httproute_types.go

+	//
+	// Support: Core
+	//
+	Response metav1.Duration `json:"response" protobuf:"bytes,1,opt,name=response"`


Is this an inactivity timeout or a deadline to complete the transfer? The field's description is suggests it is a deadline (for example, if the response is large, and it takes too long, a proxy may terminate the transfer even while data are being transferred), in which case it should be named as follows:

Suggested change

Response metav1.Duration `json:"response" protobuf:"bytes,1,opt,name=response"`

ResponseDeadlineSeconds metav1.Duration `json:"response" protobuf:"bytes,1,opt,name=responseDeadlineSeconds"`

On the other hand, if this is an inactivity timeout (for example, if the server doesn't send any data within this amount of time, a proxy may terminate the connection), then it should be named as follows:

Suggested change

Response metav1.Duration `json:"response" protobuf:"bytes,1,opt,name=response"`

ResponseTimeoutSeconds metav1.Duration `json:"response" protobuf:"bytes,1,opt,name=responseTimeoutSeconds"`

See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#naming-conventions.

On second thought, maybe "Seconds" should be omitted for metav1.Duration, which is unmarshalled using time.ParseDuration and thus accepts time-unit suffixes (https://github.com/kubernetes/apimachinery/blob/2456ebdaba229616fab2161a615148884b46644b/pkg/apis/meta/v1/duration.go#L31-L45). Oddly, I cannot find any APIs in k8s.io/api that use metav1.Duration. Do you have examples of Kubernetes APIs that do use metav1.Duration?

Ah, https://github.com/kubernetes/cloud-provider/blob/master/config/types.go has some examples of fields that use metav1.Duration.

So I think the field should be named either responseDeadline or responseTimeout.

Miciah · 2020-12-04T04:37:14Z

api/v1alpha1/httproute_types.go

+	//
+	// Support: Core
+	//
+	Idle metav1.Duration `json:"idle" protobuf:"bytes,2,opt,name=idle"`


Suggested change

Idle metav1.Duration `json:"idle" protobuf:"bytes,2,opt,name=idle"`

IdleTimeout metav1.Duration `json:"idle" protobuf:"bytes,2,opt,name=idleTimeout"`

Miciah · 2020-12-04T04:37:30Z

api/v1alpha1/httproute_types.go

+	//
+	// Support: Core
+	//
+	Interval metav1.Duration `json:"interval" protobuf:"bytes,2,opt,name=interval"`


Suggested change

Interval metav1.Duration `json:"interval" protobuf:"bytes,2,opt,name=interval"`

retryPeriod metav1.Duration `json:"interval" protobuf:"bytes,2,opt,name=retryPeriod"`

k8s-ci-robot · 2021-01-11T18:05:48Z

@danehans: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-service-apis-test	`b76d37b`	link	`/test pull-service-apis-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

nak3 · 2021-03-23T06:12:51Z

api/v1alpha1/generated.proto

@@ -658,6 +672,24 @@ message LocalObjectReference {
  optional string name = 3;
 }

+// RetryPolicy describes a policy for retrying a connection until the
+// connection is either refused or times out.


Envoy has more fine-grained policy setting.

And Knative (net-contour) leverages it - https://github.com/knative-sandbox/net-contour/blob/42ebe377c7c9d1e62fe4265ff3ef0cee285e8be3/pkg/reconciler/contour/resources/httpproxy.go#L80-L95
So it would be great if we could have some optional field to customize it.

Also, we would like to know the default retry policy.

danehans · 2021-04-14T18:36:14Z

From 4/14/21 community meeting:

Retries are not as portable as heath-checking and timeout connection policies.
Consensus around health-check, timeout, retry, etc. policies residing in BackEndPolicy while the ServicePolicy design.

robscott · 2021-07-22T22:31:10Z

I think that with GEP #713 we've determined that this is a better fit for policy attachment than being built into the route. I'm going to close this in favor of that approach but anyone can feel free to reopen if needed.

/close

k8s-ci-robot · 2021-07-22T22:31:15Z

@robscott: Closed this PR.

In response to this:

I think that with GEP #713 we've determined that this is a better fit for policy attachment than being built into the route. I'm going to close this in favor of that approach but anyone can feel free to reopen if needed.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot assigned bowei May 7, 2020

k8s-ci-robot requested a review from robscott May 7, 2020 20:50

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 7, 2020

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 7, 2020

hbagdi reviewed May 8, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2020

Adds HTTPRoute connection retry and timeout attributes

b76d37b

danehans force-pushed the timeout_retry branch from c2d09ec to b76d37b Compare May 13, 2020 02:27

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 13, 2020

This was referenced May 14, 2020

Add timeouts to destination connection details #89

Closed

Service-level configuration #196

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2020

robscott mentioned this pull request Aug 20, 2020

Adding BackendPolicy for Service-level config #268

Merged

howardjohn reviewed Oct 8, 2020

View reviewed changes

hbagdi added this to the v1alpha2 milestone Dec 3, 2020

Miciah reviewed Dec 4, 2020

View reviewed changes

nak3 reviewed Mar 23, 2021

View reviewed changes

robscott modified the milestones: v1alpha2, v0.3.0 Mar 24, 2021

nak3 mentioned this pull request Mar 30, 2021

Add TestRetry conformance test knative-extensions/net-gateway-api#20

Open

robscott added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Apr 14, 2021

robscott removed this from the v0.3.0 milestone Apr 23, 2021

k8s-ci-robot closed this Jul 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds request retry attributes to HTTPRoute API Type #184

Adds request retry attributes to HTTPRoute API Type #184

danehans commented May 7, 2020

k8s-ci-robot commented May 7, 2020

k8s-ci-robot commented May 7, 2020

hbagdi May 8, 2020

danehans May 13, 2020

hbagdi May 8, 2020

danehans May 13, 2020

danehans commented May 13, 2020

k8s-ci-robot commented May 19, 2020

robscott commented Aug 13, 2020

hbagdi commented Sep 10, 2020

howardjohn Oct 8, 2020

danehans commented Dec 1, 2020

Miciah Dec 4, 2020

Miciah Dec 4, 2020

Miciah Dec 4, 2020

Miciah Dec 4, 2020 •

edited

Loading

Miciah Dec 4, 2020 •

edited

Loading

k8s-ci-robot commented Jan 11, 2021

nak3 Mar 23, 2021 •

edited

Loading

nak3 Mar 30, 2021

danehans commented Apr 14, 2021

robscott commented Jul 22, 2021

k8s-ci-robot commented Jul 22, 2021

	Response metav1.Duration `json:"response" protobuf:"bytes,1,opt,name=response"`
	ResponseDeadlineSeconds metav1.Duration `json:"response" protobuf:"bytes,1,opt,name=responseDeadlineSeconds"`

	Response metav1.Duration `json:"response" protobuf:"bytes,1,opt,name=response"`
	ResponseTimeoutSeconds metav1.Duration `json:"response" protobuf:"bytes,1,opt,name=responseTimeoutSeconds"`

	Idle metav1.Duration `json:"idle" protobuf:"bytes,2,opt,name=idle"`
	IdleTimeout metav1.Duration `json:"idle" protobuf:"bytes,2,opt,name=idleTimeout"`

	Interval metav1.Duration `json:"interval" protobuf:"bytes,2,opt,name=interval"`
	retryPeriod metav1.Duration `json:"interval" protobuf:"bytes,2,opt,name=retryPeriod"`

Adds request retry attributes to HTTPRoute API Type #184

Adds request retry attributes to HTTPRoute API Type #184

Conversation

danehans commented May 7, 2020

k8s-ci-robot commented May 7, 2020

k8s-ci-robot commented May 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans commented May 13, 2020

k8s-ci-robot commented May 19, 2020

robscott commented Aug 13, 2020

hbagdi commented Sep 10, 2020

Choose a reason for hiding this comment

danehans commented Dec 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Miciah Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 11, 2021

nak3 Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans commented Apr 14, 2021

robscott commented Jul 22, 2021

k8s-ci-robot commented Jul 22, 2021

Miciah Dec 4, 2020 •

edited

Loading

Miciah Dec 4, 2020 •

edited

Loading

nak3 Mar 23, 2021 •

edited

Loading