Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 48 additions & 2 deletions .github/workflows/comment-to-trigger-teamcity.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,54 @@ permissions:
jobs:
check-comment-if-need-to-trigger-teamcity:

# This job only runs for pull request comments, and comment body contains 'run'
if: ${{ github.event.issue.pull_request && (contains(github.event.comment.body, 'run') || contains(github.event.comment.body, 'skip buildall') || contains(github.event.comment.body, 'skip check_coverage')) }}
# Keep this server-side filter aligned with the supported commands parsed below.
# A generic "run" match also catches performance reports such as "Total hot run time".
if: >-
${{
github.event.issue.pull_request &&
(
contains(github.event.comment.body, 'run buildall') ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Use one canonical command grammar before entering concurrency

This admission test is not equivalent to the parser below: it examines the raw body with case-insensitive contains, while lines 91/179 normalize whitespace and then shell-match case-sensitively. Thus run p0 (or a line break between the words) previously normalizes to valid run p0 but is now skipped; conversely RUN P0 is admitted and assigned the p0 group, then the parser no-ops. Because the default concurrency queue keeps only one pending job, that no-op can replace a legitimate pending p0 request. Please make admission, grouping, and dispatch use the same canonical command and preserve distinct pending requests.

contains(github.event.comment.body, 'run compile') ||
contains(github.event.comment.body, 'run beut') ||
contains(github.event.comment.body, 'run feut') ||
contains(github.event.comment.body, 'run cloudut') ||
contains(github.event.comment.body, 'run p0') ||
contains(github.event.comment.body, 'run p1') ||
contains(github.event.comment.body, 'run external') ||
contains(github.event.comment.body, 'run cloud_p0') ||
contains(github.event.comment.body, 'run cloud_p1') ||
contains(github.event.comment.body, 'run vault_p0') ||
contains(github.event.comment.body, 'run nonConcurrent') ||
contains(github.event.comment.body, 'run check_coverage') ||
contains(github.event.comment.body, 'run performance') ||
contains(github.event.comment.body, 'skip buildall') ||
contains(github.event.comment.body, 'skip check_coverage')
)
}}

# Serialize duplicate commands for one PR. The next run then sees the build
# queued by the previous run and can safely skip an identical PR revision.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Coordinate buildall through the complete fan-out

The helper and GitHub lock cover only Compile even though this workflow skips the downstream steps because Compile triggers them. This fails in both directions: for PR 66196, Compile 1008441 finished while same-revision P0 1008463, External 1008464, Cloud P0 1008465, and NonConcurrent 1008467 were still running, so another buildall starts a new root; while Compile is still active, a direct run p0 can queue P0 before Compile later enqueues the same P0 entirely inside TeamCity, bypassing this helper and every GitHub group. Please enforce exact-request coordination at the TeamCity fan-out boundary, or route both direct and fan-out requests through one atomic coordinator for the full descendant lifetime.

concurrency:
group: >-
comment-to-trigger-teamcity-${{ github.event.issue.number }}-${{
contains(github.event.comment.body, 'run buildall') && 'buildall' ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Serialize mutations of the same concrete pipeline

This key does not cover every run that executes the same check-then-trigger operation. run buildall is placed in the buildall group but later calls the helper for FEUT, BEUT, CloudUT, Compile, and Performance, while each direct command uses a different group. Also, run feut run beut is grouped as beut by this fixed precedence but the parser executes the first textual command, feut. Two such jobs can both observe no active FEUT build and then both enqueue it. Please lock by the concrete PR/pipeline mutation (or use a PR-wide/TeamCity-side atomic lock) and derive the key from the same canonical command used for dispatch.

contains(github.event.comment.body, 'run compile') && 'compile' ||
contains(github.event.comment.body, 'run beut') && 'beut' ||
contains(github.event.comment.body, 'run feut') && 'feut' ||
contains(github.event.comment.body, 'run cloudut') && 'cloudut' ||
contains(github.event.comment.body, 'run p0') && 'p0' ||
contains(github.event.comment.body, 'run p1') && 'p1' ||
contains(github.event.comment.body, 'run external') && 'external' ||
contains(github.event.comment.body, 'run cloud_p0') && 'cloud-p0' ||
contains(github.event.comment.body, 'run cloud_p1') && 'cloud-p1' ||
contains(github.event.comment.body, 'run vault_p0') && 'vault-p0' ||
contains(github.event.comment.body, 'run nonConcurrent') && 'non-concurrent' ||
contains(github.event.comment.body, 'run check_coverage') && 'check-coverage' ||
contains(github.event.comment.body, 'run performance') && 'performance' ||
contains(github.event.comment.body, 'skip buildall') && 'skip-buildall' ||
'skip-check-coverage'
}}
cancel-in-progress: false

runs-on: ubuntu-latest
env:
Expand Down
73 changes: 73 additions & 0 deletions regression-test/pipeline/common/teamcity-utils.sh
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,64 @@ get_queue_build_of_pr() {
}
# get_queue_build_of_pr "$1" "$2"

