Removes 503 as a retryable status code in tests

[bsnchan]

Given that the activator now has retry logic (#1226) and we have a temporary workaround for Istio's spurious 503's (#785), we should remove the 503 as a possible retry-able code.

Note: 503's are still allowed for newly created revisions

[bsnchan]

/test pull-knative-serving-integration-tests

[bsnchan]

/test pull-knative-serving-unit-tests

[bsnchan]

/assign @bobcatfish
/assign @adrcunha

since this change is in the e2e and conformance tests.

[bobcatfish]

Nice one @bsnchan let's do it!

[bobcatfish]

/hold

reviewing the details now

[bobcatfish]

Thanks for making this change @bsnchan and I like how you're simplifying the interface to WaitForEndpointState! Most of my feedback is around the naming and structure of the Options object but in general I like this!

[bobcatfish]

I like where you're going with this but I have a few thoughts:

• Options is a pretty generic name, but this Option only applies to tests that are querying a route endpoint for a certain status
• There is some overlap with the EnvironmentFlags object, maybe there is a way to reuse that object here? (And in fact Add --namespace flag to e2e & conformance test #1329 will be adding namespace to that object)

What about creating an Options struct that is specific to WaitForEndpointState? e.g. something like:

type ExpectedEndpointState {
    ResolvableDomain bool
    Domain string
    AllowableStatuses []int
}

It could contain the options that are specific to the WaitForEndpointState, cleaning up the interface a bit, and the rest of the options could stay individual params (or we could use EnvironmentFlags directly but I feel like there is more in that struct than WaitForEndpointState really needs).

[bobcatfish]

I prefer explicitly passing in []int{404} in the first case to hardcoding a default here because a) it makes it more obvious what the test is expecting, b) it's more obvious what is different between the calls to WaitForEndpointState and c) this seems like an unexpected side effect

[bobcatfish]

One other option, you could have a NewOptions (or NewExpectedEndpointState) method that returns an instance of Options/ExpectedEndpointState and defaults the allowable statuses to []int{404}, my opinion is that that would be a bit clearer.

[bsnchan]

@bobcatfish - just did the changes you suggested. Let me know if you think it needs anything else :)

[bsnchan]

/hold cancel

[jonjohnsonjr]

Oh no, my #1458 is going to conflict with this a lot 👀

[google-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bsnchan
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: adrcunha

If they are not already assigned, you can assign the PR to them by writing /assign @adrcunha in a comment when ready.

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• test/OWNERS

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

[bsnchan]

@jonjohnsonjr - I took your changes and rebased :)

@bobcatfish - Can you let me know if there's anything else blocking this please?

[jonjohnsonjr]

What do you think about #1528 instead?

@bobcatfish this is what I was talking about here:

serving/test/spoof/spoof.go

Line 168 in 0f1c435

// TODO(jonjohnson): This could just be pulled out into a retrying ResponseChecker middleware thing.

[mattmoor]

/hold

Wait until after we cut first release.