[failing] test/conformance/api/v1.TestRevisionTimeout

[izabelacg]

Expected Behavior

Tests passing successfully.

Actual Behavior

Consistent failures:

Test name: test/conformance/api/v1.TestRevisionTimeout/writes_first_byte_before_timeout
Repository name: serving
Testing areas: Gateway API + Contour (periodic job)

Steps to Reproduce the Problem

See: https://testgrid.k8s.io/r/knative-own-testgrid/serving#gateway-api-contour

[dprotaso]

/assign @izabelacg

[izabelacg]

RevisionSpec.Timeout has changed meaning: #12634

[izabelacg]

We need to investigate if the test needs to be updated and/or if the behaviour is actually expected. See below what I observed so far.
When curling the service with the tests parameters, we get:

$ curl -v -H "Host: revision-timeout-writes-first-byte-before-pmzegqbs.serving-tests.example.com" "$EXTERNAL_IP?timeout=15000&initialTimeout=0"
...
> GET /?timeout=15000&initialTimeout=0 HTTP/1.1
> Host: revision-timeout-writes-first-byte-before-pmzegqbs.serving-tests.example.com
> User-Agent: curl/8.4.0
> Accept: */*
>
< HTTP/1.1 200 OK
< date: Tue, 09 Apr 2024 15:40:32 GMT
< x-envoy-upstream-service-time: 2
< vary: Accept-Encoding
< server: envoy
< transfer-encoding: chunked
<
* transfer closed with outstanding read data remaining
* Closing connection
curl: (18) transfer closed with outstanding read data remaining

When curling the pod directly from within the cluster, we get:

$ curl -v "10.112.5.251:8012?timeout=15000&initialTimeout=0"
*   Trying 10.112.5.251:8012...
* Connected to 10.112.5.251 (10.112.5.251) port 8012
> GET /?timeout=15000&initialTimeout=0 HTTP/1.1
> Host: 10.112.5.251:8012
> User-Agent: curl/8.7.1
> Accept: */*
>
* Request completely sent off
< HTTP/1.1 200 OK
< Date: Tue, 09 Apr 2024 15:55:08 GMT
< Transfer-Encoding: chunked
<
* Connection #0 to host 10.112.5.251 left intact
Slept for 15000 milliseconds

[dprotaso]

Note the (net-gateway-api) changes that will land in #15136 don't address this failure.

[dprotaso]

I recall discussing this issue - something is up with the test because by the definition of the Revision.Spec.TimeoutSeconds says it's the max time allowed to process a request.

In the test we have RevisionTimeout=10s but tell the app to sleep 15s. Thus it should fail but we expect a http.StatusOK in the test which seems wrong.

[dprotaso]

Also disabling timeouts is broken in Contour see:

knative-extensions/net-gateway-api#711
projectcontour/contour#6373

[izabelacg]

Also disabling timeouts is broken in Contour see:

I think this explains why the test is failing ONLY for gateway-api.

In the test we have RevisionTimeout=10s but tell the app to sleep 15s. Thus it should fail but we expect a http.StatusOK in the test which seems wrong.

I'm wondering why the test is passing for contour and istio if this is not expected behaviour, since it seems correct behaviour for the timeouts has been established by #12634. I'm unsure if I misunderstood any of the timeouts, but here is what I've concluded so far:

• TestRevisionTimeout/writes_first_byte_before_timeout was meant to make sure that once we receive any bytes from the user's application, we can set a timeout to determine how long we should wait for the response to be fully sent. So, I think this shouldn't be testing Revision.Spec.TimeoutSeconds, but rather Revision.Spec.IdleTimeoutSeconds - which is achieved by this other test:

  serving/test/e2e/timeout_test.go

  Lines 110 to 116 in 4548446

  	}, {
  	name: "writes response before idle timeout",
  	timeoutSeconds: 10,
  	idleTimeoutSeconds: 5,
  	expectedStatus: http.StatusOK,
  	sleep: 2 * time.Second,
  	}, {

• However, the above still doesn't explain why the test works for contour and istio. As Dave stated and per docs definition the TimeoutSeconds is "the maximum duration in seconds that the request instance is allowed to respond to a request". So, in my understanding it should be "respected" even if other existing timeouts have been set. So, I'm looking to understand if there is unexpected behaviour in the current implementation.

• I think the entire file ./test/conformance/api/v1/revision_timeout_test.go could be deleted, since it is covered by the tests in ./test/e2e/timeout_test.go. I learned that e2e and conformance tests may not be interchangeable.