Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should be possible to set an actual revision timeout (max duration, not first byte) #10851

Closed
julz opened this issue Feb 26, 2021 · 10 comments · Fixed by #12322
Closed

Should be possible to set an actual revision timeout (max duration, not first byte) #10851

julz opened this issue Feb 26, 2021 · 10 comments · Fixed by #12322
Assignees
Labels
area/API API objects and controllers good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Well-understood/specified features, ready for coding. triage/accepted Issues which should be fixed (post-triage)
Milestone

Comments

@julz
Copy link
Member

julz commented Feb 26, 2021

/area API

Describe the feature

The revision spec.TimeoutSeconds feature was changed at some point to only be a timeout until the first byte of the response is sent. This is useful for WebSocket apps because it avoids timing out idle long running websockets. Unfortunately it defeats quite a lot of the point of a timeout because it doesn't stop requests running for an arbitrarily long time. This makes it hard to ensure it's safe to drain a node or avoid infinite-looping requests costing money for unbounded periods (absent a liveness check, anyway).

We should add a MaxDurationSeconds field to the spec to limit the actual max duration a request can run. This would be useful for users and operators who want to set an actual max duration a request/function can run. To avoid problems with websockets this would be an optional field defaulting to 0=infinity.

@julz julz added the kind/feature Well-understood/specified features, ready for coding. label Feb 26, 2021
@knative-prow-robot knative-prow-robot added the area/API API objects and controllers label Feb 26, 2021
@evankanderson
Copy link
Member

Is this separate from #10852? The two seem related/duplicate.

If we're updating the timeout lifecycle, it seems like it would be reasonable to have one issue to track both knobs.

/triage needs-user-input

@knative-prow-robot knative-prow-robot added the triage/needs-user-input Issues which are waiting on a response from the reporter label Mar 15, 2021
@julz
Copy link
Member Author

julz commented Mar 16, 2021

I think they're two separate features. One is an idle timeout, one is the maximum length of a "function"/request. It's easy to imagine us only implementing/prioritising one or the other, and I definitely think a PR that did both at once would be too big.

@github-actions
Copy link

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 15, 2021
@julz
Copy link
Member Author

julz commented Jun 15, 2021

/remove-lifecycle stale

I still think this is good, I just didn’t get to it yet

@knative-prow-robot knative-prow-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 15, 2021
@evankanderson
Copy link
Member

I'm going to guess here:

/good-first-issue
/help

/remove-triage needs-user-input
/triage accepted

@knative-prow-robot
Copy link
Contributor

@evankanderson:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

I'm going to guess here:

/good-first-issue
/help

/remove-triage needs-user-input
/triage accepted

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.

@knative-prow-robot knative-prow-robot added triage/accepted Issues which should be fixed (post-triage) good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed triage/needs-user-input Issues which are waiting on a response from the reporter labels Jun 23, 2021
@skonto
Copy link
Contributor

skonto commented Jul 28, 2021

@julz you mind if I work on this?

@julz
Copy link
Member Author

julz commented Jul 28, 2021

not at all!

/assign @dprotaso to make sure you're ok with the api (tldr a MaxDurationSeconds field which is the actual max request length, not just time to first byte).

/assign @skonto

@dprotaso
Copy link
Member

Yeah 👍

@skonto
Copy link
Contributor

skonto commented Oct 8, 2021

Back to this, now that idle timeout final piece is close to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API API objects and controllers good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Well-understood/specified features, ready for coding. triage/accepted Issues which should be fixed (post-triage)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants