This breaks the following pieces out of the `grpc_client_channel` BUILD target:
- backend_metric_parser
- oob_backend_metric
- child_policy_handler
- backup_poller
- service_config_channel_arg_filter
- client_channel_channelz
- client_channel_internal_header
- subchannel_connector
- subchannel_pool_interface
- config_selector
- client_channel_service_config_parser
- retry_service_config_parser
- retry_throttle
The code left in the `grpc_client_channel` target will need more work to pull apart.
Closes#35879
COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35879 from markdroth:client_channel_build_split f388a37edc
PiperOrigin-RevId: 608806548
This change is needed in order to get certain LB-policy-unit-test-framework-based tests to pass, which involve updates on PF policies.
Without this change, in such tests, health watchers on the original subchannel can be left hanging around. The underlying reason is related to 842057d8d5/test/core/client_channel/lb_policy/lb_policy_test_lib.h (L203).
Related: cl/563857636
Closes#35865
COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35865 from apolcyn:update_pf_Test 1a5e6b5591
PiperOrigin-RevId: 607844309
As title. Pulling these additions out from a larger change.
Related: cl/563857636
Closes#35860
COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35860 from apolcyn:test_lib_changes 40b6455638
PiperOrigin-RevId: 606708025
This new directory combines code from the following locations:
- src/core/ext/filters/client_channel/resolver
- src/core/lib/resolver
Closes#35804
COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35804 from markdroth:client_channel_resolver_reorg2 30660e6b00
PiperOrigin-RevId: 604665835
This new directory combines code from the following locations:
- src/core/ext/filters/client_channel/lb_policy
- src/core/lib/load_balancing
Closes#35786
COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35786 from markdroth:client_channel_resolver_reorg 98554efb98
PiperOrigin-RevId: 604351832
<!--
If you know who should review your pull request, please assign it to that
person, otherwise the pull request would get assigned randomly.
If your pull request is for a specific language, please add the appropriate
lang label.
-->
Closes#35210
COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35210 from yijiem:csm-service-label 6a6a7d1774
PiperOrigin-RevId: 597641393
I realized that this field wasn't actually necessary, since the string is already present in the map key.
Closes#35503
COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35503 from markdroth:xds_config_remove_cluster_name 94d5edc133
PiperOrigin-RevId: 597375018
Previously, `RefCountedPtr<>` and `WeakRefCountedPtr<>` incorrectly allowed
implicit casting of any type to any other type. This hadn't caused a
problem until recently, but now that it has, we need to fix it. I have
fixed this by changing these smart pointer types to allow type
conversions only when the type used is convertible to the type of the
smart pointer. This means that if `Subclass` inherits from `Base`, then
we can set a `RefCountedPtr<BaseClass>` to a value of type
`RefCountedPtr<Subclass>`, but we cannot do the reverse.
We had been (ab)using this bug to make it more convenient to deal with
down-casting in subclasses of ref-counted types. For example, because
`Resolver` inherits from `InternallyRefCounted<Resolver>`, calling
`Ref()` on a subclass of `Resolver` will return `RefCountedPtr<Resolver>`
rather than returning the subclass's type. The ability to implicitly
convert to the subclass type made this a bit easier to deal with. Now
that that ability is gone, we need a different way of dealing with that
problem.
I considered several ways of dealing with this, but none of them are
quite as ergonomic as I would ideally like. For now, I've settled on
requiring callers to explicitly down-cast as needed, although I have
provided some utility functions to make this slightly easier:
- `RefCounted<>`, `InternallyRefCounted<>`, and `DualRefCounted<>` all
provide a templated `RefAsSubclass<>()` method that will return a new
ref as a subclass. The type used with `RefAsSubclass()` must be a
subclass of the type passed to `RefCounted<>`, `InternallyRefCounted<>`,
or `DualRefCounted<>`.
- In addition, `DualRefCounted<>` provides a templated `WeakRefAsSubclass<T>()`
method. This is the same as `RefAsSubclass()`, except that it returns
a weak ref instead of a strong ref.
- In `RefCountedPtr<>`, I have added a new `Ref()` method that takes
debug tracing parameters. This can be used instead of calling `Ref()`
on the underlying object in cases where the caller already has a
`RefCountedPtr<>` and is calling `Ref()` only to specify the debug
tracing parameters. Using this method on `RefCountedPtr<>` is more
ergonomic, because the smart pointer is already using the right
subclass, so no down-casting is needed.
- In `WeakRefCountedPtr<>`, I have added a new `WeakRef()` method that
takes debug tracing parameters. This is the same as the new `Ref()`
method on `RefCountedPtr<>`.
- In both `RefCountedPtr<>` and `WeakRefCountedPtr<>`, I have added a
templated `TakeAsSubclass<>()` method that takes the ref out of the
smart pointer and returns a new smart pointer of the down-casted type.
Just as with the `RefAsSubclass()` method above, the type used with
`TakeAsSubclass()` must be a subclass of the type passed to
`RefCountedPtr<>` or `WeakRefCountedPtr<>`.
Note that I have *not* provided an `AsSubclass<>()` variant of the
`RefIfNonZero()` methods. Those methods are used relatively rarely, so
it's not as important for them to be quite so ergonomic. Callers of
these methods that need to down-cast can use
`RefIfNonZero().TakeAsSubclass<>()`.
PiperOrigin-RevId: 592327447
<!--
If you know who should review your pull request, please assign it to that
person, otherwise the pull request would get assigned randomly.
If your pull request is for a specific language, please add the appropriate
lang label.
-->
Closes#35251
COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35251 from yijiem:fix-dns-resolver-cooldown-test 857835200a
PiperOrigin-RevId: 589159895
This avoids storing unnecessary copies of the address list in each node of the LB policy tree.
Closes#34753
COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/34753 from markdroth:lb_address_list_iterator 1d39465fbc
PiperOrigin-RevId: 582891475
Changes to fake resolver:
- Add `WaitForReresolutionRequest()` method to fake resolver response
generator to allow tests to tell when re-resolution has been requested.
- Change fake resolver response generator API to have only one mechanism
for injecting results, regardless of whether the result is an error or
whether it's triggered by a re-resolution.
Changes to grpclb_end2end_test:
- Change balancer interface such that instead of setting a list of
responses with fixed delays, the test can control exactly when each
response is set.
- Change balancer impl to always send the initial LB response, as
expected by the grpclb protocol.
- Change balancer impl to always read load reports, even if load
reporting is not expected to be enabled. (The latter case will still
cause the test to fail.) Reads are done in a different thread than
writes.
- Allow each test to directly control how many backends and balancers
are started and the client load reporting interval, so that (a) we don't
waste resources starting servers we don't need and (b) there is no need
to arbitrarily split tests across different test classes.
- Add timeouts to `WaitForAllBackends()` functionality, so that tests
will fail with a useful error rather than timing out.
- Improved ergonomics of various helper functions in the test framework.
In the process of making these changes, I found a couple of bugs:
- A bug in pick_first, which I fixed in #34885.
- A bug in grpclb, in which we were using the wrong condition to decide
whether to propagate a re-resolution request from the child policy,
which I've fixed in this PR. (This bug probably originated way back in
#18344.)
This should address a lot of the flakes seen in grpclb_e2e_test
recently.
This fixes a bug accidentally introduced in #33753. The symptom is that
if we exit idle and then get a new address list before any of the
subchannels in the old list can report their initial connectivity state,
we will incorrectly ignore the new address list.
The original logic from #34615 was incorrect in cases where one address
family has a different number of addresses than the other(s).
Fixes b/307937051.
<!--
If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.
If your pull request is for a specific language, please add the
appropriate
lang label.
-->
- Fixes support for the same address being present more than once in the
address list, which was accidentally broken in #34244.
- Change the call attribute to encode the hash as an integer instead of
a string.
More changes as part of the dualstack design:
- Change resolver and LB policy APIs to support multiple addresses per
endpoint. Specifically, replace `ServerAddress` with
`EndpointAddresses`, which encodes more than one address. Per-address
channel args are retained at the same level, so they are now
per-endpoint. For now, `EndpointAddress` provides a single-address ctor
and a single-address accessor for backward compatibility, so
`ServerAdress` is an alias for `EndpointAddresses`; eventually, this
alias and the single-address methods will be removed.
- Add an `EndpointAddressSet` class, which represents an unordered set
of addresses to be used as a map key. This will be used in a number of
LB policies that need to store per-endpoint state.
- Change the LB policy API's `ChannelControlHelper::CreateSubchannel()`
method to take the address and per-endpoint channel args as separate
parameters, so that we don't need to construct a legacy `ServerAddress`
object as we create a new subchannel for each address in the endpoint.
- Change pick_first to flatten the address list.
- Change ring_hash to use `EndpointAddressSet` as the key for its
endpoint map, and to use the first address of the endpoint as the hash
key.
- Change WRR to use `EndpointAddressSet` as the key for its endpoint
weight map.
Note that support for multiple addresses per endpoint is guarded in RR
by the existing `round_robin_delegate_to_pick_fist` experiment and in
WRR by the existing `wrr_delegate_to_pick_first` experiment.
This PR does *not* include support for multiple addresses per endpoint
for the outlier_detection or xds_override_host LB policies; those will
come in subsequent PRs.
Add some basic metrics to work serializer, keep them process wide for
now (though it may be interesting to get these into channelz in the
future).
Collected are:
- time spent running a work serializer when it starts
- time spent actually executing work when the work serializer runs
- number of items executed each run
A high disparity between the first two indicates our dispatching
mechanism is adding large amounts of latency (perhaps due to thread
starvation like effects).
A high value for any of these indicate contention on the serializer.
It's likely a future iteration on these will select different metrics -
I'm not *entirely* sure which will be useful in production analysis yet.
I'm using `std::chrono::steady_clock` here for precision (nanoseconds)
with a compact representation (better than timespec) and a robust &
portable api - I think it's appropriate for metrics, but wouldn't use it
much beyond that at this point.
Most recent attempt was #34320, reverted in #34335.
The first commit here is a pure revert. The second commit fixes the
outlier_detection unit test to pass both with and without the
experiment.
Rolls forward part of the dualstack changes, mostly from #33427 and a
little bit from #32692, both of which were reverted in #33718.
Specifically:
- For petiole policies, unconditionally start health watch on
subchannels, even if client side health checking is not enabled; in this
case, the health watch will report the subchannel's raw connectivity
state.
- Fix edge cases in health check reporting that occur when a watcher is
started before the initial state is reported.
- When client-side health checking fails, add the subchannel's address
to the RPC failure status message.
- Outlier detection now works only via the health checking watch, not
via the raw connectivity state watch.
- Remove now-unnecessary hack to ensure that outlier detection does not
work for pick_first.
This rolls forward only the pick_first changes from #32692, which were
rolled back in #33718. Specifically:
- Changes PF to use its own subchannel list implementation instead of
using the subchannel_list library, since the latter will be going away
with the dualstack changes.
- As a result of no longer using the subchannel_list library, PF no
longer needs to set the `GRPC_ARG_INHIBIT_HEALTH_CHECKING` channel arg.
- Adds an option to start a health watch on the chosen subchannel, to be
used in the future when pick_first is the child of a petiole policy.
(Currently, this code is not actually called anywhere.)
WRR is showing a very high CPU cost relative to previous solutions, and
it's unclear why this is.
Add two metrics that should help us see the shape of the subchannel sets
that are being passed to high cost systems in order to confirm/deny
theories.
---------
Co-authored-by: ctiller <ctiller@users.noreply.github.com>
De-experiment pick first since we have both affinity and randomness E2E
test running successfully.
---------
Co-authored-by: Yash Tibrewal <yashkt@google.com>
Fix sticky-TF behavior such that once we enter TRANSIENT_FAILURE, we do
not leave that state if we get a new address list.
Also, fix handling of subchannels in state TRANSIENT_FAILURE.
Previously, if a subchannel was already in state TRANSIENT_FAILURE when
we wanted to start a connection attempt on it (e.g., because the
subchannel already existed from a different channel, or because it
already existed in the previous subchannel list), we would wait for it
to report IDLE before attempting to connect. This PR changes pick_first
to instead immediately skip the subchannel and move on to the next one.
Now, the only time we wait for a subchannel in TRANSIENT_FAILURE is when
we wrap back around to the first subchannel in the list.
Why: Cleanup for chttp2_transport ahead of promise conversion - lots of
logic has become interleaved throughout chttp2, so some effort to
isolate logic out is warranted ahead of that conversion.
What: Split configuration and policy tracking for each of ping rate
throttling and abuse detection into their own modules. Add tests for
them.
Incidentally: Split channel args into their own header so that we can
split the policy stuff into separate build targets.
---------
Co-authored-by: ctiller <ctiller@users.noreply.github.com>
Mark D. Roth
apolcyn
youyuanwu
Yijie Ma
Eugene Ostroukhov
Craig Tiller
Mohan Li