rm provider resource wait during init logic#447
Merged
arkodg merged 1 commit intoenvoyproxy:mainfrom Sep 28, 2022
Merged
Conversation
* removes all the waitgroup logic associated with the provider resources (gatewayclass, gateway, httproute) that was added to ensure that all resources within the watchable map was comepletely initialized before the gateway api translated the resources * removing this logic does mean that there might some churn in the dataplane during EG reboots. This should get fixed with envoyproxy#413 which plans on reconciling all resources within a single controller so we cont need to synchonize across multiple controllers using multiple waitgroups Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Codecov Report
@@ Coverage Diff @@
## main #447 +/- ##
==========================================
+ Coverage 60.04% 60.29% +0.25%
==========================================
Files 40 40
Lines 4337 4347 +10
==========================================
+ Hits 2604 2621 +17
+ Misses 1582 1575 -7
Partials 151 151
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
LukeShu
previously approved these changes
Sep 28, 2022
Contributor
LukeShu
left a comment
LukeShu
left a comment
There was a problem hiding this comment.
I sorta hate it because now startup is potentially racy, but it's good enough for now.
arkodg
force-pushed
the
rm-init-waits
branch
from
cfb0059 to
dea32d0
Compare
skriss
approved these changes
Sep 28, 2022
Contributor
skriss
left a comment
skriss
left a comment
There was a problem hiding this comment.
LGTM, I'll file an issue to track reimplementing this in some form for 0.3
This was referenced Sep 28, 2022
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
removes all the waitgroup logic associated with the provider resources (gatewayclass, gateway, httproute) that was added to ensure that all resources within the watchable map was comepletely initialized before the gateway api translated the resources
this logic is being removed because the individual waitgroups for each resource are causing a sort of deadlock within the gateway-api runner and not allowing it to process new notifications
removing this logic does mean that there might some churn in the dataplane during EG reboots. This should get fixed with Consider Merging Controllers #413 which plans on reconciling all resources within a single controller so we cont need to synchonize across multiple controllers using multiple waitgroups
Signed-off-by: Arko Dasgupta arko@tetrate.io