preload: download from github when gcs not available#21605
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: medyagh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/ok-to-test |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Why do we need a fallback? Why not switch to github?
Why the user should care about this? we need location all resources. It makes sense to add a config in case user cannot access GCS or GitHub, and want to download resources from local server. For example disconnected environment or dealing with local restrictions (e.g. China). |
nirs
left a comment
There was a problem hiding this comment.
Adding a GitHub fallback add unneeded complexity. Can we have a config option to use either GCS or GitHub and simplify the code to use the configured source?
| if !existence || checkCalled { | ||
| t.Errorf("Expected preload to exist and no check to be performed. Existence: %v, Check: %v", existence, checkCalled) | ||
| if !existence || gcsCheckCalls != 0 || ghCheckCalls != 0 { | ||
| t.Errorf("Expected preload to exist and no checks to be performed. Existence: %v, GCS Calls: %d, GH Calls: %d", existence, gcsCheckCalls, ghCheckCalls) |
There was a problem hiding this comment.
These tests are too intrusive. We need to have more high level tests that test the system behavior instead of the implementation details.
The issues exist before this change but hits change makes it worse.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6862fa4 to
a76f8ad
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/retest-this-please |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
nirs
left a comment
There was a problem hiding this comment.
I reviewed up to
preload: download from github when gcs not available
But it is impossible to review in this state - we have many partial or wrong commits fixes in later commits. Can you squash the changes to we have one commit for every correct logical change?
| @@ -276,10 +283,6 @@ func Preload(k8sVersion, containerRuntime, driverName string) error { | |||
| return errors.Wrapf(err, "download failed: %s", url) | |||
There was a problem hiding this comment.
We use this in line 279:
} else if checksum != nil {
// add URL parameter for go-getter to automatically verify the checksum
url += fmt.Sprintf("?checksum=md5:%s", hex.EncodeToString(checksum))
}This will work only for GCH when we have md5, but for GitHub we have sha256 so the library handling download will fail to verify the checksum.
There was a problem hiding this comment.
are you sure this is whats happening ? I moved the logic into a helper func maybe that helps easier to see the flow
There was a problem hiding this comment.
The comment is correct for this commit. Later commit fixed this issue, but I review the commits, expecting that every commit is a correct single logical change.
It will be better to fix this commit to make this change easier to review and keep git history clean.
| default: | ||
| // this should never happen | ||
| klog.Fatalf("unknown preload source: %s", source) | ||
| return string(preloadSourceNone) |
There was a problem hiding this comment.
This locks wrong - if we get here preloadSourceLocal or preloadSourceNone we return "".
Should we return a "file:///path/to/preload" if the preload is local?
We need to cover all possible values (GCS, GH, Local, None). If we expect only GCS and GH here the function should panic (programmer error).
|
@medyagh: The following test 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. DetailsInstructions 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. |
|
kvm2 driver with docker runtime DetailsTimes for minikube start: 43.3s 42.1s 45.0s 44.4s 46.0s Times for minikube ingress: 15.8s 15.8s 15.3s 18.9s 15.8s docker driver with docker runtime DetailsTimes for minikube start: 22.3s 25.2s 23.0s 22.6s 22.8s Times for minikube (PR 21605) ingress: 12.7s 12.7s 13.6s 10.6s 13.6s docker driver with containerd runtime DetailsTimes for minikube ingress: 23.1s 39.1s 23.1s 29.1s 23.1s Times for minikube start: 20.0s 20.3s 20.8s 19.1s 20.3s |
|
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. |
Summary
that shaves off almost 1 second off minikube download
local testing
tested the current hack/preload code compiles with these changes
also tested
and worked fine.
Follow Up PRs
https://github.com/kubernetes-sigs/minikube-preloads (this will be doable once minikube-preloads can import this PR's code that checks that has the code for checking existance of preload in GCS)
possible todos after this PR:
#21638