Support drop_overload using EDS cache while CDS update warming failure #42949
Conversation
Signed-off-by: Yanjun Xiang <[email protected]>
|
/assign @yurykats |
Signed-off-by: Yanjun Xiang <[email protected]>
|
/assign @yanavlasov |
| void EdsClusterImpl::update( | ||
| const envoy::config::endpoint::v3::ClusterLoadAssignment& cluster_load_assignment) { | ||
| // Drop overload configuration parsing. | ||
| THROW_IF_NOT_OK(parseDropOverloadConfig(cluster_load_assignment)); |
There was a problem hiding this comment.
It seems to me that since update() is void, it's not expected to be doing any error checking. Maybe just log that overload wasn't applied and move on?
There was a problem hiding this comment.
As discussed offline, we want to reject the bad configuration by throwing an exception. This follows the same logic as other EDS updates.
There was a problem hiding this comment.
I agree that throwing here should be discouraged.
It works (does not throw in a non-onConfigUpdate path) because the EDS resource is first validated before it is added to the cache, so the next call to this function when retrieving the cached EDS resource will not fail.
IMHO a better solution would be to refactor the function parseDropOverloadConfig so instead of both validating and updating the value, one would be able to validate and store the values separately. Then, the validation can be done in the onConfigUpdate() method, and the storing of the value can be done here in update().
There was a problem hiding this comment.
Good idea! Let's have a follow up PR to do the refactoring.
|
LGTM |
|
/assign @adisuissa |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
LGTM for now. Please add a release note.
| void EdsClusterImpl::update( | ||
| const envoy::config::endpoint::v3::ClusterLoadAssignment& cluster_load_assignment) { | ||
| // Drop overload configuration parsing. | ||
| THROW_IF_NOT_OK(parseDropOverloadConfig(cluster_load_assignment)); |
There was a problem hiding this comment.
I agree that throwing here should be discouraged.
It works (does not throw in a non-onConfigUpdate path) because the EDS resource is first validated before it is added to the cache, so the next call to this function when retrieving the cached EDS resource will not fail.
IMHO a better solution would be to refactor the function parseDropOverloadConfig so instead of both validating and updating the value, one would be able to validate and store the values separately. Then, the validation can be done in the onConfigUpdate() method, and the storing of the value can be done here in update().
Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Yanjun Xiang <[email protected]>
|
/retest |
Signed-off-by: yanavlasov <[email protected]>
4 participants
There is a problem that during CDS update, the drop_overload in Envoy memory is wiped out. If EDS update never arrives, the drop_overload enforcement is lost, and only start to function after next EDS update arrives.
This PR fixes this issue by moving the drop_overload parsing logic to a common function update() which will cover both EDS succeeds cases and EDS timeout cases.