-
Notifications
You must be signed in to change notification settings - Fork 329
Fix stress pipeline: rename variables to avoid env var injection, simplify runner #4329
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
base: main
Are you sure you want to change the base?
Changes from all commits
d2ea04f
6827b81
315f878
09a1d25
77fbbd6
e9fdf2c
d2ce122
73d38a1
40670e7
98f2b89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,6 @@ parameters: | |
| # True to enable debugging steps. | ||
| - name: debug | ||
| type: boolean | ||
| default: false | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoiding template parameter default values except at the top level. |
||
|
|
||
| # The prefix to prepend to the job's display name: | ||
| # | ||
|
|
@@ -36,7 +35,6 @@ parameters: | |
| # The verbosity level for the dotnet CLI commands. | ||
| - name: dotnetVerbosity | ||
| type: string | ||
| default: normal | ||
| values: | ||
| - quiet | ||
| - minimal | ||
|
|
@@ -48,20 +46,18 @@ parameters: | |
| - name: jobNameSuffix | ||
| type: string | ||
|
|
||
| # True to fail the job when stress tests fail. When false, test failures produce warnings | ||
| # (SucceededWithIssues) but do not fail the job. | ||
| - name: failOnTestFailure | ||
| # When true, test failures produce warnings (SucceededWithIssues) but do not fail the job. | ||
| # When false, test failures fail the job. All test steps always run regardless of this setting. | ||
| - name: warnOnTestFailure | ||
| type: boolean | ||
|
|
||
| # The list of .NET Framework runtimes to test against. | ||
| - name: netFrameworkTestRuntimes | ||
| type: object | ||
| default: [] | ||
|
|
||
| # The list of .NET runtimes to test against. | ||
| - name: netTestRuntimes | ||
| type: object | ||
| default: [] | ||
|
|
||
| # The name of the Azure Pipelines pool to use. | ||
| - name: poolName | ||
|
|
@@ -105,18 +101,6 @@ jobs: | |
| - name: project | ||
| value: $(Build.SourcesDirectory)/src/Microsoft.Data.SqlClient/tests/StressTests/SqlClient.Stress.Runner/SqlClient.Stress.Runner.csproj | ||
|
|
||
| # dotnet CLI arguments for build. | ||
| - name: buildArguments | ||
| value: >- | ||
| --verbosity ${{ parameters.dotnetVerbosity }} | ||
| -p:Configuration=${{ parameters.buildConfiguration }} | ||
|
|
||
| # dotnet CLI arguments for run. | ||
| - name: runArguments | ||
| value: >- | ||
| --verbosity ${{ parameters.dotnetVerbosity }} | ||
| --configuration ${{ parameters.buildConfiguration }} | ||
|
|
||
| # The contents of the config file to use for all tests. We will write this to a JSON file and | ||
| # then point to it via the STRESS_CONFIG_FILE environment variable. | ||
| - name: configContent | ||
|
|
@@ -137,9 +121,29 @@ jobs: | |
| } | ||
| ] | ||
|
|
||
| # Stress test command-line arguments. | ||
| - name: testArguments | ||
| value: -a SqlClient.Stress.Tests -console | ||
| # IMPORTANT: Do NOT name pipeline variables "runArguments", "buildArguments", or | ||
| # "testArguments". ADO exposes all pipeline variables as environment variables (uppercased), | ||
| # and the dotnet CLI's System.CommandLine reads env vars matching {COMMAND}ARGUMENTS (e.g. | ||
| # RUNARGUMENTS, BUILDARGUMENTS) and silently injects their content into the parsed arguments — | ||
| # bypassing the "--" separator. This causes app arguments to contain SDK options and triggers | ||
| # unintended behavior. | ||
|
|
||
| # dotnet CLI options for build. | ||
| - name: dotnetBuildOpts | ||
| value: >- | ||
| --verbosity ${{ parameters.dotnetVerbosity }} | ||
| -p:Configuration=${{ parameters.buildConfiguration }} | ||
|
|
||
| # dotnet run options shared by all test steps (framework is appended per-step). | ||
| - name: dotnetRunOpts | ||
| value: >- | ||
| --no-build | ||
| --verbosity ${{ parameters.dotnetVerbosity }} | ||
| --configuration ${{ parameters.buildConfiguration }} | ||
|
|
||
| # Stress test options passed after the "--" separator. | ||
| - name: stressTestOpts | ||
| value: --assembly SqlClient.Stress.Tests --console | ||
|
|
||
| steps: | ||
|
|
||
|
|
@@ -178,7 +182,7 @@ jobs: | |
| inputs: | ||
| command: build | ||
| projects: $(project) | ||
| arguments: $(buildArguments) | ||
| arguments: ${{ variables.dotnetBuildOpts }} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer to expand static variables at pipeline expansion time, rather than runtime. |
||
|
|
||
| # Set a flag so test steps can distinguish a build failure from a test failure. | ||
| - pwsh: Write-Host "##vso[task.setvariable variable=buildSucceeded]true" | ||
|
|
@@ -194,29 +198,33 @@ jobs: | |
| # - eq(variables['buildSucceeded'], 'true') gates on the flag set above, so tests are | ||
| # skipped entirely if the build or any setup step failed (since there's nothing to run). | ||
| # | ||
| # continueOnError: ${{ not(parameters.failOnTestFailure) }} | ||
| # - When failOnTestFailure is false, continueOnError is true: a test failure marks the | ||
| # continueOnError: ${{ parameters.warnOnTestFailure }} | ||
| # - When warnOnTestFailure is true, continueOnError is true: a test failure marks the | ||
| # step and job as SucceededWithIssues (orange warning) rather than Failed. | ||
| # - When failOnTestFailure is true, continueOnError is false: a test failure fails the | ||
| # - When warnOnTestFailure is false, continueOnError is false: a test failure fails the | ||
| # job (red), though subsequent runtimes still run due to the condition above. | ||
| # | ||
| - ${{ each runtime in parameters.netTestRuntimes }}: | ||
| - task: DotNetCoreCLI@2 | ||
| displayName: Test [${{ runtime }}] | ||
| condition: and(succeededOrFailed(), eq(variables['buildSucceeded'], 'true')) | ||
| continueOnError: ${{ not(parameters.failOnTestFailure) }} | ||
| continueOnError: ${{ parameters.warnOnTestFailure }} | ||
| env: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a more robust way of injecting environment variables, rather than using the |
||
| STRESS_CONFIG_FILE: config.json | ||
| inputs: | ||
| command: run | ||
| projects: $(project) | ||
| arguments: $(runArguments) --no-build -f ${{ runtime }} -e STRESS_CONFIG_FILE=config.json -- $(testArguments) | ||
| arguments: ${{ variables.dotnetRunOpts }} -f ${{ runtime }} -- ${{ variables.stressTestOpts }} | ||
|
|
||
| # Run the stress tests for each .NET Framework runtime. | ||
| - ${{ each runtime in parameters.netFrameworkTestRuntimes }}: | ||
| - task: DotNetCoreCLI@2 | ||
| displayName: Test [${{ runtime }}] | ||
| condition: and(succeededOrFailed(), eq(variables['buildSucceeded'], 'true')) | ||
| continueOnError: ${{ not(parameters.failOnTestFailure) }} | ||
| continueOnError: ${{ parameters.warnOnTestFailure }} | ||
| env: | ||
| STRESS_CONFIG_FILE: config.json | ||
| inputs: | ||
| command: run | ||
| projects: $(project) | ||
| arguments: $(runArguments) --no-build -f ${{ runtime }} -e STRESS_CONFIG_FILE=config.json -- $(testArguments) | ||
| arguments: ${{ variables.dotnetRunOpts }} -f ${{ runtime }} -- ${{ variables.stressTestOpts }} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
| # ADO.Net project: | ||
| # Triggering pipeline: MDS Main CI (branch internal/main only) | ||
| # Pipeline name: sqlclient-stress | ||
| # Pipeline URL: TODO: add URL when pipeline is created | ||
| # Pipeline URL: https://dev.azure.com/SqlClientDrivers/ADO.Net/_build?definitionId=2284 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created a |
||
|
|
||
| # Set the pipeline run name to the day-of-year and the daily run counter. | ||
| name: $(DayOfYear)$(Rev:rr) | ||
|
|
@@ -73,10 +73,10 @@ parameters: | |
| type: boolean | ||
| default: false | ||
|
|
||
| # True to fail the pipeline when stress tests fail. When false (default), test failures produce | ||
| # warnings but do not fail the overall pipeline run. | ||
| - name: failOnTestFailure | ||
| displayName: Fail pipeline on test failure | ||
| # When true, test failures produce warnings (SucceededWithIssues) but do not fail the pipeline. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This flips the default behaviour - now the entire pipeline run will fail if any tests steps fail. |
||
| # When false (default), test failures fail the pipeline. | ||
| - name: warnOnTestFailure | ||
| displayName: Warn (not fail) on test failure | ||
| type: boolean | ||
| default: false | ||
|
|
||
|
|
@@ -105,5 +105,5 @@ stages: | |
| parameters: | ||
| buildConfiguration: ${{ parameters.buildConfiguration }} | ||
| debug: ${{ parameters.debug }} | ||
| failOnTestFailure: ${{ parameters.failOnTestFailure }} | ||
| warnOnTestFailure: ${{ parameters.warnOnTestFailure }} | ||
| dotnetVerbosity: ${{ parameters.dotnetVerbosity }} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,16 +30,15 @@ parameters: | |
| # True to enable debugging steps. | ||
| - name: debug | ||
| type: boolean | ||
| default: false | ||
|
|
||
| # True to fail the job when stress tests fail. When false, test failures produce warnings. | ||
| - name: failOnTestFailure | ||
| # When true, test failures produce warnings (SucceededWithIssues) but do not fail the job. | ||
| # When false, test failures fail the job. All test steps always run regardless of this setting. | ||
| - name: warnOnTestFailure | ||
| type: boolean | ||
|
|
||
| # The verbosity level for the dotnet CLI commands. | ||
| - name: dotnetVerbosity | ||
| type: string | ||
| default: normal | ||
| values: | ||
| - quiet | ||
| - minimal | ||
|
|
@@ -52,11 +51,11 @@ parameters: | |
| type: object | ||
| default: [net462] | ||
|
|
||
| # The list of .NET runtimes to test against. These must align with the TargetFrameworks defined | ||
| # in the stress test Directory.Build.props and the TFMs that SqlClient ships. | ||
| # The list of .NET runtimes to test against. These should include the TFMs that SqlClient ships | ||
| # as well as any upcoming runtimes being validated (e.g. net10.0 is tested but not yet shipped). | ||
| - name: netTestRuntimes | ||
| type: object | ||
| default: [net8.0, net9.0] | ||
| default: [net8.0, net9.0, net10.0] | ||
|
|
||
| stages: | ||
| - stage: stress_tests_stage | ||
|
|
@@ -69,6 +68,15 @@ stages: | |
| - name: saPassword | ||
| value: $[stageDependencies.secrets_stage.secrets_job.outputs['SaPassword.Value']] | ||
|
|
||
| # The 1ES pool name, determined automatically by ADO project: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a superior approach to choosing pool name. We should plan to update our other cross-project pipelines, and remove the variable group
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hardcoding 1ES pool or image names can be tedious to manage and the more pipelines we get - the name of images can get more widespread and lost in the files. I would suggest we use a structured variable file (single 1ES artifact) that contains only 1ES pool and image names so if you need to identify and list all the 1ES pools and images being used in this repo, you can gather them from 1 location, or if any one image needs to replaced by something new you don't need to hunt down all pipelines but can update it from one location.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Captured as a future work item: https://sqlclientdrivers.visualstudio.com/ADO.Net/_workitems/edit/45495 |
||
| # - ADO.Net -> ADO-1ES-Pool | ||
| # - public -> ADO-CI-1ES-Pool | ||
| - name: poolName | ||
| ${{ if eq(variables['System.TeamProject'], 'ADO.Net') }}: | ||
| value: ADO-1ES-Pool | ||
| ${{ else }}: | ||
| value: ADO-CI-1ES-Pool | ||
|
|
||
| jobs: | ||
|
|
||
| # ---------------------------------------------------------------------------------------------- | ||
|
|
@@ -80,10 +88,12 @@ stages: | |
| debug: ${{ parameters.debug }} | ||
| displayNamePrefix: Linux | ||
| dotnetVerbosity: ${{ parameters.dotnetVerbosity }} | ||
| failOnTestFailure: ${{ parameters.failOnTestFailure }} | ||
| warnOnTestFailure: ${{ parameters.warnOnTestFailure }} | ||
| jobNameSuffix: linux | ||
| # No .NET Framework runtimes on Linux. | ||
| netFrameworkTestRuntimes: [] | ||
| netTestRuntimes: ${{ parameters.netTestRuntimes }} | ||
| poolName: ADO-CI-1ES-Pool | ||
| poolName: ${{ variables.poolName }} | ||
| saPassword: $(saPassword) | ||
| sqlSetupStep: | ||
| template: /eng/pipelines/common/templates/steps/configure-sql-server-linux-step.yml@self | ||
|
|
@@ -100,12 +110,12 @@ stages: | |
| debug: ${{ parameters.debug }} | ||
| displayNamePrefix: Win | ||
| dotnetVerbosity: ${{ parameters.dotnetVerbosity }} | ||
| failOnTestFailure: ${{ parameters.failOnTestFailure }} | ||
| warnOnTestFailure: ${{ parameters.warnOnTestFailure }} | ||
| jobNameSuffix: windows | ||
| # Note that we include the .NET Framework runtimes for test runs on Windows. | ||
| netFrameworkTestRuntimes: ${{ parameters.netFrameworkTestRuntimes }} | ||
| netTestRuntimes: ${{ parameters.netTestRuntimes }} | ||
| poolName: ADO-CI-1ES-Pool | ||
| poolName: ${{ variables.poolName }} | ||
| saPassword: $(saPassword) | ||
| sqlSetupStep: | ||
| template: /eng/pipelines/common/templates/steps/configure-sql-server-win-step.yml@self | ||
|
|
@@ -124,8 +134,10 @@ stages: | |
| debug: ${{ parameters.debug }} | ||
| displayNamePrefix: macOS | ||
| dotnetVerbosity: ${{ parameters.dotnetVerbosity }} | ||
| failOnTestFailure: ${{ parameters.failOnTestFailure }} | ||
| warnOnTestFailure: ${{ parameters.warnOnTestFailure }} | ||
| jobNameSuffix: macos | ||
| # No .NET Framework runtimes on macOS. | ||
| netFrameworkTestRuntimes: [] | ||
| netTestRuntimes: ${{ parameters.netTestRuntimes }} | ||
| # We don't have any 1ES Hosted Pool images for macOS, so we use a generic one from Azure | ||
| # Pipelines. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
|
|
||
| using System.Collections.Generic; | ||
|
|
||
| namespace Monitoring | ||
| namespace Microsoft.Data.SqlClient.Test.Stress | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I gave all of these classes a proper namespace. |
||
| { | ||
| public interface IMonitorLoader | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This took many hours to figure out.