upstream: avoid quadratic time complexity in server initialization#15347
upstream: avoid quadratic time complexity in server initialization#15347snowp merged 2 commits intoenvoyproxy:mainfrom
Conversation
…nvoyproxy#15237) Currently ClusterManagerInitHelper.secondary_init_clusters_ is a list. Upon every insert the list gets searched for an already added cluster with the same name. Since in normal circumstances all new clusters have unique names the worst case scenario is triggered and all elements of the list are checked sequentially upon every cluster added. Replace the list with a hash map. Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
|
/cc @tbarrella Could you please review the fix? I've added a very simple test case, but perhaps it needs to reflect better what actually happened in Istio's test suite. |
| cluster_list->remove(&cluster); | ||
| // present in the initializer map. If so, this is fine. | ||
| auto iter = cluster_map->find(cluster.cluster().info()->name()); | ||
| if (iter != cluster_map->end() && iter->second == &cluster) { |
There was a problem hiding this comment.
Now that you are here, can you log the when (iter != cluster_map->end() && iter->second != &cluster) ?
There was a problem hiding this comment.
Above is not the condition of the crash. But it's good to know if this branch is possible.
There was a problem hiding this comment.
Based on this code I think we are expecting it's possible that this can happen in our data model, e.g. getting a CDS update with the same cluster name as before.
If that's the case just put that in a comment. It would be good to know if that's covered in our tests. E.g. put an ASSERT failure here and just see if that passes tests.
If we don't hit that assert, may be leave it in? If we do hit that assert then replace it with a comment saying the above happens on a CDS update.
I'm not sure it's necessary to log it, if it's just going to happen on every CDS update that retains some names.
There was a problem hiding this comment.
e.g. getting a CDS update with the same cluster name as before.
Yes, this is exactly the case covered with bazel test //test/common/upstream:cluster_manager_impl_test --gtest_filter=*UpdateAlreadyInitialized*. I've added a comment referencing it.
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
Thank you!
@envoyproxy/senior-maintainers
4 participants
Commit Message: upstream: avoid quadratic time complexity in server initialization
Additional Description:
Currently ClusterManagerInitHelper.secondary_init_clusters_ is a list.
Upon every insert the list gets searched for an already added cluster
with the same name. Since in normal circumstances all new clusters
have unique names the worst case scenario is triggered and all elements
of the list are checked sequentially upon every cluster added.
Replace the list with a hash map.
This is a second attempt to submit the reverted #15237
Risk Level: Low
Testing: added a test case for removing unknown cluster
Docs Changes: N/A
Release Notes: N/A