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

[PH] Create workflow for manually dispatching performance harness runs #1378

Merged
merged 64 commits into from
Aug 2, 2023

Conversation

oschwaldp-oci
Copy link
Contributor

@oschwaldp-oci oschwaldp-oci commented Jul 7, 2023

Initial attempt of configurable ph run in cicd.

platforms.yaml

Split out Discover Platforms, Build Platforms, and Platform Matrix out into one reusable workflow. Platform Matrix is a new job that dynamically creates the matrix: platform: array from the discovered platforms or can be overridden by the input override-build-matrix. This reduces the maintenance overhead of having to update the matrix: platform arrays in the workflows anytime a platform is added or dropped.

build_base.yaml

Removes steps now contained in platforms.yaml workflow but takes as input p, the discovered platforms, and platform-matrix. Typically will be the output of the platforms.yaml workflow.

build.yaml

Updated to make use of platforms.yaml and build_base.yaml workflows.

ph_backward_compatibility.yaml

Updated to make use of platforms.yaml and build_base.yaml workflows.

performance_harness_run.yaml

A manual workflow to configure and run the Performance Harness in CICD. This workflow will attempt to reuse a build for the current head commit of the branch selected to run on, if it exists, otherwise it will build it from scratch. It has the following inputs:

  • Use workflow from
    • Select branch to run on
  • Override build platform
    • Drop down selection allows ubuntu20 or ubuntu22 as the build platform
  • Override perf harness params
    • Provide any special command line arguments to pass to performance_test.py
    • Default (if input field left blank): testBpOpMode
    • e.g.
      • testBpOpMode --max-tps-to-test 50 --test-iteration-min-step 10 --test-iteration-duration-sec 10 --final-iterations-duration-sec 10 overrideBasicTestConfig -v --tps-limit-per-generator 25 --chain-state-db-size-mb 200
      • testBpOpMode --test-iteration-duration-sec 600 --final-iterations-duration-sec 600

@oschwaldp-oci oschwaldp-oci self-assigned this Jul 7, 2023
@oschwaldp-oci oschwaldp-oci added the OCI Work exclusive to OCI team label Jul 7, 2023
@oschwaldp-oci oschwaldp-oci marked this pull request as ready for review July 10, 2023 16:00
uses: AntelopeIO/upload-artifact-large-chunks-action@v1
with:
name: ${{github.event.inputs.platform-choice}}-build
path: build.tar.zst
Copy link
Member

Choose a reason for hiding this comment

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

Similar comments as #1356 (comment): we shouldn't be copy pasting all of this, we need to come up with a better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spoonincode - I have refactored the build to be a reusable workflow in PR#1356. Hoping to have that PR go into main first, then I will pull those changes into this branch and refactor the workflow to use build-base as well.

- name: Run Performance Test
run: |
# https://github.com/actions/runner/issues/2033 -- need this because of full version label test looking at git revs
chown -R $(id -u):$(id -g) $PWD
Copy link
Member

Choose a reason for hiding this comment

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

Is the 2033 workaround really needed in this case? Unless there is a git command being run as part of the test, probably not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed: 6b4f745

run: |
echo "Workflow Stub"
# https://github.com/actions/runner/issues/2033
chown -R $(id -u):$(id -g) $PWD
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above -- is 2033 workaround needed here? Seems unlikely since there is just a tar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed: 6b4f745

runs-on: ubuntu-latest
container:
image: ${{fromJSON(needs.d.outputs.p)[github.event.inputs.platform-choice].image}}
options: --security-opt seccomp=unconfined
Copy link
Member

Choose a reason for hiding this comment

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

Does performance harness make use of unshare? (does it come along indirectly via TestHarness or something?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed: 6b4f745

needs: [v, d, Build]
if: always() && needs.Build.result == 'success'
strategy:
fail-fast: false
Copy link
Member

Choose a reason for hiding this comment

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

Without being a matrix, this is a noop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed: 6b4f745

needs: [v, d, Build]
if: always() && needs.Build.result == 'success'
strategy:
fail-fast: false
runs-on: ubuntu-latest
Copy link
Member

Choose a reason for hiding this comment

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

This means it runs on a github owned runner, is that what you wanted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 (enf-x86-beefy-long)? I know that most of the enf runners have time limits on them which could be problematic for performance runs depending on how a user configures the run.

runs-on: ["self-hosted", "enf-x86-beefy"]
container: ${{fromJSON(needs.d.outputs.p)[matrix.platform].image}}
steps:
- uses: actions/checkout@v3
with:
ref: '${{inputs.leap-ref}}'
Copy link
Member

Choose a reason for hiding this comment

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

This seems very problematic since on first look I don't believe the workflow+platform used to build leap will match up with the version of leap checked out. You will need to do more effort in at least 2 places to get this to work correctly, I would recommend against the added complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I really had this working correctly, as you observed. I have removed this feature.

This reverts commit 47ddeb6.
Use platforms in conjunction with build-base such that platforms can be run separately from build-base.  Together they accomplish the same as build-base used to, but now can separate concerns between discovering and building platform knowledge from doing the actual software build.

Update previous usages of build-base to now use platforms and build-base together.
These reusable workflows are intended to be called from other workflows, not necessarily dispatched individually in their own capacity.
name: Run Platforms Workflow
uses: ./.github/workflows/platforms.yaml
with:
override-build-matrix: ${{github.event.inputs.platform-choice}}
Copy link
Member

Choose a reason for hiding this comment

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

There doesn't look to be a platform-choice input as part of this workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I had removed that. Somehow lost in transition. It is unnecessary in the build.yaml workflow as that workflow should always be expecting to use the full input from platforms.json to determine the build platforms to support. Removed here: bf95111

@@ -2,6 +2,16 @@ name: "Performance Harness Run"

on:
workflow_dispatch:
inputs:
platform-choice:
description: 'Override build platform'
Copy link
Member

Choose a reason for hiding this comment

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

I guess this isn't really overriding the platform but rather picking which platform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Here it is a platform selection, however down in platforms.yaml it serves to override the auto-generated platform matrix (from platforms.json). I'll update the verbiage here. Addressed: 8c47cce

- name: Setup Input Params
id: overrides
env:
GH_TOKEN: ${{secrets.GITHUB_TOKEN}}
Copy link
Member

Choose a reason for hiding this comment

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

this used any where?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed: 8c47cce

id: check-matrix
run: |
echo '${{ steps.pm-results.outputs.platform-matrix }}'
echo '${{ steps.parse-pm.outputs.result }}'
Copy link
Member

Choose a reason for hiding this comment

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

quite a few echos in this workflow that look kind of like debugging, did you mean to leave them in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Thought I had caught all of these and removed them. Thanks.

Addressed: cd872b3

target: ${{github.sha}}
artifact-name: ${{github.event.inputs.platform-choice}}-build
fail-on-missing-target: false
token: ${{github.token}}
Copy link
Member

Choose a reason for hiding this comment

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

with v3 you don't need this line, if you want to save a line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: d50381b

tests:
name: Tests
needs: [v, platforms, reuse-build, build-base]
if: always() && needs.platforms.result == 'success' && (needs.build-base.result == 'success' || needs.reuse-build.result == 'success')
Copy link
Member

Choose a reason for hiding this comment

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

we really need to find a way to get #283 in good shape to avoid these tricky always() chains

@oschwaldp-oci oschwaldp-oci merged commit 1e632c9 into main Aug 2, 2023
@oschwaldp-oci oschwaldp-oci deleted the ph-manual-workflow branch August 2, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants