try to deflake CR watches in tests #46536
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
approved
|
| @@ -127,6 +127,24 @@ func CreateNewCustomResourceDefinition(customResourceDefinition *apiextensionsv1 | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
|
|
|||
| // the REST storage isn't created until you hit the endpoint (the list does this), | |||
| // at which point the storage cacher for the CR starts asynchronously. | |||
There was a problem hiding this comment.
would it not be better to inject the delay at the point where we build the rest storage? we'd at least have the possibility of fixing the coordination between the list and the cache server-side in the future and removing the delay hack instead of making multiple clients add first-use special casing
There was a problem hiding this comment.
would it not be better to inject the delay at the point where we build the rest storage? we'd at least have the possibility of fixing the coordination between the list and the cache server-side in the future and removing the delay hack instead of making multiple clients add first-use special casing
No preference. If you want it there I can do it there.
| // at which point the storage cacher for the CR starts asynchronously. | ||
| // The REST APi list gets the RV of etcd, but the storage cacher's reflector's list | ||
| // gets a different RV because etcd can be touched in between. | ||
| // Later, you can issue a a watch with the REST apis list.RV and end up earlier than the storage cacher. |
sttts
added
release-note-none
|
Not pretty, but if it helps... |
|
switched to where we build the REST storage and start the cacher. @sttts ptal. |
| // REST API operations return like list use the RV of etcd, but the storage cacher's reflector's list | ||
| // can get a different RV because etcd can be touched in between the initial list operation (if that's what you're doing first) | ||
| // and the storage cache reflector starting. | ||
| // Later, you can issue a watch with the REST apis list.RV and end up earlier than the storage cacher. |
There was a problem hiding this comment.
now with fixed typo
|
@deads2k something is missing. forgot to push? |
|
LGTM, @sttts what were you expecting? |
|
@liggitt it's alright. Didn't notice the modified file changed. lgtm |
deads2k
added
the
lgtm
|
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
|
@k8s-bot pull-kubernetes-unit test this |
|
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
|
@k8s-bot pull-kubernetes-e2e-kops-aws test this |
|
flake fix, bumping priority. |
|
The flake needs to be fixed. |
|
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
|
@k8s-bot pull-kubernetes-e2e-kops-aws test this |
|
hey bot |
|
Automatic merge from submit-queue |
|
a test started at around 6pm still failed: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-test-go/6169/ |
Well that's a bummer. The issue is re-opened. |
Fixes #46446
I've added a comment trying to explain the reasoning in the code. Without being able to expose the RV of the cache, I can't think of a reliable way to do it. Even if you tried experimenting with a watch, it essentially does this since you'd be waiting to not get an error.