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

Convert consolidated pipeline to a YAML based pipeline and enable nightly builds #4292

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

aishwaryabh
Copy link
Contributor

@aishwaryabh aishwaryabh commented Mar 10, 2025

Issue describing the changes in this PR

resolves #4270 #4271
This PR converts the consolidated pipeline which was initially based off of azure classic pipelines UX to a yaml based pipeline.

I have updated the ArtifactAssembler within core tools to be able to process each artifact type one at a time. For example, if I were to pass in Azure.Functions.Cli.win-x64 when running the artifact assembler, the artifact assembler would only assemble artifacts for that win-x64. This enables us to use parallelization when running the consolidated pipeline, which significantly improves the run time overall.

The consolidated pipeline also publishes the artifacts to the pipeline directly, rather than publishing to a storage account. Please look at this run for an example of what this would look like.

This pipeline also runs significantly faster than the azure classic based pipeline, as this pipeline takes on average 35-40 min, whereas previous runs would easily take over an hour. This runtime will increase even more once the E2E tests are updated to run in a more efficient and parallel manner outside of the consolidated pipeline.

Finally, nightly builds are enabled for the consolidated artifact pipeline to run every night, after the in-proc and main artifacts have been created from their nightly builds. This PR also adds a tag called nightly-build to those builds for the in-proc and main branch to distinguish scheduled builds from manually triggered ones. That way the consolidated pipeline knows which builds to pick for the nightly builds.

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • 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)

@aishwaryabh aishwaryabh requested a review from a team as a code owner March 10, 2025 19:59
artifact: drop
displayName: "Download out-of-proc artifact"

- powershell: |
Copy link
Member

Choose a reason for hiding this comment

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

Can we convert all of of the inline pwsh scropts into .ps1 script files and use the powershell task to run the script?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also a side note: use pwsh task instead of powershell for inline scripts. The former is the newer cross-plat pscore. The latter is the old windows-only powershell.


- job: AssembleArtifacts
strategy:
matrix:
Copy link
Member

Choose a reason for hiding this comment

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

Could this be simplied with a template where you pass in an array of the platforms/arch you want to build?

arguments: '-StagingDirectory "$(Pipeline.Workspace)\staging\coretools-cli"'
workingDirectory: '$(Build.SourcesDirectory)/src/Azure.Functions.ArtifactAssembler'
condition: and(succeeded(), or(eq(variables['artifactName'], 'Azure.Functions.Cli.win-x64'), eq(variables['artifactName'], 'Azure.Functions.Cli.win-x86')))
- task: DotNetCoreCLI@2
Copy link
Member

Choose a reason for hiding this comment

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

nit: blank lines between each task to improve readability

stages:
- stage: ConsolidateArticacts
displayName: "Assemble Artifacts"
jobs:
Copy link
Member

Choose a reason for hiding this comment

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

can each job be its own file

Copy link
Member

Choose a reason for hiding this comment

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

Consider breaking things out into job files and step files similar to how we do in the host:

https://github.com/Azure/azure-functions-host/tree/dev/eng/ci/templates

steps:
- checkout: none

- download: core-tools-default
Copy link
Contributor

Choose a reason for hiding this comment

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

Use templateContext.inputs instead

artifact: drop
displayName: "Download out-of-proc artifact"

- powershell: |
Copy link
Contributor

Choose a reason for hiding this comment

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

Also a side note: use pwsh task instead of powershell for inline scripts. The former is the newer cross-plat pscore. The latter is the old windows-only powershell.

arguments: '-ArtifactsPath "$(Pipeline.Workspace)\staging\coretools-cli"'
workingDirectory: '$(Build.SourcesDirectory)/src/Azure.Functions.ArtifactAssembler'

- task: EsrpCodeSigning@5
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the sign-files.yml@eng template:

https://github.com/Azure/azure-functions-host/blob/dev/eng/ci/templates/official/jobs/build-artifacts-windows.yml#L91

This gives us a single place to manage and update our ESRP connection.

filePath: '$(Build.SourcesDirectory)/src/Azure.Functions.ArtifactAssembler/PipelineHelpers/generateMetadataFile.ps1'
arguments: '-StagingDirectory "$(Pipeline.Workspace)\staging"'
workingDirectory: '$(Pipeline.Workspace)'
condition: and(succeeded(), eq(variables['artifactName'], 'Azure.Functions.Cli.min.win-x64'))
Copy link
Contributor

Choose a reason for hiding this comment

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

When possible, use templating syntax for conditions:

- ${{ if eq(variables.artifactName, 'Azure.Functions.Cli.min.win-x64') }}:
  - task: PowerShell@2

The benefits with this are:

  1. It is calculated at templating time, so if the if-statement does not pass, this task doesn't show up at all in the pipeline. (the condition: syntax it shows up, but as a grey skipped step).
  2. No need to worry about and(succeeded()) - that will be implied.