get_active_builds_of_revision() {
# Return active build IDs for the same PR, pipeline, and revision.
# Return 0 when a duplicate exists, 1 when none exists, and 2 when lookup fails.
local PULL_REQUEST_NUM="${PULL_REQUEST_NUM:-$1}"
local COMMENT_TRIGGER_TYPE="${COMMENT_TRIGGER_TYPE:-$2}"
local COMMIT_ID_FROM_TRIGGER="${COMMIT_ID_FROM_TRIGGER:-$3}"
if [[ -z "${PULL_REQUEST_NUM}" ||
-z "${COMMENT_TRIGGER_TYPE}" ||
-z "${COMMIT_ID_FROM_TRIGGER}" ]]; then
echo "Usage: get_active_builds_of_revision PULL_REQUEST_NUM COMMENT_TRIGGER_TYPE COMMIT_ID_FROM_TRIGGER" >&2
return 2
fi

local PIPELINE="${comment_to_pipeline[${COMMENT_TRIGGER_TYPE}]}"
local teamcity_rest_url="http://43.132.222.7:8111/app/rest"
local queue_response
local running_response
if ! queue_response=$(
curl -sSf -X GET \
-u OneMoreChance:OneMoreChance \
-H "Accept: application/json" \
"${teamcity_rest_url}/buildQueue?locator=buildType:(id:${PIPELINE})&fields=build(id,branchName,revisions(revision(version)))"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] Follow TeamCity pagination when looking for duplicates

TeamCity collection responses may be partial and advertise the next page through nextHref, but this projection does not request that field and the function performs only one request. Since the queue locator can filter by build type but not by PR branch/revision, a busy pipeline can place the exact duplicate on a later page; this returns “none”, the single-page cancel path misses it too, and line 378 queues another copy. Please follow nextHref (for queued and running collections) until a match is found or the collection is exhausted, with a multi-page fixture.

); then
echo "WARNING: failed to get queued builds for duplicate check" >&2
return 2
fi
if ! running_response=$(
curl -sSf -X GET \
-u OneMoreChance:OneMoreChance \
-H "Accept: application/json" \
"${teamcity_rest_url}/builds?locator=buildType:(id:${PIPELINE}),branch:(name:pull/${PULL_REQUEST_NUM}),running:true&fields=build(id,branchName,revisions(revision(version)))"
); then
echo "WARNING: failed to get running builds for duplicate check" >&2
return 2
fi

local build_ids
if ! build_ids=$(
printf '%s\n%s\n' "${queue_response}" "${running_response}" |
jq -s -r \
--arg branch "pull/${PULL_REQUEST_NUM}" \
--arg revision "${COMMIT_ID_FROM_TRIGGER}" \
'.[] | .build[]? |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Include repeat count in the duplicate identity

Repeat count changes the request: the parser preserves it and trigger_build sends it as env.repeat_times_from_trigger, which the regression pipelines use for their iteration count and timeout. This selector compares only PR branch and revision, so while run cloud_p0 is active, a subsequent run cloud_p0 3 on the same head is reported as a duplicate and the requested three passes are never scheduled. Please normalize the empty count to 1 and compare the active build's repeat property as part of duplicate equivalence, while keeping all requests for one PR/pipeline in the same serialization domain.

select(.branchName == $branch and .revisions.revision[0].version == $revision) |
.id'
); then
echo "WARNING: failed to parse active builds for duplicate check" >&2
return 2
fi

if [[ -n "${build_ids}" ]]; then
echo "${build_ids}"
return 0
fi
return 1
}
# get_active_builds_of_revision "$1" "$2" "$3"

cancel_running_build() {
local PULL_REQUEST_NUM="${PULL_REQUEST_NUM:-$1}"
local COMMENT_TRIGGER_TYPE="${COMMENT_TRIGGER_TYPE:-$2}"
Expand Down Expand Up @@ -300,6 +358,21 @@ trigger_or_skip_build() {
fi

if [[ "${FILE_CHANGED:-"true"}" == "true" ]]; then
local duplicate_build_ids
local duplicate_lookup_status=0
duplicate_build_ids=$(
get_active_builds_of_revision \
"${PULL_REQUEST_NUM}" \
"${COMMENT_TRIGGER_TYPE}" \
"${COMMIT_ID_FROM_TRIGGER}"
) || duplicate_lookup_status=$?
if [[ ${duplicate_lookup_status} -eq 0 ]]; then
echo "INFO: active build(s) ${duplicate_build_ids//$'\n'/,} already exist for PR ${PULL_REQUEST_NUM}, pipeline ${COMMENT_TRIGGER_TYPE}, revision ${COMMIT_ID_FROM_TRIGGER}; skip duplicate trigger"
return 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Still clean up stale revisions before returning

get_active_builds_of_revision reports only IDs matching the requested revision, so this return says nothing about other active builds for the same PR/pipeline. If a current-revision build is queued while an older revision is still running or queued (for example after an external trigger or a failed/asynchronous cancellation), this branch bypasses both cleanup calls below and leaves the stale build alive. That contradicts the stated preservation of cancel-old-revision behavior. Please classify the full active set, cancel every non-current build, and skip only the new trigger when a current build remains.

elif [[ ${duplicate_lookup_status} -ne 1 ]]; then
echo "WARNING: duplicate lookup failed for PR ${PULL_REQUEST_NUM}, pipeline ${COMMENT_TRIGGER_TYPE}; continue with the existing trigger flow"
fi

cancel_running_build "${PULL_REQUEST_NUM}" "${COMMENT_TRIGGER_TYPE}"
cancel_queue_build "${PULL_REQUEST_NUM}" "${COMMENT_TRIGGER_TYPE}"
trigger_build "${PULL_REQUEST_NUM}" "${COMMIT_ID_FROM_TRIGGER}" "${COMMENT_TRIGGER_TYPE}" "${COMMENT_REPEAT_TIMES}"
Expand Down
Loading