Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed issue with the density test failing after a successful run because... #5387

Merged
merged 1 commit into from
Mar 12, 2015
Merged

Fixed issue with the density test failing after a successful run because... #5387

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions test/e2e/density.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,14 @@ var _ = PDescribe("Density", func() {
})

AfterEach(func() {
// Remove any remaining pods from this test
DeleteRC(c, ns, RCName)
// Remove any remaining pods from this test if the
// replication controller still exists and the replica count
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could leak RCs/pods if there's an error communicating with the apiserver -- is that something that could break the other tests?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

DeleteRC(c, ns, RCName)
}
})

It("should allow starting 100 pods per node", func() {
Expand Down