blob/gcsblob: support the gRPC API and Rapid Storage (zonal) buckets - #3772
blob/gcsblob: support the gRPC API and Rapid Storage (zonal) buckets#3772stanhu wants to merge 2 commits into
Conversation
|
Can you merge with HEAD? I think that might fix the golangci-lint problem. |
| option.WithEndpoint("http://" + host + "/storage/v1/"), | ||
| option.WithHTTPClient(http.DefaultClient), | ||
| } | ||
| // storage.NewClient and storage.NewGRPCClient share the same signature; the |
There was a problem hiding this comment.
So, the "client *gcp.HTTPClient" passed in to the constructor here is getting ignored? That seems odd.
Maybe enforce that client is nil to make that more clear?
There was a problem hiding this comment.
OpenKeeper for KMS has a constructor that takes a client (so that the caller can do whatever with it); maybe that's a better pattern here?
I.e., the grpc=true URL option is fine, and controls what the URL opener does, but there's no "UseGRPC" Option; instead, there are two separate OpenBucket constructors, one for HTTP and one for gRPC, where the latter takes a storage.Client, and we provide a Dial to create it pre-wrapped similar to KMS ("cloudkms.NewKeyManagementClient(ctx, option.WithTokenSource(ts), useragent.ClientOption("secrets"))").
There was a problem hiding this comment.
@vangent I've updated this pull request to have:
func OpenBucket(ctx context.Context, client *gcp.HTTPClient, bucketName string, opts *Options) (*blob.Bucket, error)
func OpenBucketGRPC(client *storage.Client, bucketName string, opts *Options) (*blob.Bucket, error) {I've kept grpc=true and zonal=true for query parameter support because this is the standard way to access a bucket with OpenBucketURL.
| h.closer() | ||
| } | ||
|
|
||
| func TestConformance(t *testing.T) { |
There was a problem hiding this comment.
I don't want to merge this without running it through the conformance test.
You should be able to make a new function here, TestConformanceGRPC (and maybe another one, TestConformanceGRPCZonal), that uses a different newHarness-equivalent function (or refactor newHarness) that creates a gRPC client etc.
To generate the golden files locally you'll need to update the constants at the top of the file and run with --record. I'll ask you to revert the constant changes before merging, and I'll re-generate the golden files with our bucket after that.
There was a problem hiding this comment.
TestConformanceGRPCZonal currently fails for a number of reasons:
TestCopy/Works
got unexpected error copying blob: (code=InvalidArgument):
Rapid storage class objects do not support rewrite.
TestListDelimiters/backslash
(code=InvalidArgument): Invalid argument. # non-"/" delimiter on an HNS bucket
TestWrite/write_with_explicit_ContentType_overrides_discovery
NewWriter or Close got err (code=ResourceExhausted):
The object <rapid bucket>/blob-for-reading exceeded the rate limit for object
mutation operations (create, update, and delete).
https://docs.cloud.google.com/storage/docs/rapid/rapid-bucket mentions that object rewrites are not supported (https://docs.cloud.google.com/storage/docs/json_api/v1/objects/rewrite).
Rapid Storage also requires / as the delimeter (https://cloud.google.com/blog/products/storage-data-transfer/understanding-new-cloud-storage-hierarchical-namespace), so those tests fail as well.
For now I'll omit TestConformanceGRPCZonal until there's a better way to selectively disable conformance tests.
There was a problem hiding this comment.
Can you go ahead and add the test, but comment it out with some comments explaining the above? When I have time I'll try to make it easier to disable specific conformance tests (with explanation) so that they can be enabled.
There was a problem hiding this comment.
Or "skip" rather than comment out.
There was a problem hiding this comment.
@vangent Done!
To generate the golden files locally you'll need to update the constants at the top of the file and run with --record. I'll ask you to revert the constant changes before merging, and I'll re-generate the golden files with our bucket after that.
In order to generate the golden files for gRPC, we need this fix for go-replayers: google/go-replayers#70
60f3aab to
953d649
Compare
Rapid Storage (zonal) buckets cannot be used through this driver at
all. Every write is rejected, on both transports:
gs://bucket?grpc=true
InvalidArgument: This bucket type only supports appendable objects
gs://bucket
googleapi: Error 400: This bucket requires appendable objects
An appendable object is uploaded over a bidirectional stream. It
becomes visible as soon as the first bytes are flushed and stays open
for further writes until something finalizes it. Ordinary uploads,
one-shot or resumable, produce an object only once the whole payload
has been sent. Zonal buckets support the appendable form and nothing
else, which is why both transports reject a normal write.
Two things are needed. experimental.WithZonalBucketAPIs makes
ObjectHandle.NewWriter default Writer.Append to true, selecting the
appendable upload path, and switches reads to the bidirectional API.
Writer.FinalizeOnClose then makes Close finalize the object; without it
Close leaves an unfinalized object exposing only whatever prefix was
flushed, which for a small payload is a zero-length object even though
Close reported no error.
Reads already worked over both transports without any change.
The new API is a Dial plus a constructor, following secrets/gcpkms:
func DialGRPC(ctx context.Context, ts gcp.TokenSource, opts ...option.ClientOption) (*storage.Client, func(), error)
func OpenBucketGRPC(client *storage.Client, bucketName string, opts *Options) (*blob.Bucket, error)
c, cleanup, err := gcsblob.DialGRPC(ctx, ts, experimental.WithZonalBucketAPIs())
defer cleanup()
b, err := gcsblob.OpenBucketGRPC(c, "my-rapid-bucket", nil)
OpenBucketGRPC takes no context, matching gcpkms.OpenKeeper: once a
client exists there is nothing left to cancel. Options.Client, which
has accepted a *storage.Client since v0.44.0, keeps working.
For callers whose whole configuration is a URL string and who therefore
cannot supply a client, the URL opener grows two parameters. grpc=true
selects the gRPC transport; zonal=true additionally enables the zonal
APIs and implies grpc=true. Combining zonal=true with an explicit
grpc=false is a contradiction and is rejected rather than silently
overridden. URLOpener gains a TokenSource, populated by lazyCredsOpener
from the credentials it already resolves, because a gRPC client cannot
reuse an HTTP one. A URL that asks for gRPC without a token source and
without anonymous=true is now an error instead of quietly producing an
unauthenticated client.
Buckets that build their own client close it in Close, which was
previously a no-op. A gRPC client owns a connection pool, so otherwise
every OpenBucketURL leaked one for the process lifetime. Clients passed
in by the caller are left alone.
FinalizeOnClose is set unconditionally in NewTypedWriter. The storage
library reads it only on the appendable write path, which
pickBufferSender selects solely when Writer.Append is set, so the
JSON/HTTP and plain gRPC paths are unaffected.
Emulator support on the gRPC path is left to storage.NewGRPCClient.
Reusing STORAGE_EMULATOR_HOST would not work: it is the HTTP endpoint,
and a local emulator needs a separate port for gRPC. Passing it to
option.WithEndpoint alongside option.WithoutAuthentication would also
still dial over TLS, since skipping credentials does not make the
transport plaintext. defaultGRPCOptions already reads
STORAGE_EMULATOR_HOST_GRPC, strips the scheme, dials insecurely and
disables client metrics, and those defaults are merged ahead of
caller-supplied options. lazyCredsOpener checks that variable too, so
pointing only the gRPC one at an emulator does not trigger an
Application Default Credentials lookup.
gRPC is not a speedup on its own. Measured from an n2-standard-4 in
us-central1-c, 1 KiB objects each read exactly once, n=1000, p50: on a
NAM4 dual-region bucket 35.5ms over gRPC against 35.4ms over JSON, and
on a US multi-region bucket 59.2ms against 61.2ms. The win comes from
the zonal bucket, at 12.0ms. The UseGRPC docs say to measure first.
3c0fa39 to
9983ce2
Compare
|
Let me know when you're ready for another round of review, you'll need to upload the golden files as part of the PR for it to pass I think. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3772 +/- ##
==========================================
+ Coverage 79.97% 80.04% +0.07%
==========================================
Files 104 104
Lines 12219 12284 +65
==========================================
+ Hits 9772 9833 +61
- Misses 2446 2450 +4
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
9983ce2 to
7e35981
Compare
Add TestConformanceGRPC, which runs the full drivertest suite over a
gRPC client built by DialGRPC, and TestConformanceGRPCZonal, which does
the same with the zonal bucket APIs enabled.
Both run against a real bucket named by GCSBLOB_GRPC_TEST_BUCKET and
skip when it is unset, rather than using the record/replay harness
TestConformance uses.
Replay does not work with released grpcreplay. storage reads objects
with a zero-copy codec, installed unconditionally in
NewRangeReaderReadObject as grpc.ForceCodecV2(bytesCodecReadObject{}),
so RecvMsg is handed a *mem.BufferSlice instead of a proto.Message.
grpcreplay assumes every message is a proto.Message and panics on the
unchecked type assertion in message.set, aborting the test binary on
the first gRPC read. Recording writes works; it is reads that cannot be
captured.
panic: interface conversion: *mem.BufferSlice is not
protoreflect.ProtoMessage: missing method ProtoReflect
grpcreplay.(*message).set(...)
grpcreplay.(*recClientStream).RecvMsg(...)
storage.(*grpcStorageClient).NewRangeReaderReadObject...
google/go-replayers#70 fixes this by recording
such messages as raw wire bytes. Verified against that branch: writes,
full reads, out-of-order reads, range reads and unary calls all record
and then replay offline with no credentials and no network. Once it is
released and go.mod picks it up, these tests should move to the
record/replay harness and stop needing a real bucket. Until then they
need one, so they skip by default.
Against a US multi-region bucket TestConformanceGRPC passes all 88
checks.
TestConformanceGRPCZonal is additionally skipped unconditionally.
Against a Rapid Storage bucket 55 checks pass and 33 fail, and every
failure is a Cloud Storage restriction rather than a driver bug.
drivertest cannot express "this driver does not support X": the only
opt-out is returning gcerrors.Unimplemented, every place that honors it
guards SignedURL, and testCopy treats any error from Copy as a failure.
The test is kept, with a comment naming the specific subtests to
disable and why, so it can be enabled once that is possible:
- TestCopy, TestKeys and TestAs, because Rapid Storage does not
support object rewrite. TestKeys accounts for 19 of the 33 failures
on its own, since it copies once per key it exercises.
- TestListDelimiters/backslash and TestListDelimiters/abc, because a
hierarchical namespace, which Rapid Storage requires, only supports
"/" as a delimiter.
- Four TestWrite subtests that rewrite one object in a tight loop and
hit the general per-object mutation rate limit. That is not a Rapid
Storage restriction and only appears when the client is close
enough to trip it, so it may not need a permanent skip.
SignedURL is left unexercised: with no GoogleAccessID the driver
reports Unimplemented and drivertest skips those checks, so HTTPClient
returns nil. Signing is client-side and does not depend on transport.
7e35981 to
959234e
Compare
grpcreplay assumed every message on a stream was a proto.Message and did an unchecked type assertion in message.set. RPCs that install a custom gRPC codec break this assumption. The GCS client's zero-copy ReadObject codec forces grpc.ForceCodecV2 and receives each message into a *mem.BufferSlice, so RecvMsg panics: interface conversion: *mem.BufferSlice is not protoreflect.ProtoMessage Recording writes worked, but reads could not be captured, which is why the go-cloud gcsblob gRPC test ran only against a real bucket. Record such messages as raw wire bytes instead. message now holds either a proto.Message or a []byte; message.set type-switches and materializes a copy of the BufferSlice (gRPC frees the buffers after RecvMsg returns). A new raw_message field on the Entry proto carries the bytes, and replay delivers them back into the caller's *mem.BufferSlice, mirroring the codec's Unmarshal. This requires google.golang.org/grpc v1.67.1 for the mem package. Reported in google/go-cloud#3772
2 participants
This pull request adds support for the Cloud Storage gRPC API to
gcsblob, and on top of it, support for Rapid Storage (zonal) buckets.API
OpenBucketGRPCtakes no context, matchinggcpkms.OpenKeeper: once a client exists there is nothing left to cancel.Options.Client, which has accepted a*storage.Clientsince v0.44.0, keeps working unchanged.For callers whose entire configuration is a URL string, and who therefore have nowhere to put a client, there are two query parameters:
grpc=truezonal=truegrpc=true.Combining
zonal=truewith an explicitgrpc=falseis rejected rather than silently overridden.URLOpenergains aTokenSource, populated bylazyCredsOpenerfrom the credentials it already resolves, because a gRPC client cannot reuse an HTTP one.Why Rapid Storage needs more than a transport switch
Zonal buckets accept only appendable object uploads. Every ordinary write is rejected, on both transports:
An appendable object is uploaded over a bidirectional stream. It becomes visible as soon as the first bytes are flushed and stays open for further writes until something finalizes it. Ordinary uploads, one-shot or resumable, produce an object only once the whole payload has been sent. Zonal buckets support the appendable form and nothing else.
So two things are needed:
experimental.WithZonalBucketAPIs()on the client. It makesObjectHandle.NewWriterdefaultWriter.Appendto true, which selects the appendable upload path, and switches reads to the bidirectional API.Writer.FinalizeOnCloseon the writer. An appendable object stays open by default, soCloseleaves an unfinalized object exposing only the bytes that happened to be flushed. For a payload small enough to fit in one buffer that is a zero-length object, even thoughClosereturned no error.Reads already worked on both transports without any change.
Notes for review
FinalizeOnCloseis set unconditionally inNewTypedWriter. This looked risky to me too, so I traced it. The field never appears inhttp_client.go, so the JSON/HTTP writer cannot see it. Ingrpc_writer.gothe only read isgRPCAppendBidiWriteBufferSender.send, andpickBufferSenderreturns that sender only whenWriter.Appendis set. It is dead code on every pathgcsblobuses today.Buckets now close the client they created.
Closewas a no-op. A gRPC client owns a connection pool, so everyOpenBucketURLwithgrpc/zonalleaked one for the process lifetime. Clients supplied by the caller, throughOpenBucketGRPCorOptions.Client, are left alone.Emulator handling on the gRPC path is left to the storage library. Reusing
STORAGE_EMULATOR_HOSTwould not work: it is the HTTP endpoint, and a local emulator needs a separate port for gRPC. Passing it tooption.WithEndpointalongsideoption.WithoutAuthenticationwould also still dial over TLS, because skipping credentials does not make the transport plaintext.defaultGRPCOptionsalready readsSTORAGE_EMULATOR_HOST_GRPC, strips the scheme, dials insecurely and disables client metrics, and those defaults are merged ahead of caller-supplied options.lazyCredsOpenerchecks that variable too.gRPC on its own is not a speedup. From an
n2-standard-4inus-central1-c, 1 KiB objects each read exactly once, n=1000:zonal=true* Not comparable: a zonal bucket rejects every write over JSON/HTTP.
On the standard buckets plain gRPC is a wash, and on a smaller
e2-standard-4it was consistently slower, so the docs say to measure before enabling it. The win is the zonal bucket: 2.9x faster than NAM4 and 5.1x faster than multi-region.Testing
go test ./blob/gcsblob/passes and the existing replay tests are unaffected. New unit tests cover thegrpcandzonalparameters, their invalid values, thezonal=trueplusgrpc=falseconflict, andOpenBucketGRPCincluding that the caller's client survivesbucket.Close.TestConformanceGRPCruns the full drivertest suite over gRPC. Against a US multi-region bucket all 88 checks pass, repeatedly.It runs against a real bucket named by
GCSBLOB_GRPC_TEST_BUCKETand skips when unset, so it contributes nothing in CI today. That is not a choice I would have made if replay worked. With releasedgrpcreplayit cannot:storagereads objects with a zero-copy codec, installed unconditionally inNewRangeReaderReadObjectasgrpc.ForceCodecV2(bytesCodecReadObject{}), soRecvMsgis handed a*mem.BufferSliceinstead of aproto.Message.grpcreplayassumes every message is aproto.Messageand panics on the unchecked type assertion inmessage.set, aborting the test binary on the first gRPC read. Recording writes works; reads cannot be captured.google/go-replayers#70 fixes this, by recording such messages as raw wire bytes. I verified it against that branch: writes, full reads, out-of-order reads, range reads and unary calls all record, and then replay offline with no credentials and no network. 18 RPCs across 13 streams, so
grpcreplay's stream matching holds up too.So this PR has a dependency. Once that change is released and
go.modpicks it up,TestConformanceGRPCshould move to the record/replay harness, stop needing a real bucket, and start contributing in CI. Until then it skips by default. Happy to do that follow-up once the release is out, or to hold this PR for it if you would rather not merge a test that skips.I also verified against a real Rapid Storage bucket that writes, reads, attributes, range reads and 1 MiB writes all succeed where they fail outright on master, and that objects written through
zonal=truecome back finalized,size=23with a non-zeroFinalized, againstsize=0without theFinalizeOnCloseline.TestConformanceGRPCZonal is skipped
It is present but skipped unconditionally, because it cannot pass. Against a Rapid Storage bucket, 55 checks pass and 33 fail, none of them driver bugs:
Rapid storage class objects do not support rewriteaccounts for three of the five failing groups. OnlyTestCopyis about copying;TestKeysandTestAscopy incidentally, andTestKeysalone contributes 19 failures because it copies once per weird key./fails withInvalid argument. That is a hierarchical namespace restriction, which Rapid Storage inherits by requiring HNS, rather than anything to do with zonal buckets or gRPC.TestWritehits the per-object mutation rate limit. Not a Rapid Storage limit at all, just the general GCS cap, and it only appeared when running from a VM in the bucket's zone, fast enough to trip it. The failure count varied with distance: 27 from a laptop, 33 from in-zone.drivertestcannot express any of this. Its only opt-out is theUnimplementederror code, and all six places that honor it guardSignedURL;testCopytreats any error fromCopyas a failure. The test's doc comment names the exact subtests to disable and why, so it can be switched on by deleting onet.Skiponce selective disabling exists.What this PR does not do
It does not deliver the sub-millisecond reads Rapid Storage is advertised for.
Sub-millisecond is possible, but under certain conditions. You only get it on later reads of one particular object, from a process that has kept a bidirectional read stream open to that object. The first read of any given object costs 11 to 12 ms no matter which API you use.
Same 1 KiB object, same client, n=1000:
NewReader. This is what the driver does.MultiRangeDownloaderfor itReadHandlecached from the earlier readGetting there means keeping state alive between reads. Two ways:
MultiRangeDownloaders per object on the driver'sbucket. Reaches the third row. Needs idle expiry, cleanup frombucket.Close, and an adapter fromAdd's callback toio.Reader.ReadHandles per object. Much smaller, but only reaches the fourth row.Neither has anywhere to live today.
driver.Bucket.NewRangeReaderhands back a freshdriver.Readerper call, and the portable layer makes a new one for every read and discards it on a Seek.What changed since the review
Options.UseGRPCandOptions.UseZonalAPIsremoved. Replaced byDialGRPCandOpenBucketGRPC, per yourgcpkmssuggestion.gcp.HTTPClientis gone from the gRPC path. You were right that it was odd. It was not entirely ignored, its OAuth2 token source was reused, but everything else about it was dropped, and when the transport was not exactly an*oauth2.Transportthe code silently fell back tooption.WithoutAuthentication. A caller with a wrapped transport got an anonymous client and 403s at request time. Now a nil token source means unauthenticated explicitly, and a gRPC URL with neither credentials noranonymous=trueis an error.Closeused to leak.12ac9a59and fixes the golangci-lint failure. That failure was never related to this change; it was Go 1.27 against golangci-lint v2.12.TestConformanceGRPCandTestConformanceGRPCZonaladded, with the caveats above. The zonal one is skipped unconditionally with a comment naming the subtests that need disabling and why, per your suggestion.