Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Apr 24, 2015

Fixes #7013.

@ghost ghost assigned lavalamp Apr 24, 2015
test/e2e/pods.go Outdated
Copy link
Contributor

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()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, done.

@ghost
Copy link
Author

ghost commented Apr 24, 2015

All feedback addressed, and tidied up a few other related things hanging around from the previous version of this test. PTAL @lavalamp

test/e2e/pods.go Outdated
Copy link
Contributor

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. :)

Copy link
Author

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.

@ghost
Copy link
Author

ghost commented Apr 24, 2015

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.

@lavalamp
Copy link
Contributor

LGTM

@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2015
ghost pushed a commit that referenced this pull request Apr 24, 2015
…pdate-retry

Retry pod update on version conflict error in e2e test.
@ghost ghost merged commit 73c81a2 into kubernetes:master Apr 24, 2015
@ghost ghost unassigned lavalamp Aug 12, 2015
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm "Looks good to me", indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

e2e test pod update test flaw

3 participants