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

Use curl retry features to workaround transient network problems #440

Merged
merged 2 commits into from
Mar 14, 2018

Conversation

dliappis
Copy link
Contributor

curl after a certain version allows a number of --retry options that
can improve resiliency against transient network issues.

Use curl retry features and bail out if the system doesn't honor the
minimum curl version.

Ideally we'd also use --retry-connrefused but this parameter got
introduced in curl 7.52 which isn't available yet on many
distributions such as Ubuntu 16.04.

curl after a certain version allows a number of --retry options that
can improve resiliency against transient network issues.

Use curl retry features and bail out if the system doesn't honor the
minimum curl version.

Ideally we'd also use `--retry-connrefused` but this parameter got
introduced in curl 7.52 which isn't available yet on many
distributions such as Ubuntu 16.04.
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I left a couple of suggestions.

local curl_patch_release=$(curl --version | head -1 | cut -d ' ' -f 2,2 | cut -d '.' -f 3,3)

if [[ $curl_major_version < ${MIN_CURL_VERSION[0]} ]] || \
[[ $curl_major_version == ${MIN_CURL_VERSION[0]} && $curl_minor_version < ${MIN_CURL_VERSION[1]} ]] || \
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't think this will work anymore when curl 8.x will be released in the future? While the check is good, I am also not sure whether we can just avoid this check completely because we have tight control over the environments in which this runs (our CI)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should work (I've tested it too) because we are failing only if:

  • curl < 7 or
  • curl_major == 7 and curl_minor < 12 or
  • curl_major == 7 and curl_minor == 12 and curl_patch < 3

everything else passes, including curl >= 8

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I missed that. Good point.

@@ -85,9 +99,11 @@ function set_up() {
mkdir -p .rally_it/cache
cd .rally_it/cache
if [ ! -f elasticsearch-"${ES_METRICS_STORE_VERSION}".tar.gz ]; then
curl -O https://artifacts.elastic.co/downloads/elasticsearch/elasticsearch-"${ES_METRICS_STORE_VERSION}".tar.gz
# If curl fails immediately, executing all retries will take up to (2**retries)-1 seconds.
curl --retry 8 -O https://artifacts.elastic.co/downloads/elasticsearch/elasticsearch-"${ES_METRICS_STORE_VERSION}".tar.gz || { rm elasticsearch-"${ES_METRICS_STORE_VERSION}".tar.gz; exit 1; }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it makes sense to extract the artefact name (elasticsearch-"${ES_METRICS_STORE_VERSION}".tar.gz) as a constant as it is repeated multiple times now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will address.

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 pushed 20972a2 for ^^

@danielmitterdorfer danielmitterdorfer added enhancement Improves the status quo :misc Changes that don't affect users directly: linter fixes, test improvements, etc. labels Mar 14, 2018
@danielmitterdorfer danielmitterdorfer added this to the 0.10.0 milestone Mar 14, 2018
Remove code duplication through the use of common variables for the
elasticsearch downloaded artifact and its path.

Relates
elastic#440 (comment)
@danielmitterdorfer
Copy link
Member

Thanks @dliappis! LGTM.

@dliappis dliappis merged commit eb6f9ca into elastic:master Mar 14, 2018
@dliappis dliappis deleted the use-curl-retries branch March 14, 2018 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the status quo :misc Changes that don't affect users directly: linter fixes, test improvements, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants