AC: Treat Directories as blobs #34
Conversation
We currently do some cleverness to pop Directory objects into the OutputDirectories field of the ActionResult for semantic purposes. Unfortunately doing this requires jumping over many hurdles, partially of our own making and partially from REAPI. To detect whether the object is a Directory, we fetch it from CAS and attempt to unmarshal it into a Directory message. We then must convert it to a Tree to store in the ActionResult, which also requires the raw proto. The catch is that things that are not Directory messages may successfully unmarshal into one -- as a motivating example, BuildStream's Source proto is such a message. When this happens, we're very likely to fail the request as we attempt to use the corrupted Directory we've created. To solve this, we could pass through data on whether the asset is supposed to be a directory or not, however doing so is inelegant and has another subtle problem -- if the client decides to use PushDirectory to push something that isn't a Directory (which isn't explicitly banned by the spec), then we will error. As the Action and ActionResults we generate are intended only to be used by us, we can instead break the semantics somewhat and treat the Directory as an opaque file, as we do for Blobs. Note: this intentionally breaks the sharing of Actions should the RemoteExecutionFetcher be used. This feels more correct to me, as we were overwriting the one we actually ran anyway. Under this usage we instead get multiple Actions pointing to the same ActionResult -- one for the actually executed Action, and another for the asset reference.
tomcoldrick-ct
force-pushed
the
coldtom/ac-dir-semantics
branch
from
2826044 to
3e5ace2
Compare
|
Hey! Before merging any of this, take a look at this PR: bb-storage already implements this: What you need to do is write Action Cache entries with OutputDirectories that have both In other words, you won't need to abuse OutputFiles for this. Hope that helps! |
|
I am taking over this work. What I would like to do is
This uses the |
| if commandGenerator, err := qualifier.QualifiersToCommand(ref.Qualifiers); err != nil || len(ref.Uris) > 1 { | ||
| // Create the action with the qualifier directory as the input root | ||
| action, command, err = assetReferenceToAction(ref, directoryDigest) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| command = commandGenerator(ref.Uris[0]) | ||
| commandDigest, err := ProtoToDigest(command) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| action = &remoteexecution.Action{ | ||
| CommandDigest: commandDigest, | ||
| InputRootDigest: EmptyDigest, | ||
| } | ||
| // Create the action with the qualifier directory as the input root | ||
| action, command, err = assetReferenceToAction(ref, directoryDigest) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
This change makes assets with the special qualifiers we check for in QualifiersToCommand to unfetchable from the asset server.
Either we need to revert this bit or apply the corresponding change (i.e dropping QualifiersToCommand) from Get.
I do not believe its feasible to set the So I think we need to write Action Cache entries with OutputDirectories that only have root_directory_digest set. |
If we're told that the asset is supposed to be a |
In #33 you wrote:
Have we changed our mind on this? Either way, calculating the |
Note that this is something that bb-storage's CompletenessCheckingBlobAccess does not support. You must set |
Which means we avoid needing to calculate TreeDigest`s whilst still creating Actions/ActionResults that encode that an asset is a directory. Note: the bb-remote-asset server does not validate that the RootDirectoryDigest actually references a Directory proto because bb-remote-asset treats the digest as an opaque digest. See buildbarn#34 (comment)
Which means we avoid needing to calculate TreeDigest`s whilst still creating Actions/ActionResults that encode that an asset is a directory. Note: the bb-remote-asset server does not validate that the RootDirectoryDigest actually references a Directory proto because bb-remote-asset treats the digest as an opaque digest. See buildbarn#34 (comment)
Which means we avoid needing to calculate TreeDigest`s whilst still creating Actions/ActionResults that encode that an asset is a directory. Note: the bb-remote-asset server does not validate that the RootDirectoryDigest actually references a Directory proto because bb-remote-asset treats the digest as an opaque digest. See buildbarn#34 (comment)
Which means we avoid needing to calculate TreeDigest`s whilst still creating Actions/ActionResults that encode that an asset is a directory. Note: the bb-remote-asset server does not validate that the RootDirectoryDigest actually references a Directory proto because bb-remote-asset treats the digest as an opaque digest. See buildbarn#34 (comment)
Which means we avoid needing to calculate TreeDigest`s whilst still creating Actions/ActionResults that encode that an asset is a directory. Note: the bb-remote-asset server does not validate that the RootDirectoryDigest actually references a Directory proto because bb-remote-asset treats the digest as an opaque digest. See buildbarn#34 (comment)
bb-remote-asset does some cleverness to pop `Directory` objects into the OutputDirectories field of the ActionResult for semantic purposes. This commit cleans up that cleverness to allow blobs that look a bit like directories to be uploaded as asset. Prior to this commit we detected whether an object is a `Directory` by fetching it from CAS and attempting to unmarshal it into a `Directory` message. This commit requires all assets uploaded via `PushDirectory` are `Directories` (and treats assets uploaded via `PushBlob` as blobs). We still must convert Directories to a Tree to generate the `TreeDigest` needed by the `ActionResult`, so the action cache asset store will fetch from the CAS when putting a new directory. (The ActionResult needs a `TreeDigest` to satisfy bb-storage's completeness checking.) If the client decides to use `PushDirectory` to push something that isn't a `Directory` (which isn't explicitly banned by the spec), then we will error. The client will need to use `PushBlob` for these assets instead. Fixes buildbarn#33 Closes buildbarn#34
bb-remote-asset does some cleverness to pop `Directory` objects into the OutputDirectories field of the ActionResult for semantic purposes. This commit cleans up that cleverness to allow blobs that look a bit like directories to be uploaded as asset. Prior to this commit we detected whether an object is a `Directory` by fetching it from CAS and attempting to unmarshal it into a `Directory` message. This commit requires all assets uploaded via `PushDirectory` are `Directories` (and treats assets uploaded via `PushBlob` as blobs). We still must convert Directories to a Tree to generate the `TreeDigest` needed by the `ActionResult`, so the action cache asset store will fetch from the CAS when putting a new directory. (The ActionResult needs a `TreeDigest` to satisfy bb-storage's completeness checking.) If the client decides to use `PushDirectory` to push something that isn't a `Directory` (which isn't explicitly banned by the spec), then we will error. The client will need to use `PushBlob` for these assets instead. Fixes buildbarn#33 Closes buildbarn#34
bb-remote-asset does some cleverness to pop `Directory` objects into the OutputDirectories field of the ActionResult for semantic purposes. This commit cleans up that cleverness to allow blobs that look a bit like directories to be uploaded as asset. Prior to this commit we detected whether an object is a `Directory` by fetching it from CAS and attempting to unmarshal it into a `Directory` message. This commit requires all assets uploaded via `PushDirectory` are `Directories` (and treats assets uploaded via `PushBlob` as blobs). We still must convert Directories to a Tree to generate the `TreeDigest` needed by the `ActionResult`, so the action cache asset store will fetch from the CAS when putting a new directory. (The ActionResult needs a `TreeDigest` to satisfy bb-storage's completeness checking.) If the client decides to use `PushDirectory` to push something that isn't a `Directory` (which isn't explicitly banned by the spec), then we will error. The client will need to use `PushBlob` for these assets instead. Fixes buildbarn#33 Closes buildbarn#34
|
Superseded by #40 |
bb-remote-asset does some cleverness to pop `Directory` objects into the OutputDirectories field of the ActionResult for semantic purposes. This commit cleans up that cleverness to allow blobs that look a bit like directories to be uploaded as asset. Prior to this commit we detected whether an object is a `Directory` by fetching it from CAS and attempting to unmarshal it into a `Directory` message. This commit requires all assets uploaded via `PushDirectory` are `Directories` (and treats assets uploaded via `PushBlob` as blobs). We still must convert Directories to a Tree to generate the `TreeDigest` needed by the `ActionResult`, so the action cache asset store will fetch from the CAS when putting a new directory. (The ActionResult needs a `TreeDigest` to satisfy bb-storage's completeness checking.) If the client decides to use `PushDirectory` to push something that isn't a `Directory` (which isn't explicitly banned by the spec), then we will error. The client will need to use `PushBlob` for these assets instead. Fixes buildbarn#33 Closes buildbarn#34
bb-remote-asset does some cleverness to pop `Directory` objects into the OutputDirectories field of the ActionResult for semantic purposes. This commit cleans up that cleverness to allow blobs that look a bit like directories to be uploaded as asset. Prior to this commit we detected whether an object is a `Directory` by fetching it from CAS and attempting to unmarshal it into a `Directory` message. This commit requires all assets uploaded via `PushDirectory` are `Directories` (and treats assets uploaded via `PushBlob` as blobs). We still must convert Directories to a Tree to generate the `TreeDigest` needed by the `ActionResult`, so the action cache asset store will fetch from the CAS when putting a new directory. (The ActionResult needs a `TreeDigest` to satisfy bb-storage's completeness checking.) If the client decides to use `PushDirectory` to push something that isn't a `Directory` (which isn't explicitly banned by the spec), then we will error. The client will need to use `PushBlob` for these assets instead. Fixes #33 Closes #34
We currently do some cleverness to pop
Directoryobjects into theOutputDirectoriesfield of theActionResultfor semantic purposes. Unfortunately doing this requires jumping over many hurdles, partially of our own making and partially from REAPI.To detect whether the object is a
Directory, we fetch it from CAS and attempt to unmarshal it into aDirectorymessage. We then must convert it to aTreeto store in theActionResult, which also requires the raw proto.The catch is that things that are not
Directorymessages may successfully unmarshal into one -- as a motivating example, BuildStream'sSourceproto is such a message. When this happens, we're very likely to fail the request as we attempt to use the corruptedDirectorywe've created. In the BuildStream case, we hit an error with "Unknown Digest Function", as aDigestproto is loaded asnil.To solve this, we could pass through data on whether the asset is supposed to be a directory or not, however doing so is inelegant and has another subtle problem -- if the client decides to use
PushDirectoryto push something that isn't aDirectory(which isn't explicitly banned by the spec), then we will error.As the
ActionandActionResults we generate are intended only to be used by us, we can instead break the semantics somewhat and treat theDirectoryas an opaque file, as we do for blobs.Note: this intentionally breaks the sharing of
Actions should theRemoteExecutionFetcherbe used. This feels more correct to me, as we were overwriting the one we actually ran anyway. Under this usage we instead get multipleActions pointing to the sameActionResult-- one for the actually executedAction, and another for the asset reference.Fixes #33