Fixed issue with the density test failing after a successful run because... #5387
Conversation
…use of a failure to cleanup kubernetes#5385
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/. If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits. Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name. |
|
LGTM, waiting on CI to go green |
| // isn't 0. This means the controller wasn't cleaned up | ||
| // during the test so clean it up here | ||
| rc, err := c.ReplicationControllers(ns).Get(RCName) | ||
| if err == nil && rc.Spec.Replicas != 0 { |
There was a problem hiding this comment.
This could leak RCs/pods if there's an error communicating with the apiserver -- is that something that could break the other tests?
There was a problem hiding this comment.
If there's a problem communicating with the apiserver then there would likely be an rc and its pods still in the system. That could definitely impact other tests. I'm not sure there's much we can do though. If the apiserver is unresponsive I'm not sure how we clean up after the test.
There was a problem hiding this comment.
Retries would be the typical answer, but I haven't checked how much we use retries in our e2e tests. If you give a request 3 tries to succeed instead of 1, flakes are less likely.
There was a problem hiding this comment.
A retry might work but without a real world case to test against finding how many attempts and how long to wait between, or even if it will help, are all guesses. I can add retries but I wouldn't be able to verify they'd actually solve a problem. I'm also a little reluctant to cover up a communication problem. The apiserver is going to have to be responsive under load.
I don't recall there being a lot of retries for operations in the e2e tests. That is also probably because not many would stress the system to the point of causing communication timeouts like this test suite could.
ATM this test is disabled and won't be run unless explicitly enabled because of the nature of the test. This really belongs in a performance test suite.
satnam6502
added
the
lgtm
Fixed issue with the density test failing after a successful run because...
... of
a failure to cleanup #5385