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

feat: add bench workflow for AWS #1330

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

tschneider-aneo
Copy link
Contributor

@tschneider-aneo tschneider-aneo commented Oct 31, 2024

Motivation

Related to ArmoniK benchmarking automation project, Bench application is the most suited one for testing the ArmoniK's sheer orchestration performance

Description

Use Bench action from Armonik.Action.Deploy to benchmark ArmoniK with Bench application on localhost on each commit on main, and on AWS at each release.

Testing

This workflow was run on every push on the PR's branch.

Impact

This PR implements a milestone for ArmoniK benchmarking capacity, as AK would be benchmarked frequently.

Additional Information

Needs this PR to be merged before being merge

Sometimes infrastructure destruction fails on AWS due to a deadlock between a security group and a subnet that happens unpredictably.

When this happens, the destruction must be taken over manually by destroying the security group and the network interface associated with it, and finished with make recipes destroy and bootstrap-destroy with the prefix used by the GitHub workflow (currently benchmark).

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • I have thoroughly tested my modifications and added tests when necessary.
  • Tests pass locally and in the CI.
  • I have assessed the performance impact of my modifications.

ttlSecondsAfterFinished: 0
template:
spec:
containers:

Check warning

Code scanning / SonarCloud

Service account permissions should be restricted Medium

Bind this resource's automounted service account to RBAC or disable automounting. See more on SonarQube Cloud
template:
spec:
containers:
- name: bench-session

Check warning

Code scanning / SonarCloud

Memory limits should be enforced Medium

Specify a memory limit for this container. See more on SonarQube Cloud
template:
spec:
containers:
- name: bench-session

Check warning

Code scanning / SonarCloud

Storage limits should be enforced Medium

Specify a storage limit for this container. See more on SonarQube Cloud
@tschneider-aneo tschneider-aneo force-pushed the ts/add-bench-aws branch 4 times, most recently from ee84b2d to e5e9472 Compare November 6, 2024 14:25
@tschneider-aneo tschneider-aneo force-pushed the ts/add-bench-aws branch 2 times, most recently from 0f1faab to 701ae60 Compare November 20, 2024 16:29
Copy link

on:
push:
branches:
- "ts/add-bench-aws"
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with 'main'.

Comment on lines +23 to +27
if [ "$TRIGGER" = 'push' ]; then
echo '{"include":[{"type": "localhost", "ntasks":100, "polling-limit": 300}, {"type": "aws", "ntasks":1000, "polling-limit": 600}]}' > matrix.json
elif [ "$TRIGGER" = 'release' ]; then
echo '{"include":[{"type": "localhost", "ntasks":100, "polling-limit": 300}]}' > matrix.json
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Swap the two condition bodies. and set the number of tasks to a relevant value.

Comment on lines +30 to +46
# test-matrix:
# name: Test matrix
# runs-on: ubuntu-latest
# needs: define-matrix
# strategy:
# fail-fast: false
# matrix: ${{ fromJson(needs.define-matrix.outputs.matrix) }}
# steps:
# - id: test
# name: Test matrix
# env:
# TYPE: ${{ matrix.type }}
# NTASKS: ${{ matrix.ntasks }}
# POLLING_LIMIT: ${{ matrix.polling-limit }}
# run: |
# set -ex
# echo "Type: $TYPE, Number of tasks: $NTASKS, Polling Limit: $POLLING_LIMIT"
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this commented job.

AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
AWS_EC2_METADATA_DISABLED: true
run: |
aws s3 cp "$BENCH_RESULTS_PATH" "s3://test-armonik-bench-storage/benchclient_benchmark_${EVENT_NAME}_${TYPE}_${GHRUNID}.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you should use an environment variable here instead of the putting directly the bucket uri

Comment on lines +141 to +148
# WARNING FOR ARMONIK CORE TEAM

# Sometimes infrastructure destruction fails on AWS due to a deadlock
# between a security group and a subnet that happens unpredictably.

# When this happens, the destruction must be taken over manually
# by destroying the security group and the network interface associated with it,
# and finished with `make` recipes `destroy` and `bootstrap-destroy` with the prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure it's very important. But is it the right place?

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.

2 participants