[PH] Create workflow for manually dispatching performance harness runs #1378
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,19 @@ name: "Performance Harness Run" | |
|
|
||
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| platform-choice: | ||
| description: 'Override build platform' | ||
|
There was a problem hiding this comment. I guess this isn't really overriding the platform but rather picking which platform There was a problem hiding this comment. Correct. Here it is a platform selection, however down in |
||
| type: choice | ||
| options: | ||
| - ubuntu20 | ||
| - ubuntu22 | ||
| override-test-params: | ||
| description: 'Override perf harness params' | ||
| type: string | ||
| override-leap-ref: | ||
| description: 'Override leap ref' | ||
| type: string | ||
|
|
||
| permissions: | ||
| packages: read | ||
|
|
@@ -12,10 +25,93 @@ defaults: | |
| shell: bash | ||
|
|
||
| jobs: | ||
|
|
||
| v: | ||
| name: Discover Inputs | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| test-params: ${{steps.overrides.outputs.test-params}} | ||
| leap-ref: ${{steps.overrides.outputs.leap-ref}} | ||
| steps: | ||
| - name: Setup Input Params | ||
| id: overrides | ||
| env: | ||
| GH_TOKEN: ${{secrets.GITHUB_TOKEN}} | ||
|
There was a problem hiding this comment. Addressed: 8c47cce |
||
| run: | | ||
| echo test-params=testBpOpMode >> $GITHUB_OUTPUT | ||
| echo leap-ref='${{github.sha}}' >> $GITHUB_OUTPUT | ||
|
|
||
| if [[ "${{inputs.override-test-params}}" != "" ]]; then | ||
| echo test-params=${{inputs.override-test-params}} >> $GITHUB_OUTPUT | ||
| fi | ||
| if [[ "${{inputs.override-leap-ref}}" != "" ]]; then | ||
| echo leap-ref=${{inputs.override-leap-ref}} >> $GITHUB_OUTPUT | ||
| fi | ||
|
|
||
| reuse-build: | ||
| name: Reuse leap build | ||
| needs: [v] | ||
| if: | | ||
| always() && | ||
| needs.v.result == 'success' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Download builddir | ||
| uses: AntelopeIO/asset-artifact-download-action@v2 | ||
| with: | ||
| owner: AntelopeIO | ||
| repo: leap | ||
| file: ${{github.event.inputs.platform-choice}}-build | ||
| target: '${{needs.v.outputs.leap-ref}}' | ||
| artifact-name: ${{github.event.inputs.platform-choice}}-build | ||
| token: ${{github.token}} | ||
|
There was a problem hiding this comment. with |
||
|
|
||
| - name: Upload builddir | ||
| uses: AntelopeIO/upload-artifact-large-chunks-action@v1 | ||
| with: | ||
| name: ${{github.event.inputs.platform-choice}}-build | ||
| path: build.tar.zst | ||
|
There was a problem hiding this comment. Similar comments as #1356 (comment): we shouldn't be copy pasting all of this, we need to come up with a better approach. There was a problem hiding this comment. @spoonincode - I have refactored the build to be a reusable workflow in PR#1356. Hoping to have that PR go into |
||
|
|
||
| build-base: | ||
| name: Run Build Workflow | ||
| needs: [reuse-build] | ||
| if: always() && needs.reuse-build.result != 'success' | ||
| uses: ./.github/workflows/build_base.yaml | ||
| with: | ||
| override-build-matrix: ${{github.event.inputs.platform-choice}} | ||
| leap-ref: ${{needs.v.outputs.leap-ref}} | ||
| permissions: | ||
| packages: write | ||
| contents: read | ||
|
|
||
| tests: | ||
| name: Tests | ||
| needs: [v, reuse-build, build-base] | ||
| if: always() && (needs.build-base.result == 'success' || needs.reuse-build.result == 'success') | ||
| runs-on: ubuntu-latest | ||
|
There was a problem hiding this comment. This means it runs on a github owned runner, is that what you wanted? There was a problem hiding this comment. @spoonincode - Correct. I was using the github owned runner for initial testing and verification of the workflow. Now that this is closer to complete, do you have a suggestion for a runner to use ( |
||
| container: | ||
| image: ${{fromJSON(needs.build-base.outputs.p)[github.event.inputs.platform-choice].image}} | ||
| steps: | ||
| - name: Download builddir | ||
| uses: actions/download-artifact@v3 | ||
| with: | ||
| name: ${{github.event.inputs.platform-choice}}-build | ||
| - name: Run Performance Test | ||
| run: | | ||
| zstdcat build.tar.zst | tar x | ||
| cd build | ||
| ./tests/performance_tests/performance_test.py ${{needs.v.outputs.test-params}} | ||
| - name: Prepare results | ||
| id: prep-results | ||
| run: | | ||
| tar -pc build/performance_test | zstd --long -T0 -9 > performance_test_logs.tar.zst | ||
| - name: Upload results | ||
| uses: AntelopeIO/upload-artifact-large-chunks-action@v1 | ||
| with: | ||
| name: performance-test-results | ||
| path: performance_test_logs.tar.zst | ||
| - name: Upload report | ||
| uses: actions/upload-artifact@v3 | ||
| with: | ||
| name: performance-test-report | ||
| path: ./build/performance_test/**/report.json | ||
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.
What's driving the need for adding this? You only want to build leap for a single platform when doing the performance harness test?
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.
Correct.
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.
Not sure it's worth the added complexity in
build_base.yamlto accommodate that. Are you doing it just to save a dollar? I'd rather take the cost and not have 30+ more lines of yaml here to maintain forever, especially because PH runs aren't regularly done anyways.If you want to save cost I'd suggest pushing off the complexity in to the
performance_harness_run.yamlby doing something like trying to fetch an existing built leap.deb & leap-dev.deb, and then only falling back to building leap if they don't exist.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.
While being able to override the build matrix was in support of the
performance_harness_run.yamlworkflow, I also saw the generation of the platform matrix from theplatforms.jsoninput as a net gain as it removed the maintenance effort of going back and searching out everywhere that thematrix: platform: [ ubuntuXX, ubuntuYY, ...]was used any time a new platform is added or removed. Now all that is accomplished in a single step by updatingplatforms.json, which has become the single source of truth.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.
yeah, not having to copy pasta around the
[ubuntuxx..]is a good improvement. I'm a little worried we're going to have to refactor much of this soon again though because while right now there is a 1:1 between build platforms and test platforms that won't always be the case. That is, right nowubuntu22means both building and testing onubuntu22but that may not always be the case. For example, for our reproducible builds we may have a separatereproplatform that is used for builds, but then tested on bothubuntu20andubuntu22. Or ARM builds may have acrossarmplatform for builds that then runs the tests onubuntu20armor something. I can't quite wrap my head around the semantics of how that is going to work just yet (besides adding more complexity 😞) so what is in this PR is good for now on that front.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.
That's a good point. There is definitely a complexity addition that will come when that occurs. It could be that the
Platform Matrixjob provides a nice location to do the platform setup though and could be expanded to help with that? Might be that it breaks out into its own workflow. I agree that it is hard to say what will work best until some of those decisions are made. I'll consider this resolved for now per your comment.