XComp commented on code in PR #23528:
URL: https://github.com/apache/flink/pull/23528#discussion_r1368316329
##########
flink-end-to-end-tests/test-scripts/common_kubernetes.sh:
##########
@@ -87,6 +101,17 @@ function setup_kubernetes_for_linux {
sudo sysctl fs.protected_regular=0
}
+function download_circtl {
Review Comment:
Out of curiosity: Is there a reason why you moved this logic into its own
function. You didn't do it for the other two artifacts..
##########
flink-end-to-end-tests/test-scripts/common_kubernetes.sh:
##########
@@ -87,6 +101,17 @@ function setup_kubernetes_for_linux {
sudo sysctl fs.protected_regular=0
}
+function download_circtl {
+ local arch=$(get_arch)
+ local crictl_version crictl_archive arch
+ crictl_version="v1.24.2"
+ crictl_archive="crictl-$crictl_version-linux-${arch}.tar.gz"
+
download_circtl_url="https://github.com/kubernetes-sigs/cri-tools/releases/download/${crictl_version}/${crictl_archive}";
+
+ wget -nv ${download_circtl_url}
+ return $?
Review Comment:
The return call is obsolete here. The function would fail if the previous
`wget` fails.
##########
flink-end-to-end-tests/test-scripts/common_kubernetes.sh:
##########
@@ -50,17 +59,22 @@ function setup_kubernetes_for_linux {
if ! [ -x "$(command -v minikube)" ]; then
echo "Installing minikube $MINIKUBE_VERSION ..."
- 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 ${RETRY_COUNT} ${RETRY_BACKOFF_TIME} "curl --fail -Lo
minikube ${download_minikube_url}"; then
+ echo "ERROR: Could not download minikube. Aborting..."
+ exit 1
+ fi
+ chmod +x minikube && sudo mv minikube /usr/bin/minikube
fi
# conntrack is required for minikube 1.9 and later
sudo apt-get install conntrack
# crictl is required for cri-dockerd
- local crictl_version crictl_archive
- crictl_version="v1.24.2"
- crictl_archive="crictl-$crictl_version-linux-${arch}.tar.gz"
- wget -nv
"https://github.com/kubernetes-sigs/cri-tools/releases/download/${crictl_version}/${crictl_archive}";
+ if ! retry_times ${RETRY_COUNT} ${RETRY_BACKOFF_TIME} download_circtl; then
+ echo "ERROR: Could not download crictl. Aborting..."
+ exit 1
+ fi
+
sudo tar zxvf ${crictl_archive} -C /usr/local/bin
rm -f ${crictl_archive}
Review Comment:
There are a few other artifact downloads below this line. Shall we create a
generic download and retry function? We could align the tool for downloading
(`curl` vs `wget`) there as well.
##########
flink-end-to-end-tests/test-scripts/common_kubernetes.sh:
##########
@@ -21,26 +21,35 @@ source "$(dirname "$0")"/common.sh
source "$(dirname "$0")"/common_docker.sh
CONTAINER_SCRIPTS=${END_TO_END_DIR}/test-scripts/container-scripts
-MINIKUBE_START_RETRIES=3
-MINIKUBE_START_BACKOFF=5
+RETRY_COUNT=3
+RETRY_BACKOFF_TIME=5
RESULT_HASH="e682ec6622b5e83f2eb614617d5ab2cf"
MINIKUBE_VERSION="v1.28.0"
NON_LINUX_ENV_NOTE="****** Please start/stop minikube manually in non-linux
environment. ******"
+function get_arch() {
+ if [[ `uname -i` == 'aarch64' ]]; then
+ local arch='arm64'
+ else
+ local arch='amd64'
+ fi
+ echo "${arch}"
Review Comment:
```suggestion
echo 'arm64'
else
echo 'amd64'
fi
```
nit: we don't need the local variable here
##########
flink-end-to-end-tests/test-scripts/common_kubernetes.sh:
##########
@@ -87,6 +101,17 @@ function setup_kubernetes_for_linux {
sudo sysctl fs.protected_regular=0
}
+function download_circtl {
+ local arch=$(get_arch)
+ local crictl_version crictl_archive arch
+ crictl_version="v1.24.2"
Review Comment:
Moving the version declaration further away from the Minikube installation
might not be what we want. Because the versions are depending on each other: If
we need to change the Minikube version, we might also need to change `crictl`
and/or the `cri-dockerd` version. Having such variables spread over a script
(or even different scripts) increases the chance on missing a version.
So in this sense, it might make sense to reconsider the code reorg here.
Maybe, we should collect the related version variables next to
`MINIKUBE_VERSION` in the header of the script. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
