-
Notifications
You must be signed in to change notification settings - Fork 843
[ci] Migrate reexec tasks to a script to simplify maintenance #4761
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
Conversation
| description: 'The password for the Prometheus instance.' | ||
| required: true | ||
| default: '' | ||
| workspace: |
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.
I noticed this was unused.
| role-to-assume: ${{ inputs.aws-role }} | ||
| aws-region: ${{ inputs.aws-region }} | ||
| role-duration-seconds: ${{ inputs.aws-role-duration-seconds }} | ||
| - name: Validate inputs |
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.
Validation moved to the script so its defined once for use locally and in CI and it can be subject to shellcheck linting.
| run: echo "BENCHMARK_OUTPUT_FILE=${GITHUB_WORKSPACE}/benchmark-output.json" >> "$GITHUB_ENV" | ||
| - name: Run C-Chain Re-execution Benchmark | ||
| shell: nix develop --impure --command bash -x {0} | ||
| run: | |
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.
Moved this configuration to the script
| - name: Set task env | ||
| - name: Set benchmark output file | ||
| shell: bash | ||
| run: | |
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.
EXECUTION_DATA_DIR is now set in the script
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.
Pull request overview
This PR consolidates C-Chain re-execution logic from multiple Taskfile tasks into a single unified shell script to simplify maintenance and usage.
Key Changes:
- Introduces
reexecute_cchain.shscript with preset test configurations and custom test support - Removes complex Taskfile orchestration in favor of direct script execution
- Simplifies GitHub Actions workflows by replacing task-based invocation with direct script calls
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/reexecute_cchain.sh | New unified script that handles test presets, S3 data import, execution, and optional post-state pushing |
| Taskfile.yml | Removes reexecution-related tasks and adds simplified test-c-chain-reexecution entry point; simplifies export-cchain-block-range task |
| .github/workflows/c-chain-reexecution-benchmark-gh-native.yml | Updates workflow to use test parameter instead of task parameter |
| .github/workflows/c-chain-reexecution-benchmark-gh-native.json | Updates matrix configuration to use test instead of task |
| .github/workflows/c-chain-reexecution-benchmark-container.yml | Updates workflow to use test parameter instead of task parameter |
| .github/workflows/c-chain-reexecution-benchmark-container.json | Updates matrix configuration to use test instead of task |
| .github/actions/c-chain-reexecution-benchmark/action.yml | Simplifies action inputs and logic to call script directly with environment variable overrides |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/reexecute_cchain.sh
Outdated
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| S3_BOOTSTRAP_BUCKET="${S3_BOOTSTRAP_BUCKET:-s3://avalanchego-bootstrap-testing}" |
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.
If I'm not mistaken, we switched to turning each of these sets of parameters into tasks to comply with the notion that tasks should be fully encompassed within the task instead of passing in a bunch of parameters.
I'd argue that a wide range of different sets of options like this is not as common to place within a bash script.
Would you still push to keep each of these baked into one configuration options within this script if we are going to move it out of the taskfile?
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.
I intended for tasks to be both discoverable and to serve as entrypoints into CI so that reproducing CI was as simple as possible. The goal was not to implement anything non-trivial directly in the tasks, though, and I think that the reexec task logic is better maintained outside of the taskfile as I've proposed in this PR.
I think creating a task per reexec test configuration makes about as much sense as creating a task per e2e test. It's still possible to target the execution of an e2e test but it's the e2e test suite that's responsible for enabling that rather than something we need to do in the taskfile. I think it similarly makes sense for there to be an entrypoint in the taskfile for executing reexec tests, and that the entrypoint be responsible for defining both common test configurations and enabling ad-hoc configuration.
|
Could you please validate the change by stepping through the README and updating it as needed? |
Done |
91851f9 to
0df3f05
Compare
|
Not ideal that we have a README with complex instructions that have to be manually verified when things change. Mental note: prefer an executable script in future with inline comments instead of a static README. |
0df3f05 to
809baac
Compare
…#1580) ## Why this should be merged Patch is related to #1578 The `aws-launch.sh` script is currently not collecting metrics because it uses the old avalanchego task interface. The new version of avalanchego's [Taskfile.yml](https://github.com/ava-labs/avalanchego/blob/10f374649645f3093023633d60825875c5645589/Taskfile.yml#L204) changed how metrics configuration is passed: - Old: `METRICS_ENABLED` passed as a task variable - New: `METRICS_SERVER_ENABLED` and / or `METRICS_COLLECTOR_ENABLED` passed as environment variables This mismatch causes metrics to silently not be configured when running benchmarks on EC2 instances. Related PRs: - [ava-labs/avalanchego#4761](ava-labs/avalanchego#4761) - [Move from task vars to env](https://github.com/ava-labs/avalanchego/blob/0eebefb80b34849c44c7657a431b57b50e194300/scripts/benchmark_cchain_range.sh#L15) - [ava-labs/avalanchego#4443](ava-labs/avalanchego#4443) ## How this works Changed the bootstrap command from: ```bash task reexecute-cchain-range ... METRICS_ENABLED=false ``` To: ```bash METRICS_SERVER_ENABLED=true task reexecute-cchain-range ... ``` This aligns with how avalanchego's `benchmark_cchain_range.sh` now reads these settings from the environment rather than from task variables. ## How this was tested ### 1. AvalancheGo environment variable behavior Navigate to or clone AvalancheGo: ```bash git clone [email protected]:ava-labs/avalanchego.git cd avalanchego ``` **Default (metrics disabled):** ```bash ./scripts/run_task.sh reexecute-cchain-range \ BLOCK_DIR=/tmp/test \ CURRENT_STATE_DIR=/tmp/test \ START_BLOCK=101 \ END_BLOCK=102 2>&1 | grep -o '"metrics-server-enabled": "[^"]*"' ``` Expected: `"metrics-server-enabled": "false"` **With metrics enabled:** ```bash METRICS_SERVER_ENABLED=true ./scripts/run_task.sh reexecute-cchain-range \ BLOCK_DIR=/tmp/test \ CURRENT_STATE_DIR=/tmp/test \ START_BLOCK=101 \ END_BLOCK=102 2>&1 | grep -o '"metrics-server-enabled": "[^"]*"' ``` Expected: `"metrics-server-enabled": "true"` ### 2. Firewood `aws-launch.sh` flag behavior **Default (metrics enabled):** ```bash ./benchmark/bootstrap/aws-launch.sh --dry-run 2>&1 | grep "Metrics Server:" ``` Expected: `Metrics Server: true` **Disable metrics:** ```bash ./benchmark/bootstrap/aws-launch.sh --dry-run --metrics-server false 2>&1 | grep "Metrics Server:" ``` Expected: `Metrics Server: false` **Case insensitivity:** ```bash ./benchmark/bootstrap/aws-launch.sh --dry-run --metrics-server TRUE 2>&1 | grep "Metrics Server:" ``` Expected: `Metrics Server: true` (normalized to lowercase) **Invalid value (fail fast):** ```bash ./benchmark/bootstrap/aws-launch.sh --metrics-server tru 2>&1 ``` Expected: ``` Error: Invalid --metrics-server value 'tru' Valid values: true, false ```
0eebefb to
b241482
Compare
|
Rebased |
Doing anything non-trivial in tasks is not recommended. This change moves the reexec logic to a new script for ease of maintenance and to simplify usage.
b241482 to
0d573ef
Compare
RodrigoVillar
left a comment
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.
LGTM
## Why this should be merged With ava-labs/avalanchego#4761, invocation of the reexecution test underwent a major refactor. Running the current bootstrap script against the latest version of AvalancheGo results in an error. ## How this works Updates bootstrapping script to use the new invocation of the reexecution test. ## How this was tested Ran `./aws-launch.sh --firewood-branch main --avalanchego-branch master --libevm-branch b8e76562a300 --nblocks 1m` and observed a successful reexecution test.
Why this should be merged
Doing anything non-trivial in tasks is not recommended. This change moves the reexec logic to a new script for ease of maintenance and to simplify usage.
How this was tested
Need to be documented in RELEASES.md?
N/A