cds: fix blocking a revert of a warming cluster #15269
Conversation
Signed-off-by: Taylor Barrella <tabarr@google.com>
Signed-off-by: Taylor Barrella <tabarr@google.com>
| existing_active_cluster->second->blockUpdate(new_hash)) || | ||
| (existing_warming_cluster != warming_clusters_.end() && | ||
| existing_warming_cluster->second->blockUpdate(new_hash))) { | ||
| if (existing_warming_cluster != warming_clusters_.end()) { |
There was a problem hiding this comment.
@mattklein123 @tbarrella do you think the same issue may exist in LDS in
envoy/source/server/listener_manager_impl.cc
Line 397 in 3bddd1a
It's hard for me to tell, since the logic here is subtle and orders are different.
There was a problem hiding this comment.
That's from #12645 which looks like it was meant to fix the same issue with LDS. I guess the approach is different; I didn't do it that way because it seemed simpler to take the newest config as a typical update rather than introduce cancellation
There was a problem hiding this comment.
Would it make sense to factor out this pattern? Divergence seems scary :)
There was a problem hiding this comment.
I hear what you mean in terms of divergence. For this do you mean actual code refactoring (I'm not sure about the ROI here) or just making sure the logic is the same? What are your thoughts on the LDS (cancel the warming listener and block the current update) vs. CDS approach (accept the current update)?
There was a problem hiding this comment.
I don't feel strongly on how we do it as long as we make them consistent and keep them that way (code comments explaining when to update what? Structural is always nicer if it's not too much complexity).
I think whatever is simplest here makes sense, but CC @adisuissa @dmitri-d
Commit Message:
cds: fix blocking a revert of a warming cluster
Signed-off-by: Taylor Barrella tabarr@google.com
Additional Description:
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: Noted bug fix
Fixes #14598