Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

add instrumentation for intermittent DNS failures #34934

Merged
merged 4 commits into from
Jan 30, 2019
Merged

add instrumentation for intermittent DNS failures #34934

merged 4 commits into from
Jan 30, 2019

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jan 29, 2019

per discussion in #34187, this PR adds new exception so we can track if retry would make improvements for TryAgain error cases.

@wfurt wfurt added os-linux Linux OS (any supported distro) arch-arm64 labels Jan 29, 2019
@wfurt wfurt self-assigned this Jan 29, 2019
@wfurt wfurt requested review from stephentoub and a team January 29, 2019 20:01
@wfurt wfurt mentioned this pull request Jan 29, 2019
Closed
stephentoub
stephentoub approved these changes Jan 29, 2019
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@davidsh davidsh added this to the 3.0 milestone Jan 29, 2019
@wfurt
Copy link
Member Author

wfurt commented Jan 29, 2019

note that I saw one failure when running on my Ubuntu system attached to corp net:

   Starting:    System.Net.NameResolution.Pal.Tests (parallel test collections = on, max threads = 8)
      System.Net.NameResolution.PalTests.NameResolutionPalTests.TryGetAddrInfo_ExternalHost [FAIL]
        Assert.Equal() Failure
        Expected: Success
        Actual:   HostNotFound
        Stack Trace:
          /home/furt/github/wfurt-corefx-arm/src/System.Net.NameResolution/tests/PalTests/NameResolutionPalTests.cs(146,0): at System.Net.NameResolution.PalTests.NameResolutionPalTests.TryGetAddrInfo_ExternalHost()
    Finished:    System.Net.NameResolution.Pal.Tests
  === TEST EXECUTION SUMMARY ===
     System.Net.NameResolution.Pal.Tests  Total: 11, Errors: 0, Failed: 1, Skipped: 0, Time: 0.348s
  ~/github/wfurt-corefx-arm/src/System.Net.NameResolution/tests/PalTests

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.

@karelz
Copy link
Member

karelz commented Jan 29, 2019

@wfurt can you please change the title to be more descriptive? References to PRs/issues in title are usually not useful. Thanks!

@wfurt wfurt changed the title add instrumentation for #32797 add instrumentation for intermittent DNS failures Jan 29, 2019
rmkerr
rmkerr approved these changes Jan 29, 2019
View reviewed changes
Copy link
Contributor

@rmkerr rmkerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wfurt wfurt merged commit 673fe78 into dotnet:master Jan 30, 2019
caesar-chen
caesar-chen reviewed Jan 30, 2019
View reviewed changes
src/System.Net.NameResolution/tests/PalTests/NameResolutionPalTests.cs
@@ -35,7 +35,6 @@ public void TryGetAddrInfo_LocalHost()
Assert.NotNull(hostEntry.Aliases);
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotArm64Process))] // [ActiveIssue(32797)]
Copy link
Contributor

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

Copy link
Contributor

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.

@wfurt wfurt mentioned this pull request Jan 30, 2019
Merged
stephentoub
stephentoub reviewed Jan 31, 2019
View reviewed changes
src/System.Net.NameResolution/tests/PalTests/NameResolutionPalTests.cs
{
throw new InvalidOperationException("Name resolution failure preventable with retry");
}
}

Assert.Equal(SocketError.Success, error);
Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another:
https://mc.dot.net/#/user/dotnet-bot/pr~2Fdotnet~2Fcorefx~2Frefs~2Fpull~2F35024~2Fmerge/test~2Ffunctional~2Fcli~2F/20190201.12/workItem/System.Net.NameResolution.Pal.Tests/analysis/xunit/System.Net.NameResolution.PalTests.NameResolutionPalTests~2FTryGetAddrInfo_HostName

@wfurt
Copy link
Member Author

wfurt commented Feb 1, 2019

yes, this change proved me wrong. I did query last night and saw 73 failures where retry yield same result.
I'm working in plan B to stabilize DNS lookups.

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@wfurt
add instrumentation for intermittent DNS failures (dotnet/corefx#34934)
a0d2073 
* add instrumentation for dotnet/corefx#32797

* actually retry the lookup

* use PlatformID.Unix


Commit migrated from dotnet/corefx@673fe78
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@davidsh davidsh davidsh approved these changes

@caesar-chen caesar-chen caesar-chen left review comments

@rmkerr rmkerr rmkerr approved these changes

Assignees

@wfurt wfurt

Labels
arch-arm64 area-System.Net os-linux Linux OS (any supported distro)
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.
6 participants
@wfurt @karelz @stephentoub @rmkerr @davidsh @caesar-chen