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

H/3 stress #55098

Merged
merged 11 commits into from
Jul 9, 2021
Merged

H/3 stress #55098

merged 11 commits into from
Jul 9, 2021

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Jul 2, 2021

Spin off #54762

  • updates docker images
  • fixes some build warnings in stress apps
  • add H/3 support for HttpStress
  • renames Quic event source

Fixes #40389

@ghost
Copy link

ghost commented Jul 2, 2021

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

Issue Details

Spin off #54762

  • updates docker images
  • fixes some build warnings in stress apps
  • add H/3 support for HttpStress
  • renames Quic event source
Author: ManickaP
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@dotnet dotnet deleted a comment from azure-pipelines bot Jul 6, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 6, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 6, 2021
@ManickaP
Copy link
Member Author

ManickaP commented Jul 6, 2021

The stress tests have run. So the pipelines work, docker files are OK, etc.
There's tons of error in H/2 (Windows and Linux) for content sending requests (PUT, POST etc.) looking like this:

System.Exception: Expected status code OK, got InternalServerError
client_1  |    at HttpStress.ClientOperations.ValidateStatusCode(HttpResponseMessage m, HttpStatusCode expectedStatus) in /app/ClientOperations.cs:line 487
client_1  |    at HttpStress.ClientOperations.<>c.<<get_Operations>b__1_9>d.MoveNext() in /app/ClientOperations.cs:line 407
client_1  | --- End of stack trace from previous location ---
client_1  |    at HttpStress.StressClient.<>c__DisplayClass17_0.<<StartCore>g__RunWorker|0>d.MoveNext() in /app/StressClient.cs:line 204

This might be a recent change in kestrel or our client that breaks it since I'm updating here to 6.0 nightly.

@antonfirsov Do you know anything about it?

cc: @CarnaViire

@ManickaP ManickaP marked this pull request as ready for review July 6, 2021 17:43
@ManickaP
Copy link
Member Author

ManickaP commented Jul 7, 2021

So the error reproduces locally without docker. It happened sometime between 6.0 preview 4 and preview 6. I tested it firstly on preview 4 - no error, downloaded preview 6 and retested and it happens.

@ManickaP
Copy link
Member Author

ManickaP commented Jul 7, 2021

H/2 issue filed: #55261
I guess we can get this in and fix the issue separately.

@@ -1,4 +1,4 @@
ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/nightly/sdk:5.0-buster-slim
ARG SDK_BASE_IMAGE=mcr.microsoft.com/dotnet/nightly/sdk:6.0-bullseye-slim
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? Seems like buster is still latest stable...

Copy link
Member Author

Choose a reason for hiding this comment

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

No we don't. I didn't realize it's "testing" and not "stable". I just took the newest without checking. I'll revert it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll need the bullseye after all. Seems like buster doesn't have current 6.0 preview SDK...

<configuration>
<packageSources>
<!-- Add public nuget feed. -->
<add key="nuget.org" value="https://api.nuget.org/v3/index.json" />
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this default? I don't mind to be explicit but I'm wondering if we need it in case we do not need custom feeds

Copy link
Member Author

Choose a reason for hiding this comment

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

Not if run from runtime repo, ours explicitly removes nuget.org: https://github.com/dotnet/runtime/blob/main/NuGet.config
So it all works in docker, where it's isolated, but when you ran it locally from S.N.Http/tests/... it failed due to one dependency coming from the public feed.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

generally looks ok to me.

@dotnet dotnet deleted a comment from azure-pipelines bot Jul 7, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 7, 2021
@@ -47,6 +47,14 @@ jobs:
name: buildStress
displayName: Build HttpStress

- bash: |
Copy link
Member Author

Choose a reason for hiding this comment

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

We'll see, but on Linux we should be now able to run stress tests.

@@ -7,6 +7,18 @@ RUN echo "DOTNET_VERSION="$DOTNET_VERSION
WORKDIR /app
COPY . .

# Pulling the msquic Debian package from msquic-ci public pipeline and from a hardcoded build.
# Note that this is a temporary solution until we have properly published Linux packages.
# Also note that in order to update to a newer msquic build, you have update this link.
Copy link
Member

Choose a reason for hiding this comment

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

Does this link already contain changes needed for #55291? If not, can we update it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it does contain them.

@ManickaP
Copy link
Member Author

ManickaP commented Jul 8, 2021

/azp run runtime-libraries stress-http

@ManickaP
Copy link
Member Author

ManickaP commented Jul 8, 2021

/azp run runtime-libraries stress-ssl

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot Jul 8, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 8, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 8, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 8, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 8, 2021
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 8, 2021
@ManickaP
Copy link
Member Author

ManickaP commented Jul 8, 2021

Stress tests on Linux are running H/3 🥳

@ManickaP
Copy link
Member Author

ManickaP commented Jul 8, 2021

Seeing 10s thousands of System.Exception: Expected status code OK, got InternalServerError in H/2 (#55261)
I'll handle this particular error before merging and put an ActiveIssue note in the code.

@ManickaP
Copy link
Member Author

ManickaP commented Jul 8, 2021

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP
Copy link
Member Author

ManickaP commented Jul 8, 2021

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP
Copy link
Member Author

ManickaP commented Jul 9, 2021

Looks good now: https://dev.azure.com/dnceng/public/_build/results?buildId=1227406&view=results
1 error on H 1.1 Win and several errors on H/3 GET Aborted, after the suppression of #55261

CI errors are unrelated, merging.

@ManickaP ManickaP merged commit d431d6a into dotnet:main Jul 9, 2021
@ManickaP ManickaP deleted the mapichov/h3_stress branch July 9, 2021 07:41
@karelz karelz added this to the 6.0.0 milestone Jul 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
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.

[HTTP/3] Add HTTP/3 and QUIC stress tests
4 participants