PHP & Ruby xds interop tests: add support for xds splitting and routing by stanley-cheung · Pull Request #23763 · grpc/grpc

stanley-cheung · 2020-08-07T09:27:02Z

Internal bug: PHP b/161801379, Ruby b/161801377.

Add support in xDS interop tests for traffic splitting and route matching, for PHP and Ruby, in particular, the new path_matching and header_matching test cases. For the spec doc, please check the internal bug linked at the top.

Current status:

The main changes are in src/php/tests/interop/xds_client.php and src/ruby/pb/test/xds_client.rb.
- Need to support additional command line parameters --rpc= and --metadata=.
- Instead of just sending UnaryCall RPC, now we can be sending a list of RPCs specified by --rpc=.
- Need to add rpcs_by_method result histogram, in addition to the rpcs_by_peers already being returned.
[Done and resolved] The generated code might need some more clean up. Please ignore them for now. For Ruby generated code, we might need to get [ruby] module path names seems to be broken in >= v1.30.2 #23672 resolved first. There will be no generated code change for Ruby. All already done in Ruby: use absolute module name for request/response namespaces #23765 .
There will be some PHP generated code changes.
~~Doing a run of~~ Adhoc xds test each for PHP and Ruby all passed.

stanley-cheung · 2020-08-07T17:23:44Z

Tests are not passing if run from kokoro for some reason. Looking into it. Things are fine when I run this locally. I was able to pass path_matching and header_matching when run locally. But now everything is failing when run from Kokoro for some reason. Still looking into it.

menghanl · 2020-08-07T19:15:37Z

-  --test_case=all \
+  --test_case=ping_pong \
  --project_id=grpc-testing \
  --source_image=projects/grpc-testing/global/images/xds-test-server \


Update this to projects/grpc-testing/global/images/xds-test-server-2 (Append -2)
grpc/grpc-go#3764 (files)

Done. Thanks for the pointer. Yea I also noticed the discrepancy.

stanley-cheung · 2020-08-07T22:43:30Z

Now all PHP xds test cases are passing. Adhoc run: https://g3c.corp.google.com/results/invocations/7f2b5361-2808-4c17-a1d0-2e5fa87ad43c/targets.

And Ruby is passing too: https://g3c.corp.google.com/results/invocations/0f46ed89-b354-4957-b9ce-1e90f6f66e88/targets.

menghanl

Thanks for the change and the test results. LGTM!

menghanl · 2020-08-11T21:41:19Z

@stanley-cheung This PR is approved, but it still a draft. Are you planning to merge it soon? Thanks!

stanley-cheung · 2020-08-11T21:46:35Z

There's still some issues with the Ruby generated code in this PR - I'd like to have #23672 resolved first. We are getting close there.

stanley-cheung · 2020-08-14T08:09:04Z

The PR #23765 is going to fix a bug in the Ruby protoc plugin. In the process, we are going to re-generate some generated files, which will contain some changes to the .proto files here (e.g. the new LoadBalancerStatsResponse.rpcs_by_method field that will be used by these updated new xDS test cases here).

That PR is almost done. Will update this PR, to take advantage of those updated .rb files, after that PR is merged.

stanley-cheung · 2020-08-15T00:31:15Z

Somehow I am having trouble with Ruby + round_robin right now. Looking into it.

One adhoc run that failed: https://g3c.corp.google.com/results/invocations/283bb414-5ea4-489b-88bc-dc3829d7f933/targets;showStatuses=failed,passed

stanley-cheung · 2020-08-15T03:58:34Z

@apolcyn Can you please review the Ruby part src/ruby/pb/test/xds_client.rb? For the spec doc, please see the top of this PR.

PHP Adhoc run passing: https://g3c.corp.google.com/results/invocations/6cb7ed07-e304-47d8-b55d-4957b0027ea1/targets
Ruby Adhoc run passing: https://g3c.corp.google.com/results/invocations/a0790105-a410-45aa-b075-a8471aab1547/targets

apolcyn · 2020-08-17T17:37:25Z

    end
+    # convert results into proper proto object
+    rpcs_by_method = {}
+    watcher['rpcs_by_method'].each do | rpc_name, rpcs_by_peer |


small nit: we don't have spaces before or after opening and closing | parameter brackets in do blocks elsewhere in the code, lets remove for consistency

apolcyn · 2020-08-17T17:45:21Z

+  remote_peer = ""
+  begin
+    op.execute
+    if op.metadata.key?('hostname')


Where is the hostname metadata key getting populated?

This is the updated spec for this xDS interop test - that the remote hostname will be returned as part of the call's initial metadata, instead of in the RPC's response proto.

In the past, the remote hostname is being returned as a field in the SimpleResponse proto https://github.com/grpc/grpc/blob/v1.31.x/src/ruby/pb/test/xds_client.rb#L127, https://github.com/grpc/grpc/blob/master/src/proto/grpc/testing/messages.proto#L120. But since now, after the updated spec, we could be sending EmptyCall RPC, we can't be expecting this field to be in the response proto uniformly. So the updated spec specified that this key will be populated as part of the initial metadata.

Also, an RPC may be designed to not be successful as part of the test, so we need to check whether that key exists in the initial metadata as well.

Got it, thanks.

Also, an RPC may be designed to not be successful as part of the test, so we need to check whether that key exists in the initial metadata as well.

Slightly confused about this part, though. Looks like we currently don't check the peer via metadata at all if the call fails.

