refactor retry to close watchFactory #3226
Merged
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
|
/lgtm |
bpickard22
changed the title
trozet
approved these changes
Oct 20, 2022
bpickard22
force-pushed
the
data-race
branch
3 times, most recently
from
01622a4
to
e1a3919
Compare
|
/lgtm |
ricky-rav
reviewed
Oct 21, 2022
trozet
requested changes
Oct 24, 2022
|
@ricky-rav PTAL |
flavio-fernandes
requested changes
Oct 26, 2022
|
/test Build-PR |
|
/test build-pr |
|
/retest |
ricky-rav
reviewed
Oct 26, 2022
bpickard22
force-pushed
the
data-race
branch
2 times, most recently
from
876dae2
to
6a6bf96
Compare
|
Now we've introduced a new data race between So |
leverage stop channel on watch factory when periodic retry stopchannel is called Problem: Hybrid test was creating creating an ovn-controller for some tests. The tests could not use the waitGroup in the test so the afterEach function (cleanup function after each test) could execute while leaving controllers up. In the tests, we were leveraging the code in ovn.go to run the controllers and creating a watcher pod on them. This issue was just a symptom of the main issue which was that we were using the incorrect stopChannel in the retry logic. Solution: The stopChannel that we use in the watchFactory is private, so we created a function that when called will close the watcher stopChannel, closing the watcher. We then call this function in the select (basically a switch statement in go but for channels) to close the watcher, because when we stop watching an object, that means we no longer care about it and should stop retrying it. This also adds a public waitgroup property to the watchfactory that we are using to ensure that the retry loop has exited before we shutdown the watcher to avoid any races there Since we no longer use the stopChannel in the retryFramework, this also removes that stopChannel from the newRetryFramework function definition, and all its refrences (which at this time was just in obj_retry_master) Signed-off-by: Ben Pickard <bpickard@redhat.com> Closes: ovn-org#3203 Remove stopChannel from retryFrameowork We no longer use the controller stopChannel passed to the retryFramework, as we use the watcher stopChannel, so we can remove it Signed-off-by: Ben Pickard <bpickard@redhat.com>
|
/lgtm |
trozet
approved these changes
Oct 27, 2022
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.
leverage stop channel on watch factory when periodic retry stopchannel is called
Problem: Hybrid test was creating creating an ovn-controller for some tests.
The tests could not use the waitGroup in the test so the afterEach function
(cleanup function after each test) could execute while leaving controllers up.
In the tests, we were leveraging the code in ovn.go to run the controllers
and creating a watcher pod on them. This issue was just a symptom of the main
issue which was that we were using the incorrect stopChannel in the retry logic.
Solution: The stopChannel that we use in the watchFactory is private, so we
created a function that when called will close the watcher stopChannel,
closing the watcher. We then call this function in the select
(basically a switch statement in go but for channels) to close the watcher,
because when we stop watching an object, that means we no longer care about it
and should stop retrying it.
Signed-off-by: Ben Pickard bpickard@redhat.com
Closes: #3203