-
Notifications
You must be signed in to change notification settings - Fork 4.9k
add instrumentation for intermittent DNS failures #34934
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
src/System.Net.NameResolution/tests/PalTests/NameResolutionPalTests.cs
Outdated
Show resolved
Hide resolved
note that I saw one failure when running on my Ubuntu system attached to corp net:
when I run it again it passed. This is annoying and more difficult to handle as it returns permanent failure claiming that microsoft.com does not exist. |
@wfurt can you please change the title to be more descriptive? References to PRs/issues in title are usually not useful. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -35,7 +35,6 @@ public void TryGetAddrInfo_LocalHost() | |||
Assert.NotNull(hostEntry.Aliases); | |||
} | |||
|
|||
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotArm64Process))] // [ActiveIssue(32797)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test case inside a test project, we need [Fact]/[Theory] label to run the test. Here seems we remove the [ConditionalFact] without adding a new [Fact] label, and I don't think the test will actually run - or this is intentional? not running the test?
cc: @davidsh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. And there is another test in this file also that now no longer runs. It needs to be changed to "[Fact]" as well.
{ | ||
throw new InvalidOperationException("Name resolution failure preventable with retry"); | ||
} | ||
} | ||
|
||
Assert.Equal(SocketError.Success, error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test just failed again at this assert, meaning the retry didn't help, at least not in that case:
https://mc.dot.net/#/user/dotnet-bot/pr~2Fdotnet~2Fcorefx~2Frefs~2Fpull~2F34956~2Fmerge/test~2Ffunctional~2Fcli~2F/20190130.31/workItem/System.Net.NameResolution.Pal.Tests/analysis/xunit/System.Net.NameResolution.PalTests.NameResolutionPalTests~2FTryGetAddrInfo_HostName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this change proved me wrong. I did query last night and saw 73 failures where retry yield same result. |
* add instrumentation for dotnet/corefx#32797 * actually retry the lookup * use PlatformID.Unix Commit migrated from dotnet/corefx@673fe78
per discussion in #34187, this PR adds new exception so we can track if retry would make improvements for TryAgain error cases.