From 48906cc13a0ab51d92cd0555dc1909c558688957 Mon Sep 17 00:00:00 2001 From: Arjun Yogidas Date: Mon, 14 Aug 2023 22:50:03 +0000 Subject: [PATCH 1/8] Add some print statements to test Added print statements to test the benchmark results Signed-off-by: Arjun Raja Yogidas --- benchmark/framework/framework.go | 1 + benchmark/performanceTest/main.go | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmark/framework/framework.go b/benchmark/framework/framework.go index 28c3a7dd2..ae10bd422 100644 --- a/benchmark/framework/framework.go +++ b/benchmark/framework/framework.go @@ -94,6 +94,7 @@ func (frame *BenchmarkFramework) Run(ctx context.Context) { } } + print("should We add timeout here for testing?") json, err := json.MarshalIndent(frame, "", " ") if err != nil { fmt.Printf("JSON Marshalling Error: %v\n", err) diff --git a/benchmark/performanceTest/main.go b/benchmark/performanceTest/main.go index 83afc1011..1b54d444f 100644 --- a/benchmark/performanceTest/main.go +++ b/benchmark/performanceTest/main.go @@ -47,7 +47,6 @@ func main() { flag.BoolVar(&showCom, "show-commit", false, "tag the commit hash to the benchmark results") flag.IntVar(&numberOfTests, "count", 5, "Describes the number of runs a benchmarker should run. Default: 5") flag.StringVar(&configCsv, "f", "default", "Path to a csv file describing image details in this order ['Name','Image ref', 'Ready line', 'manifest ref'].") - flag.Parse() if showCom { From 93c90509b00db92a021352f879196ee6cf99016c Mon Sep 17 00:00:00 2001 From: Arjun Yogidas Date: Mon, 14 Aug 2023 23:42:41 +0000 Subject: [PATCH 2/8] Add more log Commit test 4 Signed-off-by: Arjun Raja Yogidas --- benchmark/framework/framework.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/benchmark/framework/framework.go b/benchmark/framework/framework.go index ae10bd422..c449d23d2 100644 --- a/benchmark/framework/framework.go +++ b/benchmark/framework/framework.go @@ -129,6 +129,8 @@ func (testStats *BenchmarkTestStats) calculateTestStat() { fmt.Printf("Error Calculating Mean: %v\n", err) testStats.Mean = -1 } + + print("testStats.BenchmarkTimes: ", testStats.BenchmarkTimes) testStats.Min, err = stats.Min(testStats.BenchmarkTimes) if err != nil { fmt.Printf("Error Calculating Min: %v\n", err) From 8d7c78e9e110fc4b8a08dad7c1c4aea3660235cb Mon Sep 17 00:00:00 2001 From: Arjun Yogidas Date: Mon, 7 Aug 2023 20:29:18 +0000 Subject: [PATCH 3/8] Add regression check automation This commit adds benchmark_regression_test.yml workflow that runs a regression test by comparing the benchmark results of the current branch with the previous benchmark results of the code in main branch. Signed-off-by: Arjun Raja Yogidas --- .../workflows/benchmark_regression_test.yml | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 .github/workflows/benchmark_regression_test.yml diff --git a/.github/workflows/benchmark_regression_test.yml b/.github/workflows/benchmark_regression_test.yml new file mode 100644 index 000000000..379dee060 --- /dev/null +++ b/.github/workflows/benchmark_regression_test.yml @@ -0,0 +1,68 @@ +name: Benchmark Regression Check + +on: + pull_request: + branches: [ main ] + paths: + - '**.go' + - 'go.*' + - 'cmd/go.*' + - 'Makefile' + - 'Dockerfile' + - 'integration/**' + - 'scripts/**' + - '.github/workflows/**' + +jobs: + benchmark-and-fetch-previous-results_and_compare: + name: Run Benchmark on current Branch + runs-on: ubuntu-20.04 + steps: + - uses: actions/checkout@v3 + - uses: actions/setup-go@v4 + with: + go-version: '1.18.10' + - run: make + - name: Run benchmark + run: make benchmarks-perf-test + - name: Make previous directory + run: mkdir -v ${{ github.workspace }}/previous + - name: Download previous run artifact + id: download-artifact + uses: dawidd6/action-download-artifact@v2 + with: + github_token: ${{secrets.GITHUB_TOKEN}} + name: benchmark-result-artifact + name_is_regexp: true + path: ${{ github.workspace }}/previous + repo: ${{ github.repository }} + check_artifacts: false + search_artifacts: true + skip_unpack: false + if_no_artifact_found: fail + workflow: benchmark_visualization.yml + - name: Perform Comparison and log results + id: run-compare + run: | + sudo chmod +x ${{ github.workspace }}/scripts/check_regression.sh + if sudo ${{ github.workspace }}/scripts/check_regression.sh ${{ github.workspace }}/previous/benchmark-result-artifact/results.json ${{github.workspace}}/benchmark/performanceTest/output/results.json; then + echo "Comparison successful. All P90 values are within the acceptable range." + else + echo "Comparison failed. Current P90 values exceed 110% of the corresponding past values." + echo "regression-detected=true" >> $GITHUB_OUTPUT + fi + - name: Stop the workflow if regression is detected + if: steps.run-compare.outputs.regression-detected == 'true' + uses: actions/github-script@v6 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const comment = ` + :warning: **Regression Detected** :warning: + + The benchmark comparison indicates that there has been a performance regression. + Please investigate and address the issue. + To Investigate check logs of the previous job above. + `; + + core.setFailed(comment); \ No newline at end of file From fae62ef168afa8f78e372b1713e6821d7e4ce148 Mon Sep 17 00:00:00 2001 From: Arjun Yogidas Date: Tue, 15 Aug 2023 18:23:08 +0000 Subject: [PATCH 4/8] Update workflow to run two benchmarks This commit is a test to check if running both the benchmarks in the same workflow would make any difference to the results Signed-off-by: Arjun Raja Yogidas --- .../workflows/benchmark_regression_test.yml | 45 +++++++++++-------- Makefile | 2 +- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/.github/workflows/benchmark_regression_test.yml b/.github/workflows/benchmark_regression_test.yml index 379dee060..4e5e62083 100644 --- a/.github/workflows/benchmark_regression_test.yml +++ b/.github/workflows/benchmark_regression_test.yml @@ -12,40 +12,47 @@ on: - 'integration/**' - 'scripts/**' - '.github/workflows/**' - + jobs: - benchmark-and-fetch-previous-results_and_compare: - name: Run Benchmark on current Branch + test-twice: runs-on: ubuntu-20.04 + steps: - - uses: actions/checkout@v3 - uses: actions/setup-go@v4 with: go-version: '1.18.10' + - name: Checkout main + uses: actions/checkout@v3 + with: + ref: main - run: make - name: Run benchmark - run: make benchmarks-perf-test + run: make benchmarks-perf-test - name: Make previous directory run: mkdir -v ${{ github.workspace }}/previous - - name: Download previous run artifact - id: download-artifact - uses: dawidd6/action-download-artifact@v2 + - name: Copy results to previous directory + run: cp -r ${{ github.workspace }}/benchmark/performanceTest/output ${{ github.workspace }}/previous + - name: Check out PR + uses: actions/checkout@v3 with: - github_token: ${{secrets.GITHUB_TOKEN}} - name: benchmark-result-artifact - name_is_regexp: true - path: ${{ github.workspace }}/previous - repo: ${{ github.repository }} - check_artifacts: false - search_artifacts: true - skip_unpack: false - if_no_artifact_found: fail - workflow: benchmark_visualization.yml + ref: ${{ github.event.pull_request.head.sha }} + - run: make + - name: Run benchmark + run: make benchmarks-perf-test + - name: Make current directory + run: mkdir -v ${{ github.workspace }}/current + - name: Stash uncommitted changes + run: git stash push --keep-index --include-untracked -m "Stashing changes for tests" + # If you're using Git version 2.28 or later, use this line instead: + # run: git stash push --keep-index --include-untracked -m "Stashing changes for tests" + # This will ensure that uncommitted changes do not interfere with the tests. + - name: Copy results to current directory + run: cp -r ${{ github.workspace }}/benchmark/performanceTest/output ${{ github.workspace }}/current - name: Perform Comparison and log results id: run-compare run: | sudo chmod +x ${{ github.workspace }}/scripts/check_regression.sh - if sudo ${{ github.workspace }}/scripts/check_regression.sh ${{ github.workspace }}/previous/benchmark-result-artifact/results.json ${{github.workspace}}/benchmark/performanceTest/output/results.json; then + if sudo ${{ github.workspace }}/scripts/check_regression.sh ${{ github.workspace }}/previous/results.json ${{github.workspace}}/current/results.json; then echo "Comparison successful. All P90 values are within the acceptable range." else echo "Comparison failed. Current P90 values exceed 110% of the corresponding past values." diff --git a/Makefile b/Makefile index ec32cac7f..e10fb90cd 100644 --- a/Makefile +++ b/Makefile @@ -104,7 +104,7 @@ build-benchmarks: benchmarks-perf-test: @echo "$@" - @cd benchmark/performanceTest ; sudo rm -rf output ; GO111MODULE=$(GO111MODULE_VALUE) go build -o ../bin/PerfTests . && sudo ../bin/PerfTests -show-commit + @cd benchmark/performanceTest ; sudo rm -rf output ; GO111MODULE=$(GO111MODULE_VALUE) go build -o ../bin/PerfTests . && sudo ../bin/PerfTests -show-commit -count 2 benchmarks-stargz: @echo "$@" From f865a178b7a9879286e67934c378adea59ecc0a1 Mon Sep 17 00:00:00 2001 From: Arjun Yogidas Date: Tue, 15 Aug 2023 22:12:48 +0000 Subject: [PATCH 5/8] Add timeout of 10 secs This commit adds 10 secs to the RunContainerTaskForReadyLine function Signed-off-by: Arjun Raja Yogidas --- benchmark/framework/containerd_utils.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/benchmark/framework/containerd_utils.go b/benchmark/framework/containerd_utils.go index b0c2b1ced..b0fca77b3 100644 --- a/benchmark/framework/containerd_utils.go +++ b/benchmark/framework/containerd_utils.go @@ -209,6 +209,8 @@ func (proc *ContainerdProcess) RunContainerTaskForReadyLine( stdoutScanner := bufio.NewScanner(taskDetails.stdoutReader) stderrScanner := bufio.NewScanner(taskDetails.stderrReader) + time.Sleep(10 * time.Second) + exitStatusC, err := taskDetails.task.Wait(ctx) if err != nil { return nil, err From 0177053752e3524687b8a59cf04f4f2ec7c7af0f Mon Sep 17 00:00:00 2001 From: Arjun Yogidas Date: Tue, 15 Aug 2023 18:23:08 +0000 Subject: [PATCH 6/8] Update workflow to run two benchmarks This commit is a test to check if running both the benchmarks in the same workflow would make any difference to the results Signed-off-by: Arjun Raja Yogidas --- .github/workflows/benchmark_regression_test.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.github/workflows/benchmark_regression_test.yml b/.github/workflows/benchmark_regression_test.yml index 4e5e62083..d3a6067b0 100644 --- a/.github/workflows/benchmark_regression_test.yml +++ b/.github/workflows/benchmark_regression_test.yml @@ -41,11 +41,6 @@ jobs: run: make benchmarks-perf-test - name: Make current directory run: mkdir -v ${{ github.workspace }}/current - - name: Stash uncommitted changes - run: git stash push --keep-index --include-untracked -m "Stashing changes for tests" - # If you're using Git version 2.28 or later, use this line instead: - # run: git stash push --keep-index --include-untracked -m "Stashing changes for tests" - # This will ensure that uncommitted changes do not interfere with the tests. - name: Copy results to current directory run: cp -r ${{ github.workspace }}/benchmark/performanceTest/output ${{ github.workspace }}/current - name: Perform Comparison and log results From a3fdb6b701bc216212732f58d0dc9bdf4e3e42a8 Mon Sep 17 00:00:00 2001 From: Arjun Yogidas Date: Tue, 15 Aug 2023 22:03:54 +0000 Subject: [PATCH 7/8] Change regression test logic This commit changes the regression test logic to run benchmarks on main brnach by checking out in the workflow itself instead of a previously uploaded artifact Signed-off-by: Arjun Raja Yogidas --- .../workflows/benchmark_regression_test.yml | 94 +++++++++++++------ 1 file changed, 64 insertions(+), 30 deletions(-) diff --git a/.github/workflows/benchmark_regression_test.yml b/.github/workflows/benchmark_regression_test.yml index d3a6067b0..e49ead97b 100644 --- a/.github/workflows/benchmark_regression_test.yml +++ b/.github/workflows/benchmark_regression_test.yml @@ -28,10 +28,15 @@ jobs: - run: make - name: Run benchmark run: make benchmarks-perf-test - - name: Make previous directory - run: mkdir -v ${{ github.workspace }}/previous - - name: Copy results to previous directory - run: cp -r ${{ github.workspace }}/benchmark/performanceTest/output ${{ github.workspace }}/previous + - name: Upload latest benchmark result + uses: actions/upload-artifact@v3 + with: + name: benchmark-result-artifact-main + path: ${{github.workspace}}/benchmark/performanceTest/output/results.json + - name: remove output directory + run: sudo rm -rf ${{ github.workspace }}/benchmark/performanceTest/output + - name: Stash uncommitted changes + run: git stash push --keep-index --include-untracked -m "Stashing changes for tests" - name: Check out PR uses: actions/checkout@v3 with: @@ -39,32 +44,61 @@ jobs: - run: make - name: Run benchmark run: make benchmarks-perf-test - - name: Make current directory - run: mkdir -v ${{ github.workspace }}/current - - name: Copy results to current directory - run: cp -r ${{ github.workspace }}/benchmark/performanceTest/output ${{ github.workspace }}/current - - name: Perform Comparison and log results - id: run-compare - run: | - sudo chmod +x ${{ github.workspace }}/scripts/check_regression.sh - if sudo ${{ github.workspace }}/scripts/check_regression.sh ${{ github.workspace }}/previous/results.json ${{github.workspace}}/current/results.json; then - echo "Comparison successful. All P90 values are within the acceptable range." - else - echo "Comparison failed. Current P90 values exceed 110% of the corresponding past values." - echo "regression-detected=true" >> $GITHUB_OUTPUT - fi - - name: Stop the workflow if regression is detected - if: steps.run-compare.outputs.regression-detected == 'true' - uses: actions/github-script@v6 + - name: Upload latest benchmark result + uses: actions/upload-artifact@v3 with: - github-token: ${{ secrets.GITHUB_TOKEN }} - script: | - const comment = ` - :warning: **Regression Detected** :warning: + name: benchmark-result-artifact-pr + path: ${{github.workspace}}/benchmark/performanceTest/output/results.json + + download_and_perform_comparison: + runs-on: ubuntu-20.04 + needs: test-twice + steps: + - uses: actions/setup-go@v4 + with: + go-version: '1.18.10' + - name: Checkout main + uses: actions/checkout@v3 + with: + ref: main + - run: make + + - name: Create previous directory + run: mkdir -v ${{ github.workspace }}/previous + - name: Create current directory + run: mkdir -v ${{ github.workspace }}/current + - name: Download previous benchmark result + uses: actions/download-artifact@v3 + with: + name: benchmark-result-artifact-main + path: ${{github.workspace}}/previous + - name: Download current benchmark result + uses: actions/download-artifact@v3 + with: + name: benchmark-result-artifact-pr + path: ${{github.workspace}}/current + - name: Perform Comparison and log results + id: run-compare + run: | + sudo chmod +x ${{ github.workspace }}/scripts/check_regression.sh + if sudo ${{ github.workspace }}/scripts/check_regression.sh ${{ github.workspace }}/previous/results.json ${{github.workspace}}/current/results.json; then + echo "Comparison successful. All P90 values are within the acceptable range." + else + echo "Comparison failed. Current P90 values exceed 110% of the corresponding past values." + echo "regression-detected=true" >> $GITHUB_OUTPUT + fi + - name: Stop the workflow if regression is detected + if: steps.run-compare.outputs.regression-detected == 'true' + uses: actions/github-script@v6 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const comment = ` + :warning: **Regression Detected** :warning: - The benchmark comparison indicates that there has been a performance regression. - Please investigate and address the issue. - To Investigate check logs of the previous job above. - `; + The benchmark comparison indicates that there has been a performance regression. + Please investigate and address the issue. + To Investigate check logs of the previous job above. + `; - core.setFailed(comment); \ No newline at end of file + core.setFailed(comment); \ No newline at end of file From da4b66654b938877244408e3979ba1b3ed03bffd Mon Sep 17 00:00:00 2001 From: Arjun Yogidas Date: Wed, 16 Aug 2023 16:41:59 +0000 Subject: [PATCH 8/8] Skip the first value for regression check This commit adds changes to the check_regression.sh script to skip the first benchmark times in both the old and new benchmark results and check for regression in the newly calculated p90, this is to combat the skewed reults in the benchmark results due to the variability in github runners. It also moves the threshold limit to 150% Signed-off-by: Arjun Raja Yogidas --- .../workflows/benchmark_regression_test.yml | 8 +++--- Makefile | 2 +- scripts/check_regression.sh | 27 +++++++++++++------ 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/.github/workflows/benchmark_regression_test.yml b/.github/workflows/benchmark_regression_test.yml index e49ead97b..88f18772c 100644 --- a/.github/workflows/benchmark_regression_test.yml +++ b/.github/workflows/benchmark_regression_test.yml @@ -14,7 +14,7 @@ on: - '.github/workflows/**' jobs: - test-twice: + run_benchmark_twice: runs-on: ubuntu-20.04 steps: @@ -52,7 +52,7 @@ jobs: download_and_perform_comparison: runs-on: ubuntu-20.04 - needs: test-twice + needs: run_benchmark_twice steps: - uses: actions/setup-go@v4 with: @@ -60,8 +60,10 @@ jobs: - name: Checkout main uses: actions/checkout@v3 with: - ref: main + ref: ${{ github.event.pull_request.head.sha }} - run: make + - name: Install basic calculator + run: sudo apt-get install bc - name: Create previous directory run: mkdir -v ${{ github.workspace }}/previous diff --git a/Makefile b/Makefile index e10fb90cd..ec32cac7f 100644 --- a/Makefile +++ b/Makefile @@ -104,7 +104,7 @@ build-benchmarks: benchmarks-perf-test: @echo "$@" - @cd benchmark/performanceTest ; sudo rm -rf output ; GO111MODULE=$(GO111MODULE_VALUE) go build -o ../bin/PerfTests . && sudo ../bin/PerfTests -show-commit -count 2 + @cd benchmark/performanceTest ; sudo rm -rf output ; GO111MODULE=$(GO111MODULE_VALUE) go build -o ../bin/PerfTests . && sudo ../bin/PerfTests -show-commit benchmarks-stargz: @echo "$@" diff --git a/scripts/check_regression.sh b/scripts/check_regression.sh index 65ae5fd33..070f2572c 100755 --- a/scripts/check_regression.sh +++ b/scripts/check_regression.sh @@ -22,12 +22,12 @@ compare_stat_p90() { local current_value="$2" local stat_name="$3" - # Calculate 110% of the past value + # Calculate 115% of the past value local threshold=$(calculate_threshold "$past_value") # Compare the current value with the threshold - if (( $(awk 'BEGIN {print ("'"$current_value"'" > "'"$threshold"'")}') )); then - echo "ERROR: $stat_name - Current P90 value ($current_value) exceeds the 110% threshold ($threshold) of the past P90 value ($past_value)" + if (( $(echo "$current_value > $current_value" |bc -l) )); then + echo "ERROR: $stat_name - Current P90 value ($current_value) exceeds the 115% threshold ($current_value) of the past P90 value ($past_value)" return 1 fi @@ -36,7 +36,18 @@ compare_stat_p90() { calculate_threshold() { local past_value="$1" - awk -v past="$past_value" 'BEGIN { print past * 1.1 }' + awk -v past="$past_value" 'BEGIN { print past * 1.15 }' +} + +calculate_p90_after_skip() { + local times_array="$1" + local num_entries=$(echo "$times_array" | jq 'length') + local times=$(echo "$times_array" | jq -r '.[1:] | .[]') + local sorted_times=$(echo "$times" | tr '\n' ' ' | xargs -n1 | sort -g) + local index=$((num_entries * 90 / 100)) + + local p90=$(echo "$sorted_times" | sed -n "${index}p") + echo "$p90" } # Loop through each object in past.json and compare P90 values with current.json for all statistics @@ -52,8 +63,10 @@ compare_p90_values() { for test_name in $test_names; do echo "Checking for regression in '$test_name'" for stat_name in "fullRunStats" "pullStats" "lazyTaskStats" "localTaskStats"; do - local past_p90=$(echo "$past_json" | jq -r --arg test "$test_name" '.benchmarkTests[] | select(.testName == $test) | .'"$stat_name"'.pct90') - local current_p90=$(echo "$current_json" | jq -r --arg test "$test_name" '.benchmarkTests[] | select(.testName == $test) | .'"$stat_name"'.pct90') + local past_p90_array=$(echo "$past_json" | jq -r --arg test "$test_name" '.benchmarkTests[] | select(.testName == $test) | .'"$stat_name"'.BenchmarkTimes') + local past_p90=$(calculate_p90_after_skip "$past_p90_array") + local current_p90_array=$(echo "$current_json" | jq -r --arg test "$test_name" '.benchmarkTests[] | select(.testName == $test) | .'"$stat_name"'.BenchmarkTimes') + local current_p90=$(calculate_p90_after_skip "$current_p90_array") # Call the compare_stat_p90 function compare_stat_p90 "$past_p90" "$current_p90" "$stat_name" || regression_detected=1 @@ -64,8 +77,6 @@ compare_p90_values() { return $regression_detected } -# ... (remaining code) - # Call compare_p90_values and store the exit code in a variable compare_p90_values "$past_data" "$current_data" exit_code=$?