config: invoke initialization callbacks on remote close #5671
Conversation
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
| @@ -268,7 +268,7 @@ void GrpcMuxImpl::onRemoteClose(Grpc::Status::GrpcStatus status, const std::stri | |||
| ENVOY_LOG(warn, "gRPC config stream closed: {}, {}", status, message); | |||
| stream_ = nullptr; | |||
| control_plane_stats_.connected_state_.set(0); | |||
| setRetryTimer(); | |||
| handleFailure(); | |||
There was a problem hiding this comment.
One thought here: If you don't want to consider this a failure for stat purposes, you could just invoke the initialize callbacks without calling handleFailure(). I don't mind either way but throwing it out there.
Either way we go, is it possible to craft a test specific to this issue? Can we do a remote close on the first call and make sure the initialize callbacks fire? Do the test changes below specifically cover that?
There was a problem hiding this comment.
I think I will leave it as is.
yes, the test covers the case that for 3 resource subscribes, the first response it self is Remote Close (with out any previous messages). If that is not what you are looking for, please LMK what you are looking for - I will try to craft that
There was a problem hiding this comment.
I don't see an expectation that the initialized callbacks is called though. Can you add that?
There was a problem hiding this comment.
Oh. Sorry. I added it to grpc_mux_impl and thought I added it to subscription_impl as well. Added now - Is that what we are asking for?
There was a problem hiding this comment.
Not exactly. There is an intialized callback that needs to be fired the first time any update/failure/etc. happens. Is the test expecting those? If not, can you add them, specifically for covering the bug we are fixing?
/wait
There was a problem hiding this comment.
Ok. Are you referring to InitManager's initialize() callback? I may be missing some thing, but GrpcMuxImpl which these tests are written, does not have it injected. It is at higher level that it is called based on configupate/failed. Or there is a way to get InitiManager here?
There was a problem hiding this comment.
Hmm sorry yes I see. OK yes I think this is right (we do the logic in config update failed). Will take a look again.
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Description: Invokes initialization callbacks on remote close
Risk Level: Low
Testing: Changed existing automated tests
Docs Changes: N/A
Release Notes: N/A
Fixes #5622