fix: Move "should run tests" from buildkite to github action

Our internal check to alarm us if a post-PR A/B-test fails does not fire
if a "bailed out" run (e.g. one where we decide not to run A/B-tests
because we only modified, say, .md files) happens while a previous
A/B-test is still running. This is because it only looks at the most
recent buildkite run.

Therefore, move the check whether to run an A/B-test at all from
buildkite to the GitHub action that orchestrates the runs, so that "bail
outs" happen outside of buildkite.

Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

.buildkite/pipeline_perf.py

@@ -5,13 +5,7 @@
 """Generate Buildkite performance pipelines dynamically"""
 import os
 
-from common import (
-    COMMON_PARSER,
-    get_changed_files,
-    group,
-    overlay_dict,
-    pipeline_to_json,
-)
+from common import COMMON_PARSER, group, overlay_dict, pipeline_to_json
 
 # In `devtool_opts`, we restrict both the set of CPUs on which the docker container's threads can run,
 # and its memory node. For the cpuset, we pick a continuous set of CPUs from a single NUMA node
@@ -96,37 +90,25 @@ def build_group(test):
     action="append",
 )
 
-RUN_TESTS = True
-if REVISION_A is not None:
-    changed_files = get_changed_files(f"{REVISION_A}..{REVISION_B}")
-    # Our A/B-Testing setup by design only A/B-tests firecracker binaries.
-    # So we only trigger A/B-tests on file changes that have impact on the firecracker
-    # binary. These include ".rs" files, "Cargo.toml" and "Cargo.lock" files, as well
-    # as ".cargo/config".
-    RUN_TESTS = any(
-        x.suffix in [".rs", ".toml", ".lock", "config"] for x in changed_files
-    )
-
 group_steps = []
 
-if RUN_TESTS:
-    args = parser.parse_args()
-    tests = [perf_test[test] for test in args.test or perf_test.keys()]
-    for test_data in tests:
-        test_data.setdefault("platforms", args.platforms)
-        test_data.setdefault("instances", args.instances)
-        # use ag=1 instances to make sure no two performance tests are scheduled on the same instance
-        test_data.setdefault("agents", {"ag": 1})
-        test_data = overlay_dict(test_data, args.step_param)
-        test_data["retry"] = {
-            "automatic": [
-                # Agent was lost, retry one time
-                # this can happen if we terminate the instance or the agent gets
-                # disconnected for whatever reason
-                {"exit_status": -1, "limit": 1},
-            ]
-        }
-        group_steps.append(build_group(test_data))
+args = parser.parse_args()
+tests = [perf_test[test] for test in args.test or perf_test.keys()]
+for test_data in tests:
+    test_data.setdefault("platforms", args.platforms)
+    test_data.setdefault("instances", args.instances)
+    # use ag=1 instances to make sure no two performance tests are scheduled on the same instance
+    test_data.setdefault("agents", {"ag": 1})
+    test_data = overlay_dict(test_data, args.step_param)
+    test_data["retry"] = {
+        "automatic": [
+            # Agent was lost, retry one time
+            # this can happen if we terminate the instance or the agent gets
+            # disconnected for whatever reason
+            {"exit_status": -1, "limit": 1},
+        ]
+    }
+    group_steps.append(build_group(test_data))
 
 pipeline = {
     "env": {},

.github/workflows/trigger_ab_tests.yml

@@ -9,16 +9,34 @@ jobs:
     runs-on: ubuntu-latest
     if: ${{ github.event.forced == false }}
     steps:
+      - name: "Check out repository"
+        uses: actions/checkout@v4
+        with:
+          # Required to make it fetch more than the just fetched commits (we need it to resolve at least one commit
+          # _before_ what was pushed so that below "git diff" works.
+          fetch-depth: 0
       - name: "Trigger Buildkite Pipeline"
         run: |
-          curl -X POST https://api.buildkite.com/v2/organizations/firecracker/pipelines/performance-a-b-tests/builds \
-               -H 'Content-Type: application/json' \
-               -H 'Authorization: Bearer ${{ secrets.BUILDKITE_TOKEN }}' \
-               -d "{
-                    \"commit\": \"HEAD\",
-                    \"branch\": \"$GITHUB_REF_NAME\",
-                    \"env\": {
-                      \"REVISION_A\": \"${{ github.event.before }}\",
-                      \"REVISION_B\": \"${{ github.event.after }}\"
-                    }
-                  }"
+          should_schedule_ab_test=0
+          # Iterates over all files modified in the just-merged PR. If any of them is rust-related (e.g. .rs, .toml, 
+          # .lock or .cargo/config) or a seccomp definition (resources/seccomp/*), sets `should_schedule_ab_test` to 1, 
+          # meaning we will schedule a build of the A/B-testing pipeline to check the just-merged PR for
+          # performance regressions.
+          for f in $(git --no-pager diff --name-only ${{ github.event.before }}..${{ github.event.after }}); do
+            if [[ "$(basename $f)" =~ (\.(rs|toml|lock)|config)$ ]] || [[ "$f" ~= ^resources/seccomp/ ]]; then
+              should_schedule_ab_test=1
+            fi
+          done
+          if [[ $should_schedule_ab_test -eq 1 ]]; then
+            curl -X POST https://api.buildkite.com/v2/organizations/firecracker/pipelines/performance-a-b-tests/builds \
+                 -H 'Content-Type: application/json' \
+                 -H 'Authorization: Bearer ${{ secrets.BUILDKITE_TOKEN }}' \
+                 -d "{
+                      \"commit\": \"HEAD\",
+                      \"branch\": \"$GITHUB_REF_NAME\",
+                      \"env\": {
+                        \"REVISION_A\": \"${{ github.event.before }}\",
+                        \"REVISION_B\": \"${{ github.event.after }}\"
+                      }
+                    }"
+          fi