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

Fix the official build #3020

Merged
merged 15 commits into from
Jan 31, 2025
Merged

Fix the official build #3020

merged 15 commits into from
Jan 31, 2025

Conversation

andystaples
Copy link
Contributor

@andystaples andystaples commented Jan 29, 2025

Issue describing the changes in this PR

Fixes several pipeline issues:

  • Official pipeline (1ES)
    • E2E function app was written with .NET 8, but .NET 8 SDK was not installed as part of the pipeline. Install it.
    • Non-official NuGet sources were causing indeterministic failures by downloading a version of DurableTask.ApplicationInsights without a strong name. Removed these sources
      • Removing these sources meant that the smoke tests couldn't get the worker SDK version 1.16.0-preview2. Updated to 1.18.1
    • DurableFunctions.TypedInterfaces.Example DurableTask.Core updated from 2.13.0 to 2.15.1 to match other sln in project
  • .NET Isolated Smoke tests

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)
  • My changes do not require any extra work to be leveraged by OutOfProc SDKs
    • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
  • My changes do not change the version of the WebJobs.Extensions.DurableTask package
    • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs
  • My changes do not add EventIds to our EventSource logs
    • Otherwise: Ensure the EventIds are within the supported range in our existing Windows infrastructure. You may validate this with a deployed app's telemetry. You may also extend the range by completing a PR such as this one.
  • My changes should be added to v2.x branch.
    • Otherwise: This change applies exclusively to WebJobs.Extensions.DurableTask v3.x. It will be retained only in the dev and main branches and will not be merged into the v2.x branch.

@andystaples andystaples force-pushed the andystaples/fix-official-build branch from 80beb3b to bf1e431 Compare January 29, 2025 20:34
@@ -4,8 +4,5 @@
<packageSources>
<clear/>
<add key="nuget.org" value="https://api.nuget.org/v3/index.json" />
<add key="AzureFunctionsTempStaging" value="https://pkgs.dev.azure.com/azfunc/e6a70c92-4128-439f-8012-382fe78d6396/_packaging/AzureFunctionsTempStaging/nuget/v3/index.json" />
<add key="durabletask" value="https://pkgs.dev.azure.com/durabletaskframework/734e7913-2fab-4624-a174-bc57fe96f95d/_packaging/durabletask/nuget/v3/index.json" />
<add key="durabletask-test" value="https://durabletaskframework.pkgs.visualstudio.com/734e7913-2fab-4624-a174-bc57fe96f95d/_packaging/durabletask-test/nuget/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.

I like this change because it makes the nuget restore behavior deterministic when the same version exists in multiple feeds, but I just want to check with @bachuv and @nytian that this change is safe to make. In particular, I'm wondering whether it changes how we need to go about our release process.

If we need to keep relying on some of these feeds, we could look into cleaning up these alternate feeds to make sure we don't have any duplicate packages across them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with removing these package sources.

add key="AzureFunctionsTempStaging" value="https://pkgs.dev.azure.com/azfunc/e6a70c92-4128-439f-8012-382fe78d6396/_packaging/AzureFunctionsTempStaging/nuget/v3/index.json"

I'm actually not sure why we have this package source. Maybe @nytian knows.

add key="durabletask" value="https://pkgs.dev.azure.com/durabletaskframework/734e7913-2fab-4624-a174-bc57fe96f95d/_packaging/durabletask/nuget/v3/index.json" />
add key="durabletask-test" value="https://durabletaskframework.pkgs.visualstudio.com/734e7913-2fab-4624-a174-bc57fe96f95d/_packaging/durabletask-test/nuget/v3/index.json" />

These package sources are from our old ADO durabletaskframework org which we don't use anymore so we should remove these.

We can consider adding our new azfunc artifact feed to the package sources: add key="durabletask-test" value="https://azfunc.pkgs.visualstudio.com/internal/_packaging/durabletask-test/nuget/v3/index.json", but we can also add this in the future if it's helpful.

Overall, I only use package sources that are not nuget.org when I'm performing local tests.

Copy link
Contributor Author

@andystaples andystaples Jan 30, 2025

Choose a reason for hiding this comment

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

I'm actually not sure why we have this package source. Maybe @nytian knows.

Actually, I may be able to answer this, although I'll let Naiyuan confirm
The smoke tests were relying on a prerelease package of the .NET worker SDK for some behavior fix, version 1.16.0-preview2. I assume that we brought in the staging source to get this package. This PR updates to a released version that is on nuget.org

Copy link
Collaborator

@nytian nytian Jan 31, 2025

Choose a reason for hiding this comment

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

Sorry for the late reply! Actually, I am not quite sure about what this is, and I thought nuget.org is enough for downloading all the packages. I didn't recall any package downloaded from this source at VS, but I might be wrong :)@andystaples please let us know if you have context about this, thank you!

@@ -134,7 +134,7 @@ private void ThrowIfPlatformLevelException(FailureDetails failureDetails)
// It's unclear if all OOMs are caught by the isolated process (probably not), and also if there are other platform-level
// errors that are also caught in the isolated process and returned as a `OrchestratorExecutionResult`. Let's add them
// to this method as we encounter them.
if (failureDetails.InnerFailure?.IsCausedBy<OutOfMemoryException>() ?? false)
if (failureDetails.IsCausedBy<OutOfMemoryException>())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, which test failed before you made this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The smoke test for Out of Memory exception retry handling - when David and I were writing that feature, every case we saw had the OOM wrapped at least one layer of InnerFailure deep, so we skipped checking the outermost failure details. Turns out, (likely when I upgraded the worker SDK version) this behavior changed and OOM became the outermost failure, meaning our check didn't catch it. Likely why we've had customers saying that the fix didn't help their app - because they were on latest worker SDK

Copy link
Collaborator

@nytian nytian left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! just left a small question

@@ -15,7 +15,7 @@ on:

jobs:
build:
runs-on: ubuntu-latest
runs-on: ubuntu-22.04
Copy link
Collaborator

@nytian nytian Jan 31, 2025

Choose a reason for hiding this comment

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

just to confirm, this change is because the ubuntu-latest image in Github is having issue, so we have to pick a version for now? I am asking this because I am wondering if we need to change this back in future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - we should change this back in the future, once the bug is fixed in ubuntu-latest

@andystaples andystaples merged commit 3bfd149 into dev Jan 31, 2025
13 checks passed
@andystaples andystaples deleted the andystaples/fix-official-build branch January 31, 2025 20:40
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