config: fix a bug where the on demand subscription not works

[wbpcode]

Commit Message: config: fix a bug where the on demand subscription not works
Additional Description:

In the previous xds implementation, the requestOnDemandUpdate() will update the resources in the discovery request but doesn't update the watch self which make the responded resource will be silently dropped.

The VHDS works because it use a namespace matching to match all resource with same prefix. The use_namespace_matching self is subscription level option, but will be affected all subscriptions under the same type_url. So, it more like a hack.
The ODCDS is a problem. The integration show it can never works correctly. 😞

Anyway, this PR fixed these problems by:

• Added append to update subscription state and also the watch map.
• Used a new subscription level accept() method to replace the use_namespace_matching which is more clearly.

Risk Level: mid, core code change.
Testing: unit, integration.
Docs Changes: n/a.
Release Notes: added.
Platform Specific Features: n/a.

[wbpcode]

The use_namespace_matching_ here make it's impossible to subscribe other resource in the start() and update interest method because they all will be cleaned.

Now, we will always respect the resources set provided by the start() and updateResourceInterest() here, and use the accept() to customize the resource routing.

[wbpcode]

Also related to #44129

[adisuissa]

Thanks for raising the bug, and proposing a fix.
To the best of my understanding, w.r.t. 'append', the proposed design essentially introduces the on-demand concept into the GrpcMuxWatch, which may weaken the abstraction and encapsulation of the on-demand concept. I believe the proposed design leaks that concept into the GrpcMux (similar to how the namespace-matching is a leak of the watched resources, which as you are pointing out is a problem). Note that Envoy already supports multiple GrpcSubscription object over the same GrpcMux (so the watch is updated correctly while there are dynamic "additions" to the watched objects).
Assuming that on-demand is for a named resource, I believe the issue can also be solved by introducing multiple GrpcSubscription objects (for each VDHS resource that needs to be fetched), that will be fetched over the same GrpcMux.

W.r.t. 'accept' I agree that there is an issue with VHDS. I think the issue is even a bit more challenging, for example, say there are 2 routes '/shared/prefix/route_1', and '/shared/prefix', which subscriptions would a VHDS for the two different routes will be invoked once a response comes back. In general, this is a challenge with wildcard subscriptions and it would be good to get a good solution for this. I'm not sure if the proposed aliasing solution is the right thing to do, but it seems to be a bit more explicit - telling the xDS system, what to 'accept' by defining it in the resource the client subscribes to.

[adisuissa]

Is this a breaking change with the VHDS default wildcard subscription?

[wbpcode]

Nope, this PR didn't change any behavior. The VHDS worked in a special way before.

It will specific a route configuration name (for example rc) in the start() But this rc will not be sent out because the use_namespace_matching won't add anything to the subscription state. See the #46216 (comment)

Then this rc will be used to match received resources based on prefix matching.

For control plane's perspective, nothing is changed.

[sschepens]

Assuming that on-demand is for a named resource, I believe the issue can also be solved by introducing multiple GrpcSubscription objects (for each VDHS resource that needs to be fetched), that will be fetched over the same GrpcMux.

Wouldn't this be much more expensive than just adding resources to the watch? we could be talking of many thousands of resources.

[adisuissa]

Wouldn't this be much more expensive than just adding resources to the watch? we could be talking of many thousands of resources.

Really depends on whether there's support for subscription removal or not.
Once a subscription is removed, for example when a listener is removed, then the proposed approach needs to keep track of whether there are multiple listeners on the same subscribed VHDS resource or not. The per-GrpcSubscription shifts that responsibility to the mechanism that is already implemented.

[wbpcode]

To the best of my understanding, w.r.t. 'append', the proposed design essentially introduces the on-demand concept into the GrpcMuxWatch, which may weaken the abstraction and encapsulation of the on-demand concept. I believe the proposed design leaks that concept into the GrpcMux (similar to how the namespace-matching is a leak of the watched resources, which as you are pointing out is a problem). Note that Envoy already supports multiple GrpcSubscription object over the same GrpcMux (so the watch is updated correctly while there are dynamic "additions" to the watched objects).

I don't think this is actual problem. Note, we have supported update here. And the append is only a special update. And I actually want to support the on-demand update more generally, like to CDS self (not the ODCDS). Based on the delta xDS, the on-demand should be simple, cheap, light, and easy.

Assuming that on-demand is for a named resource, I believe the issue can also be solved by introducing multiple GrpcSubscription objects (for each VDHS resource that needs to be fetched), that will be fetched over the same GrpcMux.

