Runtime: Better Retry Mechanism Test For Networking failures

[Anipik]

Some of our networking failures are due to problems in the network.
Our current Retry mechanism is not optimal and is susceptible to false positives. currently we have to put the method body into a delegate in order to make the retry work.
I suggest here to introduce a new method and class based Xunit Attribute which could retry a test "N" number of times.
Retrying could be limited to some particular kind of exception. We could add more arguments to the attribute to limit the amount of false positives.
This would help us in having a better pass rate in our builds and make our ci system more reliant

cc @ViktorHofer @karelz @jaredpar

[ViktorHofer]

Thanks @Anipik for starting this conversation.

Our current Retry mechanism is not optimal and is susceptible to false positives. currently we have to put the method body into a delegate in order to make the retry work.

You are talking about RetryHelper which we rarely use in our tests.

Some of our networking failures are due to problems in the network.

I would phrase that slightly different. Most of the time, networking tests are failing because of their inherent flakiness (network transmission). As we want to get to a >=90% CI pass rate, we were thinking about adding a retry mechanism in-code.

This is different to what we had before with Helix in Mission Control as we don't retry on work items but as part of the test execution.

The implementation could look like this:

namespace Xunit
{
    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class | AttributeTargets.Assembly, AllowMultiple = true)]
    public class RetryAttribute
    {
        public RetryAttribute(int retryCount, Type exceptionType);
    }
}

[ViktorHofer]

cc @wfurt @ericstj @danmosemsft @dotnet/ncl

[davidsh]

I would phrase that slightly different. Most of the time, networking tests are failing because of their inherent flakiness (network transmission).

I don't think this is entirely proven with data that we have. Yes, in theory, network transmission could be flaky with packets being dropped especially over the Internet. But I don't think we have seen this specifically as the cause of our network tests failures.

For tests talking to the Azure remote servers, we've seen cases of Azure being down and this results in things like 503 HTTP status code being returned, sometimes for a few minutes. So, retries here won't help.

MacOS test failures have occurred several times but data points to DNS configuration issues or capacity/caching issues with the local Mac OS/network. This same is also true for some Linux tests we diagnosed where DNS seemed to "stop working" for a while. @wfurt suspected that some Linux OS cache setting needs to be adjusted for those CI machines.

And for other networking tests we have where it is going against the loopback server, it is not "network transmission" errors that are at fault here. Many times, we see OperationCanceledException from HttpClient tests. But we suspect those are really caused by timeouts being generated due to the threadpool not dispatching the async tasks of the network operation.

So, I think this proposed retry mechanism is interesting. And having it be attribute based might be useful to try out. But I don't think it will really move the needle on CI test reliability.

@stephentoub

[wfurt]

AFAIK all the tests talking to Azure were moved to OuterLoop. I don't know if/how that impacts 90% CI pass rate.

I agree with @davidsh that packer loss as source is questionable. That is what TCP was invented to deal with it.

I think part of the problem is that networking is nondeterministic so it is more difficult to cover all the cases and make the outcome predictable. @alnikola is working on data analyses to show actual failure rate. If we have only some tests failing it would suggest more test or product bug rather than general flakiness.

[karelz]

Agreed with @davidsh and @wfurt
We talked about retries in Networking several times over the last few years, but never found a real case for it, just theoretical -- the closest and slightly better option was to have dynamic precondition (e.g. check that Azure / external URL is reachable, before running test that depends on it -- while not perfect, it would address some external downtimes).

As @wfurt mentioned, we are aware of flakiness of Networking CI in general and we started (again) working on it -- @alnikola is driving the effort and data analysis where to focus as of last week.
@ViktorHofer if you have additional data to help us focus, that would be great. We are harnessing Kusto data from official runs (since ~12/5) and also from PRs (last month) - I can send you link to our OneNote if you are interested.

[scalablecory]

I am generally not happy with retries as a solution, because it will cover up actual errors.

My preferred approach would be to concurrently:

a) improve tests, in the ways @karelz mentioned, and

b) improve our workflow assuming there will be always (hopefully as few as possible) be some flaky tests. better tools to both analyze trends and to look at specific PR failures.

[jaredpar]

AFAIK all the tests talking to Azure were moved to OuterLoop. I don't know if/how that impacts 90% CI pass rate.

Awesome. Those won't impact the pass rate of the core runtime-ci. That is generally what we're tracking here.

As @wfurt mentioned, we are aware of flakiness of Networking CI in general and we started (again) working on it

Can you share the issues you're using to track the flakiness? I'd like to make sure they have the blocking-clean-ci + the intermittent labels so they show up in our health status queries.

[karelz]

@jaredpar we are tracking failing tests primarily via looking at CI history in Kusto. Test failures that are bothering CI too much are usually disabled, or fixed right away.
Rare test failures are filed and part of backlog.
I'd like to know more about how you are using the the labels you mentioned. We have been using test-run-core label to track CI failures.

[karelz]

Triage: We would be open to have retries if we can see all (even first-chance) failures.

We should wait for our test stabilization to see how it would look like and if it is really going to help with externally/infra influenced tests (which are majority of flakiness at least now) -- that will help us assess priority 5.0 vs. Future.

[danmosemsft]

FWIW we have had offline discussions recently about building resilience into our test system as a whole. Thinking of the larger system, it is probably impractical for our test cases to consistently pass 100% when we run 10's of millions of them each day across CI jobs, batched CI, official builds etc. Over the last few years we have tried to address this by disabling tests or adding retries at the test level but our data tells us that often the failures (I am not referring to networking tests necessarily) are tests that have not failed in a long time. The failures are often infrastructure or OS issues, or test defects rather than product issues. Hypothesis: almost always failures caused by changes in PR validation are consistent. As pointed out above, local retries also mask issues because they aren't reported. The ideal to me would be the ability to mark a group of tests (perhaps individual test libraries) as requiring resilience. If any tests fail in such a group, the system would record that as it does today (so we could follow up on problem areas asynchronously) and then retry the batch (on another machine) and mark the tests as green if they pass on retry. We would selectively opt test groups into resilience so we did not retry tests that have proven fully robust (eg., collections). This gives us reporting and also the ability to centrally set policy, eg., so we could have a signoff run that did not do retry where all failures were fully analyzed.

Not attempting to start a discussion here (sorry for hijacking networking issue) but giving larger context. If an effort starts to think about this I'd certainly like to bring in someone from networking.