Update to bb-storage without a CAS blobAccess - #248
Conversation
This commit updates to bb-storage that doesn't export the Content Addressable Storage (CAS) as a blobstore.BlobAccess. The CAS is instead built from two low level storage primitives. The Chunk Storage (CS) which contains raw chunks of data and the Chunk List Storage (CLS) which contains lists that describes which chunks make up a blob in the CAS. This had a fairly large impact on bb-remote-execution as many components assumed that it would interact with the CAS via a blobstore.BlobAccess but this has now been replaced with a cdc.ContentAddressableStorage.
EdSchouten
left a comment
EdSchouten
left a comment
There was a problem hiding this comment.
Will continue reviewing later!
| } | ||
| } | ||
|
|
||
| func (cas *existencePreconditionContentAddressableStorage) FetchChunk(ctx context.Context, d digest.Digest) ([]byte, error) { |
There was a problem hiding this comment.
Hmmm... This brings up an interesting question. In case of chunking, what digest should be used as part of FAILED_PRECONDITION errors? I would argue that you always want to use the digest of the containing object, not the chunk. Clients may be oblivious of chunking. And there's also no guaranteed/fully normalized form of chunking, so returning the digest of the full file is the only meaningful thing you can do here. If that's the case, then there is no way we can convert the error at this specific level/layer.
This is probably worth raising in the working group as well?
| re_util "github.com/buildbarn/bb-remote-execution/pkg/util" | ||
| "github.com/buildbarn/bb-storage/pkg/blobstore" | ||
| "github.com/buildbarn/bb-storage/pkg/blobstore/buffer" | ||
| "github.com/buildbarn/bb-storage/pkg/blobstore/cdc" |
There was a problem hiding this comment.
I think that this is a bit of an odd package for having the ContentAddressableStorage interface. Why can't we just call it pkg/cas?
| historicalExecuteResponse, err := b.ToProto(&cas_proto.HistoricalExecuteResponse{}, 10000) | ||
| require.NoError(t, err) | ||
| DoAndReturn(func(ctx context.Context, digest digest.Digest, b []byte) error { | ||
| historicalExecuteResponse := testutil.MustUnmarshal(t, b, &cas_proto.HistoricalExecuteResponse{}) |
There was a problem hiding this comment.
My suggestion would be to simply write this as:
var historicalExecuteResponse cas_proto.HistoricalExecuteResponse
require.NoError(t, proto.Unmarshal(&historicalExecuteResponse))It's not significantly longer, but has the advantage that we have strong typing (and don't need to resort to generics).
| attachErrorToExecuteResponse(response, status.Errorf(codes.InvalidArgument, "Command is %d bytes which exceeds the maximum message size byte of %d bytes", commandDigest.GetSizeBytes(), be.maximumMessageSizeBytes)) | ||
| return response | ||
| } | ||
| command, err := cdc.GetProto(ctx, be.contentAddressableStorage, commandDigest, &remoteexecution.Command{}) |
There was a problem hiding this comment.
What are your thoughts on simply letting GetProto() take the maximumMessageSizeBytes?
| file.Close() | ||
| return digest.BadDigest, util.StatusWrap(err, "Failed to compute file digest") | ||
| } | ||
| blobDigest := digestGenerator.Sum() |
There was a problem hiding this comment.
In the pre-CDC era, this overall design for UploadFile() sort of made sense. First fully hash the file, and then create a buffer.Buffer out of it. However, with CDC this feels a bit odd. I don't know how to express it just yet; let me think about this a bit more. I guess I'll also need to read up on the bb-storage changes first.
| // it should be preferred over the default template. | ||
| contentAddressableStorage.EXPECT().Get(ctx, digest.MustNewDigest("build", remoteexecution.DigestFunction_SHA256, "9da17cb226048f5bb3e6a20311b551e73ce8ac0d408e69e737d28a8f3179d0ce", 300)). | ||
| Return(buffer.NewProtoBufferFromProto(&remoteexecution.Command{ | ||
| protoBytes, err := proto.Marshal( |
There was a problem hiding this comment.
It's a bit of a shame we need to write proto.Marshal()/proto.Unmarshal() more often in our tests now. Not that I know how to fix this exactly.
2 participants
This commit updates to bb-storage that doesn't export the Content Addressable Storage (CAS) as a blobstore.BlobAccess. The CAS is instead built from two low level storage primitives. The Chunk Storage (CS) which contains raw chunks of data and the Chunk List Storage (CLS) which contains lists that describes which chunks make up a blob in the CAS.
This had a fairly large impact on bb-remote-execution as many components assumed that it would interact with the CAS via a blobstore.BlobAccess but this has now been replaced with a cdc.ContentAddressableStorage.