inputs:
command: run
projects: '$(Build.SourcesDirectory)/src/Azure.Functions.ArtifactAssembler/Azure.Functions.ArtifactAssembler.csproj'
arguments: 'zip -c release'
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you had to supply app arguments like so for dotnet run:

Suggested change
arguments: 'zip -c release'
arguments: '-c release -- zip'

The -- delimits arguments for run vs arguments for the application, as documented here https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-run


- pwsh: |
# Guardian files in TEMP add up and can cause the AntiMalware task to timeout.
Get-ChildItem -Path $env:TEMP -Filter 'MpCmdRun.*' -Recurse -ErrorAction SilentlyContinue | Remove-Item -Recurse -Force
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these artifacts you have generated in this pipeline? Or from SDL? We shouldn't be deleting SDL files.

inputs:
command: run
projects: '$(Build.SourcesDirectory)/src/Azure.Functions.ArtifactAssembler/Azure.Functions.ArtifactAssembler.csproj'
arguments: 'zip -c release'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can gain slight improvements by building ArtifactAssembler once, and then using dotnet run --no-build -c release after that.

@@ -81,6 +81,17 @@ jobs:
- pwsh: |
.\check-vulnerabilities.ps1
displayName: "Check for security vulnerabilities"
- pwsh: |
Copy link
Contributor

Choose a reason for hiding this comment

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

Can also condition this step:

- ${{ if eq(variables['Build.Reason'], 'Schedule') }}:
  - pwsh: |
      Write-Host "##vso[build.addbuildtag]nightly-build"
    displayName: Tag nightly

}
else
{
await Task.Run(() => FileUtilities.CopyDirectory(artifactZipPath, destinationDirectory));
Copy link
Contributor

Choose a reason for hiding this comment

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

await Task.Run here doesn't offer any benefit. It just adds extra thread pool scheduling. The only time to use Task.Run is if you really don't want that work on the current thread (and want to delegate it to the thread pool). An example is to do a fire & forget as part of an HTTP request. Or in old AspNet (not AspNetCore), HTTP requests had a special sync context and it was crucial to offload work to the threadpool to free up that sync context.

But given this is just a cli tool, there is no real multi-threaded-ness going on here and no restrictions on the current thread.

Suggested change
await Task.Run(() => FileUtilities.CopyDirectory(artifactZipPath, destinationDirectory));
FileUtilities.CopyDirectory(artifactZipPath, destinationDirectory);

@@ -225,7 +274,7 @@ private async Task CreateVisualStudioCoreToolsAsync()
Directory.CreateDirectory(consolidatedArtifactDirPath);

// Copy in-proc8 files and delete directory after
await Task.Run(() => FileUtilities.CopyDirectory(inProcArtifactDirPath, createDirectory ? Path.Combine(consolidatedArtifactDirPath, Constants.InProc8DirectoryName): consolidatedArtifactDirPath));
await Task.Run(() => FileUtilities.CopyDirectory(inProcArtifactDirPath, createDirectory ? Path.Combine(consolidatedArtifactDirPath, Constants.InProc8DirectoryName) : consolidatedArtifactDirPath));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await Task.Run(() => FileUtilities.CopyDirectory(inProcArtifactDirPath, createDirectory ? Path.Combine(consolidatedArtifactDirPath, Constants.InProc8DirectoryName) : consolidatedArtifactDirPath));
FileUtilities.CopyDirectory(inProcArtifactDirPath, createDirectory ? Path.Combine(consolidatedArtifactDirPath, Constants.InProc8DirectoryName) : consolidatedArtifactDirPath);

- template: /eng/ci/templates/official/jobs/assemble-artifacts.yml@self
parameters:
arch: 'win-x64'
displayName: 'winx64'
Copy link
Member

@liliankasem liliankasem Mar 12, 2025

Choose a reason for hiding this comment

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

Is display name needed? You can use the arch value as the display name for the job, no?

Copy link
Contributor Author

@aishwaryabh aishwaryabh Mar 12, 2025

Choose a reason for hiding this comment

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

that's what I thought but display name has to strictly be alpha-numeric (so no - or . allowed which is in all of the arch names)

arch: 'win-x64'
displayName: 'winx64'

- template: /eng/ci/templates/official/jobs/assemble-artifacts.yml@self
Copy link
Member

Choose a reason for hiding this comment

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

This is looking better! Now we can go a step further and create a variable:

variables:
  archs:
    - min.win-x64
    - osx-x64
    - linux-x64

Then loop for each arch

    jobs:
      - ${{ each arch in variables.archs }}:
          - template: /eng/ci/templates/official/jobs/assemble-artifacts.yml@self
            parameters:
              arch: ${{ arch }}

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.

Convert consolidated pipeline to YAML based pipeline
3 participants