The spec mentioned that All clients read hostname from header[“hostname”] instead of from RPC response. It seems that it's unclear whether the initial metadata should be read if the RPC failed. I will follow up with Menghan and Eric to see what the behavior should be. As it stands, it passes all the test cases so perhaps it doesn't matter for now. I am inclined to merge this PR as is. If there's an issue with this particular piece of logic we can always fix it later.

Right, whether to read "hostname" from metadata when the RPC fails is currently not defined. Mostly because none of the existing tests depends on hostname when the RPC fails. It's OK for the client to ignore it.

This may change in the future, to test failure cases (fault injection for example). But it not super clear how the behavior will be, so let's worry about it later.

apolcyn · 2020-08-17T17:48:12Z


+# execute 1 RPC and return remote hostname
+def execute_rpc(op, fail_on_failed_rpcs)
+  remote_peer = ""


Question about the spec -- are the keys in rpcs_by_peer supposed to be hostnames, or socket addresses?

If socket addresses, we can use the peer getter on ops.

Similar response to above - according to the new updated test spec, we need to expect this hostname key in the call's initial metadata.

apolcyn · 2020-08-17T17:49:30Z

-          watcher['no_remote_peer'] += 1
-        else
-          watcher['rpcs_by_peer'][remote_peer] += 1
+        results.each do | rpc_name, remote_peer |


same nit here for spaces around do/end block parameter brackets

apolcyn

LGTM for ruby, with just a couple of questions

apolcyn · 2020-08-17T20:26:54Z

+  remote_peer = ""
+  begin
+    op.execute
+    if op.metadata.key?('hostname')


Got it, thanks.

Also, an RPC may be designed to not be successful as part of the test, so we need to check whether that key exists in the initial metadata as well.

Slightly confused about this part, though. Looks like we currently don't check the peer via metadata at all if the call fails.

apolcyn · 2020-08-17T20:32:00Z

    end
    $watchers_mutex.synchronize do
      $watchers.each do |watcher|
+        # this is counted once when each group of all rpcs_to_send were done


is there a place in the spec that says this? If so can we link this comment to there?

The "change" to the spec, which this PR addressed, hasn't been merged to the main xDS test description doc yet. It's just in an internal doc linked from the internal bug b/161801377.

In there, it specified that the new --rpc param will specify the list of RPCs to make (i.e. UnaryCall, EmptyCall, etc). And --qps=10 means to make 10 RPCs/second for each type.

stanley-cheung · 2020-08-17T21:02:20Z

This should be good to go. All review comments addressed. Merging. Thanks @apolcyn and @menghanl for the review!

stanley-cheung · 2020-08-18T00:33:18Z

Both PHP and Ruby have a green run on master after this PR is merged.

stanley-cheung added lang/php lang/ruby release notes: no Indicates if PR should not be in release notes labels Aug 7, 2020

stanley-cheung requested review from apolcyn and ericgribkoff August 7, 2020 09:27

stanley-cheung marked this pull request as draft August 7, 2020 17:22

menghanl reviewed Aug 7, 2020

View reviewed changes

stanley-cheung force-pushed the xds-enhancement branch from 00167fc to d984bbe Compare August 7, 2020 19:54

stanley-cheung force-pushed the xds-enhancement branch from 89a6900 to 62eaa25 Compare August 8, 2020 00:18

menghanl approved these changes Aug 10, 2020

View reviewed changes

stanley-cheung mentioned this pull request Aug 14, 2020

Ruby: use absolute module name for request/response namespaces #23765

Merged

stanley-cheung force-pushed the xds-enhancement branch 3 times, most recently from 1a1a77e to 5bdab78 Compare August 15, 2020 00:22

stanley-cheung force-pushed the xds-enhancement branch 3 times, most recently from 76fe9c1 to 7fc5b52 Compare August 15, 2020 03:55

stanley-cheung marked this pull request as ready for review August 15, 2020 03:55

apolcyn reviewed Aug 17, 2020

View reviewed changes

stanley-cheung added 2 commits August 17, 2020 18:45

Update XDS interop tests for PHP and Ruby

cbe0f31

Review fixes

7c794b9

stanley-cheung force-pushed the xds-enhancement branch from 7fc5b52 to 7c794b9 Compare August 17, 2020 18:45

apolcyn approved these changes Aug 17, 2020

View reviewed changes

stanley-cheung merged commit f3c80ad into grpc:master Aug 17, 2020

stanley-cheung deleted the xds-enhancement branch August 17, 2020 21:27

stanley-cheung mentioned this pull request Aug 18, 2020

Backport "PHP & Ruby xds interop tests: add support for xds splitting and routing" to v1.31.x #23874

Merged

Conversation

stanley-cheung commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stanley-cheung commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stanley-cheung commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

menghanl left a comment

Choose a reason for hiding this comment

Uh oh!

menghanl commented Aug 11, 2020

Uh oh!

stanley-cheung commented Aug 11, 2020

Uh oh!

stanley-cheung commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stanley-cheung commented Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stanley-cheung commented Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apolcyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stanley-cheung commented Aug 17, 2020

Uh oh!

stanley-cheung commented Aug 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stanley-cheung commented Aug 7, 2020 •

edited

Loading

stanley-cheung commented Aug 7, 2020 •

edited

Loading

stanley-cheung commented Aug 7, 2020 •

edited

Loading

stanley-cheung commented Aug 14, 2020 •

edited

Loading

stanley-cheung commented Aug 15, 2020 •

edited

Loading

stanley-cheung commented Aug 15, 2020 •

edited

Loading