-
Notifications
You must be signed in to change notification settings - Fork 136
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
Bug 1845202: Fix deadlock, fix service lag. #178
Bug 1845202: Fix deadlock, fix service lag. #178
Conversation
The only reason to lock the events array was to ensure that informer shutdown worked correctly (mostly for unit tests), and that when a nil event queue was found to stop distributing events to it. Instead wait for all processEvent() goroutines for each event queue to terminate before returning from Shutdown(), which handles stopping event distribution. Just leave the event queues open as there is no way to safely close now, because the shared informer may still be delivering events to the factories handler before it notices that the stop channel has been closed. This isn't a problem though, since we only ever stop the informer on exit anyway. Signed-off-by: Dan Williams <dcbw@redhat.com>
Save on some nbctl calls. Signed-off-by: Casey Callendrello <cdc@redhat.com>
@squeed: This pull request references Bugzilla bug 1841828, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@squeed: This pull request references Bugzilla bug 1841828, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test e2e-gcp-ovn |
@squeed: The specified target(s) for
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@squeed: This pull request references Bugzilla bug 1845202, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/hold |
/bugzilla refresh |
@squeed: This pull request references Bugzilla bug 1845202, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@squeed: This pull request references Bugzilla bug 1845202, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 6 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@squeed: This pull request references Bugzilla bug 1845202. The bug has been updated to no longer refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@squeed: This pull request references Bugzilla bug 1845202, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 6 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest |
aws terraform failure gcp passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pecameron, squeed The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
/retest Please review the full test history for this PR and help us cut down flakes. |
e2e-aws and e2e-gcp passed! Awesome. |
Does e2e-metal-ipi even use ovn-kubernetes? Also, I see that openshift/release#9514 was just merged, so it won't ever go green on this PR. |
/override ci/prow/e2e-metal-ipi |
@squeed: Overrode contexts on behalf of squeed: ci/prow/e2e-metal-ipi In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Yeah, confirmed, metal-ipi doesn't even install ovn-kubernetes. It's safe to override. |
@squeed: All pull requests linked via external trackers have merged: openshift/ovn-kubernetes#178. Bugzilla bug 1845202 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@abhat: cannot fork openshift/ovn-kubernetes: fork of openshift/ cannot show up on GitHub: timed out waiting for openshift-cherrypick-robot/ to appear on GitHub: status code 404 not one of [200], body: {"message":"Not Found","documentation_url":"https://developer.github.com/v3"} In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-4.4 |
@abhat: cannot fork openshift/ovn-kubernetes: fork of openshift/ cannot show up on GitHub: timed out waiting for openshift-cherrypick-robot/ to appear on GitHub: status code 404 not one of [200], body: {"message":"Not Found","documentation_url":"https://developer.github.com/v3"} In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@abhat: cannot fork openshift/ovn-kubernetes: fork of openshift/ cannot show up on GitHub: timed out waiting for openshift-cherrypick-robot/ to appear on GitHub: status code 404 not one of [200], body: {"message":"Not Found","documentation_url":"https://developer.github.com/v3"} In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-4.4 |
@stevekuznetsov: #178 failed to apply on top of branch "release-4.4":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Backport of two upstream PRs:
ovn-org/ovn-kubernetes#1378 - long service creation time.
ovn-org/ovn-kubernetes#1384 - informer deadlock
This fixes two issues I've seen during debugging tests.