Add testbench support for GCS+gRPC

[vnghia]

I will work on this feature by extending Python client to support GCS+gRPC with a different endpoint ( /grpc ).
The changes will be:

• Change GrpcClient to work with different endpoint when testing with testbench.
• Add /grpc endpoint to testbench.
• Add test for existing RPCs like CreateBucket()

cc @coryan

[vnghia]

@coryan
Some updates:
HybridClient is now passing integration test. I am using grpc_integration_test with this env set to media

google-cloud-cpp/google/cloud/storage/tests/grpc_integration_test.cc

Line 55 in 70e5077

"GOOGLE_CLOUD_CPP_STORAGE_GRPC_CONFIG", "metadata"};

Because it is a large project, I have created a new repository for it https://github.com/vnvo2409/gcs-testbench

I have some questions:

1. Is it possible if I turn this project into a pip package and google-cloud-cpp will use it instead of the current testbench ? Because there isn't an offical emulator for gcs that supports both grpc and rest, we could extend this project and I think there will be many projects find this emulator useful.

2. Are all the integration tests inside google/cloud/storage/tests/ ?

[coryan]

Some updates:
HybridClient is now passing integration test. I am using grpc_integration_test with this env set to media

google-cloud-cpp/google/cloud/storage/tests/grpc_integration_test.cc

Line 55 in 70e5077

"GOOGLE_CLOUD_CPP_STORAGE_GRPC_CONFIG", "metadata"};

Really cool, thanks!

Because it is a large project, I have created a new repository for it https://github.com/vnvo2409/gcs-testbench

Please remember to include the right license and copyright notices in this code. The majority of that code is owned by Google, and the License requires that you reproduce the Copyright notice in any copies of the code:

google-cloud-cpp/LICENSE

Lines 89 to 128 in 8d87874

	4. Redistribution. You may reproduce and distribute copies of the
	Work or Derivative Works thereof in any medium, with or without
	modifications, and in Source or Object form, provided that You
	meet the following conditions:
	(a) You must give any other recipients of the Work or
	Derivative Works a copy of this License; and
	(b) You must cause any modified files to carry prominent notices
	stating that You changed the files; and
	(c) You must retain, in the Source form of any Derivative Works
	that You distribute, all copyright, patent, trademark, and
	attribution notices from the Source form of the Work,
	excluding those notices that do not pertain to any part of
	the Derivative Works; and
	(d) If the Work includes a "NOTICE" text file as part of its
	distribution, then any Derivative Works that You distribute must
	include a readable copy of the attribution notices contained
	within such NOTICE file, excluding those notices that do not
	pertain to any part of the Derivative Works, in at least one
	of the following places: within a NOTICE text file distributed
	as part of the Derivative Works; within the Source form or
	documentation, if provided along with the Derivative Works; or,
	within a display generated by the Derivative Works, if and
	wherever such third-party notices normally appear. The contents
	of the NOTICE file are for informational purposes only and
	do not modify the License. You may add Your own attribution
	notices within Derivative Works that You distribute, alongside
	or as an addendum to the NOTICE text from the Work, provided
	that such additional attribution notices cannot be construed
	as modifying the License.
	You may add Your own copyright statement to Your modifications and
	may provide additional or different license terms and conditions
	for use, reproduction, or distribution of Your modifications, or
	for any such Derivative Works as a whole, provided Your use,
	reproduction, and distribution of the Work otherwise complies with
	the conditions stated in this License.

I have some questions:

1. Is it possible if I turn this project into a pip package and google-cloud-cpp will use it instead of the current testbench ? Because there isn't an offical emulator for gcs that supports both grpc and rest, we could extend this project and I think there will be many projects find this emulator useful.

There may be some interest in doing that, let's ask @frankyn if he can suggest a repository to host the testbench separately.

2. Are all the integration tests inside google/cloud/storage/tests/ ?

