Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automated fix for refs/heads/cf-event-engine-client-ios-test #67

Open
wants to merge 39 commits into
base: cf-event-engine-client-ios-test
Choose a base branch
from

Conversation

github-actions[bot]
Copy link

@github-actions github-actions bot commented Apr 7, 2023

PanCakes to the rescue!

We noticed that our 'sanity' test was going to fail, but we think we can fix that automatically, so we put together this PR to do just that!

If you'd like to opt-out of these PR's, add yourself to NO_AUTOFIX_USERS in .github/workflows/pr-auto-fix.yaml

ctiller and others added 30 commits April 3, 2023 22:19
Notes:
- `+trace` fixtures haven't run since 2016, so they're disabled for now
(grpc@7ad2d0b#diff-780fce7267c34170c1d0ea15cc9f65a7f4b79fefe955d185c44e8b3251cf9e38R76)
- all current fixtures define `FEATURE_MASK_SUPPORTS_AUTHORITY_HEADER`
and hence `authority_not_supported` has not been run in years - deleted
- bad_hostname similarly hasn't been triggered in a long while, so
deleted
- load_reporting_hook has never been enabled, so deleted
(https://github.com/grpc/grpc/blame/f23fb4cf3114787806c330985c8fb3213597a09b/test/core/end2end/generate_tests.bzl#L145-L148)
- filter_latency & filter_status_code rely on global variables and so
don't convert particularly cleanly - and their value seems marginal, so
deleted

---------

Co-authored-by: ctiller <[email protected]>
<!--

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.

-->
This is a prerequisite for upgrading to protobuf 22.x
(upb and protobuf now depend on utf8_range)

Currently utf8_range isn't referenced by anything, but it's better to
bring the subtree in advance to make the protobuf upgrade PR smaller.
Improved the workaround from grpc#32764 as
suggested in
grpc#32764 (comment).

Once merged, I'll backport to 1.54.x and 1.53.x together with
grpc#32764 (these have switched to VS2019)
Followup for grpc#32649 (which disabled the
tests mentioned below).
Also sets correct path for tests build by ninja on windows, so that they
don't get skipped.

Once merged, I'll backport to 1.54.x and 1.53.x

The original issue with tests being skipped.
```
+ python3 workspace_c_windows_dbg_native/tools/run_tests/run_tests.py -t -j 8 -x run_tests/c_windows_dbg_native/sponge_log.xml --report_suite_name c_windows_dbg_native -l c -c dbg --iomgr_platform native --bq_result_table aggregate_results --measure_cpu_costs
2023-03-20 07:56:53,523 START: tools\run_tests\helper_scripts\build_cxx.bat

2023-03-20 08:04:51,388 PASSED: tools\run_tests\helper_scripts\build_cxx.bat [time=477.9sec, retries=0:0; cpu_cost=0.0; estimated=1.0]

2023-03-20 08:04:52,434 detected port server running version 21

2023-03-20 08:04:52,672 my port server is version 21

2023-03-20 08:04:52,703 SUCCESS: All tests passed



WARNING: binary not found, skipping cmake/build/Debug/bad_server_response_test.exe



WARNING: binary not found, skipping cmake/build/Debug/connection_refused_test.exe



WARNING: binary not found, skipping cmake/build/Debug/goaway_server_test.exe



WARNING: binary not found, skipping cmake/build/Debug/invalid_call_argument_test.exe



WARNING: binary not found, skipping cmake/build/Debug/multiple_server_queues_test.exe



WARNING: binary not found, skipping cmake/build/Debug/no_server_test.exe



WARNING: binary not found, skipping cmake/build/Debug/pollset_windows_starvation_test.exe



WARNING: binary not found, skipping cmake/build/Debug/public_headers_must_be_c89.exe



```
Fix python type hints for async iteration on `UnaryStreamCall` and
`StreamStreamCall`. Those classes _are_ `AsyncIterable`s and `__aiter__`
returns an `AsyncIterator`.


<!--

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.

-->
This PR aims to de-experimentalize the APIs for GCP Observability. 

We would have ideally wanted public feedback before declaring the APIs
stable, but we need stable APIs for GA.

Changes made after API review with @markdroth @veblush, @ctiller and the
entire Core/C++ team -
* The old experimental APIs `grpc::experimental::GcpObservabilityInit`
and `grpc::experimental::GcpObservabilityClose` are now deprecated and
will be deleted after v.1.55 release.
* The new API gets rid of the Close method and follows the RAII idiom
with a single `grpc::GcpObservability::Init()` call that returns an
`GcpObservability` object, the lifetime of which controls when
observability data is flushed.
* The `GcpObservability` class could in the future add more methods. For
example, a debug method that shows the current configuration.
* Document that GcpObservability initialization and flushing (on
`GcpObservability` destruction) are blocking calls.
* Document that gRPC is still usable if GcpObservability initialization
failed. (Added a test to prove the same).
* Since we don't have a good way to flush stats and tracing with
OpenCensus, the examples required users to sleep for 25 seconds. This
sleep is now part of `GcpObservability` destruction.

Additional Implementation details -
* `GcpObservability::Init` is now marked with `GRPC_MUST_USE_RESULT` to
make sure that the results are used. We ideally want users to store it,
but this is better than nothing.
* Added a note on GCP Observability lifetime guarantees.

<!--

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.

-->
…r in Aysncio server (grpc#32551)

### Description
When invoking a RPC, sometimes operations in core will fail which result
in an `ExecuteBatchError`, currently we [simply raise this
error](https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi#L705),
and left this Exception unhandled in event loop with error message `Task
exception was never retrieved`.

### Changes included
This PR changes the behavior to log the `ExecuteBatchError` instead of
raising it.

This shouldn't change existing behavior because even without this
change, the exception will simply left on event loop.

### Testing

Tested by manually `set_expection` in [this
future](https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_cython/_cygrpc/aio/callback_common.pyx.pxi#L84),
verified that error was logged correctly and no exception was left in
event loop, sample log:

```
ERROR:grpc._cython.cygrpc:ExecuteBatchError raised in core by servicer method [/grpc.testing.TestService/UnaryCall]
Traceback (most recent call last):
  File "src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi", line 682, in _cython.cygrpc._handle_exceptions
  File "src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi", line 802, in _handle_rpc
  File "src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi", line 547, in _handle_unary_unary_rpc
  File "src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi", line 452, in _finish_handler_with_unary_response
  File "src/python/grpcio/grpc/_cython/_cygrpc/aio/callback_common.pyx.pxi", line 106, in execute_wrong_batch
_cython.cygrpc.ExecuteBatchError: Failed "execute_batch_including_SendInitialMetadataOperation": (<_cython.cygrpc.SendInitialMetadataOperation object at 0x7fa86d936ca0>, <_cython.cygrpc.SendMessageOperation object at 0x7fa86cf48cb0>, <_cython.cygrpc.SendStatusFromServerOperation object at 0x7fa86cf4c110>)
```
grpc#32812)

I added the `CallDispatchController` interface back in grpc#26200 in
preparation for supporting the retry circuit breaker in xDS. However, we
subsequently decided not to support that feature, so we no longer need
this interface. And switching back to a simple on-commit callback should
make it easier to implement stateful session affinity for
WeightedClusters.
This reverts commit 4b46dbc.

Reason: this seems to be breaking load reports in certain cases,
b/276944116

Let's revert so this doesn't accidentally get released.
…32817)

.. as it is now part of the `GcpObservabilityClose()` routine
[itself](https://github.com/grpc/grpc/pull/32715/files#diff-e1ce0ccb4650e20b62a6d6b31655f446ad9ef72efca5849143e65f4a5a5e0310R223).

Background: grpc#32715 may have broken the CI for observability interop
testing. The client seems to be taking too long to finish. More info at
b/277145074

We need to backport this to the `v1.54.x` branch if this is the right
fix.
update to main commit 68d4315167352ffac71f149a43b8088397d3f33d

This is to include the latest RBAC related change
(envoyproxy/envoy#26415) for audit logging.

I intentionally did not run steps in
https://github.com/grpc/grpc/tree/master/third_party#updating-third_partyenvoy-api
because I saw some earlier changes also didn't do that and this
envoy-api change isn't going to be needed in Python any time soon.
…#32809)

This error can trigger for either initial or trailing metadata (and
we've had outages where the latter was the cause).

I don't think we know at this layer if we're parsing initial or trailing
- though it'd be a good exercise to plumb that through.

For now remove the word initial because it's better to give less
information than wrong information.

<!--

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.

-->
These need to run at about the same times, and each runs quickly.
Doesn't seem worthwhile to fire up a VM for each of them individually.




<!--

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.

-->
Various fixes that can be merged in advance, to simplify the 22.x
upgrade.

- Use NOMINMAX for grpcio_tools build to avoid build failure when absl
is added as a dependency. Fixes
grpc#32779
- in spawn_patch command file, put arguments on multiple lines to avoid
lines going over the limit for large link commands: Fixes
grpc#32795
- Use `python/generator.h` instead of the deprecated
`python/python_generator.h` in grpcio_tools.
Expand server promises to run with C++ end2end tests.

Across connected_channel/call/batch_builder/pipe/transport:
- fix a bug where read errors weren't propagated from transport to call
so that we can populate failed_before_recv_message for the c++ bindings
- ensure those errors are not, however, used to populate the returned
call status

Add a new latch call arg to lazily propagate the bound CQ for a server
call (and client call, but here it's used degenerately - it's always
populated). This allows server calls to be properly bound to
pollsets.(1)/(2)

In call.cc:
- move some profiling code from FilterStackCall to Call, and then use it
in PromiseBasedCall (this should be cleaned up with tracing work)
- implement GetServerAuthority

In server.cc:
- use an RAII pattern on `MatchResult` to avoid a bug whereby a tag
could be dropped if we cancel a request after it's been matched but
before it's published
- fix deadline export to ServerContext

In resource_quota_server.cc:
- fix some long standing flakes (that were finally obvious with the new
test code) - it's legal here to have client calls not arrive at the
server due to resource starvation, work through that (includes adding
expectations during a `Step` call, which required some small tweaks to
cq_verifier)

In the C++ end2end_test.cc:
- strengthen a flaky test so it passes consistently (it's likely we'll
revisit this with the fuzzing efforts to strengthen it into an actually
robust test)

(1) It's time to remove this concept
(2) Surprisingly the only test that *reliably* demonstrates this not
being done is time_change_test

---------

Co-authored-by: ctiller <[email protected]>
This changes the `JSON` class to use `absl::variant<>` internally for
storing the type and value.

The only API-visible change here is that now the accessor methods will
crash if you use them on the wrong type, courtesy of `absl::get<>`. This
does not appear to break any tests, so it appears that we have already
cleaned up all of the cases we had in the past where we might have done
something like that.
@HannahShiSFB HannahShiSFB force-pushed the cf-event-engine-client-ios-test branch 2 times, most recently from ba2c057 to 66ce61a Compare April 7, 2023 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.