Skip to content

Add sleep to EtcdTestServer Terminate. #19187

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
Merged
merged 1 commit into from
Jan 4, 2016
Merged

Add sleep to EtcdTestServer Terminate. #19187

merged 1 commit into from
Jan 4, 2016

Conversation

wojtek-t
Copy link
Member

Hack to help with #18928, we should write a proper fix ASAP. @xiang90

@k8s-github-robot
Copy link

Labelling this PR as size/XS

@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 30, 2015
@wojtek-t wojtek-t assigned wojtek-t and unassigned thockin Dec 30, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 30, 2015

GCE e2e test build/test passed for commit 4e08c8ad64b6fef094cf3ee700cef1bb2b7c7d63.

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

8 similar comments
@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t wojtek-t force-pushed the add_sleep_in_etcd_server branch from 4e08c8a to e41d66f Compare December 31, 2015 08:53
@k8s-bot
Copy link

k8s-bot commented Dec 31, 2015

GCE e2e test build/test passed for commit e41d66f0e207f8d32268b80e8d0d1c227663cc48.

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

4 similar comments
@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

wojtek-t commented Jan 1, 2016

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

wojtek-t commented Jan 1, 2016

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

wojtek-t commented Jan 1, 2016

It seems that we need to rebuild the test image too...

@wojtek-t
Copy link
Member Author

wojtek-t commented Jan 1, 2016

@k8s-bot unit test this please

5 similar comments
@wojtek-t
Copy link
Member Author

wojtek-t commented Jan 1, 2016

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

wojtek-t commented Jan 1, 2016

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

wojtek-t commented Jan 1, 2016

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

wojtek-t commented Jan 2, 2016

@k8s-bot unit test this please

@wojtek-t
Copy link
Member Author

wojtek-t commented Jan 2, 2016

@k8s-bot unit test this please

@k8s-bot
Copy link

k8s-bot commented Jan 3, 2016

GCE e2e build/test failed for commit c8ad311.

@wojtek-t
Copy link
Member Author

wojtek-t commented Jan 3, 2016

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Jan 3, 2016

GCE e2e test build/test passed for commit c8ad311.

@gmarek gmarek assigned gmarek and unassigned wojtek-t Jan 4, 2016
@gmarek gmarek added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2016
@gmarek gmarek changed the title [WIP][Do NOT merge] Add sleep Add sleep to EtcdTestServer Terminate. Jan 4, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@timothysc
Copy link
Member

@gmarek @xiang90 @wojtek-t

Back from PTO... So this is the shutdown synchronization with the test server, does anyone know why jenkins is a special case here?

@gmarek
Copy link
Contributor

gmarek commented Jan 4, 2016

My best guess is that our jenkins machines are loaded way more than any workstation we use (and I doubt anyone actually tried to run it when machine is heavily loaded).

@k8s-bot
Copy link

k8s-bot commented Jan 4, 2016

GCE e2e build/test failed for commit c8ad311.

@xiang90
Copy link
Contributor

xiang90 commented Jan 4, 2016

@timothysc My guess is that jenkins is slow. What we did is to cancel all pending requests and return to make sure there is no in-flight requests. However, this is not robust since the request might already have arrived the server side and it is not cancellable at the moment. When this happens, there is a data race. The possibility of this increases when the latency between request+response increases.

To solve this @wojtek-t sleeps a few here to wait for server finishing all pending requests. We can do better by accounting pending TCP connection at client side and waits for all TCP level shutdown. But I am not sure if it worth the effort. We should not wait for TCP shutdown in reality anyway.

@gmarek
Copy link
Contributor

gmarek commented Jan 4, 2016

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented Jan 4, 2016

GCE e2e build/test failed for commit c8ad311.

@gmarek
Copy link
Contributor

gmarek commented Jan 4, 2016

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented Jan 4, 2016

GCE e2e test build/test passed for commit c8ad311.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@timothysc
Copy link
Member

We have no barrier synchronization on test-server shutdown so we're band-aiding with sleep calls. @gmarek could someone pastebin the stack calls. Or provide a link which I can access to view?

@gmarek
Copy link
Contributor

gmarek commented Jan 4, 2016

They are in the issue mentioned in the first comment: #18928

@k8s-bot
Copy link

k8s-bot commented Jan 4, 2016

GCE e2e test build/test passed for commit c8ad311.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@gmarek gmarek mentioned this pull request Jan 4, 2016
Closed
@gmarek
Copy link
Contributor

gmarek commented Jan 4, 2016

Merging to deflake tests.

@gmarek gmarek closed this Jan 4, 2016
@gmarek gmarek reopened this Jan 4, 2016
gmarek pushed a commit that referenced this pull request Jan 4, 2016
Merge pull request #19187 from wojtek-t/add_sleep_in_etcd_server
4606171 
Add sleep to EtcdTestServer Terminate.
@gmarek gmarek merged commit 4606171 into kubernetes:master Jan 4, 2016
@k8s-bot
Copy link

k8s-bot commented Jan 4, 2016

GCE e2e test build/test passed for commit c8ad311.

@gmarek gmarek mentioned this pull request Jan 7, 2016
Closed
@wojtek-t wojtek-t deleted the add_sleep_in_etcd_server branch February 11, 2016 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@gmarek gmarek

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@wojtek-t @k8s-github-robot @k8s-bot @timothysc @gmarek @xiang90 @thockin @k8s-teamcity-mesosphere @googlebot