There are also examples (in google/cloud/storage/examples/), which may use slightly different code paths to access the testbench.

[vnghia]

if we make it into a long term project, I will consider switching to Go since there are libraries allow us to run http and grpc in the same port

[vnghia]

@coryan
Any update about hosting the testbench as separate repository ?

[coryan]

We discussed this internally, we would prefer to keep this in google-cloud-cpp. Please let me know how I can help you make progress.

[vnghia]

I got 30/32 integration tests passed.

• error_injection_integration_test always throws Aborting because exceptions are disabled: send() has not been called yet.
• thread_integration_test.ReuseConnections fails. I think the reason is using flask without gunicorn but I am not sure.

Since I didn't follow the current testbench strictly, the new testbench is not perfect. It will need more improvements and I will tell you when it is ready to use.

[coryan]

Thanks for the update!

ReuseConnections may be failing because the server (or testbench) is closing the connections before they can be reused, maybe this needs to become a test we only run against production.

error_injection_integration_test could be the test itself. It performs all kinds of dirty trickscool stuff to return -1 from the C library functions at exactly the right time. The sequence of calls might be different with flask-only and could need adjustment.

[vnghia]

A quick question about googleapis

When we send a POST request to get the resumable session URI, it will always return 200 even if the generation's conditions aren't correct. This issue is already 5 years old googleapis/google-cloud-java#201. GCS REST appends the query parameters to the location and only check for the generation's conditions when upload is done ( we have to use full location resumable_session_id for anything related to resumable upload instead of just upload_id indeed ).

• How is GCS gRPC's behavior ?
• Is there any reason to keep the current GCS REST's behavior ? IMO, checking those conditions in the first place is more reasonable.

[coryan]

GCS REST appends the query parameters to the location and only check for the generation's conditions when upload is done ( we have to use full location resumable_session_id for anything related to resumable upload instead of just upload_id indeed ).

• How is GCS gRPC's behavior ?

I expect that GCS+gRPC will also (a) return OK when the upload is started, regardless of the pre-conditions, (b) save the preconditions on the server side, maybe encoded in the upload id too (say base64 encoded), and (c) will check them at the end.

• Is there any reason to keep the current GCS REST's behavior ?

Yes.

IMO, checking those conditions in the first place is more reasonable.

I do not see how that would work. Consider ifGeneration==0 and two simultaneous uploads, only one of them should succeed so the conditions need to be checked at the end of the upload. More generally, checking the pre-conditions and taking the action that changes them should be an atomic operation, and that can only be done at the end of the upload.

[vnghia]

The new testbench is almost done. My questions:

• The PR will be quite large. Is it ok or I have to do something else ?
• How to deal with protobuf generated python files ? It is particularly hard because they will break the import path if we don't place them in the right place.
• I see some PRs releated to gRPC test: test gRPC data plane on GCS #5031 , etc. In fact, I see many comments related to gRPC in integration tests for REST (e.g in test: test gRPC data plane on GCS #5031 ). What do those comments mean ? Why do we have those comments when we don't have the functions to do it yet ?
• We've already seen the issue session_url is actually session_id in gRPC #5030 before ( in the PR for DeleteResumableUpload ). The new testbench will save all query parameters server-side and only return the location localhost/upload/storage/v1/b/myBucket/o?uploadType=resumable&upload_id=<UPLOAD_ID> without any query parameter. Is this the behavior you want ?

[coryan]

The new testbench is almost done.

Awesome.

• The PR will be quite large. Is it ok or I have to do something else ?

Can you tell me what is "quite large"? How many lines of changes? If it is over 500 lines it might be best to split it, maybe one PR to introduce the gRPC components (without using them at first). Would it be possible to then split even further, say one that uses gRPC for buckets (but nothing else) and then another for objects? Or maybe something along those lines, you would know better what is a good split.

• How to deal with protobuf generated python files ? It is particularly hard because they will break the import path if we don't place them in the right place.

