-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Improve jdk download retries #48560
Improve jdk download retries #48560
Conversation
Look like the `--retry` argument is not always affective. Closes elastic#40531
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I left a few comments, mainly highlighting the need to emit the error at the end.
tar -C /opt -zxf /tmp/jdk.tar.gz && \ | ||
rm -Rf /tmp/jdk.tar.gz | ||
RUN for iter in {1..10}; do curl -L -s -S ${jdkUrl} | tar -C /opt -zx &&\ | ||
exit_code=0 && break || exit_code=\$? && echo "download error: retry \$iter in 10s" && sleep 10; done; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to emit the error here (similar to how we do it for yum install
later on) using:
\
(exit $exit_code)
otherwise as it is now, if all retries are exhausted it will continue to the next step.
Unrelated, but while testing this, I observed that (when running ./gradlew
with --info
) retry messages are shown as:
download error: retry $iter in 10s
instead of
download error: retry 1<or 2, 3 and so on> in 10s
I thought this was ok and tested in the past in the yum
section, but apparently \
shouldn't be used inside the double quoted section.
In other words the complete fixed command should be:
RUN for iter in {1..10}; do curl -L -s -S ${jdkUrl} | tar -C /opt -zx && \
exit_code=0 && break || exit_code=\$? && echo "download error: retry $iter in 10s" && sleep 10; done; \
(exit $exit_code)
RUN curl -L --retry 8 -s -S ${jdkUrl} --continue-at - --output /tmp/jdk.tar.gz && \ | ||
tar -C /opt -zxf /tmp/jdk.tar.gz && \ | ||
rm -Rf /tmp/jdk.tar.gz | ||
RUN for iter in {1..10}; do curl -L -s -S ${jdkUrl} | tar -C /opt -zx &&\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's have a space between &&
and \
for consistency i.e. && \
rm -Rf /tmp/jdk.tar.gz | ||
RUN for iter in {1..10}; do curl -L -s -S ${jdkUrl} | tar -C /opt -zx &&\ | ||
exit_code=0 && break || exit_code=\$? && echo "download error: retry \$iter in 10s" && sleep 10; done; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One new line is enough, I think we can remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM I suppose. Pretty annoying that --retry
doesn't do what we want. Did we discover what kind of errors we are encountering that aren't caught by --retry
?
Connection resets -- which is something that occasionally happens from the GitHub URL we are relying -- aren't being retried (others reporting the same here). For the record |
but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left a minor comment, there's no need for another review cycle.
RUN for iter in {1..10}; do curl -L -s -S ${jdkUrl} | tar -C /opt -zx && \ | ||
exit_code=0 && break || exit_code=\$? && echo "download error: retry $iter in 10s" && sleep 10; done; \ | ||
(exit $exit_code) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(super) nit: we could remove the 4 space characters in line 22 (so that there's just a newline).
Good point and additionally, my earlier comment attempts to show that there are other errors, like connection resets, that |
Look like the
--retry
argument is not always effective.Closes #40531