Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/5.0] Fix OS version check and testhost copying in stress tests #41321

Merged
merged 9 commits into from
Aug 28, 2020

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 25, 2020

Backport of #40980 and #41397 to release/5.0

/cc @alnikola

Customer Impact

This PR fixes broken HTTP and SSL stress pipelines. No direct customer impact, but having the stress pipeline up and running is crucial to unblock investigation of stress failures we observed on the master branch.

Testing

Risk

None. No changes to production code.

@ghost
Copy link

ghost commented Aug 25, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

@ghost
Copy link

ghost commented Aug 25, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@alnikola
Copy link
Contributor

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alnikola
Copy link
Contributor

/azp run runtime-libraries stress-ssl

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alnikola
Copy link
Contributor

@karelz We need to port it back to 5.0 to investigate stress test failures.

@alnikola alnikola marked this pull request as draft August 25, 2020 16:10
@alnikola
Copy link
Contributor

We found an issue in the master branch. Holding off the backport for now.

@karelz karelz added this to the 5.0.0 milestone Aug 27, 2020
Fixed docker files not using currently built runtime for http stress test.
@ManickaP
Copy link
Member

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP
Copy link
Member

/azp run runtime-libraries stress-ssl

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP ManickaP self-assigned this Aug 28, 2020
@ManickaP
Copy link
Member

Linux: client_1 | System.Net.Http: /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0-rc.1.20425.16/System.Net.Http.dll, modified 08/28/2020 11:09:07
Windows: client_1 | System.Net.Http: C:\Program Files\dotnet\shared\Microsoft.NETCore.App\5.0.0-rc.1.20425.16\System.Net.Http.dll, modified 8/28/2020 11:05:11 AM

Based on modified date, the tests are running on freshly built bits.

Copy link
Contributor

@aik-jahoda aik-jahoda left a comment

Choose a reason for hiding this comment

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

LGTM with nits

Comment on lines +116 to +117
public bool Equals([AllowNull] T left, [AllowNull] T right) => left != null && left.Equals(right, StructuralComparisons.StructuralEqualityComparer);
public int GetHashCode([DisallowNull] T value) => value.GetHashCode(StructuralComparisons.StructuralEqualityComparer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public bool Equals([AllowNull] T left, [AllowNull] T right) => left != null && left.Equals(right, StructuralComparisons.StructuralEqualityComparer);
public int GetHashCode([DisallowNull] T value) => value.GetHashCode(StructuralComparisons.StructuralEqualityComparer);
public bool Equals(T? left, T? right) => left != null && left.Equals(right, StructuralComparisons.StructuralEqualityComparer);
public int GetHashCode(T value) => value.GetHashCode(StructuralComparisons.StructuralEqualityComparer);

Copy link
Member

Choose a reason for hiding this comment

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

Addressed in #41513, it's not necessary to be ported. The code works as-is.

Comment on lines +455 to +456
public bool Equals([AllowNull] T left, [AllowNull] T right) => left != null && left.Equals(right, StructuralComparisons.StructuralEqualityComparer);
public int GetHashCode([DisallowNull] T value) => value.GetHashCode(StructuralComparisons.StructuralEqualityComparer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public bool Equals([AllowNull] T left, [AllowNull] T right) => left != null && left.Equals(right, StructuralComparisons.StructuralEqualityComparer);
public int GetHashCode([DisallowNull] T value) => value.GetHashCode(StructuralComparisons.StructuralEqualityComparer);
public bool Equals(T? left, T? right) => left != null && left.Equals(right, StructuralComparisons.StructuralEqualityComparer);
public int GetHashCode(T value) => value.GetHashCode(StructuralComparisons.StructuralEqualityComparer);

Copy link
Member

Choose a reason for hiding this comment

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

Addressed in #41513, it's not necessary to be ported. The code works as-is.

@ManickaP
Copy link
Member

CI runtime failure JIT\\Regression\\JitBlue\\Runtime_40444\\Runtime_40444\\Runtime_40444.cmd: #40885 --> seems like the fix is not backported in 5.0 branch.

@ManickaP
Copy link
Member

The failures from http-stress test are being tracked in #40388. They're not the goal of this PR, this PR is just fixing the infra running the tests.

@ManickaP ManickaP marked this pull request as ready for review August 28, 2020 17:37
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

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

lgtm

@scalablecory
Copy link
Contributor

@ericstj okay to merge this? no changes to product code, only to our test suite so we can run stress tests for 5.0.

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Approved. Test only

@ericstj
Copy link
Member

ericstj commented Aug 28, 2020

Make sure it’s green or failures are understood and issues filed

1 similar comment
@ericstj
Copy link
Member

ericstj commented Aug 28, 2020

Make sure it’s green or failures are understood and issues filed

@ManickaP
Copy link
Member

CI runtime failures:
HTTP: #41078
SMTP: unrelated, the change doesn't touch SMTP at all.

@ManickaP ManickaP merged commit 10068f4 into release/5.0 Aug 28, 2020
@ManickaP ManickaP deleted the backport/pr-40980-to-release/5.0 branch August 28, 2020 19:58
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants