envoy
envoy copied to clipboard
envoyproxy
config-plane: refactor resource decoder to be a shared pointer
Commit Message: refactor resource decoder to be a shared pointer
Additional Description:
Prior to this PR the OpaqueResourceDecoder was owned by each of the Subscriptions, and a reference to it was used by the GrpcMux's. This can cause an issue if the Mux outlives the subscription, as can happen in the newly added test ValidResourceDecoderAfterRemoval, and in the unified Mux implementation.
This PR converts the OpaqueResourceDecoder to a shared pointer, that can be used by both.
Risk Level: Low/Medium - modifying many places in the config-plane codebase, but the refactor switches from a reference to a shared pointer. Testing: Added a unit test that fails if non-shared pointer is used. Docs Changes: N/A. Release Notes: TBD. Platform Specific Features: N/A
Signed-off-by: Adi Suissa-Peleg [email protected]
/assign @htuch
I was also able to convert the reference to a unique-ptr, but not sure if there's a higher risk in choosing unique-ptr over shared-ptr.
@adisuissa shouldn't the watch removal when the subscription disappears also cleanup this stale reference?
@adisuissa shouldn't the watch removal when the subscription disappears also cleanup this stale reference?
In the old-code (non-unified) the resource decoder was always taken from the front watcher, and whenever a new watcher was added, it was added with a new decoder. The assumption is that in order to parse the resource, each subscription must have a watcher, and that watcher must be valid.
The new code (unified) assigned the decoder in the subscription-state (one level above the watchers). This causes an issue if the watcher is destroyed (and the decoder with it).
The new test fails when the watcher is removed, because the subscription-state isn't removed.
The reason to keep the subscription-state is essentially due to the difference between the UnwatchedTypeAcceptsEmptyResources test in the old-code (not xDS-protocol compliant), and in the new-code (which is xDS-compliant).
Retrying Azure Pipelines: Retried failed jobs in: envoy-presubmit
:cat:
Caused by: a https://github.com/envoyproxy/envoy/pull/22596#issuecomment-1209060048 was created by @adisuissa.
![repokitteh-read-only[bot] avatar](https://avatars.githubusercontent.com/in/16729?v=4 "Published by a pub.dev verified publisher"){kind=small}
Retrying Azure Pipelines: Retried failed jobs in: envoy-presubmit
:cat:
Caused by: a https://github.com/envoyproxy/envoy/pull/22596#issuecomment-1209087522 was created by @adisuissa.
Retrying Azure Pipelines: Retried failed jobs in: envoy-presubmit
:cat:
Caused by: a https://github.com/envoyproxy/envoy/pull/22596#issuecomment-1209327437 was created by @adisuissa.
Retrying Azure Pipelines: Retried failed jobs in: envoy-presubmit
:cat:
Caused by: a https://github.com/envoyproxy/envoy/pull/22596#issuecomment-1209690783 was created by @adisuissa.
OK. I wonder if these can be const shared pointers?
Maybe there is a smarter way here to avoid the whole "take the resource decoder from first in list" style, since it seems a bit redundant, but it's probably fine to make the incremental step and switch to some shared pointers to consts.
Maybe there is a smarter way here to avoid the whole "take the resource decoder from first in list" style, since it seems a bit redundant, but it's probably fine to make the incremental step and switch to some shared pointers to consts.
Once we deprecate the non-unified code, we should essentially have a single decoder when a subscription is created. I think there should be a single manager, maybe as part of the factory context.
I'll see if const shared pointers can be used.
OK. I wonder if these can be const shared pointers?
Unfortunately these cannot be shared pointers, as the resource-decoder contains a reference to the validation-context (for protobuf validations), and it is non-const.
You mention that the long term model is something like singleton resource decoders and a factory model. In that scenario, we probably wouldn't need shared pointers right? Just checking, as the style guide discourages shared pointers when the ownership semantics don't require them.
You mention that the long term model is something like singleton resource decoders and a factory model. In that scenario, we probably wouldn't need shared pointers right? Just checking, as the style guide discourages shared pointers when the ownership semantics don't require them.
Yeah. Also, I was able to convert the shared-pointers to unique-pointers, and all the tests pass. I'm just a bit hesitant to change to unique-pointers because it may cause subtle bugs that the test may not cover. Feel free to overrule my thinking here, as you may perceive it differently.
Can you share your branch with the unique_ptrs? I'm not convinced that is hte right thing, but I'd like to take a quick look to see what it looks like.
Can you share your branch with the unique_ptrs? I'm not convinced that is hte right thing, but I'd like to take a quick look to see what it looks like.
Added the branch in #22764.
Once we decide which one should be merged, let me know if you think it deserves a release note (not sure this is a user-facing change).
@adisuissa feel free to merge after verifying we don't break anything since your last push.
Retrying Azure Pipelines: Retried failed jobs in: envoy-presubmit
:cat:
Caused by: a https://github.com/envoyproxy/envoy/pull/22596#issuecomment-1238296351 was created by @adisuissa.