Every GrpcSubscription for single resource (like a domain) is much more heavy if there are thousands resources. And if there are multiple resources/subscriptions, we still need to abstract a layer to manager all these subscriptions, like the XdstpOdcdsSubscriptionsManager. And a new subscriptions manager layer is more like a reproduce of current watch and watch map design.

I don't think multiple subscription is a bad idea. But with our current subscription and watch map's design, after this fix, a single subscription could manage a set of resource very well, and more efficient, easier to use. I guess there is no strong reason to use multiple subscriptions' solution.

Really depends on whether there's support for subscription removal or not.
Once a subscription is removed, for example when a listener is removed, then the proposed approach needs to keep track of whether there are multiple listeners on the same subscribed VHDS resource or not. The per-GrpcSubscription shifts that responsibility to the mechanism that is already implemented.

Our current underlying subscription and watch map design have supported the resource removal well. To expose a remove() is also very simple, because remove() is also another update only.
Although, until now, we didn't see the requirements to the removal.

---

In short conclusion, our current subscription and watch map are well designed for multiple resources in same subscription. And it match the actual requirement (one observer, multiple resources) very well. Fix it and enhance it should bring us more benefit rather than to deprecate it.

[wbpcode]

W.r.t. 'accept' I agree that there is an issue with VHDS. I think the issue is even a bit more challenging, for example, say there are 2 routes '/shared/prefix/route_1', and '/shared/prefix', which subscriptions would a VHDS for the two different routes will be invoked once a response comes back. In general, this is a challenge with wildcard subscriptions and it would be good to get a good solution for this. I'm not sure if the proposed aliasing solution is the right thing to do, but it seems to be a bit more explicit - telling the xDS system, what to 'accept' by defining it in the resource the client subscribes to.

In very long time, we still need to handle canonical xDS. So, I guess the accept() still make sense in very long time. And there are two points:

• Envoy is not actually interact with xDS protocols, it interact with specific xDS implementations, and in practice, they may be different. For example, in current implementations, the VHDS will send empty resource names which should be treat as subscribe all. But in implementation, the server treat it as on demand.
• When supporting the on demand cds in Istio, the maintainers raised a requirement to allow Istio send additional clusters that Envoy not required directly (for tracing, logging and so on because these clusters will processed by on demand filter), with accept(), we can set a * and transfer the control to control plane.

Basically, accept() is used to provide more flexibility, and harmless if you don't use it.

[adisuissa]

To the best of my understanding, w.r.t. 'append', the proposed design essentially introduces the on-demand concept into the GrpcMuxWatch, which may weaken the abstraction and encapsulation of the on-demand concept. I believe the proposed design leaks that concept into the GrpcMux (similar to how the namespace-matching is a leak of the watched resources, which as you are pointing out is a problem). Note that Envoy already supports multiple GrpcSubscription object over the same GrpcMux (so the watch is updated correctly while there are dynamic "additions" to the watched objects).

I don't think this is actual problem. Note, we have supported update here. And the append is only a special update. And I actually want to support the on-demand update more generally, like to CDS self (not the ODCDS). Based on the delta xDS, the on-demand should be simple, cheap, light, and easy.

How does it handle a removal of a subscription from the watcher?
What happens if there are 2 GrpcSubscription objects that have been added in different time subscribing to the same resource, and then one is deleted. Does the underlying GrpcMuxWatch keeps track of which GrpcSusbcription were needed for it?
I think this is the "abstraction" breaking that hopefully can be avoided by designing it a bit better.

Assuming that on-demand is for a named resource, I believe the issue can also be solved by introducing multiple GrpcSubscription objects (for each VDHS resource that needs to be fetched), that will be fetched over the same GrpcMux.

Every GrpcSubscription for single resource (like a domain) is much more heavy if there are thousands resources. And if there are multiple resources/subscriptions, we still need to abstract a layer to manager all these subscriptions, like the XdstpOdcdsSubscriptionsManager. And a new subscriptions manager layer is more like a reproduce of current watch and watch map design.

I don't think multiple subscription is a bad idea. But with our current subscription and watch map's design, after this fix, a single subscription could manage a set of resource very well, and more efficient, easier to use. I guess there is no strong reason to use multiple subscriptions' solution.

