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

SymUploader removal stage 1: Remove usage of symuploader in Feed task #15012

Merged
merged 14 commits into from
Aug 29, 2024

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented Aug 20, 2024

  • The first commit: Adds symbol helper lib to help manage calls to symbol.exe. This can be shared between the shared between release/stage and nightly.
  • the second commit: Change Microsoft.DotNet.Build.Tasks.Feed to use SymbolHelper.

Validation with builds happening manually currently.

A few things have changed as breaking changes:

  • We restrict the process to Windows x64 - we can only achieve conversion in such platforms. If no conversion needs to happen, we can relax this a bit.
  • Loose files only get their symbols converted if both the PDB and DLL are supplied. Before it tried converting if the PDB was in the directory - but this avoids uploading unrequested information.
  • Before we only published loose files in non-streaming mode if at least 1 package were to be published. Now we always check.
  • DryRun no longer validates information on the binaries. Now that the contract is to call symbol.exe, we don't assert what information it needs. It just prints what the invocation would be.
  • Request name must always be specified for all operations.
  • Special file indexing must be requested - CLR file publishing is no longer the default.

@hoyosjs hoyosjs force-pushed the dev/juhoyosa/remove-use-of-symuploader branch from 82e3a37 to edbe691 Compare August 20, 2024 18:29
- Use polly for resilience on symbol.exe download.
- Fix some redirect and potential stream issues
- Add a new SymbolPromotionHelper to help with symbol promotion
@hoyosjs hoyosjs force-pushed the dev/juhoyosa/remove-use-of-symuploader branch from edbe691 to f6fd117 Compare August 20, 2024 18:43
@hoyosjs hoyosjs force-pushed the dev/juhoyosa/remove-use-of-symuploader branch from 81ddbc9 to da0bbbe Compare August 21, 2024 01:58
@hoyosjs
Copy link
Member Author

hoyosjs commented Aug 21, 2024

The failures are related to token issues - I am using a PAT as a token and I didn't realize that the "Bearer" scheme won't work for that. I've changed to always using basic with both PAT and access token for the client download.

@hoyosjs
Copy link
Member Author

hoyosjs commented Aug 21, 2024

@hoyosjs
Copy link
Member Author

hoyosjs commented Aug 22, 2024

Contributes to #14501

@hoyosjs
Copy link
Member Author

hoyosjs commented Aug 22, 2024

That worked e2e. And so does one with the special indexing: https://dev.azure.com/dnceng/internal/_build/results?buildId=2521600&view=results.

@hoyosjs
Copy link
Member Author

hoyosjs commented Aug 22, 2024

To leave a cookie behind about perf issues:

  • Process invocations are expensive and we are doing a per-package.
    • If no "special indexing" is requested, packages are trivially batch-indexable. Usefulness is limited a little by the way streaming publishing is implemented though. There are no memory back pressure mechanisms in place so there's only a client throttle doing N downloads and publish at the time, each independent of each other. The N packages could go in the same invocation of symbol.exe.
    • If special indexing is requested, we could do some massaging of the manifests so we can also batch stuff.
  • Significantly more verbose logging happens now. We can try to limit it.
  • We use streaming redirection and pay async costs for it. Symbol has a file tracing mode. We could consider creating a file for the invocation and then load that and write that in a batch to avoid console stream contention. This one is more of an experiment than a sure-footed perf gain.

@hoyosjs
Copy link
Member Author

hoyosjs commented Aug 22, 2024

epananth
epananth previously approved these changes Aug 23, 2024
@hoyosjs hoyosjs dismissed stale reviews from epananth and mikem8361 via 578db0c August 26, 2024 20:49
@hoyosjs hoyosjs force-pushed the dev/juhoyosa/remove-use-of-symuploader branch from 578db0c to 3c409d7 Compare August 26, 2024 20:49
@hoyosjs hoyosjs force-pushed the dev/juhoyosa/remove-use-of-symuploader branch from 3c409d7 to 0d7ff45 Compare August 26, 2024 20:56
mmitche
mmitche previously approved these changes Aug 27, 2024
@hoyosjs
Copy link
Member Author

hoyosjs commented Aug 27, 2024

@hoyosjs
Copy link
Member Author

hoyosjs commented Aug 27, 2024

There was a failure but there's nothing in the console logs and the binlog isn't uploaded. rerun with binlog collection https://dev.azure.com/dnceng/internal/_build/results?buildId=2525547

@hoyosjs
Copy link
Member Author

hoyosjs commented Aug 27, 2024

Promotion of arcade with latest bits https://dev.azure.com/dnceng/internal/_build/results?buildId=2525549

@hoyosjs
Copy link
Member Author

hoyosjs commented Aug 28, 2024

Both of them ran clean... To make sure, one last runtime run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2525650

@mmitche
Copy link
Member

mmitche commented Aug 28, 2024

Both of them ran clean... To make sure, one last runtime run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2525650

Failed!

@hoyosjs hoyosjs merged commit d7f6456 into dotnet:main Aug 29, 2024
11 checks passed
@hoyosjs hoyosjs deleted the dev/juhoyosa/remove-use-of-symuploader branch August 29, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants