diff --git a/.github/workflows/benchmark_regression_test.yml b/.github/workflows/benchmark_regression_test.yml new file mode 100644 index 000000000..af633ff0e --- /dev/null +++ b/.github/workflows/benchmark_regression_test.yml @@ -0,0 +1,106 @@ +name: Benchmark Regression Check + +on: + pull_request: + branches: [ main ] + paths: + - '**.go' + - 'go.*' + - 'cmd/go.*' + - 'Makefile' + - 'Dockerfile' + - 'integration/**' + - 'scripts/**' + - '.github/workflows/**' + +jobs: + run_benchmark_twice: + runs-on: ubuntu-20.04 + 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: Run benchmark + run: make benchmarks-perf-test + - 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: + ref: ${{ github.event.pull_request.head.sha }} + - run: make + - name: Run benchmark + run: make benchmarks-perf-test + - name: Upload latest benchmark result + uses: actions/upload-artifact@v3 + with: + name: benchmark-result-artifact-pr + path: ${{github.workspace}}/benchmark/performanceTest/output/results.json + + download_and_perform_comparison: + runs-on: ubuntu-20.04 + needs: run_benchmark_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: Install basic calculator + run: sudo apt-get install bc + + - 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. + `; + + core.setFailed(comment); 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 "$@" 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 diff --git a/benchmark/framework/framework.go b/benchmark/framework/framework.go index 28c3a7dd2..c449d23d2 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) @@ -128,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) 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 { diff --git a/scripts/check_regression.sh b/scripts/check_regression.sh index 65ae5fd33..056f943c9 100755 --- a/scripts/check_regression.sh +++ b/scripts/check_regression.sh @@ -25,9 +25,12 @@ compare_stat_p90() { # Calculate 110% of the past value local threshold=$(calculate_threshold "$past_value") + echo "Current P90 value for $stat_name: $current_value" + # echo "Past P90 value for $stat_name: $past_value" + echo "Threshold for $stat_name: $threshold" # 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 150% threshold ($current_value) of the past P90 value ($past_value)" return 1 fi @@ -36,7 +39,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.5 }' +} + +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 +66,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 +80,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=$?