Please remind me: are these generated on the fly by the build system or do we need to commit them to the code? Can we install them via pip?

• I see some PRs related to gRPC test: test gRPC data plane on GCS #5031 , etc. In fact, I see many comments related to gRPC in integration tests for REST (e.g in test: test gRPC data plane on GCS #5031 ). What do those comments mean ? Why do we have those comments when we don't have the functions to do it yet ?

We are trying to enable at least one build to run against production using gRPC. That requires disabling a few tests here and there because production has gaps between gRPC and REST. I think this is orthogonal to your changes.

• We've already seen the issue session_url is actually session_id in gRPC #5030 before ( in the PR for DeleteResumableUpload ). The new testbench will save all query parameters server-side and only return the location localhost/upload/storage/v1/b/myBucket/o?uploadType=resumable&upload_id=<UPLOAD_ID> without any query parameter. Is this the behavior you want ?

I think so, thanks for checking.

[vnghia]

Can you tell me what is "quite large"? How many lines of changes? If it is over 500 lines it might be best to split it, maybe one PR to introduce the gRPC components (without using them at first). Would it be possible to then split even further, say one that uses gRPC for buckets (but nothing else) and then another for objects? Or maybe something along those lines, you would know better what is a good split.

The testbench has been rewritten from scratch, mainly to keep the compatibility between REST and GRPC. Here is the philosophy behind the testbench:

• Pass request (protobuf message, flask.request, FakeRequest) around and only extract the information when need. It allows us to have a common, short function definition for both Rest and gRPC ( something like update(flask.request) / update(request_message) instead of update(flask.request.args.get(...)) / update(request_message.if_match_generation) ).
• The backend uses protobuf instead of dict.
• Most functions have a variable context. It allow us to abort the server without raising an error. (gRPC: context.abort, REST: abort ). I also use context to distinguish between gRPC ( context is not None ) and REST.
• __init__ has no side-effect. I have several init_* to construct the class based on the input request. It is useful if we want to save the data to hard driver and load it back after.
• Break utils into smaller files. Database operations should not be imported into gcs_bucket, gcs_object, ... It ensures that we don't have circular dependency. ( Database should know about the type of gcs_bucket, ... )
• The testbench is fully-compatible with gRPC and REST. New gRPC methods should work just by passing the request into the corresponding funcitons.

You could take a look here https://github.com/vnvo2409/gcs-testbench/tree/refactor. I haven't done the cleanup yet but It could give you a general idea about what I am doing. ( server/rest_server.py, server/grpc_server.py, common/gcs_bucket.py, common/gcs_object.py ). I could break it into PRs. But it is completely unrelated to the old testbench so I am not sure it is the right way.

Please remind me: are these generated on the fly by the build system or do we need to commit them to the code? Can we install them via pip?

These files are used by the testbench so it could be either generated on the fly or commit them to the code. But I think commit them to the code will be much easier. About the pip, I can't find any package but I think you could ask internally about it. pip is the best way of course.

We are trying to enable at least one build to run against production using gRPC. That requires disabling a few tests here and there because production has gaps between gRPC and REST. I think this is orthogonal to your changes.

This test for example.

google-cloud-cpp/google/cloud/storage/tests/object_integration_test.cc

