-
Notifications
You must be signed in to change notification settings - Fork 39.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Retry pod update on version conflict error in e2e test. #7297
Conversation
|
||
expectNoError(waitForPodRunning(c, pod.Name)) | ||
|
||
By("verifying the updated pod is in kubernetes") | ||
pods, err = podClient.List(labels.SelectorFromSet(labels.Set(map[string]string{"time": value})), fields.Everything()) | ||
Expect(len(pods.Items)).To(Equal(1)) | ||
fmt.Println("pod update OK") | ||
By("pod update OK") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a Logf here instead of By()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, done.
All feedback addressed, and tidied up a few other related things hanging around from the previous version of this test. PTAL @lavalamp |
} | ||
if errors.IsConflict(err) { | ||
Logf("Conflicting update to pod, re-get and re-update: %v", err) | ||
pod, err = podClient.Get((*pod).Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing the update/get in this order gives an entire 5 seconds for someone to modify the pod and force you into another loop... I'd recommend changing the order back. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry, and I just remembered I forgot to change the time periods.
Let me change those and get back to you.
OK, I reduced the retry interval to 500ms for 30s, and changed the code so that the first update attempt (which usually succeeds) uses the original return value of the Create call. Only retries do another Get. Hope you like it. I could fairly easily be convinced to just do a Get before every Update to get rid of the "if" statement - but that would add a completely unnecessary API call in the most common case. |
LGTM |
…pdate-retry Retry pod update on version conflict error in e2e test.
Fixes #7013.