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: Concurrent file uploads #53

Merged
merged 33 commits into from
Jul 3, 2023
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
03b6952
feat: Upload concurrency
js-murph Jun 1, 2023
a5f5453
fix: Add debug logging
js-murph Jun 1, 2023
565ea3b
fix: Use command grouping so output is captured
js-murph Jun 1, 2023
c8aea0b
fix: intentation
js-murph Jun 1, 2023
5cab305
chore: Debug line
js-murph Jun 1, 2023
bde27fc
chore: Remove tracing
js-murph Jun 1, 2023
1f8e739
chore: add tracing, remove subshell
js-murph Jun 1, 2023
52de3bf
chore: change increment operation
js-murph Jun 1, 2023
032a0c3
chore: Re-add subshell
js-murph Jun 1, 2023
0d4eaef
chore: Remove tracing
js-murph Jun 1, 2023
3ea88ad
chore: More debug logs
js-murph Jun 1, 2023
56add33
fix: Store pids instead of using counters
js-murph Jun 1, 2023
0509e90
fix: accidental char
js-murph Jun 1, 2023
5936cb8
fix: Remove pids no longer running
js-murph Jun 1, 2023
0090dfe
chore: Change default concurrency back to 1
js-murph Jun 1, 2023
980cc62
docs: Add docs & tests
js-murph Jun 1, 2023
26ce593
chore: Test tests
js-murph Jun 1, 2023
9e58efc
chore: Test tests
js-murph Jun 1, 2023
712af76
chore: Test tests
js-murph Jun 1, 2023
e6d475d
chore: decrease verbosity
js-murph Jun 1, 2023
47302c3
Update tests/pre-exit-success.bats
js-murph Jun 16, 2023
d4b4e21
fix: Concurrency tests
js-murph Jun 19, 2023
7b96c14
Merge pull request #1 from js-murph/johnm/fix-tests
js-murph Jun 19, 2023
0e15880
Update tests/pre-exit-success.bats
js-murph Jun 19, 2023
14fce48
fix: BSD compatability
js-murph Jun 19, 2023
bc84ece
Merge pull request #2 from js-murph/johnm/fix-bsd-compatability
js-murph Jun 19, 2023
57b7297
fix: Use timeout var instead of magic numbers for concurrency
js-murph Jun 19, 2023
139f14e
fix: Add missing concurrency tests (#4)
js-murph Jun 20, 2023
f8ca0cf
fix: Various test fixes and new timeout test (#5)
js-murph Jun 20, 2023
a85a3ae
chore: Remove non-functional timeout test
js-murph Jun 23, 2023
b9aded8
fix: Re-add working concurrency timeout test
js-murph Jun 29, 2023
d1f196b
fix: timeout test consistency and response race condition
js-murph Jun 30, 2023
dbb5716
fix: skip multiple files concurrently
js-murph Jul 2, 2023
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
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,12 @@ Adds an annotation to the build run with a link to the uploaded report.

Default value: `false`

#### `upload-concurrency`(number)

The number of concurrent file uploads to perform to the Buildkite analytics API.

Default value: `1`

## Examples

### Upload a JUnit file
Expand Down
56 changes: 48 additions & 8 deletions hooks/pre-exit
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ FORMAT="${BUILDKITE_PLUGIN_TEST_COLLECTOR_FORMAT:-}"
TIMEOUT="${BUILDKITE_PLUGIN_TEST_COLLECTOR_TIMEOUT:-30}"
BASE_PATH="${BUILDKITE_PLUGIN_TEST_COLLECTOR_BASE_PATH:-.}"
ANNOTATE="${BUILDKITE_PLUGIN_TEST_COLLECTOR_ANNOTATION_LINK:-false}"
UPLOAD_CONCURRENCY="${BUILDKITE_PLUGIN_TEST_COLLECTOR_UPLOAD_CONCURRENCY:-1}"
REPORT_URLS_FILE=$(mktemp)
CURL_RESP_FILE="${CURL_RESP_FILE:-response.json}"
DEBUG="false"
Expand Down Expand Up @@ -160,16 +161,55 @@ find_and_upload() {
exit "${BUILDKITE_PLUGIN_TEST_COLLECTOR_MISSING_ERROR:-1}"
fi
else
declare -a uploads_in_progress=()
echo "Uploading '${#matching_files[@]}' files matching '${FILES_PATTERN}'"

# needs to be part of else for bash4.3 compatibility
for file in "${matching_files[@]}"; do
echo "Uploading '$file'..."
if ! upload "$TOKEN_VALUE" "$FORMAT" "${file}"; then
echo "Error uploading, will continue"
fi
if [[ "$ANNOTATE" != "false" ]]; then
save-report-url "${CURL_RESP_FILE}"
fi
iterations_waited=0
while [[ "${#uploads_in_progress[@]}" -ge $UPLOAD_CONCURRENCY ]]; do
iterations_waited=$((iterations_waited + 1))
if [[ "${DEBUG}" == "true" ]]; then
echo "Waiting for uploads to finish..."
fi

sleep 1

for index in "${!uploads_in_progress[@]}"; do
# Note: kill -0 does not kill the pid, it provides a *nix compatible way to test the pid is responding.
if ! kill -0 "${uploads_in_progress[index]}" > /dev/null; then
unset 'uploads_in_progress[index]'
elif [[ "$iterations_waited" -gt $TIMEOUT ]]; then
echo "Upload '${uploads_in_progress[index]}' has been running for more than '${TIMEOUT}' seconds, killing it"
kill "${uploads_in_progress[index]}"
unset 'uploads_in_progress[index]'
fi
done
done

# Spawn a subcommand group to allow parallel uploads
{
echo "Uploading '$file'..."

if ! upload "$TOKEN_VALUE" "$FORMAT" "${file}"; then
echo "Error uploading, will continue"
fi

if [[ "$ANNOTATE" != "false" ]]; then
save-report-url "${CURL_RESP_FILE}"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized there is a bug here: the file is shared among all upload calls so there is a race condition as to which URL is saved for annotations and which are not. There are two alternatives to fix this:

1- make a note/check about annotation not compatible with concurrent uploads
2- change the upload function to accept another parameter that defines the filename to save the file that you can then pass through to the save-report-url

Whichever you prefer I'll be OK with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch. I'll investigate this further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is fixed now, I use mktemp to generate a temporary file in the command group if CURL_RESP_FILE is unset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is fixed now, I use mktemp to generate a temporary file in the command group if CURL_RESP_FILE is unset.


if [[ "${DEBUG}" == "true" ]]; then
echo "Finished uploading '$file'"
fi
} &

# Store the PID of the upload
uploads_in_progress+=($!)
done

# Wait for all uploads to finish
wait "${uploads_in_progress[@]}"
fi
}

Expand All @@ -194,4 +234,4 @@ else
fi
if [ "$ANNOTATE" != "false" ]; then
annotation-link "${REPORT_URLS_FILE}"
fi
fi
2 changes: 2 additions & 0 deletions plugin.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ configuration:
type: integer
annotation-link:
type: boolean
upload-concurrency:
type: integer
required:
- files
- format
Expand Down
78 changes: 78 additions & 0 deletions tests/pre-exit-success.bats
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,45 @@ COMMON_CURL_OPTIONS='--form \* --form \* --form \* --form \* --form \* --form \*
assert_output --partial "curl success 3"
}

@test "Uploads multiple files concurrently does not break basic functionality" {
# would love to test functionality but can not do so due to limitations on bats-mock :(
export BUILDKITE_PLUGIN_TEST_COLLECTOR_FILES='**/*/junit-*.xml'
export BUILDKITE_PLUGIN_TEST_COLLECTOR_UPLOAD_CONCURRENCY='3'

stub curl "-X POST --silent --show-error --max-time 30 --form format=junit ${COMMON_CURL_OPTIONS} \* -H \* : echo \"curl success \${10}\""

run "$PWD/hooks/pre-exit"

unstub curl

assert_success
assert_output --partial "Uploading './tests/fixtures/junit-1.xml'..."
assert_output --partial "Uploading './tests/fixtures/junit-2.xml'..."
assert_output --partial "Uploading './tests/fixtures/junit-3.xml'..."
assert_equal "$(echo "$output" | grep -c "curl success")" "3"
}

@test "Concurrency waits when the queue is full" {
export BUILDKITE_PLUGIN_TEST_COLLECTOR_FILES='**/*/junit-*.xml'
export BUILDKITE_PLUGIN_TEST_COLLECTOR_UPLOAD_CONCURRENCY='2'
export BUILDKITE_PLUGIN_TEST_COLLECTOR_DEBUG='true'

stub curl \
"-X POST --silent --show-error --max-time 30 --form format=junit ${COMMON_CURL_OPTIONS} --form \* \* -H \* : sleep 3; echo \"curl success \${10}\"" \
"-X POST --silent --show-error --max-time 30 --form format=junit ${COMMON_CURL_OPTIONS} --form \* \* -H \* : echo \"curl success \${10}\""

run "$PWD/hooks/pre-exit"

unstub curl

assert_success
assert_output --partial "Uploading './tests/fixtures/junit-1.xml'..."
assert_output --partial "Uploading './tests/fixtures/junit-2.xml'..."
assert_output --partial "Waiting for uploads to finish..."
assert_output --partial "Uploading './tests/fixtures/junit-3.xml'..."
assert_equal "$(echo "$output" | grep -c "curl success")" "3"
}

@test "Single file pattern through array" {
export BUILDKITE_PLUGIN_TEST_COLLECTOR_FILES_0='**/*/junit-1.xml'
unset BUILDKITE_PLUGIN_TEST_COLLECTOR_FILES
Expand Down Expand Up @@ -149,6 +188,25 @@ COMMON_CURL_OPTIONS='--form \* --form \* --form \* --form \* --form \* --form \*
assert_output --partial "curl success"
}

@test "Concurrency gracefully handles command-group timeout" {
export BUILDKITE_PLUGIN_TEST_COLLECTOR_FILES='**/*/junit-*.xml'
export BUILDKITE_PLUGIN_TEST_COLLECTOR_UPLOAD_CONCURRENCY='2'
export BUILDKITE_PLUGIN_TEST_COLLECTOR_TIMEOUT='3'

stub curl "if [ \${10} != 'data=@\"./tests/fixtures/junit-3.xml\"' ]; then echo sleeping for \${10}; sleep 10 & wait \$!; else echo curl success \${10}; fi"

run "$PWD/hooks/pre-exit"

unstub curl

assert_success
assert_output --partial "Uploading './tests/fixtures/junit-1.xml'..."
assert_output --partial "Uploading './tests/fixtures/junit-2.xml'..."
assert_output --partial "Error uploading, will continue"
assert_output --partial "Uploading './tests/fixtures/junit-3.xml'..."
assert_equal "$(echo "$output" | grep -c "curl success")" "1"
}

@test "Git available sends plugin version" {
stub git "rev-parse --short HEAD : echo 'some-commit-id'"
stub curl "-X POST --silent --show-error --max-time 30 --form format=junit ${COMMON_CURL_OPTIONS} --form \* \* -H \* : echo \"curl success with \${30}\""
Expand Down Expand Up @@ -234,3 +292,23 @@ COMMON_CURL_OPTIONS='--form \* --form \* --form \* --form \* --form \* --form \*
assert_output --partial "Uploading './tests/fixtures/junit-1.xml'..."
assert_output --partial "Error uploading, will continue"
}

@test "Concurrency gracefully handles failure" {
export BUILDKITE_PLUGIN_TEST_COLLECTOR_FILES='**/*/junit-*.xml'
export BUILDKITE_PLUGIN_TEST_COLLECTOR_UPLOAD_CONCURRENCY='2'

stub curl \
"-X POST --silent --show-error --max-time 30 --form format=junit ${COMMON_CURL_OPTIONS} \* -H \* : exit 10" \
"-X POST --silent --show-error --max-time 30 --form format=junit ${COMMON_CURL_OPTIONS} \* -H \* : echo 'curl success'"

run "$PWD/hooks/pre-exit"

unstub curl

assert_success
assert_output --partial "Uploading './tests/fixtures/junit-1.xml'..."
assert_output --partial "Uploading './tests/fixtures/junit-2.xml'..."
assert_equal "$(echo "$output" | grep -c "Error uploading, will continue")" "2"
assert_output --partial "Uploading './tests/fixtures/junit-3.xml'..."
assert_equal "$(echo "$output" | grep -c "curl success")" "1"
}