RevisionSpec.Timeout has deviated from the original specification

[dprotaso]

/area API

I dropped the ball regarding how we handled RevisionSpec.Timeout in the OSS implementation. We deviated from what was originally in the spec and opt'd to change the spec (#10806) rather than fix the behaviour.

We prioritized backwards compatibility over remaining conformant.

In hindsight this was a mistake since:

1. This led us to introduced a new field to cover the functional gap - (Should be possible to set an actual revision timeout (max duration, not first byte) #10851)
2. Timeout and MaxDuration being so similar is confusing and bad UX for end-users
3. Workloads running on the OSS implementation will behave differently on other compliant Knative offerings.

New plan is here: #12634 (comment)

I think the best path forward here is:

• Drop MaxDurationSeconds from the RevisionSpec #12635
  • Yes this is poor form but it's only been out for two weeks in the latest release at the time of writing and hasn't been documented
• create a feature flag that enables the first byte timeout logic (on by default)
• notify users about this default and be explicit they should set it for compatibility
• after N releases we swap the default to be conformant to the spec's original intention
• For those who opt into the first-byte timeout we provide a maxDuration annotation so they still have that ability
• Allow a per-revision override via annotations so existing revisions can work independently of the operator settings

[dprotaso]

/assign @dprotaso

[evankanderson]

It feels like "time to first response byte" is a valid but niche use case. Rather than introducing a flag to change the meaning of timeoutSeconds, have we considered introducing a firstByteMaxTime or the like?

Thinking about it, it seems like the main use case for fBMT is for server or bidirectional streaming scenarios, which are definitely not the default.

[evankanderson]

(I don't like firstByteMaxTime as a name, for the record.)

It still seems useful to be able to specify a first but and max stream duration at the Revision level, to enable safe lame-duck operation and bound times for proxy-level drain and restart without complicated TCP handoffs.

[dprotaso]

Rather than introducing a flag to change the meaning of timeoutSeconds, have we considered introducing a firstByteMaxTime or the like?

I had not. Though now I wonder if adding support for idleTimeout (#10852) would cover the niche case of first byte timeout

[skonto]

Timeout and MaxDuration being so similar is confusing and bad UX for end-users

It is confusing inded. IMHO MaxDuration is a self-explanatory name and seems better that Timeout to me.

Should we capture this in a design doc and describe how previous cases can be handled with the "new" design?
It would help also documenting some of the scenarios eg. "if I use websockets then..." (diagrams would be nice too to explain options).