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
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
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
|
I updated deps so that we've got the latest go-replayers including your contribution, so you should be able to add the gRPC replay golden files now. |
959234e to
6911d6d
Compare
Thanks! It looks like |
Done |
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.
e58b4a9 to
8a23bf5
Compare
Add TestConformanceGRPC, which runs the full drivertest suite over the
Cloud Storage gRPC API, recording to and replaying from golden files the
same way TestConformance does. All 88 checks pass.
grpcBucketName is separate from bucketName because the bucket name is
part of every recorded gRPC request, so a replay only matches against
the bucket it was recorded from. Keeping the two apart means
regenerating one set of golden files cannot invalidate the other. These
were recorded against a personal bucket, so the constant and
testdata/TestConformanceGRPC need regenerating together; the procedure
is in the test's doc comment.
TestConformanceGRPCZonal covers the same suite with the zonal bucket
APIs enabled, and is skipped unconditionally. Against a Rapid Storage
bucket 55 checks pass and 33 fail, every one a Cloud Storage restriction
rather than a driver bug, and drivertest cannot express "this driver
does not support X": its only opt-out is gcerrors.Unimplemented, every
place honoring it guards SignedURL, and testCopy treats any error from
Copy as a failure. The test carries a comment naming the subtests to
disable and why, so it can be enabled by deleting one t.Skip:
- 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. Not a Rapid Storage
restriction, and only visible 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 the transport.
8a23bf5 to
8584a07
Compare
|
Thanks! I've updated the conformance tests. To re-record the golden files:
Recording needs Application Default Credentials with write access to that bucket. Restricting |
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:
storage.WithAppendableUploads()andstorage.WithGRPCBidiReads()on the client. The first makesObjectHandle.NewWriterdefaultWriter.Appendto true, which selects the appendable upload path; the second switches reads to the bidirectional API. These were a singleexperimental.WithZonalBucketAPIs()until storage v1.67.0 split them and graduated both out ofexperimental.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 records to and replays from
testdata/TestConformanceGRPC, the same wayTestConformancedoes. 79 golden files, 748 KB. Four consecutive offline replays with credentials pointed at nonexistent paths give identical results.Getting there needed three fixes to
grpcreplay, all now merged and released in v1.5.0, which master already depends on:storagereads objects with a zero-copy codec, soRecvMsgis handed a*mem.BufferSlicerather than aproto.Messageandgrpcreplaypanicked on the first read.The golden files need regenerating.
grpcBucketNameis currently a personal bucket, because the bucket name is part of every recorded gRPC request and a replay only matches against the bucket that was recorded. It is deliberately a separate constant frombucketNameso that regenerating the gRPC files cannot invalidate the JSON/HTTP ones. The constant andtestdata/TestConformanceGRPChave to be regenerated together, with--record.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. Deleting one
t.Skipenables it once specific conformance tests can be disabled. 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.TestConformanceGRPCis now a record/replay test with committed golden files, passing 88 of 88. This needed threegrpcreplayfixes, all merged and released in v1.5.0.experimental.WithZonalBucketAPIs, which storage v1.67.0 removed. It is nowstorage.WithAppendableUploads()plusstorage.WithGRPCBidiReads(), both of which have graduated out ofexperimental, so this PR no longer depends on an experimental package.Two things I would rather you decide:
Options.Clientbe marked deprecated now thatOpenBucketGRPCexists? It is released API, added inab74300b, so it cannot be withdrawn, but it is now a second way to do the same thing.grpcandzonalURL parameters stay? You said the URL option is fine, and I have kept them, but they are the part of this PR that adds surface area. The argument for them is that a caller configured purely by a connection string has no other way to reach a Rapid Storage bucket; anyone writing Go already hasOpenBucketGRPC.