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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1d37ef8
Updated build.yml
andystaples Jan 27, 2025
537862b
Added test.yml
andystaples Jan 27, 2025
f5512ab
Updated official-build.yml
andystaples Jan 27, 2025
40f5cfa
Updated test.yml
andystaples Jan 27, 2025
d062e3f
Updated test.yml
andystaples Jan 27, 2025
f38df82
Fix build issue for test on ado
andystaples Jan 27, 2025
1e97cfe
Updated nuget.config
andystaples Jan 28, 2025
7fe5c45
Updated DurableFunctions.TypedInterfaces.Example.csproj
andystaples Jan 29, 2025
5f9976c
Remove irrelevant changes
andystaples Jan 29, 2025
c5cc336
Update Worker SDK package in smoke test
andystaples Jan 29, 2025
bf1e431
Run smoketest on older ubuntu
andystaples Jan 29, 2025
0803255
Revert to HelloCitiesUntyped for .NET isolated
andystaples Jan 29, 2025
4dc5c92
Probable bugfix for OOME handling
andystaples Jan 29, 2025
03ba8ea
Merge branch 'dev' into andystaples/fix-official-build
nytian Jan 31, 2025
e371aac
Update release_notes.md
andystaples Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions .github/workflows/smoketest-dotnet-isolated-v4.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ on:

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

@nytian nytian Jan 31, 2025 •
edited
Loading

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

steps:
- uses: actions/checkout@v2

Expand Down Expand Up @@ -73,11 +73,19 @@ jobs:
# when building the smoke test app in docker, causing the build to fail. This is a temporary workaround until the
# root cause is identified and fixed.

# Due to a known issue with class-based orchestrators, this test fails to discover the orchestrator definition.
# Should re-enable once this bug is fixed: https://github.com/microsoft/durabletask-dotnet/issues/247
# - name: Run smoke tests (Hello Cities)
# shell: pwsh
# run: azurite --silent --blobPort 10000 --queuePort 10001 --tablePort 10002 &
# cd ./test/SmokeTests/OOProcSmokeTests/DotNetIsolated && func host start --port 7071 &
# ./test/SmokeTests/OOProcSmokeTests/DotNetIsolated/run-smoke-tests.ps1 -HttpStartPath api/StartHelloCitiesTyped

- name: Run smoke tests (Hello Cities)
shell: pwsh
run: azurite --silent --blobPort 10000 --queuePort 10001 --tablePort 10002 &
cd ./test/SmokeTests/OOProcSmokeTests/DotNetIsolated && func host start --port 7071 &
./test/SmokeTests/OOProcSmokeTests/DotNetIsolated/run-smoke-tests.ps1 -HttpStartPath api/StartHelloCitiesTyped
./test/SmokeTests/OOProcSmokeTests/DotNetIsolated/run-smoke-tests.ps1 -HttpStartPath api/StartHelloCitiesUntyped

- name: Run smoke tests (Process Exit)
shell: pwsh
Expand Down
2 changes: 1 addition & 1 deletion eng/ci/official-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,4 @@ extends:
- stage: BuildAndSign
dependsOn: []
jobs:
- template: /eng/templates/build.yml@self
- template: /eng/templates/build.yml@self
6 changes: 6 additions & 0 deletions eng/templates/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ jobs:
inputs:
packageType: 'sdk'
version: '6.0.x'

- task: UseDotNet@2
displayName: 'Use the .NET 8 SDK'
inputs:
packageType: 'sdk'
version: '8.0.x'

# Start by restoring all the dependencies.
- task: DotNetCoreCLI@2
Expand Down
3 changes: 0 additions & 3 deletions nuget.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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!

</packageSources>
</configuration>
1 change: 1 addition & 0 deletions release_notes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

- Fix custom connection name not working when using IDurableClientFactory.CreateClient() - contributed by [@hctan](https://github.com/hctan)
- Made durable extension for isolated worker configuration idempotent, allowing multiple calls safely. (#2950)
- Fixes a bug with Out of Memory exception handling in Isolated, improving reliability of retries for this case. (part of #3020)

### Breaking Changes

Expand Down
2 changes: 1 addition & 1 deletion src/DurableFunctions.TypedInterfaces/Example/DurableFunctions.TypedInterfaces.Example.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
<ItemGroup>
<PackageReference Include="Microsoft.AspNetCore.Http.Abstractions" Version="2.2.0" />
<PackageReference Include="Microsoft.Azure.DurableTask.AzureStorage" Version="2.0.0-preview.4" />
<PackageReference Include="Microsoft.Azure.DurableTask.Core" Version="2.13.0" />
<PackageReference Include="Microsoft.Azure.DurableTask.Core" Version="2.15.1" />
<PackageReference Include="Microsoft.Azure.WebJobs.Extensions.DurableTask" Version="2.4.3" />
<PackageReference Include="Microsoft.NET.Sdk.Functions" Version="3.0.11" />
</ItemGroup>
Expand Down
2 changes: 1 addition & 1 deletion src/WebJobs.Extensions.DurableTask/ContextImplementations/RemoteOrchestratorContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

{
throw new SessionAbortedException(failureDetails.ErrorMessage);
}
Expand Down
2 changes: 1 addition & 1 deletion test/SmokeTests/OOProcSmokeTests/DotNetIsolated/DotNetIsolated.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
This preview package is to enable better NuGet restore behavior with custom feeds.
TODO: upgrade to 1.16.0 when it's released.
-->
<PackageReference Include="Microsoft.Azure.Functions.Worker.Sdk" Version="1.16.0-preview2" OutputItemType="Analyzer" />
<PackageReference Include="Microsoft.Azure.Functions.Worker.Sdk" Version="1.18.1" OutputItemType="Analyzer" />
<PackageReference Include="Microsoft.DurableTask.Generators" Version="1.0.0-preview.1" OutputItemType="Analyzer" />
</ItemGroup>

Expand Down
Loading