Move linux/darwin binaries into cache/binaries folder (#21179) #21664
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @henry3260. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/S
|
Can one of the admins verify this patch? |
pkg/minikube/download/binary.go
Outdated
| func Binary(binary, version, osName, archName, binaryURL string) (string, error) { | ||
| targetDir := localpath.MakeMiniPath("cache", osName, archName, version) | ||
| // targetDir := localpath.MakeMiniPath("cache", osName, archName, version) | ||
| targetDir := localpath.MakeMiniPath("cache", "binaries", osName, archName, version) |
There was a problem hiding this comment.
I think we should use "bin", it is more consistent with ~/.minikube/bin and the rest of the world.
We are using "binaries" internally which would nice to change also to bin but it may be harder since we may code using it. Since this cache directory is new we don't have backward compatibility concerns.
There was a problem hiding this comment.
I think we should use "bin", it is more consistent with ~/.minikube/bin and the rest of the world.
We are using "binaries" internally which would nice to change also to bin but it may be harder since we may code using it. Since this cache directory is new we don't have backward compatibility concerns.
Thanks for the suggestion! That makes sense, I’ll update it to use bin for consistency.
henry3260
force-pushed
the
feature/move-cache
branch
from
d3e6101 to
2e7c8cf
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @medyagh |
|
|
||
| // check that the bin directory was created | ||
| targetDir := filepath.Join( | ||
| os.Getenv("HOME"), ".minikube", "cache", "bin", "linux", "amd64", "v1.31.0", |
There was a problem hiding this comment.
~/.minikube is the default, but if MINIKUBE_HOME is defined it will be used. There is a helper function to return the right path (but I don't remember the function name). Check what other tests are doing.
k8s-ci-robot
removed
the
lgtm
| // check that the bin directory was created | ||
| targetDir := filepath.Join( | ||
| os.Getenv("HOME"), ".minikube", "cache", "bin", "linux", "amd64", "v1.31.0", | ||
| localpath.MiniPath(), "cache", "bin", "linux", "amd64", "v1.31.0", |
There was a problem hiding this comment.
Looks good, to make it even better squash this commit into the first one. Having this in git history is not helpful.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
henry3260
force-pushed
the
feature/move-cache
branch
from
aa3b2e4 to
fca5789
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
updated! |
It is still outdated:
|
Thanks for pointing that out! I've corrected it now. |
|
@henry3260 this change broke few test: Please run integration tests locally and fix the failing tests. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: henry3260 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
It looks like the current logic no longer downloads all KubernetesReleaseBinaries locally. Before I made any changes, I ran the integration tests on main and saw similar errors: So the previous behavior already failed to download kubeadm and kubelet locally. Only kubectl seems to be downloaded on the host. This means the test assumption that all binaries are always cached locally is outdated. I think we need to update the tests to only check for binaries that Minikube actually downloads on the host or to conditionally check depending on the runtime driver. |
henry3260
force-pushed
the
feature/move-cache
branch
from
fca5789 to
0ce7767
Compare
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
added
size/M
|
@henry3260: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Do you think we should introduce a helper function like |
|
It is possible that the test was broken before this change. To be sure please share complete test command and output with master showing the failures. We meed to fix the test before we change the code. Otherwise we may break things. I think kubeadm is cached only when disabling preloads. By default preloads are enabled and kubeadm is onstalled from the preloaded tarball. Maybe the test does not use the right flags. Let’s open another issue for this test and work on fixing it in another PR. This PR can wait until we fix the test. |
|
kvm2 driver with docker runtime Times for minikube start: 44.2s 45.0s 42.3s 42.5s 43.9s Times for minikube (PR 21664) ingress: 21.3s 16.8s 17.3s 15.8s 17.4s docker driver with docker runtime Times for minikube start: 22.7s 21.8s 22.6s 23.2s 23.8s Times for minikube ingress: 11.8s 12.7s 10.7s 12.7s 13.7s docker driver with containerd runtime Times for minikube ingress: 20.2s 20.2s 22.2s 20.2s 22.2s Times for minikube start: 19.2s 25.1s 21.6s 24.2s 21.4s |
|
@henry3260 looking in the test is does not disable preloads, and it checks if reload exists after starting minikube. If the preload tarball exists it does check for binaries. If preload tarball does not exit, it fallback to checking the binaries. This test has few problems:
preloadExists := false
t.Run("preload-exists", func(t *testing.T) {
// skip for none, as none driver does not have preload feature.
if NoneDriver() {
t.Skip("None driver does not have preload")
}
// Driver does not matter here, since the only exception is none driver,
// which cannot occur here.
if !download.PreloadExists(v, containerRuntime, "docker", true) {
t.Skip("No preload image")
}
// Just make sure the tarball path exists
if _, err := os.Stat(download.TarballPath(v, containerRuntime)); err != nil {
t.Errorf("failed to verify preloaded tarball file exists: %v", err)
}
preloadExists = true
})
t.Run("cached-images", func(t *testing.T) {
// skip verify for cache images if --driver=none
if NoneDriver() {
t.Skip("None driver has no cache")
}
if preloadExists {
t.Skip("Preload exists, images won't be cached")
}
imgs, err := images.Kubeadm("", v)
if err != nil {
t.Errorf("failed to get kubeadm images for %v: %+v", v, err)
}
for _, img := range imgs {
pathToImage := []string{localpath.MiniPath(), "cache", "images", runtime.GOARCH}
img = strings.Replace(img, ":", "_", 1) // for example kube-scheduler:v1.15.2 --> kube-scheduler_v1.15.2
imagePath := strings.Split(img, "/") // changes "gcr.io/k8s-minikube/storage-provisioner_v5" into ["gcr.io", "k8s-minikube", "storage-provisioner_v5"] to match cache folder structure
pathToImage = append(pathToImage, imagePath...)
fp := filepath.Join(pathToImage...)
_, err := os.Stat(fp)
if err != nil {
t.Errorf("expected image file exist at %q but got error: %v", fp, err)
}
}
})We need is to test all cases in all runs:
Note that we also have preload_test.go, which tests --preload=false. But it does not test downloaded binaries. It seem to be focused on something else, not sure how it is related to preloads. |
I opened the issue for this #21805 |
|
Here are the number of top 10 failed tests in each environments with lowest flake rate.
Besides the following environments also have failed tests: To see the flake rates of all tests by environment, click here. |
6 participants
This PR addresses issue #21179 by reorganizing the cache folder structure for binaries.
Before
~/.minikube/cache/linux~/.minikube/cache/darwinAfter
binariessubfolder:~/.minikube/cache/bin/linux~/.minikube/cache/bin/darwinWhy
Fixes #21179
Screenshot (Before)
Screenshot (After)