Lines 806 to 810 in 387409c

	TEST_F(ObjectIntegrationTest, DeleteResumableUpload) {
	if (UsingGrpc()) {
	// TODO(5030): DeleteResumableUpload doesn't work with gRPC yet.
	return;
	}

To me, UsingGrpc() will always return false because we don't set any environment variable at all. I think the if is no-op ?

[coryan]

Can you tell me what is "quite large"? How many lines of changes? If it is over 500 lines it might be best to split it,

You could take a look here https://github.com/vnvo2409/gcs-testbench/tree/refactor.

Looks like over 2,900 lines of changes, definitely we need to split this in several PRs.

The testbench has been rewritten from scratch, mainly to keep the compatibility between REST and GRPC. Here is the philosophy behind the testbench:

• Pass request (protobuf message, flask.request, FakeRequest) around and only extract the information when need. It allows us to have a common, short function definition for both Rest and gRPC ( something like update(flask.request) / update(request_message) instead of update(flask.request.args.get(...)) / update(request_message.if_match_generation) ).

Okay, that sounds sensible, but I guess pushes complexity into the update thing as it needs to deal with different request types.

• The backend uses protobuf instead of dict.

SGTM.

• Most functions have a variable context. It allow us to abort the server without raising an error. (gRPC: context.abort, REST: abort ). I also use context to distinguish between gRPC ( context is not None ) and REST.

SGTM too. Maybe this is a good start can you create a PR to make this change to most functions? This is just a suggestion, you would know better how to start the changes.

• __init__ has no side-effect. I have several init_* to construct the class based on the input request. It is useful if we want to save the data to hard driver and load it back after.

• Break utils into smaller files. Database operations should not be imported into gcs_bucket, gcs_object, ... It ensures that we don't have circular dependency. ( Database should know about the type of gcs_bucket, ... )

That SGTM also. Maybe another good place to start.

• The testbench is fully-compatible with gRPC and REST. New gRPC methods should work just by passing the request into the corresponding funcitons.

I could break it into PRs. But it is completely unrelated to the old testbench so I am not sure it is the right way.

Well, one way to do it is to:

• Start a new directory for the new testbench, testbench/grpc or testbench-grpc are my suggestions.
• Implement the testbench in this directory in a series of PRs, think about 200-400 lines of code for each new one, avoid anything over 500.
• Once completed, change one of the build scripts to use this new testbench, it may require several iterations of fixing the new testbench and/or the scripts.
• Once it is working, change all the scripts to use the new testbench.
• Then remove the old one.
• Then (if we want to) rename the new one to simply testbench.

Please remind me: are these generated on the fly by the build system or do we need to commit them to the code? Can we install them via pip?

These files are used by the testbench so it could be either generated on the fly or commit them to the code. But I think commit them to the code will be much easier. About the pip, I can't find any package but I think you could ask internally about it. pip is the best way of course.

We are trying to enable at least one build to run against production using gRPC. That requires disabling a few tests here and there because production has gaps between gRPC and REST. I think this is orthogonal to your changes.

This test for example.

google-cloud-cpp/google/cloud/storage/tests/object_integration_test.cc

Lines 806 to 810 in 387409c

	TEST_F(ObjectIntegrationTest, DeleteResumableUpload) {
	if (UsingGrpc()) {
	// TODO(5030): DeleteResumableUpload doesn't work with gRPC yet.
	return;
	}

To me, UsingGrpc() will always return false because we don't set any environment variable at all. I think the if is no-op ?

That is true for most builds, but we are preparing a build that will set GOOGLE_CLOUD_CPP_STORAGE_GRPC_CONFIG, in that case UsingGrpc() will return true. We may have to change the code to something like:

// TODO(#...) - production does not support DeleteResumableUpload() yet.
if (!UsingTestbench() && UsingGrpc()) GTEST_SKIP();

Or maybe we get to remove the code.

[vnghia]

A series of PRs will take some time to complete. I think it will finish by October.

These files are used by the testbench so it could be either generated on the fly or commit them to the code. But I think commit them to the code will be much easier. About the pip, I can't find any package but I think you could ask internally about it. pip is the best way of course.

How about the generated files ?

[coryan]

A series of PRs will take some time to complete. I think it will finish by October.

These files are used by the testbench so it could be either generated on the fly or commit them to the code. But I think commit them to the code will be much easier. About the pip, I can't find any package but I think you could ask internally about it. pip is the best way of course.

How about the generated files ?

Trying to find if there are any. I would prefer to generate them on the fly over committing them, and I would prefer to use pip over generating them.