Fix timeout handlers to max duration instead of time to first byte

[nader-ziada]

Fixes #12634

Proposed Changes

• Use max duration instead of time to first byte in timeout handler
• add RevisionRequestStartTimeout field to use a first byte timeout for a request
• expose RevisionIdleTimeout

Release Note

Revision timeouts now has the following three values, TimeoutSeconds represents the max duration a request can take, ResponseStartTimeoutSeconds is the timeout allowed before a request starts responding and IdleTimeoutSeconds is the max duration a request can remain open without getting any data. 

/hold

[vagababov]

That's not a good idea in general — how are you going to deal with long lasting streaming requests? Is there an issue attached?

[codecov[bot]]

Codecov Report

Merging #12970 (8c0f7bd) into main (5bba016) will decrease coverage by 0.08%. The diff coverage is 71.05%.

@@            Coverage Diff             @@
##             main   #12970      +/-   ##
==========================================
- Coverage   86.72%   86.64%   -0.09%     
==========================================
  Files         196      196              
  Lines       14463    14493      +30     
==========================================
+ Hits        12543    12557      +14     
- Misses       1624     1638      +14     
- Partials      296      298       +2     

Impacted Files	Coverage Δ
pkg/apis/serving/v1/revision_types.go	100.00% <ø> (ø)
pkg/queue/sharedmain/main.go	0.64% <0.00%> (-0.01%)	:arrow_down:
pkg/http/handler/timeout.go	83.44% <70.58%> (-6.36%)	:arrow_down:
pkg/apis/config/defaults.go	84.48% <84.21%> (-1.24%)	:arrow_down:
pkg/reconciler/revision/resources/queue.go	98.23% <100.00%> (+0.09%)	:arrow_up:
pkg/reconciler/configuration/configuration.go	83.67% <0.00%> (-1.54%)	:arrow_down:
pkg/autoscaler/scaling/multiscaler.go	87.24% <0.00%> (-1.35%)	:arrow_down:
pkg/autoscaler/statforwarder/leases.go	73.95% <0.00%> (+1.56%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[nader-ziada]

That's not a good idea in general — how are you going to deal with long lasting streaming requests? Is there an issue attached?

I'm just starting to work on this based on this issue here: https://github.com/knative/serving/issues/12634#issuecomment-1137765904 , its not complete yet

[nader-ziada]

@vagababov would appreciate your feedback since you know this area of the code pretty well

[vagababov]

Mostly as above: you have no way of killing failed/deadlocked, but longer running tasks without TTFB. Reading the issue, it seems that there's some inconsistency with spec. But also removing this might break some backward compatibility :)

[dprotaso]

long running streams (ie. websockets) is already broken

you have no way of killing failed/deadlocked, but longer running tasks without TTFB

I want to introduce IdleTimeout to handle the TTFB - we can introduce a sane default there - min(10s,Spec.TimeoutSeconds) or something

See the issue here: https://github.com/knative/serving/issues/12634

[vagababov]

We must go deeper! :) Idle timeout is certainly useful in some scenarios, and not for others — so your default is infinite or huge? e.g. I do on demand video conversion. All in all, all three might be necessary.

But yeah, my main concern would be not breaking existing users.

[vagababov]

I still think the idea of profiles we had (not quite sure if it went anywhere, I look only at things that catch my eye) — would provide canned sets of these settings, but oh well :)

[dprotaso]

All in all, all three might be necessary.

Yeah that's fair

But yeah, my main concern would be not breaking existing users.

OSS broke portability and I think that's critical to fix. Providing alternative knobs (idle, ttfb, timeout) should give users enough flexibility.

[nader-ziada]

All in all, all three might be necessary.

Yeah that's fair

But yeah, my main concern would be not breaking existing users.

OSS broke portability and I think that's critical to fix. Providing alternative knobs (idle, ttfb, timeout) should give users enough flexibility.

how about we go back to keeping all three and allowing the user to choose the behaviour, for example enabling or disabling the ttfb?

[dprotaso]

how about we go back to keeping all three and allowing the user to choose the behaviour, for example enabling or disabling the ttfb?

I would prefer three different attributes - vs. some extra annotation that tweaks the semantics of TimeoutSeconds

[nader-ziada]

how about we go back to keeping all three and allowing the user to choose the behaviour, for example enabling or disabling the ttfb?

I would prefer three different attributes - vs. some extra annotation that tweaks the semantics of TimeoutSeconds

so add MaxDuration back to the spec?

[dprotaso]

so add MaxDuration back to the spec?

No - TimeoutSeconds is MaxDuration

[nader-ziada]

so add MaxDuration back to the spec?

No - TimeoutSeconds is MaxDuration

I'm confused, TimeoutSeconds is currently used by timeoutFirstByte, and we were going to remove that and use it for MaxDuration but now thinking we should not remove timeoutFirstByte after all.

Are you saying use TimeoutSeconds for max duration and add a new fields for timeoutFirstByte and idleTimeout?

[dprotaso]

Are you saying use TimeoutSeconds for max duration and add a new fields for timeoutFirstByte and idleTimeout?

yup

TimeoutSeconds should behave as the spec originally intended which is max duration

[nader-ziada]

/retest

[nader-ziada]

/test upgrade-tests_serving_main

[nader-ziada]

The upgrade tests are failing because we are added a new field, related issue here https://github.com/knative/serving/issues/11980

[nader-ziada]

/test upgrade-tests_serving_main

[nader-ziada]

/retest

[nader-ziada]

/retest

[nader-ziada]

/retest

[nader-ziada]

@dprotaso ready for review

[knative-prow[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, nader-ziada

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

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [dprotaso]
• ~~pkg/apis/OWNERS~~ [dprotaso]

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

[nader-ziada]

/hold cancel