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.

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

Pass ListOptions to List in ListWatch. #18080

Merged
merged 1 commit into from
Dec 9, 2015

Conversation

wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Dec 2, 2015

Ref #15945

This will allow passing "0" resourceVersion to List() operation in our frameworks (Reflector, Informer, etc.)

@wojtek-t wojtek-t self-assigned this Dec 2, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 2, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 2, 2015

GCE e2e test build/test passed for commit d87024d9346ae30fcf02020a2bb056bd500af03d.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2015
@wojtek-t wojtek-t force-pushed the list_options_in_listwatch branch from d87024d to 344c5ad Compare December 3, 2015 10:36
@wojtek-t wojtek-t removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2015
@wojtek-t wojtek-t force-pushed the list_options_in_listwatch branch from 344c5ad to 34f35b0 Compare December 3, 2015 10:40
@wojtek-t wojtek-t changed the title [WIP[Do NOT review]Pass ListOptions to List in ListWatch. Pass ListOptions to List in ListWatch. Dec 3, 2015
@wojtek-t wojtek-t assigned lavalamp and unassigned wojtek-t Dec 3, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 3, 2015

GCE e2e test build/test passed for commit 344c5ad111edceeda0da8ff699f0529265163a86.

@k8s-bot
Copy link

k8s-bot commented Dec 3, 2015

GCE e2e test build/test passed for commit 34f35b081e833e269b31d329d542092645f9ed59.

@wojtek-t wojtek-t closed this Dec 3, 2015
@wojtek-t wojtek-t reopened this Dec 3, 2015
@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 3, 2015
@lavalamp
Copy link
Member

lavalamp commented Dec 3, 2015

LGTM, thanks for cleanup!

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 3, 2015

LGTM, thanks for cleanup!

Thanks - yeah - we have much more consistent interfaces now...

@k8s-github-robot
Copy link

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@k8s-bot
Copy link

k8s-bot commented Dec 5, 2015

GCE e2e test build/test passed for commit 34f35b081e833e269b31d329d542092645f9ed59.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e build/test failed for commit 34f35b081e833e269b31d329d542092645f9ed59.

@wojtek-t wojtek-t force-pushed the list_options_in_listwatch branch from 34f35b0 to b0fcb5a Compare December 7, 2015 10:54
@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 7, 2015

trivial rebase (there was a new call with old syntax).

@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Dec 7, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e build/test failed for commit b0fcb5a.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 7, 2015

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e build/test failed for commit b0fcb5a.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 7, 2015

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e test build/test passed for commit b0fcb5a.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e build/test failed for commit b0fcb5a.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 8, 2015

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e build/test failed for commit b0fcb5a.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 8, 2015

cluster didn't started

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e build/test failed for commit b0fcb5a.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 8, 2015

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e build/test failed for commit b0fcb5a.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 9, 2015

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e build/test failed for commit b0fcb5a.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 9, 2015

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e build/test failed for commit b0fcb5a.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 9, 2015

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e test build/test passed for commit b0fcb5a.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e test build/test passed for commit b0fcb5a.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e build/test failed for commit b0fcb5a.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 9, 2015

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e test build/test passed for commit b0fcb5a.

@wojtek-t
Copy link
Member Author

wojtek-t commented Dec 9, 2015

Manually merging to unblock different experiments.

wojtek-t added a commit that referenced this pull request Dec 9, 2015
@wojtek-t wojtek-t merged commit a915b8b into kubernetes:master Dec 9, 2015
@wojtek-t wojtek-t deleted the list_options_in_listwatch branch December 15, 2015 14:00
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Jan 24, 2018
Automatic merge from submit-queue (batch tested with PRs 17976, 17195, 18093, 18080, 17922).

UPSTREAM: 58107: Fix quota controller worker deadlock

The resource quota controller worker pool can deadlock when:

* Worker goroutines are idle waiting for work from queues
* The Sync() method detects discovery updates to apply

The problem is workers acquire a read lock while idle, making write lock
acquisition dependent upon the presence of work in the queues.

The Sync() method blocks on a pending write lock acquisition and won't unblock
until every existing worker processes one item from their queue and releases
their read lock. While the Sync() method's lock is pending, all new read lock
acquisitions will block; if a worker does process work and release its lock, it
will then become blocked on a read lock acquisition; they become blocked on
Sync(). This can easily deadlock all the workers processing from one queue while
any workers on the other queue remain blocked waiting for work.

Fix the deadlock by refactoring workers to acquire a read lock *after* work is
popped from the queue. This allows writers to get locks while workers are idle,
while preserving the worker pause semantics necessary to allow safe sync.

Origin-commit: 3a20d5947b17284e56f828f9b40380446cb04ed0
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Jan 25, 2018
Automatic merge from submit-queue (batch tested with PRs 17976, 17195, 18093, 18080, 17922).

UPSTREAM: 58107: Fix quota controller worker deadlock

The resource quota controller worker pool can deadlock when:

* Worker goroutines are idle waiting for work from queues
* The Sync() method detects discovery updates to apply

The problem is workers acquire a read lock while idle, making write lock
acquisition dependent upon the presence of work in the queues.

The Sync() method blocks on a pending write lock acquisition and won't unblock
until every existing worker processes one item from their queue and releases
their read lock. While the Sync() method's lock is pending, all new read lock
acquisitions will block; if a worker does process work and release its lock, it
will then become blocked on a read lock acquisition; they become blocked on
Sync(). This can easily deadlock all the workers processing from one queue while
any workers on the other queue remain blocked waiting for work.

Fix the deadlock by refactoring workers to acquire a read lock *after* work is
popped from the queue. This allows writers to get locks while workers are idle,
while preserving the worker pause semantics necessary to allow safe sync.

Origin-commit: 3a20d5947b17284e56f828f9b40380446cb04ed0
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Feb 27, 2018
Automatic merge from submit-queue (batch tested with PRs 17976, 17195, 18093, 18080, 17922).

UPSTREAM: 58107: Fix quota controller worker deadlock

The resource quota controller worker pool can deadlock when:

* Worker goroutines are idle waiting for work from queues
* The Sync() method detects discovery updates to apply

The problem is workers acquire a read lock while idle, making write lock
acquisition dependent upon the presence of work in the queues.

The Sync() method blocks on a pending write lock acquisition and won't unblock
until every existing worker processes one item from their queue and releases
their read lock. While the Sync() method's lock is pending, all new read lock
acquisitions will block; if a worker does process work and release its lock, it
will then become blocked on a read lock acquisition; they become blocked on
Sync(). This can easily deadlock all the workers processing from one queue while
any workers on the other queue remain blocked waiting for work.

Fix the deadlock by refactoring workers to acquire a read lock *after* work is
popped from the queue. This allows writers to get locks while workers are idle,
while preserving the worker pause semantics necessary to allow safe sync.

Origin-commit: 3a20d5947b17284e56f828f9b40380446cb04ed0
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants