Resolve race in db tests #9117
Merged
Resolve race in db tests #9117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When disconnecting a client due to an expired certificate, the monitor
was emitting the disconnection event before forcing the client to
disconnect. This led to races in tests that wait on the disonnection
event, where tests would:
1. wait for the disconnection event before proceeding
2. receive the disconnection event
2. take action based on the disconnection event, assuming that the
client was disconnected, when there is no guarantee that the
disconnection has actually happened yet.
This patch re-orders the disconnection and event broadcast, such that
the disconnection happens _first_, meaning that if a watcher receives
the doisconnection event, then the dosconnection has already been
attempted.
smallinsky
approved these changes
Nov 24, 2021
codingllama
approved these changes
Nov 24, 2021
|
FYI, fixed a few typos in the description. |
russjones
approved these changes
Nov 24, 2021
tcsc
added a commit
that referenced
this pull request
Dec 1, 2021
When disconnecting a client due to an expired certificate, the monitor
was emitting the disconnection event before forcing the client to
disconnect. This led to races in tests that wait on the disconnection
event, where tests would:
1. wait for the disconnection event before proceeding
2. receive the disconnection event
3. take action based on the disconnection event, assuming that the
client was disconnected, when there is no guarantee that the
disconnection has actually happened yet.
This patch re-orders the disconnection and event broadcast, such that
the disconnection happens first, meaning that if a watcher receives
the disconnection event, then the disconnection has already been
attempted.
tcsc
added a commit
that referenced
this pull request
Dec 1, 2021
When disconnecting a client due to an expired certificate, the monitor
was emitting the disconnection event before forcing the client to
disconnect. This led to races in tests that wait on the disconnection
event, where tests would:
1. wait for the disconnection event before proceeding
2. receive the disconnection event
3. take action based on the disconnection event, assuming that the
client was disconnected, when there is no guarantee that the
disconnection has actually happened yet.
This patch re-orders the disconnection and event broadcast, such that
the disconnection happens first, meaning that if a watcher receives
the disconnection event, then the disconnection has already been
attempted.
tcsc
added a commit
that referenced
this pull request
Dec 1, 2021
Part of this change is implementing a "no secrets" policy for CI. Given that 1. we have to support CI for arbitrary external contributors, and 2. it is easy to craft a malicious PR that exfiltrates secrets during a CI build any test that runs under CI must be able to do so without any injected secrets. This means that several of the test we currently run under Drone will not be run on GCB, at least as part of the regular CI. The plan is to create a separate task that periodically runs tests that require external credentials (e.g. Kube tests, various backend data stores, etc.) in a more secure way and report failures asynchronously. And while these tests will not run under CI, the should still be built under CI so that required changes are caught during review. Note: this backport includes various data race fixes added separately in the master branch: See-Also: #8643 See-Also: #8888 See-Also: #9117 See-Also: #9119
tcsc
added a commit
that referenced
this pull request
Dec 16, 2021
Part of this change is implementing a "no secrets" policy for CI. Given that
we have to support CI for arbitrary external contributors, and
it is easy to craft a malicious PR that exfiltrates secrets during a CI build
any test that runs under CI must be able to do so without any injected secrets.
This means that several of the test we currently run under Drone will not be run
on GCB, at least as part of the regular CI. The plan is to create a separate task
that periodically runs tests that require external credentials (e.g. Kube tests,
various backend data stores, etc.) in a more secure way and report failures
asynchronously. And while these tests will not run under CI, the should still be
built under CI so that required changes are caught during review.
See-Also: #8608
See-Also: #8643
See-Also: #8888
See-Also: #9117
See-Also: #9119
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When disconnecting a client due to an expired certificate, the monitor
was emitting the disconnection event before forcing the client to
disconnect. This led to races in tests that wait on the disconnection
event, where tests would:
client was disconnected, when there is no guarantee that the
disconnection has actually happened yet.
This patch re-orders the disconnection and event broadcast, such that
the disconnection happens first, meaning that if a watcher receives
the disconnection event, then the disconnection has already been
attempted.