-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[FLINK-31180] [Test Infrastructure] Fail early when installing minikube and check whether we can retry #23497
Conversation
@@ -54,6 +54,17 @@ function setup_kubernetes_for_linux { | |||
chmod +x minikube && sudo mv minikube /usr/bin/minikube | |||
fi | |||
|
|||
if ! [ -x "$(command -v minikube)" ]; then |
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.
What is the idea behind using https://kubernetes.oss-cn-hangzhou.aliyuncs.com/minikube
instead of https://storage.googleapis.com/minikube/releases/
?
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.
Sorry, I misunderstood before, I understood to change another download address
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.
The PR description mentions a retry, but I cannot locate any.
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 @afedulov for the review. Modified to Attempt restart when the download fails
@@ -48,10 +48,19 @@ function setup_kubernetes_for_linux { | |||
sudo rm "$(which minikube)" | |||
fi | |||
|
|||
# Retry loop download minikube. | |||
local MINIKUBE_DOWNLOAD_RETRY_COUNT=2 | |||
for i in $(seq 1 ${MINIKUBE_DOWNLOAD_RETRY_COUNT}); do |
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.
I mentioned it in the other PR already, but just for the sake of transparency: You could use retry_times (see apache/flink:flink-end-to-end-tests/test-scripts/common.sh:883ff) instead of reimplementing the logic.
Would that work?
Thanks @XComp for the review. |
curl -Lo minikube https://storage.googleapis.com/minikube/releases/$MINIKUBE_VERSION/minikube-linux-$arch && \ | ||
chmod +x minikube && sudo mv minikube /usr/bin/minikube | ||
download_minikube_url="https://storage.googleapis.com/minikube/releases/$MINIKUBE_VERSION/minikube-linux-$arch" | ||
if ! retry_times ${MINIKUBE_START_RETRIES} ${MINIKUBE_START_BACKOFF} "curl -Lo minikube ${download_minikube_url}"; then |
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.
have you tried the loop? Looks like curl doesn't return an error code in case of a failure. 🤔 hint: --fail
might be what we want according to man curl
@@ -48,10 +48,15 @@ function setup_kubernetes_for_linux { | |||
sudo rm "$(which minikube)" | |||
fi | |||
|
|||
# Retry loop download minikube. |
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.
This comment doesn't add any additional value. The code below makes it quite clear that retries are done. I guess we can remove the comment again.
Thanks for addressing my comment. It looks like the retry mechanism doesn't work properly, yet. Please see my comments above. |
…be and check whether we can retry
…be and check whether we can retry
Ok, sounds good to me. I'm gonna close this PR in favor of FLINK-32107 and PR #23528 |
What is the purpose of the change
Fail early when installing minikube and check whether we can retry.
Brief change log
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation