Performance bot wait for binary before run #9988
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @daehyeok. 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/test-infra repository. |
k8s-ci-robot
added
the
size/M
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Hey @daehyeok thank you for taking this issue on. I've left a few comments, but overall I think this can be simplified considerably. Instead of writing new functions, we can just set a timer on b.Download() in mkcmp, and continuously retry for up to 10 minutes. WDYT?
pkg/minikube/perf/binary.go
Outdated
| obj := client.Bucket(bucket).Object(fmt.Sprintf("%d/minikube-%s-amd64", b.pr, runtime.GOOS)) | ||
| if _, err := obj.Attrs(ctx); err != nil { | ||
| return fmt.Errorf("minikube binary for pr %v does not exist in bucket", b.pr) | ||
| obj, err := newObjectFromStorage(ctx, client, b.pr) |
There was a problem hiding this comment.
Why was this code moved into a new function? It doesn't seem like there is any difference between newObjectFromStorage and the code that was here before
There was a problem hiding this comment.
I just want to reuse code for IsExist function.
pkg/minikube/perf/binary.go
Outdated
| pr = strings.TrimPrefix(pr, prPrefix) | ||
| // try to convert to int | ||
| i, err := strconv.Atoi(pr) | ||
| i, err := extractPRIDFromPath(pr) |
There was a problem hiding this comment.
We shouldn't need to extract the PR ID from path, since it is passed into the function as a string
There was a problem hiding this comment.
Maybe it's bad naming for function. It's convert pr number to integer(from path string). Original code also convert it for Binary struct, I want to reuse too for new function.
b := &Binary{
path: localMinikubePath(i),
pr: i,
}
pkg/minikube/perf/binary.go
Outdated
| obj := client.Bucket(bucket).Object(fmt.Sprintf("%d/minikube-%s-amd64", b.pr, runtime.GOOS)) | ||
| if _, err := obj.Attrs(ctx); err != nil { | ||
| return fmt.Errorf("minikube binary for pr %v does not exist in bucket", b.pr) | ||
| obj, err := newObjectFromStorage(ctx, client, b.pr) |
There was a problem hiding this comment.
I just want to reuse code for IsExist function.
pkg/minikube/perf/binary.go
Outdated
| pr = strings.TrimPrefix(pr, prPrefix) | ||
| // try to convert to int | ||
| i, err := strconv.Atoi(pr) | ||
| i, err := extractPRIDFromPath(pr) |
There was a problem hiding this comment.
Maybe it's bad naming for function. It's convert pr number to integer(from path string). Original code also convert it for Binary struct, I want to reuse too for new function.
b := &Binary{
path: localMinikubePath(i),
pr: i,
}
There was a problem hiding this comment.
Hey @daehyeok sorry I wasn't clear.
We try to download the binary here:
minikube/pkg/minikube/perf/binary.go
Line 112 in 92511de
and we fail if it doesn't work. We could add a retry here, to continuously attempt to download the binary once every minute for 10 minutes. I think 10 minutes is a safe bet because jenkins take around 4 minutes to build and upload the binaries.
We shouldn't need any additional code.
|
@priyawadhwa oh sorry i misunderstood. I thought you want to mkcmp return error but still retying in |
daehyeok
force-pushed
the
wait_binary
branch
from
67bc11d
to
f3a5ac4
Compare
k8s-ci-robot
added
size/XS
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: daehyeok, tstromberg 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 |
k8s-ci-robot
added
the
approved
fixes #9697