Replace CAS with CS and CLS - #366
Conversation
This commit adds support for the SplitBlob and SpliceBlob methods from the Remote Execution v2 (REv2) api. SplitBlob and SpliceBlob can be used to facilitate uploads and downloads of large files but a naïve implementation like this has some major drawbacks as well. The blobs must exist in both their chunked and non chunked form, which may significantly increase storage requirements for large blobs. The protocol gives no guarantee that a large blob stored in the CAS exists in its chunked form which forces you to perform a fairly heavy Split call that loads the entire large blob in order to decomposition it into its chunks. This implementation mostly exists as a stepping stone for a different implementation where Buildbarn internally manages all blobs as chunked blobs.
This commit builds on top of our split and splice blob support to make it a mandatory first class feature in Buildbarn. With this commit the Content Addressable Storage (CAS) is created from two Storage configurations that work in tandem. A Chunk Storage (CS) which is content addressed and contains chunks of blobs, and a Chunk List Storage (CLS) which is addressed by a blob digest and contains a manifest describing the chunks that make up the blob. All api calls are automatically translated to use Chunk Lists created with RepMaxCDC. Effectively this means that large blobs no longer exists in the storage layer, individual chunks of the large blobs are in turn deduplicated in such a manner that the chunks are stored only once. The automatic translation makes certain that clients that are not cdc aware can still continue to use the storage backend without performing any changes. Clients which support RepMaxCDC also gets a significant reduction in the amount of blobs to transfer as they only need to transfer modified chunks rather than the entire blob.
a6c6de2 to
eb4b94a
Compare
| // that signals downstream storage layers that the chunk list has | ||
| // already been validated (or was freshly generated) and does not need | ||
| // expensive re-validation. | ||
| func NewContextWithChunkListValidationBypass(ctx context.Context) context.Context { |
There was a problem hiding this comment.
I think that we should get rid of this, and turn it into a function argument. For example, change BlobAccess.Put() to take a validated bool parameter.
|
|
||
| // NewTTLCacheFromConfiguration wraps NewTTLCache with the parameters | ||
| // specified in a configuration message. | ||
| func NewTTLCacheFromConfiguration[K comparable, V any](configuration *digest_pb.ExistenceCacheConfiguration, clock clock.Clock, name string) (*TTLCache[K, V], error) { |
There was a problem hiding this comment.
On the Bonanza side I actually have a somewhat comparable abstraction:
https://pkg.go.dev/bonanza.build/pkg/ds/lossymap
It started out as a generalization to the key-location map of Bonanza's equivalent of LocalBlobAccess. But it can also be used for other types of 'lossy maps'.
What are your thoughts on moving the lossymap package into bb-storage?
| // Optional: Storage backend to use when Reference objects refer to | ||
| // objects stored in another Content Addressable Storage. | ||
| BlobAccessConfiguration content_addressable_storage = 5; | ||
| ContentAddressableStorageConfiguration content_addressable_storage = 5; |
There was a problem hiding this comment.
Yeah, this is interesting. We'll need to figure out how to make ICAS work in the post chunking world. But are we sure that this is conceptually correct? Could a Chunk Store of type ReferenceExpanding be backed by another CAS? I can understand how it can be backed by another Chunk Store.
If we don't know how to solve this specific aspect, I'd be happy to phase out this specific feature. As in, keep ICAS support there, but remove the ability to reference another CAS.
| @@ -0,0 +1,13 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
chunklist.proto to match the directory name.
| } | ||
| if digest.SizeBytes < 0 { | ||
| return BadDigest, status.Error(codes.InvalidArgument, "Negative size digest provided") | ||
| } |
There was a problem hiding this comment.
Why is this specific change necessary? Function.NewDigest() also has a similar check, right?
| serverCapabilities, err := getServerCapabilitiesWithCacheCapabilities(ctx, ba.capabilitiesClient, instanceName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cacheCapabilities := serverCapabilities.CacheCapabilities | ||
| // Only return fields that pertain to Chunk List Storage. | ||
| return &remoteexecution.ServerCapabilities{ | ||
| CacheCapabilities: &remoteexecution.CacheCapabilities{ | ||
| SplitBlobSupport: cacheCapabilities.SplitBlobSupport, | ||
| SpliceBlobSupport: cacheCapabilities.SpliceBlobSupport, | ||
| }, | ||
| }, nil |
There was a problem hiding this comment.
I thought we agreed that GetCapabilities() should always be invoked against the Chunk Store, not the Chunk List Store. In other words, this needs a panic(), just like we have for ICAS, ISCC, etc.
| } | ||
|
|
||
| // GetBytes returns the bytes of a digest from the CAS as a byteslice. | ||
| func GetBytes(ctx context.Context, cas ContentAddressableStorage, d digest.Digest) ([]byte, error) { |
There was a problem hiding this comment.
A function like this should respect an upper size limit.
|
|
||
| // ContentAddressableStorage is an interface which describes | ||
| // interactions with a Content Addressable Storage (CAS). | ||
| type ContentAddressableStorage interface { |
There was a problem hiding this comment.
I think we need to take a step back and ask ourselves what this interface actually abstracts. Right now it's only there to stitch a couple of things together: Chunk Store, Chunk List Store, and fetching of capabilities, and I'm not sure that's worth a lot.
Why can't we have implementations of MessageReader and StreamReader that are directly backed by:
- An instance of cdc.ParametersFetcher
- A Chunk Store BlobAccess
- A Chunk List Store BlobAccess
?
For uploading files/protos into the CAS, that's where we need to write a new abstraction. But that can potentially live in bb-remote-execution?
| SplitBlobSupport: cacheCapabilities.SplitBlobSupport, | ||
| SpliceBlobSupport: cacheCapabilities.SpliceBlobSupport, |
There was a problem hiding this comment.
So the entire idea behind these GetCapabilities() implementations is that they return/yield the kinds of properties that are variable. In this implementation we're going to require that implementations support chunking.
So with that in mind, I think that this function should only check that the CacheCapabilities returned by the server have SplitBlobSupport and SpliceBlobSupport set to true. It doesn't need to return those fields here. Then in cmd/bb_storage/main.go, we should set these fields to true (at the same place where we set SupportedCompressors).
Similarly, this code should never return a ServerCapabilities that doesn't have RepMaxCdcParams set.
| var casCapabilitiesProvider = capabilities.NewStaticProvider(&remoteexecution.ServerCapabilities{ | ||
| var csCapabilitiesProvider = capabilities.NewStaticProvider(&remoteexecution.ServerCapabilities{ | ||
| CacheCapabilities: &remoteexecution.CacheCapabilities{ | ||
| DigestFunctions: digest.SupportedDigestFunctions, |
There was a problem hiding this comment.
This should have SupportsSplitBlobs and such set.
| @@ -16,7 +15,6 @@ type BlobAccess interface { | |||
| capabilities.Provider | |||
|
|
|||
| Get(ctx context.Context, digest digest.Digest) buffer.Buffer | |||
There was a problem hiding this comment.
This may now return (buffer.Buffer, error)
|
|
||
| Get(ctx context.Context, digest digest.Digest) buffer.Buffer | ||
| GetFromComposite(ctx context.Context, parentDigest, childDigest digest.Digest, slicer slicing.BlobSlicer) buffer.Buffer | ||
| Put(ctx context.Context, digest digest.Digest, b buffer.Buffer) error |
There was a problem hiding this comment.
Consider making Buffer a generic type now.
- CAS:
buffer.Buffer[[]byte] - AC:
buffer.Buffer[*remoteexecution.ActionResult] - CLS:
buffer.Buffer[cdc.ChunkList]
?
type BufferMarshaler[T any] {
Marshal(digestFunction digest.Function, T any) ([]byte, error)
Unmarshal(digestFunction digest.Function, []byte) (T, error)
}
This commit builds on top of our split and splice blob support to make
it a mandatory first class feature in Buildbarn. With this commit the
Content Addressable Storage (CAS) is created from two Storage
configurations that work in tandem. A Chunk Storage (CS) which is
content addressed and contains chunks of blobs, and a Chunk List Storage
(CLS) which is addressed by a blob digest and contains a manifest
describing the chunks that make up the blob.
All api calls are automatically translated to use Chunk Lists created
with RepMaxCDC. Effectively this means that large blobs no longer exists
in the storage layer, individual chunks of the large blobs are in turn
deduplicated in such a manner that the chunks are stored only once.
The automatic translation makes certain that clients that are not cdc
aware can still continue to use the storage backend without performing
any changes. Clients which support RepMaxCDC also gets a significant
reduction in the amount of blobs to transfer as they only need to
transfer modified chunks rather than the entire blob.