Really depends on whether there's support for subscription removal or not.
Once a subscription is removed, for example when a listener is removed, then the proposed approach needs to keep track of whether there are multiple listeners on the same subscribed VHDS resource or not. The per-GrpcSubscription shifts that responsibility to the mechanism that is already implemented.

Our current underlying subscription and watch map design have supported the resource removal well. To expose a remove() is also very simple, because remove() is also another update only. Although, until now, we didn't see the requirements to the removal.

Why isn't there a requirement for removal? What happens if the listener that subscribed to the VHDS resource is removed?
What if there are 2 listeners pointing to the same VHDS resource.
Why should the discussion be around VHDS? This is not a VHDS-specific change, but a generic xDS-level change, so it should be supported for other xDS-types as well, no?

In short conclusion, our current subscription and watch map are well designed for multiple resources in same subscription. And it match the actual requirement (one observer, multiple resources) very well. Fix it and enhance it should bring us more benefit rather than to deprecate it.

The breaking of that abstraction is one of the things that caused issues with on-demand out-of-sync subscriptions.
Note that watchers were added for GrpcMux, and not for filesystem/REST transport types. This was an intentional design choice to avoid having features that only work for one type and not for another.

[wbpcode]

How does it handle a removal of a subscription from the watcher?
What happens if there are 2 GrpcSubscription objects that have been added in different time subscribing to the same resource, and then one is deleted. Does the underlying GrpcMuxWatch keeps track of which GrpcSusbcription were needed for it?
I think this is the "abstraction" breaking that hopefully can be avoided by designing it a bit better.

When a subscription is removed, it will be removed from the watch map. For specific resource, if the removed subscription is only one to subscribe the resource, the resource will be removed. If there are other subscriptions want this resource, resource will be kept.

Yeah, watch map have tracked everything. Every subscription's resource list and every resource is subscribed by which subscriptions.
And even you use single subscription for single resource, it will finally enter this watch map and be recored.

[wbpcode]

Why isn't there a requirement for removal? What happens if the listener that subscribed to the VHDS resource is removed?
What if there are 2 listeners pointing to the same VHDS resource.
Why should the discussion be around VHDS? This is not a VHDS-specific change, but a generic xDS-level change, so it should be supported for other xDS-types as well, no?

Sorry, I meant to remove a specific resource from a subscription. Until now, no related requirements. All your referred are cases to remove whole subscription. For these cases, they will be removed correctly as I described in the #46216 (comment)

And you are right, I only use VHDS as an example. This is general problem, and this PR is also a general fix for all xDS-types (i mean CDS/LDS/... and so on, not means file/rest/...).
After this PR, all xDS type could support on demand update easily.

[wbpcode]

The breaking of that abstraction is one of the things that caused issues with on-demand out-of-sync subscriptions.
Note that watchers were added for GrpcMux, and not for filesystem/REST transport types. This was an intentional design choice to avoid having features that only work for one type and not for another.

Sorry. I didn't get you here. Could you explain a little more about the breaking of that abstraction? I personally think this is only a implementation bug because the existing abstraction (before this PR) have provided update() (in GrpcMuxWatch) and updateResourceInterest() (in Subscription), the new append() won't make thing worse because the append is only variant of update?
Or did you mean the exiting abstraction (before this PR) not in a correct direction? (I actually think the whole existing design is flexible and powerful, although can be better in some detail).

[wbpcode]

BTW, to avoid there is any information gap, when I say observer, I mean a configuration receiver who implemented the SubscriptionCallbacks (onConfigUpdate/...), .

[wbpcode]

After a rethinking and discuss with AI, did you mean the current design shouldn't leak the gRPC implementation detail like requestOnDemandUpdate/updateResourceInterest to Subscription abstraction because they are implementation dependent?

If you mean that, I guess it's too late to discuss that. GRPC is special from start. For example, the file xds even cannot support a the start() correctly because it has no name based discovery. Back to 10 years ago, we may could have better design. But for now, the canonical xDS is mature and almost no one complain the limit of file/rest, I guess it's fine to go head along current way and fix/improve the implementation/detail.

The next generation should is the xdstp, but it's another topic.

[wbpcode]

/coverage

[repokitteh-read-only]

Coverage for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-cncf-pr/46216/coverage/index.html

For comparison, current coverage on main branch is here:

https://storage.googleapis.com/envoy-cncf-postsubmit/main/coverage/index.html

The coverage results are (re-)rendered each time the CI Envoy/Checks (coverage) job completes.

🐱

Caused by: a #46216 (comment) was created by @wbpcode.

see: more, trace.