Add automatic arch and os selection for the deploy cni plugin #120312
Conversation
82f93a2
to
ba13736
Compare
|
/test pull-kubernetes-local-e2e |
|
This is not really very sig-network. It's more sig-whoever-maintains- |
hack/local-up-cluster.sh
Outdated
| ;; | ||
| *) | ||
| echo "Unsupported host arch. Must be x86_64, 386, arm, arm64, s390x or ppc64le." >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
|
|
||
| GO_OUT="${KUBE_ROOT}/_output/local/bin/${host_os}/${host_arch}" | ||
| GO_OUT="${KUBE_ROOT}/_output/local/bin/${HOST_OS}/${HOST_ARCH}" |
There was a problem hiding this comment.
What's the point of this renaming? Even if HOST_ARCH and HOST_OS are initially set, this code would not be reached if host OS or arch are not detected.
There was a problem hiding this comment.
If you want to use host os and arch values you should consider returning them from this function instead of changing global variables.
There was a problem hiding this comment.
What's the point of this renaming
The reason for setting HOST_ARCH and HOST_OS as global variables is that need to be referenced in the install_cni function
There was a problem hiding this comment.
If we want to pass variables using return values, how about the following?
+function detect_arch {
+ local host_arch
+
+ case "$(uname -m)" in
+ x86_64*)
+ host_arch=amd64
+ ;;
+ i?86_64*)
+ host_arch=amd64
+ ;;
+
+ ......
+
+ esac
+
+ echo -n "${host_arch}"
+}
+
+function detect_os {
+ local host_os
+
+ case "$(uname -s)" in
+ Darwin)
+ host_os=darwin
+ ;;
+
+ ......
+
+ esac
+
+ echo -n "${host_os}"
+}
function detect_binary {
+ host_arch=$(detect_arch)
+ host_os=$(detect_os)
- case "$(uname -s)" in
- Darwin)
- host_os=darwin
- ;;
- Linux)
-
- ......
-
- esac
GO_OUT="${KUBE_ROOT}/_output/local/bin/${host_os}/${host_arch}"
}
function install_cni {
+ host_arch=$(detect_arch)
+ cni_plugin_sha=CNI_PLUGINS_${host_arch^^}_SHA256SUM
+ cni_plugin_tarball="${CNI_PLUGINS_VERSION}/cni-plugins-linux-${host_arch}-${CNI_PLUGINS_VERSION}.tgz"
....
}There was a problem hiding this comment.
This looks better to me, thanks.
| CNI_PLUGINS_AMD64_SHA256SUM=${CNI_PLUGINS_AMD64_SHA256SUM:-"754a71ed60a4bd08726c3af705a7d55ee3df03122b12e389fdba4bea35d7dd7e"} | ||
| CNI_PLUGINS_ARM64_SHA256SUM=${CNI_PLUGINS_ARM64_SHA256SUM:-"86c4c866a01a8073ad14f6feec74de1fd63669786850c7be47521433f9570902"} | ||
| CNI_PLUGINS_ARM64_SHA256SUM=${CNI_PLUGINS_ARM64_SHA256SUM:-"de7a666fd6ad83a228086bd55756db62ef335a193d1b143d910b69f079e30598"} |
There was a problem hiding this comment.
providing checksums without any urls looks a bit unusual to me.
There was a problem hiding this comment.
It is obtained from https://github.com/containernetworking/plugins/releases/download/v1.3.0/cni-plugins-linux-arm64-v1.3.0.tgz.sha256, the previous one seems to be wrong
There was a problem hiding this comment.
Sorry for not being clear enough. You've removed lines with urls and checksums started to look strange because of that. Updating checksums is ok.
|
/triage accepted |
I think it's a cleanup, it helps us to be able to automate the installation of Arm64 or other arch cni binaries without modifying the local-up-cluster.sh script, but it also fixes an incorrect hash value. |
Thanks. I thought that it's a fix for arm64 arch. |
|
hack/local-up-cluster.sh is a developer only thing, there is no guarantees |
I've encountered this issue frequently in local debugging so I want to fix it, if we don't need it, I can close this PR and just fix the wrong hash value. |
sorry, I was trying to answer this other one comment :)
|
this job is failing on network problems, most probablt related to this PR |
I'm looking at this error, but according to my local tests, it existed before this PR was submitted. |
| @@ -1146,15 +1146,18 @@ function tolerate_cgroups_v2 { | |||
| } | |||
|
|
|||
| function install_cni { | |||
| cni_plugin_sha=CNI_PLUGINS_${CNI_TARGETARCH^^}_SHA256SUM | |||
There was a problem hiding this comment.
This is backward incompatible I think. It practically means ignoring CNI_TARGETARCH environment variable.
There was a problem hiding this comment.
The CNI_TARGETARCH variable is expected to be deleted and replaced by the host_arch
There was a problem hiding this comment.
Yes, and this would break developers workflows if they set CNI_TARGETARCH.
Even if we consider doing this we should at least mention it in release notes.
There was a problem hiding this comment.
Given the addition of the ability to automatically recognize the Arch of the CNI binary, I would prefer to remove CNI_TARGETARCH and add release note.
There was a problem hiding this comment.
I'd suggest to avoid breaking backwards compatibility. In this case it means only do automatic recognition when CNI_TARGETARCH is not set.
You are absolutely right, https://testgrid.k8s.io/conformance-all#local-up-cluster,%20master%20(dev) , this job has failing for a long time on these tests |
ba13736
to
f966e91
Compare
f966e91
to
c1a0aa0
Compare
|
/priority important-longterm |
|
LGTM label has been added. Git tree hash: 37f71a617644c9ed39a911184ca1354fa88e6848
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HirazawaUi, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Use local-up-cluster.sh on arm64 or other arch Linux to automatically download the cni binary of the corresponding arch.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This change will not affect our CI, it will only facilitate local debug using local-up-cluster.sh
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: